Discussion:
[PATCH v2] ieee1275: obdisk driver
(too old to reply)
Eric Snowberg
2018-05-30 23:55:22 UTC
Permalink
Add a new disk driver called obdisk for IEEE1275 platforms. Currently
the only platform using this disk driver is SPARC, however other IEEE1275
platforms could start using it if they so choose. While the functionality
within the current IEEE1275 ofdisk driver may be suitable for PPC and x86, it
presented too many problems on SPARC hardware.

Within the old ofdisk, there is not a way to determine the true canonical
name for the disk. Within Open Boot, the same disk can have multiple names
but all reference the same disk. For example the same disk can be referenced
by its SAS WWN, using this form:

/***@302/***@2/***@0/***@17/LSI,***@0/***@w5000cca02f037d6d,0

It can also be referenced by its PHY identifier using this form:

/***@302/***@2/***@0/***@17/LSI,***@0/***@p0

It can also be referenced by its Target identifier using this form:

/***@302/***@2/***@0/***@17/LSI,***@0/***@0

Also, when the LUN=0, it is legal to omit the ,0 from the device name. So with
the disk above, before taking into account the device aliases, there are 6 ways
to reference the same disk.

Then it is possible to have 0 .. n device aliases all representing the same disk.
Within this new driver the true canonical name is determined using the the
IEEE1275 encode-unit and decode-unit commands when address_cells == 4. This
will determine the true single canonical name for the device so multiple ihandles
are not opened for the same device. This is what frequently happens with the old
ofdisk driver. With some devices when they are opened multiple times it causes
the entire system to hang.

Another problem solved with this driver is devices that do not have a device
alias can be booted and used within GRUB. Within the old ofdisk, this was not
possible, unless it was the original boot device. All devices behind a SAS
or SCSI parent can be found. Within the old ofdisk, finding these disks
relied on there being an alias defined. The alias requirement is not
necessary with this new driver. It can also find devices behind a parent
after they have been hot-plugged. This is something that is not possible
with the old ofdisk driver.

The old ofdisk driver also incorrectly assumes that the device pointing to by a
device alias is in its true canonical form. This assumption is never made with
this new driver.

Another issue solved with this driver is that it properly caches the ihandle
for all open devices. The old ofdisk tries to do this by caching the last
opened ihandle. However this does not work properly because the layer above
does not use a consistent device name for the same disk when calling into the
driver. This is because the upper layer uses the bootpath value returned within
/chosen, other times it uses the device alias, and other times it uses the
value within grub.cfg. It does not have a way to figure out that these devices
are the same disk. This is not a problem with this new driver.

Due to the way GRUB repeatedly opens and closes the same disk. Caching the
ihandle is important on SPARC. Without caching, some SAS devices can take
15 - 20 minutes to get to the GRUB menu. This ihandle caching is not possible
without correctly having the canonical disk name.

When available, this driver also tries to use the deblocker #blocks and
a way of determining the disk size.

Finally and probably most importantly, this new driver is also capable of
seeing all partitions on a GPT disk. With the old driver, the GPT
partition table can not be read and only the first partition on the disk
can be seen.

Signed-off-by: Eric Snowberg <***@oracle.com>
---
Changes from v1:
- Addressed various coding style changes requested by Daniel Kipper
---
grub-core/Makefile.core.def | 1 +
grub-core/commands/nativedisk.c | 1 +
grub-core/disk/ieee1275/obdisk.c | 1106 ++++++++++++++++++++++++++++++++++++++
grub-core/kern/ieee1275/cmain.c | 3 +
grub-core/kern/ieee1275/init.c | 12 +-
include/grub/disk.h | 1 +
include/grub/ieee1275/ieee1275.h | 2 +
include/grub/ieee1275/obdisk.h | 25 +
8 files changed, 1150 insertions(+), 1 deletions(-)
create mode 100644 grub-core/disk/ieee1275/obdisk.c
create mode 100644 include/grub/ieee1275/obdisk.h

diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index fc4767f..ab84aa4 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -292,6 +292,7 @@ kernel = {
sparc64_ieee1275 = kern/sparc64/cache.S;
sparc64_ieee1275 = kern/sparc64/dl.c;
sparc64_ieee1275 = kern/sparc64/ieee1275/ieee1275.c;
+ sparc64_ieee1275 = disk/ieee1275/obdisk.c;

arm = kern/arm/dl.c;
arm = kern/arm/dl_helper.c;
diff --git a/grub-core/commands/nativedisk.c b/grub-core/commands/nativedisk.c
index 2f56a87..2f2315d 100644
--- a/grub-core/commands/nativedisk.c
+++ b/grub-core/commands/nativedisk.c
@@ -66,6 +66,7 @@ get_uuid (const char *name, char **uuid, int getnative)
/* Firmware disks. */
case GRUB_DISK_DEVICE_BIOSDISK_ID:
case GRUB_DISK_DEVICE_OFDISK_ID:
+ case GRUB_DISK_DEVICE_OBDISK_ID:
case GRUB_DISK_DEVICE_EFIDISK_ID:
case GRUB_DISK_DEVICE_NAND_ID:
case GRUB_DISK_DEVICE_ARCDISK_ID:
diff --git a/grub-core/disk/ieee1275/obdisk.c b/grub-core/disk/ieee1275/obdisk.c
new file mode 100644
index 0000000..0bc37e6
--- /dev/null
+++ b/grub-core/disk/ieee1275/obdisk.c
@@ -0,0 +1,1106 @@
+/* obdisk.c - Open Boot disk access. */
+/*
+ * GRUB -- GRand Unified Bootloader
+ * Copyright (C) 2018 Free Software Foundation, Inc.
+ *
+ * GRUB is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * GRUB is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with GRUB. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/disk.h>
+#include <grub/env.h>
+#include <grub/i18n.h>
+#include <grub/kernel.h>
+#include <grub/list.h>
+#include <grub/misc.h>
+#include <grub/mm.h>
+#include <grub/scsicmd.h>
+#include <grub/time.h>
+#include <grub/ieee1275/ieee1275.h>
+#include <grub/ieee1275/obdisk.h>
+
+#define IEEE1275_DEV "ieee1275/"
+#define IEEE1275_DISK_ALIAS "/disk@"
+
+struct disk_dev
+{
+ struct disk_dev *next;
+ struct disk_dev **prev;
+ char *name;
+ char *raw_name;
+ char *grub_devpath;
+ char *grub_alias_devpath;
+ char *grub_decoded_devpath;
+ grub_ieee1275_ihandle_t ihandle;
+ grub_uint32_t block_size;
+ grub_uint64_t num_blocks;
+ unsigned int log_sector_size;
+ grub_uint32_t opened;
+ grub_uint32_t valid;
+ grub_uint32_t boot_dev;
+};
+
+struct parent_dev
+{
+ struct parent_dev *next;
+ struct parent_dev **prev;
+ char *name;
+ char *type;
+ grub_ieee1275_ihandle_t ihandle;
+ grub_uint32_t address_cells;
+};
+
+static struct grub_scsi_test_unit_ready tur =
+{
+ .opcode = grub_scsi_cmd_test_unit_ready,
+ .lun = 0,
+ .reserved1 = 0,
+ .reserved2 = 0,
+ .reserved3 = 0,
+ .control = 0,
+};
+
+static int disks_enumerated = 0;
+static struct disk_dev *disk_devs = NULL;
+static struct parent_dev *parent_devs = NULL;
+
+static const char *block_blacklist[] = {
+ /* Requires addition work in grub before being able to be used. */
+ "/iscsi-hba",
+ /* This block device should never be used by grub. */
+ "/reboot-***@0",
+ 0
+};
+
+#define STRCMP(a, b) ((a) && (b) && (grub_strcmp (a, b) == 0))
+
+static char *
+strip_ob_partition (char *path)
+{
+ char *sptr;
+
+ sptr = grub_strstr (path, ":");
+
+ if (sptr)
+ *sptr = '\0';
+
+ return path;
+}
+
+static char *
+replace_escaped_commas (char *src)
+{
+ char *iptr;
+
+ if (src == NULL)
+ return NULL;
+ for (iptr = src; *iptr; )
+ {
+ if ((*iptr == '\\') && (*(iptr + 1) == ','))
+ {
+ *iptr++ = '_';
+ *iptr++ = '_';
+ }
+ iptr++;
+ }
+
+ return src;
+}
+
+static int
+count_commas (const char *src)
+{
+ int count = 0;
+
+ for ( ; *src; src++)
+ if (*src == ',')
+ count++;
+
+ return count;
+}
+
+static void
+escape_commas (const char *src, char *dest)
+{
+ const char *iptr;
+
+ for (iptr = src; *iptr; )
+ {
+ if (*iptr == ',')
+ *dest++ ='\\';
+
+ *dest++ = *iptr++;
+ }
+
+ *dest = '\0';
+}
+
+static char *
+decode_grub_devname (const char *name)
+{
+ char *devpath = grub_malloc (grub_strlen (name) + 1);
+ char *p, c;
+
+ if (devpath == NULL)
+ return NULL;
+
+ /* Un-escape commas. */
+ p = devpath;
+ while ((c = *name++) != '\0')
+ {
+ if (c == '\\' && *name == ',')
+ {
+ *p++ = ',';
+ name++;
+ }
+ else
+ *p++ = c;
+ }
+
+ *p++ = '\0';
+
+ return devpath;
+}
+
+static char *
+encode_grub_devname (const char *path)
+{
+ char *encoding, *optr;
+
+ if (path == NULL)
+ return NULL;
+
+ encoding = grub_malloc (sizeof (IEEE1275_DEV) + count_commas (path) +
+ grub_strlen (path) + 1);
+
+ if (encoding == NULL)
+ {
+ grub_print_error ();
+ return NULL;
+ }
+
+ optr = grub_stpcpy (encoding, IEEE1275_DEV);
+ escape_commas (path, optr);
+ return encoding;
+}
+
+static char *
+get_parent_devname (const char *devname)
+{
+ char *parent, *pptr;
+
+ parent = grub_strdup (devname);
+
+ if (parent == NULL)
+ {
+ grub_print_error ();
+ return NULL;
+ }
+
+ pptr = grub_strstr (parent, IEEE1275_DISK_ALIAS);
+
+ if (pptr)
+ *pptr = '\0';
+
+ return parent;
+}
+
+static void
+free_parent_dev (struct parent_dev *parent)
+{
+ if (parent)
+ {
+ grub_free (parent->name);
+ grub_free (parent->type);
+ grub_free (parent);
+ }
+}
+
+static struct parent_dev *
+init_parent (const char *parent)
+{
+ struct parent_dev *op;
+
+ op = grub_zalloc (sizeof (struct parent_dev));
+
+ if (op == NULL)
+ {
+ grub_print_error ();
+ return NULL;
+ }
+
+ op->name = grub_strdup (parent);
+ op->type = grub_malloc (IEEE1275_MAX_PROP_LEN);
+
+ if ((op->name == NULL) || (op->type == NULL))
+ {
+ grub_print_error ();
+ free_parent_dev (op);
+ return NULL;
+ }
+
+ return op;
+}
+
+static struct parent_dev *
+open_new_parent (const char *parent)
+{
+ struct parent_dev *op = init_parent(parent);
+ grub_ieee1275_ihandle_t ihandle;
+ grub_ieee1275_phandle_t phandle;
+ grub_uint32_t address_cells = 2;
+ grub_ssize_t actual = 0;
+
+ if (op == NULL)
+ return NULL;
+
+ grub_ieee1275_open (parent, &ihandle);
+
+ if (ihandle == 0)
+ {
+ grub_error (GRUB_ERR_BAD_DEVICE, "unable to open %s", parent);
+ grub_print_error ();
+ free_parent_dev (op);
+ return NULL;
+ }
+
+ if (grub_ieee1275_instance_to_package (ihandle, &phandle))
+ {
+ grub_error (GRUB_ERR_BAD_DEVICE, "unable to get parent %s", parent);
+ grub_print_error ();
+ free_parent_dev (op);
+ return NULL;
+ }
+
+ /*
+ * IEEE Std 1275-1994 page 110: A missing "address-cells" property
+ * signifies that the number of address cells is two. So ignore on error.
+ */
+ grub_ieee1275_get_integer_property (phandle, "#address-cells", &address_cells,
+ sizeof (address_cells), 0);
+
+ grub_ieee1275_get_property (phandle, "device_type", op->type,
+ IEEE1275_MAX_PROP_LEN, &actual);
+ op->ihandle = ihandle;
+ op->address_cells = address_cells;
+ return op;
+}
+
+static struct parent_dev *
+open_parent (const char *parent)
+{
+ struct parent_dev *op;
+
+ op = grub_named_list_find (GRUB_AS_NAMED_LIST (parent_devs), parent);
+
+ if (op == NULL)
+ {
+ op = open_new_parent (parent);
+
+ if (op)
+ grub_list_push (GRUB_AS_LIST_P (&parent_devs), GRUB_AS_LIST (op));
+ }
+
+ return op;
+}
+
+static void
+display_parents (void)
+{
+ struct parent_dev *parent;
+
+ grub_printf ("-------------------- PARENTS --------------------\n");
+
+ FOR_LIST_ELEMENTS (parent, parent_devs)
+ {
+ grub_printf ("name: %s\n", parent->name);
+ grub_printf ("type: %s\n", parent->type);
+ grub_printf ("address_cells %x\n", parent->address_cells);
+ }
+
+ grub_printf ("-------------------------------------------------\n");
+}
+
+static char *
+canonicalise_4cell_ua (grub_ieee1275_ihandle_t ihandle, char *unit_address)
+{
+ grub_uint32_t phy_lo, phy_hi, lun_lo, lun_hi;
+ int valid_phy = 0;
+ grub_size_t size;
+ char *canon = NULL;
+
+ valid_phy = grub_ieee1275_decode_unit4 (ihandle, unit_address,
+ grub_strlen (unit_address), &phy_lo,
+ &phy_hi, &lun_lo, &lun_hi);
+
+ if ((valid_phy == 0) && (phy_hi != 0xffffffff))
+ canon = grub_ieee1275_encode_uint4 (ihandle, phy_lo, phy_hi,
+ lun_lo, lun_hi, &size);
+
+ return canon;
+}
+
+static char *
+canonicalise_disk (const char *devname)
+{
+ char *canon, *parent;
+ struct parent_dev *op;
+
+ canon = grub_ieee1275_canonicalise_devname (devname);
+
+ if (canon == NULL)
+ {
+ /* This should not happen. */
+ grub_error (GRUB_ERR_BAD_DEVICE, "canonicalise devname failed");
+ grub_print_error ();
+ return NULL;
+ }
+
+ /* Don't try to open the parent of a virtual device. */
+ if (grub_strstr (canon, "virtual-devices"))
+ return canon;
+
+ parent = get_parent_devname (canon);
+
+ if (parent == NULL)
+ return NULL;
+
+ op = open_parent (parent);
+
+ /*
+ * Devices with 4 address cells can have many different types of addressing
+ * (phy, wwn, and target lun). Use the parents encode-unit / decode-unit
+ * to find the true canonical name.
+ */
+ if ((op) && (op->address_cells == 4))
+ {
+ char *unit_address, *real_unit_address, *real_canon;
+ grub_size_t real_unit_str_len;
+
+ unit_address = grub_strstr (canon, IEEE1275_DISK_ALIAS);
+ unit_address += grub_strlen (IEEE1275_DISK_ALIAS);
+
+ if (unit_address == NULL)
+ {
+ /*
+ * This should not be possible, but return the canonical name for
+ * the non-disk block device.
+ */
+ grub_free (parent);
+ return (canon);
+ }
+
+ real_unit_address = canonicalise_4cell_ua (op->ihandle, unit_address);
+
+ if (real_unit_address == NULL)
+ {
+ /*
+ * This is not an error, since this function could be called with a devalias
+ * containing a drive that isn't installed in the system.
+ */
+ grub_free (parent);
+ return NULL;
+ }
+
+ real_unit_str_len = grub_strlen (op->name) + sizeof (IEEE1275_DISK_ALIAS)
+ + grub_strlen (real_unit_address);
+
+ real_canon = grub_malloc (real_unit_str_len);
+
+ grub_snprintf (real_canon, real_unit_str_len, "%s/disk@%s",
+ op->name, real_unit_address);
+
+ grub_free (canon);
+ canon = real_canon;
+ }
+
+ grub_free (parent);
+ return (canon);
+}
+
+static struct disk_dev *
+add_canon_disk (const char *cname)
+{
+ struct disk_dev *dev;
+
+ dev = grub_zalloc (sizeof (struct disk_dev));
+
+ if (dev == NULL)
+ goto failed;
+
+ if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_RAW_DEVNAMES))
+ {
+ /*
+ * Append :nolabel to the end of all SPARC disks.
+ * nolabel is mutually exclusive with all other
+ * arguments and allows a client program to open
+ * the entire (raw) disk. Any disk label is ignored.
+ */
+ dev->raw_name = grub_malloc (grub_strlen (cname) + sizeof (":nolabel"));
+
+ if (dev->raw_name == NULL)
+ goto failed;
+
+ grub_snprintf (dev->raw_name, grub_strlen (cname) + sizeof (":nolabel"),
+ "%s:nolabel", cname);
+ }
+
+ /*
+ * Don't use grub_ieee1275_encode_devname here, the devpath in grub.cfg doesn't
+ * understand device aliases, which the layer above sometimes sends us.
+ */
+ dev->grub_devpath = encode_grub_devname(cname);
+
+ if (dev->grub_devpath == NULL)
+ goto failed;
+
+ dev->name = grub_strdup (cname);
+
+ if (dev->name == NULL)
+ goto failed;
+
+ dev->valid = 1;
+ grub_list_push (GRUB_AS_LIST_P (&disk_devs), GRUB_AS_LIST (dev));
+ return dev;
+
+ failed:
+ grub_print_error ();
+
+ if (dev)
+ {
+ grub_free (dev->name);
+ grub_free (dev->grub_devpath);
+ grub_free (dev->raw_name);
+ }
+
+ grub_free (dev);
+ return NULL;
+}
+
+static grub_err_t
+add_disk (const char *path)
+{
+ grub_err_t ret = GRUB_ERR_NONE;
+ struct disk_dev *dev;
+ char *canon;
+
+ canon = canonicalise_disk (path);
+ dev = grub_named_list_find (GRUB_AS_NAMED_LIST (disk_devs), canon);
+
+ if ((canon != NULL) && (dev == NULL))
+ {
+ struct disk_dev *ob_device;
+
+ ob_device = add_canon_disk (canon);
+
+ if (ob_device == NULL)
+ ret = grub_error (GRUB_ERR_OUT_OF_MEMORY, "failure to add disk");
+ }
+ else if (dev)
+ dev->valid = 1;
+
+ grub_free (canon);
+ return (ret);
+}
+
+static grub_err_t
+grub_obdisk_read (grub_disk_t disk, grub_disk_addr_t sector,
+ grub_size_t size, char *dest)
+{
+ grub_err_t ret = GRUB_ERR_NONE;
+ struct disk_dev *dev;
+ unsigned long long pos;
+ grub_ssize_t result = 0;
+
+ if (disk->data == NULL)
+ return grub_error (GRUB_ERR_BAD_DEVICE, "invalid disk data");
+
+ dev = (struct disk_dev *)disk->data;
+ pos = sector << disk->log_sector_size;
+ grub_ieee1275_seek (dev->ihandle, pos, &result);
+
+ if (result < 0)
+ {
+ dev->opened = 0;
+ return grub_error (GRUB_ERR_READ_ERROR, "seek error, can't seek block %llu",
+ (long long) sector);
+ }
+
+ grub_ieee1275_read (dev->ihandle, dest, size << disk->log_sector_size,
+ &result);
+
+ if (result != (grub_ssize_t) (size << disk->log_sector_size))
+ {
+ dev->opened = 0;
+ return grub_error (GRUB_ERR_READ_ERROR, N_("failure reading sector 0x%llx "
+ "from `%s'"),
+ (unsigned long long) sector,
+ disk->name);
+ }
+ return ret;
+}
+
+static void
+grub_obdisk_close (grub_disk_t disk)
+{
+ grub_memset (disk, 0, sizeof (*disk));
+}
+
+static void
+scan_usb_disk (const char *parent)
+{
+ struct parent_dev *op;
+ grub_ssize_t result;
+
+ op = open_parent (parent);
+
+ if (op == NULL)
+ {
+ grub_error (GRUB_ERR_BAD_DEVICE, "unable to open %s", parent);
+ grub_print_error ();
+ return;
+ }
+
+ if ((grub_ieee1275_set_address (op->ihandle, 0, 0) == 0) &&
+ (grub_ieee1275_no_data_command (op->ihandle, &tur, &result) == 0) &&
+ (result == 0))
+ {
+ char *buf;
+
+ buf = grub_malloc (IEEE1275_MAX_PATH_LEN);
+
+ if (buf == NULL)
+ {
+ grub_error (GRUB_ERR_OUT_OF_MEMORY, "disk scan failure");
+ grub_print_error ();
+ return;
+ }
+
+ grub_snprintf (buf, IEEE1275_MAX_PATH_LEN, "%s/***@0", parent);
+ add_disk (buf);
+ grub_free (buf);
+ }
+}
+
+static void
+scan_nvme_disk (const char *path)
+{
+ char *buf;
+
+ buf = grub_malloc (IEEE1275_MAX_PATH_LEN);
+
+ if (buf == NULL)
+ {
+ grub_error (GRUB_ERR_OUT_OF_MEMORY, "disk scan failure");
+ grub_print_error ();
+ return;
+ }
+
+ grub_snprintf (buf, IEEE1275_MAX_PATH_LEN, "%s/***@1", path);
+ add_disk (buf);
+ grub_free (buf);
+}
+
+static void
+scan_sparc_sas_2cell (struct parent_dev *op)
+{
+ grub_ssize_t result;
+ grub_uint8_t tgt;
+ char *buf;
+
+ buf = grub_malloc (IEEE1275_MAX_PATH_LEN);
+
+ if (buf == NULL)
+ {
+ grub_error (GRUB_ERR_OUT_OF_MEMORY, "disk scan failure");
+ grub_print_error ();
+ return;
+ }
+
+ for (tgt = 0; tgt < 0xf; tgt++)
+ {
+
+ if ((grub_ieee1275_set_address(op->ihandle, tgt, 0) == 0) &&
+ (grub_ieee1275_no_data_command (op->ihandle, &tur, &result) == 0) &&
+ (result == 0))
+ {
+
+ grub_snprintf (buf, IEEE1275_MAX_PATH_LEN, "%s/disk@%"
+ PRIxGRUB_UINT32_T, op->name, tgt);
+
+ add_disk (buf);
+ }
+ }
+}
+
+static void
+scan_sparc_sas_4cell (struct parent_dev *op)
+{
+ grub_uint16_t exp;
+ grub_uint8_t phy;
+ char *buf;
+
+ buf = grub_malloc (IEEE1275_MAX_PATH_LEN);
+
+ if (buf == NULL)
+ {
+ grub_error (GRUB_ERR_OUT_OF_MEMORY, "disk scan failure");
+ grub_print_error ();
+ return;
+ }
+
+ for (exp = 0; exp <= 0x100; exp+=0x100)
+
+ for (phy = 0; phy < 0x20; phy++)
+ {
+ char *canon = NULL;
+
+ grub_snprintf (buf, IEEE1275_MAX_PATH_LEN, "p%" PRIxGRUB_UINT32_T ",0",
+ exp | phy);
+
+ canon = canonicalise_4cell_ua (op->ihandle, buf);
+
+ if (canon)
+ {
+ grub_snprintf (buf, IEEE1275_MAX_PATH_LEN, "%s/disk@%s",
+ op->name, canon);
+
+ add_disk (buf);
+ grub_free (canon);
+ }
+ }
+
+ grub_free (buf);
+}
+
+static void
+scan_sparc_sas_disk (const char *parent)
+{
+ struct parent_dev *op;
+
+ op = open_parent (parent);
+
+ if ((op) && (op->address_cells == 4))
+ scan_sparc_sas_4cell (op);
+ else if ((op) && (op->address_cells == 2))
+ scan_sparc_sas_2cell (op);
+}
+
+static void
+iterate_devtree (const struct grub_ieee1275_devalias *alias)
+{
+ struct grub_ieee1275_devalias child;
+
+ if ((grub_strcmp (alias->type, "scsi-2") == 0) ||
+ (grub_strcmp (alias->type, "scsi-sas") == 0))
+ return scan_sparc_sas_disk (alias->path);
+
+ else if (grub_strcmp (alias->type, "nvme") == 0)
+ return scan_nvme_disk (alias->path);
+
+ else if (grub_strcmp (alias->type, "scsi-usb") == 0)
+ return scan_usb_disk (alias->path);
+
+ else if (grub_strcmp (alias->type, "block") == 0)
+ {
+ const char **bl = block_blacklist;
+
+ while (*bl != NULL)
+ {
+ if (grub_strstr (alias->path, *bl))
+ return;
+ bl++;
+ }
+
+ add_disk (alias->path);
+ return;
+ }
+
+ FOR_IEEE1275_DEVCHILDREN (alias->path, child)
+ iterate_devtree (&child);
+}
+
+static void
+unescape_devices (void)
+{
+ struct disk_dev *dev;
+
+ FOR_LIST_ELEMENTS (dev, disk_devs)
+ {
+ grub_free (dev->grub_decoded_devpath);
+
+ if ((dev->grub_alias_devpath) &&
+ (grub_strcmp (dev->grub_alias_devpath, dev->grub_devpath) != 0))
+ dev->grub_decoded_devpath =
+ replace_escaped_commas (grub_strdup (dev->grub_alias_devpath));
+ else
+ dev->grub_decoded_devpath =
+ replace_escaped_commas (grub_strdup (dev->grub_devpath));
+ }
+}
+
+static void
+enumerate_disks (void)
+{
+ struct grub_ieee1275_devalias alias;
+
+ FOR_IEEE1275_DEVCHILDREN("/", alias)
+ iterate_devtree (&alias);
+}
+
+static grub_err_t
+add_bootpath (void)
+{
+ struct disk_dev *ob_device;
+ grub_err_t ret = GRUB_ERR_NONE;
+ char *dev, *alias;
+ char *type;
+
+ dev = grub_ieee1275_get_boot_dev ();
+
+ if (dev == NULL)
+ return grub_error (GRUB_ERR_OUT_OF_MEMORY, "failure adding boot device");
+
+ type = grub_ieee1275_get_device_type (dev);
+
+ if (type == NULL)
+ {
+ grub_free (dev);
+ return grub_error (GRUB_ERR_OUT_OF_MEMORY, "failure adding boot device");
+ }
+
+ alias = NULL;
+
+ if (grub_strcmp (type, "network") != 0)
+ {
+ dev = strip_ob_partition (dev);
+ ob_device = add_canon_disk (dev);
+
+ if (ob_device == NULL)
+ ret = grub_error (GRUB_ERR_OUT_OF_MEMORY, "failure adding boot device");
+
+ ob_device->valid = 1;
+
+ alias = grub_ieee1275_get_devname (dev);
+
+ if (alias && grub_strcmp (alias, dev) != 0)
+ ob_device->grub_alias_devpath = grub_ieee1275_encode_devname (dev);
+
+ ob_device->boot_dev = 1;
+ }
+
+ grub_free (type);
+ grub_free (dev);
+ grub_free (alias);
+ return ret;
+}
+
+static void
+enumerate_aliases (void)
+{
+ struct grub_ieee1275_devalias alias;
+
+ /*
+ * Some block device aliases are not in canonical form
+ *
+ * For example:
+ *
+ * disk3 /***@301/***@1/***@0/***@p3
+ * disk2 /***@301/***@1/***@0/***@p2
+ * disk1 /***@301/***@1/***@0/***@p1
+ * disk /***@301/***@1/***@0/***@p0
+ * disk0 /***@301/***@1/***@0/***@p0
+ *
+ * None of these devices are in canonical form.
+ *
+ * Also, just because there is a devalias, doesn't mean there is a disk
+ * at that location. And a valid boot block device doesn't have to have
+ * a devalias at all.
+ *
+ * At this point, all valid disks have been found in the system
+ * and devaliases that point to canonical names are stored in the
+ * disk_devs list already.
+ */
+ FOR_IEEE1275_DEVALIASES (alias)
+ {
+ struct disk_dev *dev;
+ char *canon;
+
+ if (grub_strcmp (alias.type, "block") != 0)
+ continue;
+
+ canon = canonicalise_disk (alias.name);
+
+ if (canon == NULL)
+ /*
+ * This is not an error, a devalias could point to a
+ * nonexistent disk
+ */
+ continue;
+
+ dev = grub_named_list_find (GRUB_AS_NAMED_LIST (disk_devs), canon);
+
+ if (dev)
+ {
+ /*
+ * If more than one alias points to the same device,
+ * remove the previous one unless it is the boot dev,
+ * since the upper level will use the first one. The reason
+ * all the others are redone is in the case of hot-plugging
+ * a disk. If the boot disk gets hot-plugged, it will come
+ * thru here with a different name without the boot_dev flag
+ * set.
+ */
+ if ((dev->boot_dev) && (dev->grub_alias_devpath))
+ continue;
+
+ grub_free (dev->grub_alias_devpath);
+ dev->grub_alias_devpath = grub_ieee1275_encode_devname (alias.path);
+ }
+ grub_free (canon);
+ }
+}
+
+static void
+display_disks (void)
+{
+ struct disk_dev *dev;
+
+ grub_printf ("--------------------- DISKS ---------------------\n");
+
+ FOR_LIST_ELEMENTS (dev, disk_devs)
+ {
+ grub_printf ("name: %s\n", dev->name);
+ grub_printf ("grub_devpath: %s\n", dev->grub_devpath);
+ grub_printf ("grub_alias_devpath: %s\n", dev->grub_alias_devpath);
+ grub_printf ("grub_decoded_devpath: %s\n", dev->grub_decoded_devpath);
+ grub_printf ("valid: %s\n", (dev->valid) ? "yes" : "no");
+ grub_printf ("boot_dev: %s\n", (dev->boot_dev) ? "yes" : "no");
+ grub_printf ("opened: %s\n", (dev->ihandle) ? "yes" : "no");
+ grub_printf ("block size: %" PRIuGRUB_UINT32_T "\n",
+ dev->block_size);
+ grub_printf ("num blocks: %" PRIuGRUB_UINT64_T "\n",
+ dev->num_blocks);
+ grub_printf ("log sector size: %" PRIuGRUB_UINT32_T "\n",
+ dev->log_sector_size);
+ grub_printf ("\n");
+ }
+
+ grub_printf ("-------------------------------------------------\n");
+}
+
+static void
+display_stats (void)
+{
+ const char *debug = grub_env_get ("debug");
+
+ if (debug == NULL)
+ return;
+
+ if (grub_strword (debug, "all") || grub_strword (debug, "obdisk"))
+ {
+ display_parents ();
+ display_disks ();
+ }
+}
+
+static void
+invalidate_all_disks (void)
+{
+ struct disk_dev *dev = NULL;
+
+ if (disks_enumerated)
+ FOR_LIST_ELEMENTS (dev, disk_devs)
+ dev->valid = 0;
+}
+
+static struct disk_dev *
+find_legacy_grub_devpath (const char *name)
+{
+ struct disk_dev *dev = NULL;
+ char *canon, *devpath = NULL;
+
+ devpath = decode_grub_devname (name + sizeof ("ieee1275"));
+ canon = canonicalise_disk (devpath);
+
+ if (canon != NULL)
+ dev = grub_named_list_find (GRUB_AS_NAMED_LIST (disk_devs), canon);
+
+ grub_free (devpath);
+ grub_free (canon);
+ return dev;
+}
+
+static void
+enumerate_devices (void)
+{
+ invalidate_all_disks ();
+ enumerate_disks ();
+ enumerate_aliases ();
+ unescape_devices ();
+ disks_enumerated = 1;
+ display_stats ();
+}
+
+static struct disk_dev *
+find_grub_devpath_real (const char *name)
+{
+ struct disk_dev *dev = NULL;
+
+ FOR_LIST_ELEMENTS (dev, disk_devs)
+ {
+ if ((STRCMP (dev->grub_devpath, name))
+ || (STRCMP (dev->grub_alias_devpath, name))
+ || (STRCMP (dev->grub_decoded_devpath, name)))
+ break;
+ }
+
+ return dev;
+}
+
+static struct disk_dev *
+find_grub_devpath (const char *name)
+{
+ struct disk_dev *dev = NULL;
+ int enumerated;
+
+ do {
+ enumerated = disks_enumerated;
+ dev = find_grub_devpath_real (name);
+
+ if (dev)
+ break;
+
+ dev = find_legacy_grub_devpath (name);
+
+ if (dev)
+ break;
+
+ enumerate_devices ();
+ } while (enumerated == 0);
+
+ return dev;
+}
+
+static int
+grub_obdisk_iterate (grub_disk_dev_iterate_hook_t hook, void *hook_data,
+ grub_disk_pull_t pull)
+{
+ struct disk_dev *dev;
+
+ if (pull != GRUB_DISK_PULL_NONE)
+ return 0;
+
+ enumerate_devices ();
+
+ FOR_LIST_ELEMENTS (dev, disk_devs)
+ {
+ if (dev->valid == 1)
+ if (hook (dev->grub_decoded_devpath, hook_data))
+ return 1;
+ }
+
+ return 0;
+}
+
+static grub_err_t
+grub_obdisk_open (const char *name, grub_disk_t disk)
+{
+ grub_ieee1275_ihandle_t ihandle = 0;
+ struct disk_dev *dev = NULL;
+
+ if (grub_strncmp (name, IEEE1275_DEV, sizeof (IEEE1275_DEV) - 1) != 0)
+ return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "not IEEE1275 device");
+
+ dev = find_grub_devpath (name);
+
+ if (dev == NULL)
+ {
+ grub_printf ("UNKNOWN DEVICE: %s\n", name);
+ return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "%s", name);
+ }
+
+ if (dev->opened == 0)
+ {
+ if (dev->raw_name)
+ grub_ieee1275_open (dev->raw_name, &ihandle);
+ else
+ grub_ieee1275_open (dev->name, &ihandle);
+
+ if (ihandle == 0)
+ {
+ grub_printf ("Can't open device %s\n", name);
+ return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "can't open device %s", name);
+ }
+
+ dev->block_size = grub_ieee1275_get_block_size (ihandle);
+ dev->num_blocks = grub_ieee1275_num_blocks (ihandle);
+
+ if (dev->num_blocks == 0)
+ dev->num_blocks = grub_ieee1275_num_blocks64 (ihandle);
+
+ if (dev->num_blocks == 0)
+ dev->num_blocks = GRUB_DISK_SIZE_UNKNOWN;
+
+ if (dev->block_size != 0)
+ {
+ for (dev->log_sector_size = 0;
+ (1U << dev->log_sector_size) < dev->block_size;
+ dev->log_sector_size++);
+ }
+ else
+ dev->log_sector_size = 9;
+
+ dev->ihandle = ihandle;
+ dev->opened = 1;
+ }
+
+ disk->total_sectors = dev->num_blocks;
+ disk->id = dev->ihandle;
+ disk->data = dev;
+ disk->log_sector_size = dev->log_sector_size;
+ return GRUB_ERR_NONE;
+}
+
+
+static struct grub_disk_dev grub_obdisk_dev =
+ {
+ .name = "obdisk",
+ .id = GRUB_DISK_DEVICE_OBDISK_ID,
+ .iterate = grub_obdisk_iterate,
+ .open = grub_obdisk_open,
+ .close = grub_obdisk_close,
+ .read = grub_obdisk_read,
+ };
+
+void
+grub_obdisk_init (void)
+{
+ grub_disk_firmware_fini = grub_obdisk_fini;
+ add_bootpath ();
+ grub_disk_dev_register (&grub_obdisk_dev);
+}
+
+void
+grub_obdisk_fini (void)
+{
+ struct disk_dev *dev;
+
+ FOR_LIST_ELEMENTS (dev, disk_devs)
+ {
+ if (dev->opened)
+ grub_ieee1275_close (dev->ihandle);
+ }
+
+ grub_disk_dev_unregister (&grub_obdisk_dev);
+}
diff --git a/grub-core/kern/ieee1275/cmain.c b/grub-core/kern/ieee1275/cmain.c
index 3e12e6b..20cbbd7 100644
--- a/grub-core/kern/ieee1275/cmain.c
+++ b/grub-core/kern/ieee1275/cmain.c
@@ -108,6 +108,9 @@ grub_ieee1275_find_options (void)
if (rc >= 0)
{
char *ptr;
+
+ if (grub_strncmp (tmp, "sun4v", 5) == 0)
+ grub_ieee1275_set_flag (GRUB_IEEE1275_FLAG_RAW_DEVNAMES);
for (ptr = tmp; ptr - tmp < actual; ptr += grub_strlen (ptr) + 1)
{
if (grub_memcmp (ptr, "MacRISC", sizeof ("MacRISC") - 1) == 0
diff --git a/grub-core/kern/ieee1275/init.c b/grub-core/kern/ieee1275/init.c
index 0d8ebf5..d483e35 100644
--- a/grub-core/kern/ieee1275/init.c
+++ b/grub-core/kern/ieee1275/init.c
@@ -30,6 +30,9 @@
#include <grub/time.h>
#include <grub/ieee1275/console.h>
#include <grub/ieee1275/ofdisk.h>
+#ifdef __sparc__
+#include <grub/ieee1275/obdisk.h>
+#endif
#include <grub/ieee1275/ieee1275.h>
#include <grub/net.h>
#include <grub/offsets.h>
@@ -280,8 +283,11 @@ grub_machine_init (void)
grub_console_init_early ();
grub_claim_heap ();
grub_console_init_lately ();
+#ifdef __sparc__
+ grub_obdisk_init ();
+#else
grub_ofdisk_init ();
-
+#endif
grub_parse_cmdline ();

#ifdef __i386__
@@ -296,7 +302,11 @@ grub_machine_fini (int flags)
{
if (flags & GRUB_LOADER_FLAG_NORETURN)
{
+#ifdef __sparc__
+ grub_obdisk_fini ();
+#else
grub_ofdisk_fini ();
+#endif
grub_console_fini ();
}
}
diff --git a/include/grub/disk.h b/include/grub/disk.h
index b385af8..bd58b11 100644
--- a/include/grub/disk.h
+++ b/include/grub/disk.h
@@ -49,6 +49,7 @@ enum grub_disk_dev_id
GRUB_DISK_DEVICE_CBFSDISK_ID,
GRUB_DISK_DEVICE_UBOOTDISK_ID,
GRUB_DISK_DEVICE_XEN,
+ GRUB_DISK_DEVICE_OBDISK_ID,
};

struct grub_disk;
diff --git a/include/grub/ieee1275/ieee1275.h b/include/grub/ieee1275/ieee1275.h
index 8868f3a..73e2f46 100644
--- a/include/grub/ieee1275/ieee1275.h
+++ b/include/grub/ieee1275/ieee1275.h
@@ -146,6 +146,8 @@ enum grub_ieee1275_flag
GRUB_IEEE1275_FLAG_BROKEN_REPEAT,

GRUB_IEEE1275_FLAG_CURSORONOFF_ANSI_BROKEN,
+
+ GRUB_IEEE1275_FLAG_RAW_DEVNAMES,
};

extern int EXPORT_FUNC(grub_ieee1275_test_flag) (enum grub_ieee1275_flag flag);
diff --git a/include/grub/ieee1275/obdisk.h b/include/grub/ieee1275/obdisk.h
new file mode 100644
index 0000000..2fbe934
--- /dev/null
+++ b/include/grub/ieee1275/obdisk.h
@@ -0,0 +1,25 @@
+/*
+ * GRUB -- GRand Unified Bootloader
+ * Copyright (C) 2018 Free Software Foundation, Inc.
+ *
+ * GRUB is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * GRUB is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with GRUB. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef GRUB_OBDISK_HEADER
+#define GRUB_OBDISK_HEADER 1
+
+extern void grub_obdisk_init (void);
+extern void grub_obdisk_fini (void);
+
+#endif
--
1.7.1
Daniel Kiper
2018-06-15 12:02:18 UTC
Permalink
Post by Eric Snowberg
Add a new disk driver called obdisk for IEEE1275 platforms. Currently
the only platform using this disk driver is SPARC, however other IEEE1275
platforms could start using it if they so choose. While the functionality
within the current IEEE1275 ofdisk driver may be suitable for PPC and x86, it
presented too many problems on SPARC hardware.
Within the old ofdisk, there is not a way to determine the true canonical
name for the disk. Within Open Boot, the same disk can have multiple names
but all reference the same disk. For example the same disk can be referenced
Also, when the LUN=0, it is legal to omit the ,0 from the device name. So with
the disk above, before taking into account the device aliases, there are 6 ways
to reference the same disk.
Then it is possible to have 0 .. n device aliases all representing the same disk.
Within this new driver the true canonical name is determined using the the
IEEE1275 encode-unit and decode-unit commands when address_cells == 4. This
will determine the true single canonical name for the device so multiple ihandles
are not opened for the same device. This is what frequently happens with the old
ofdisk driver. With some devices when they are opened multiple times it causes
the entire system to hang.
Another problem solved with this driver is devices that do not have a device
alias can be booted and used within GRUB. Within the old ofdisk, this was not
possible, unless it was the original boot device. All devices behind a SAS
or SCSI parent can be found. Within the old ofdisk, finding these disks
relied on there being an alias defined. The alias requirement is not
necessary with this new driver. It can also find devices behind a parent
after they have been hot-plugged. This is something that is not possible
with the old ofdisk driver.
The old ofdisk driver also incorrectly assumes that the device pointing to by a
device alias is in its true canonical form. This assumption is never made with
this new driver.
Another issue solved with this driver is that it properly caches the ihandle
for all open devices. The old ofdisk tries to do this by caching the last
opened ihandle. However this does not work properly because the layer above
does not use a consistent device name for the same disk when calling into the
driver. This is because the upper layer uses the bootpath value returned within
/chosen, other times it uses the device alias, and other times it uses the
value within grub.cfg. It does not have a way to figure out that these devices
are the same disk. This is not a problem with this new driver.
Due to the way GRUB repeatedly opens and closes the same disk. Caching the
ihandle is important on SPARC. Without caching, some SAS devices can take
15 - 20 minutes to get to the GRUB menu. This ihandle caching is not possible
without correctly having the canonical disk name.
When available, this driver also tries to use the deblocker #blocks and
a way of determining the disk size.
Finally and probably most importantly, this new driver is also capable of
seeing all partitions on a GPT disk. With the old driver, the GPT
partition table can not be read and only the first partition on the disk
can be seen.
---
- Addressed various coding style changes requested by Daniel Kipper
---
grub-core/Makefile.core.def | 1 +
grub-core/commands/nativedisk.c | 1 +
grub-core/disk/ieee1275/obdisk.c | 1106 ++++++++++++++++++++++++++++++++++++++
grub-core/kern/ieee1275/cmain.c | 3 +
grub-core/kern/ieee1275/init.c | 12 +-
include/grub/disk.h | 1 +
include/grub/ieee1275/ieee1275.h | 2 +
include/grub/ieee1275/obdisk.h | 25 +
8 files changed, 1150 insertions(+), 1 deletions(-)
create mode 100644 grub-core/disk/ieee1275/obdisk.c
create mode 100644 include/grub/ieee1275/obdisk.h
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index fc4767f..ab84aa4 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -292,6 +292,7 @@ kernel = {
sparc64_ieee1275 = kern/sparc64/cache.S;
sparc64_ieee1275 = kern/sparc64/dl.c;
sparc64_ieee1275 = kern/sparc64/ieee1275/ieee1275.c;
+ sparc64_ieee1275 = disk/ieee1275/obdisk.c;
arm = kern/arm/dl.c;
arm = kern/arm/dl_helper.c;
diff --git a/grub-core/commands/nativedisk.c b/grub-core/commands/nativedisk.c
index 2f56a87..2f2315d 100644
--- a/grub-core/commands/nativedisk.c
+++ b/grub-core/commands/nativedisk.c
@@ -66,6 +66,7 @@ get_uuid (const char *name, char **uuid, int getnative)
/* Firmware disks. */
diff --git a/grub-core/disk/ieee1275/obdisk.c b/grub-core/disk/ieee1275/obdisk.c
new file mode 100644
index 0000000..0bc37e6
--- /dev/null
+++ b/grub-core/disk/ieee1275/obdisk.c
@@ -0,0 +1,1106 @@
+/* obdisk.c - Open Boot disk access. */
+/*
+ * GRUB -- GRand Unified Bootloader
+ * Copyright (C) 2018 Free Software Foundation, Inc.
+ *
+ * GRUB is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * GRUB is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with GRUB. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/disk.h>
+#include <grub/env.h>
+#include <grub/i18n.h>
+#include <grub/kernel.h>
+#include <grub/list.h>
+#include <grub/misc.h>
+#include <grub/mm.h>
+#include <grub/scsicmd.h>
+#include <grub/time.h>
+#include <grub/ieee1275/ieee1275.h>
+#include <grub/ieee1275/obdisk.h>
+
+#define IEEE1275_DEV "ieee1275/"
+
+struct disk_dev
+{
+ struct disk_dev *next;
+ struct disk_dev **prev;
+ char *name;
+ char *raw_name;
+ char *grub_devpath;
+ char *grub_alias_devpath;
+ char *grub_decoded_devpath;
+ grub_ieee1275_ihandle_t ihandle;
+ grub_uint32_t block_size;
+ grub_uint64_t num_blocks;
+ unsigned int log_sector_size;
+ grub_uint32_t opened;
+ grub_uint32_t valid;
+ grub_uint32_t boot_dev;
+};
+
+struct parent_dev
+{
+ struct parent_dev *next;
+ struct parent_dev **prev;
+ char *name;
+ char *type;
+ grub_ieee1275_ihandle_t ihandle;
+ grub_uint32_t address_cells;
+};
+
+static struct grub_scsi_test_unit_ready tur =
+{
+ .opcode = grub_scsi_cmd_test_unit_ready,
+ .lun = 0,
+ .reserved1 = 0,
+ .reserved2 = 0,
+ .reserved3 = 0,
+ .control = 0,
+};
+
+static int disks_enumerated = 0;
+static struct disk_dev *disk_devs = NULL;
+static struct parent_dev *parent_devs = NULL;
I would drop all these 0/NULL initializations.
Compiler will do work for you. I asked about
that in earlier comments.
Post by Eric Snowberg
+static const char *block_blacklist[] = {
+ /* Requires addition work in grub before being able to be used. */
s/addition/additional/?
Post by Eric Snowberg
+ "/iscsi-hba",
+ /* This block device should never be used by grub. */
+ 0
+};
+
+#define STRCMP(a, b) ((a) && (b) && (grub_strcmp (a, b) == 0))
+
+static char *
+strip_ob_partition (char *path)
+{
+ char *sptr;
+
+ sptr = grub_strstr (path, ":");
+
+ if (sptr)
I saw that you have decided to use "src == NULL". Great! So, I would
expect that in cases like that you use "sptr != NULL". Could you fix
that here and below. Same applies to 0 comparison.
Post by Eric Snowberg
+ *sptr = '\0';
+
+ return path;
+}
+
+static char *
+replace_escaped_commas (char *src)
+{
+ char *iptr;
+
+ if (src == NULL)
+ return NULL;
+ for (iptr = src; *iptr; )
+ {
+ if ((*iptr == '\\') && (*(iptr + 1) == ','))
+ {
+ *iptr++ = '_';
+ *iptr++ = '_';
+ }
+ iptr++;
+ }
+
+ return src;
+}
+
+static int
+count_commas (const char *src)
+{
+ int count = 0;
+
+ for ( ; *src; src++)
+ if (*src == ',')
+ count++;
+
+ return count;
+}
+
+static void
+escape_commas (const char *src, char *dest)
I am confused by this play with commas. Could explain somewhere
where this commas are needed unescaped, escaped, replaced, etc.
Could not we simplify this somehow?

If all of this functions are really needed I would put them in
that order in the file: escape_commas(), replace_escaped_commas(),
and finally count_commas().
Post by Eric Snowberg
+{
+ const char *iptr;
+
+ for (iptr = src; *iptr; )
+ {
+ if (*iptr == ',')
+ *dest++ ='\\';
+
+ *dest++ = *iptr++;
+ }
+
+ *dest = '\0';
+}
+
+static char *
+decode_grub_devname (const char *name)
+{
+ char *devpath = grub_malloc (grub_strlen (name) + 1);
+ char *p, c;
+
+ if (devpath == NULL)
+ return NULL;
+
+ /* Un-escape commas. */
Ugh... Another play with commas...
Post by Eric Snowberg
+ p = devpath;
+ while ((c = *name++) != '\0')
+ {
+ if (c == '\\' && *name == ',')
+ {
+ *p++ = ',';
+ name++;
+ }
+ else
+ *p++ = c;
+ }
+
+ *p++ = '\0';
+
+ return devpath;
+}
+
+static char *
+encode_grub_devname (const char *path)
+{
+ char *encoding, *optr;
+
+ if (path == NULL)
+ return NULL;
+
+ encoding = grub_malloc (sizeof (IEEE1275_DEV) + count_commas (path) +
+ grub_strlen (path) + 1);
+
+ if (encoding == NULL)
+ {
+ grub_print_error ();
+ return NULL;
+ }
+
+ optr = grub_stpcpy (encoding, IEEE1275_DEV);
+ escape_commas (path, optr);
+ return encoding;
+}
+
+static char *
+get_parent_devname (const char *devname)
+{
+ char *parent, *pptr;
+
+ parent = grub_strdup (devname);
+
+ if (parent == NULL)
+ {
+ grub_print_error ();
+ return NULL;
+ }
+
+ pptr = grub_strstr (parent, IEEE1275_DISK_ALIAS);
+
+ if (pptr)
+ *pptr = '\0';
+
+ return parent;
+}
+
+static void
+free_parent_dev (struct parent_dev *parent)
+{
+ if (parent)
+ {
+ grub_free (parent->name);
+ grub_free (parent->type);
+ grub_free (parent);
+ }
+}
+
+static struct parent_dev *
+init_parent (const char *parent)
+{
+ struct parent_dev *op;
+
+ op = grub_zalloc (sizeof (struct parent_dev));
+
+ if (op == NULL)
+ {
+ grub_print_error ();
+ return NULL;
+ }
+
+ op->name = grub_strdup (parent);
+ op->type = grub_malloc (IEEE1275_MAX_PROP_LEN);
+
+ if ((op->name == NULL) || (op->type == NULL))
+ {
+ grub_print_error ();
+ free_parent_dev (op);
+ return NULL;
+ }
+
+ return op;
+}
+
+static struct parent_dev *
+open_new_parent (const char *parent)
+{
+ struct parent_dev *op = init_parent(parent);
+ grub_ieee1275_ihandle_t ihandle;
+ grub_ieee1275_phandle_t phandle;
+ grub_uint32_t address_cells = 2;
+ grub_ssize_t actual = 0;
+
+ if (op == NULL)
+ return NULL;
+
+ grub_ieee1275_open (parent, &ihandle);
+
+ if (ihandle == 0)
+ {
+ grub_error (GRUB_ERR_BAD_DEVICE, "unable to open %s", parent);
+ grub_print_error ();
+ free_parent_dev (op);
+ return NULL;
+ }
+
+ if (grub_ieee1275_instance_to_package (ihandle, &phandle))
+ {
+ grub_error (GRUB_ERR_BAD_DEVICE, "unable to get parent %s", parent);
+ grub_print_error ();
+ free_parent_dev (op);
+ return NULL;
+ }
+
+ /*
+ * IEEE Std 1275-1994 page 110: A missing "address-cells" property
+ * signifies that the number of address cells is two. So ignore on error.
+ */
+ grub_ieee1275_get_integer_property (phandle, "#address-cells", &address_cells,
+ sizeof (address_cells), 0);
I have a feeling that you assume that grub_ieee1275_get_integer_property()
does not touch address_cells if it fails. I think that it is dangerous. Hence,
I would check for error and if it happens then assign 2 to address_cells.
Post by Eric Snowberg
+ grub_ieee1275_get_property (phandle, "device_type", op->type,
+ IEEE1275_MAX_PROP_LEN, &actual);
s/&actual/NULL/?
Post by Eric Snowberg
+ op->ihandle = ihandle;
+ op->address_cells = address_cells;
+ return op;
+}
+
+static struct parent_dev *
+open_parent (const char *parent)
+{
+ struct parent_dev *op;
+
+ op = grub_named_list_find (GRUB_AS_NAMED_LIST (parent_devs), parent);
+
+ if (op == NULL)
+ {
+ op = open_new_parent (parent);
+
+ if (op)
+ grub_list_push (GRUB_AS_LIST_P (&parent_devs), GRUB_AS_LIST (op));
+ }
+
+ return op;
+}
+
+static void
+display_parents (void)
+{
+ struct parent_dev *parent;
+
+ grub_printf ("-------------------- PARENTS --------------------\n");
+
+ FOR_LIST_ELEMENTS (parent, parent_devs)
+ {
+ grub_printf ("name: %s\n", parent->name);
+ grub_printf ("type: %s\n", parent->type);
+ grub_printf ("address_cells %x\n", parent->address_cells);
+ }
+
+ grub_printf ("-------------------------------------------------\n");
+}
+
+static char *
+canonicalise_4cell_ua (grub_ieee1275_ihandle_t ihandle, char *unit_address)
+{
+ grub_uint32_t phy_lo, phy_hi, lun_lo, lun_hi;
+ int valid_phy = 0;
+ grub_size_t size;
+ char *canon = NULL;
+
+ valid_phy = grub_ieee1275_decode_unit4 (ihandle, unit_address,
+ grub_strlen (unit_address), &phy_lo,
+ &phy_hi, &lun_lo, &lun_hi);
+
+ if ((valid_phy == 0) && (phy_hi != 0xffffffff))
+ canon = grub_ieee1275_encode_uint4 (ihandle, phy_lo, phy_hi,
+ lun_lo, lun_hi, &size);
+
+ return canon;
+}
+
+static char *
+canonicalise_disk (const char *devname)
+{
+ char *canon, *parent;
+ struct parent_dev *op;
+
+ canon = grub_ieee1275_canonicalise_devname (devname);
+
+ if (canon == NULL)
+ {
+ /* This should not happen. */
+ grub_error (GRUB_ERR_BAD_DEVICE, "canonicalise devname failed");
+ grub_print_error ();
+ return NULL;
+ }
+
+ /* Don't try to open the parent of a virtual device. */
+ if (grub_strstr (canon, "virtual-devices"))
+ return canon;
+
+ parent = get_parent_devname (canon);
+
+ if (parent == NULL)
+ return NULL;
+
+ op = open_parent (parent);
+
+ /*
+ * Devices with 4 address cells can have many different types of addressing
+ * (phy, wwn, and target lun). Use the parents encode-unit / decode-unit
+ * to find the true canonical name.
+ */
+ if ((op) && (op->address_cells == 4))
+ {
+ char *unit_address, *real_unit_address, *real_canon;
+ grub_size_t real_unit_str_len;
+
+ unit_address = grub_strstr (canon, IEEE1275_DISK_ALIAS);
+ unit_address += grub_strlen (IEEE1275_DISK_ALIAS);
+
+ if (unit_address == NULL)
+ {
+ /*
+ * This should not be possible, but return the canonical name for
+ * the non-disk block device.
+ */
+ grub_free (parent);
+ return (canon);
+ }
+
+ real_unit_address = canonicalise_4cell_ua (op->ihandle, unit_address);
+
+ if (real_unit_address == NULL)
+ {
+ /*
+ * This is not an error, since this function could be called with a devalias
+ * containing a drive that isn't installed in the system.
+ */
+ grub_free (parent);
+ return NULL;
+ }
+
+ real_unit_str_len = grub_strlen (op->name) + sizeof (IEEE1275_DISK_ALIAS)
+ + grub_strlen (real_unit_address);
+
+ real_canon = grub_malloc (real_unit_str_len);
+
+ op->name, real_unit_address);
+
+ grub_free (canon);
+ canon = real_canon;
+ }
+
+ grub_free (parent);
+ return (canon);
+}
+
+static struct disk_dev *
+add_canon_disk (const char *cname)
+{
+ struct disk_dev *dev;
+
+ dev = grub_zalloc (sizeof (struct disk_dev));
+
+ if (dev == NULL)
+ goto failed;
+
+ if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_RAW_DEVNAMES))
+ {
+ /*
+ * Append :nolabel to the end of all SPARC disks.
+ * nolabel is mutually exclusive with all other
+ * arguments and allows a client program to open
+ * the entire (raw) disk. Any disk label is ignored.
+ */
+ dev->raw_name = grub_malloc (grub_strlen (cname) + sizeof (":nolabel"));
+
+ if (dev->raw_name == NULL)
+ goto failed;
+
+ grub_snprintf (dev->raw_name, grub_strlen (cname) + sizeof (":nolabel"),
+ "%s:nolabel", cname);
+ }
+
+ /*
+ * Don't use grub_ieee1275_encode_devname here, the devpath in grub.cfg doesn't
+ * understand device aliases, which the layer above sometimes sends us.
+ */
+ dev->grub_devpath = encode_grub_devname(cname);
+
+ if (dev->grub_devpath == NULL)
+ goto failed;
+
+ dev->name = grub_strdup (cname);
+
+ if (dev->name == NULL)
+ goto failed;
+
+ dev->valid = 1;
+ grub_list_push (GRUB_AS_LIST_P (&disk_devs), GRUB_AS_LIST (dev));
+ return dev;
+
+ grub_print_error ();
+
+ if (dev)
+ {
+ grub_free (dev->name);
+ grub_free (dev->grub_devpath);
+ grub_free (dev->raw_name);
+ }
+
+ grub_free (dev);
+ return NULL;
+}
+
+static grub_err_t
+add_disk (const char *path)
+{
+ grub_err_t ret = GRUB_ERR_NONE;
+ struct disk_dev *dev;
+ char *canon;
+
+ canon = canonicalise_disk (path);
+ dev = grub_named_list_find (GRUB_AS_NAMED_LIST (disk_devs), canon);
+
+ if ((canon != NULL) && (dev == NULL))
+ {
+ struct disk_dev *ob_device;
+
+ ob_device = add_canon_disk (canon);
+
+ if (ob_device == NULL)
+ ret = grub_error (GRUB_ERR_OUT_OF_MEMORY, "failure to add disk");
+ }
+ else if (dev)
+ dev->valid = 1;
What will happen if canon == NULL?
Post by Eric Snowberg
+ grub_free (canon);
+ return (ret);
+}
+
+static grub_err_t
+grub_obdisk_read (grub_disk_t disk, grub_disk_addr_t sector,
+ grub_size_t size, char *dest)
+{
+ grub_err_t ret = GRUB_ERR_NONE;
+ struct disk_dev *dev;
+ unsigned long long pos;
+ grub_ssize_t result = 0;
+
+ if (disk->data == NULL)
+ return grub_error (GRUB_ERR_BAD_DEVICE, "invalid disk data");
+
+ dev = (struct disk_dev *)disk->data;
+ pos = sector << disk->log_sector_size;
+ grub_ieee1275_seek (dev->ihandle, pos, &result);
+
+ if (result < 0)
+ {
+ dev->opened = 0;
+ return grub_error (GRUB_ERR_READ_ERROR, "seek error, can't seek block %llu",
+ (long long) sector);
+ }
+
+ grub_ieee1275_read (dev->ihandle, dest, size << disk->log_sector_size,
+ &result);
+
+ if (result != (grub_ssize_t) (size << disk->log_sector_size))
+ {
+ dev->opened = 0;
+ return grub_error (GRUB_ERR_READ_ERROR, N_("failure reading sector 0x%llx "
+ "from `%s'"),
+ (unsigned long long) sector,
+ disk->name);
+ }
+ return ret;
+}
+
+static void
+grub_obdisk_close (grub_disk_t disk)
s/grub_obdisk_close/grub_obdisk_clear/?
Post by Eric Snowberg
+{
+ grub_memset (disk, 0, sizeof (*disk));
+}
+
+static void
+scan_usb_disk (const char *parent)
+{
+ struct parent_dev *op;
+ grub_ssize_t result;
+
+ op = open_parent (parent);
+
+ if (op == NULL)
+ {
+ grub_error (GRUB_ERR_BAD_DEVICE, "unable to open %s", parent);
+ grub_print_error ();
+ return;
+ }
+
+ if ((grub_ieee1275_set_address (op->ihandle, 0, 0) == 0) &&
+ (grub_ieee1275_no_data_command (op->ihandle, &tur, &result) == 0) &&
+ (result == 0))
+ {
+ char *buf;
+
+ buf = grub_malloc (IEEE1275_MAX_PATH_LEN);
+
+ if (buf == NULL)
+ {
+ grub_error (GRUB_ERR_OUT_OF_MEMORY, "disk scan failure");
+ grub_print_error ();
+ return;
+ }
+
+ add_disk (buf);
+ grub_free (buf);
+ }
+}
+
+static void
+scan_nvme_disk (const char *path)
+{
+ char *buf;
+
+ buf = grub_malloc (IEEE1275_MAX_PATH_LEN);
+
+ if (buf == NULL)
+ {
+ grub_error (GRUB_ERR_OUT_OF_MEMORY, "disk scan failure");
+ grub_print_error ();
+ return;
+ }
+
+ add_disk (buf);
+ grub_free (buf);
+}
+
+static void
+scan_sparc_sas_2cell (struct parent_dev *op)
+{
+ grub_ssize_t result;
+ grub_uint8_t tgt;
+ char *buf;
+
+ buf = grub_malloc (IEEE1275_MAX_PATH_LEN);
+
+ if (buf == NULL)
+ {
+ grub_error (GRUB_ERR_OUT_OF_MEMORY, "disk scan failure");
+ grub_print_error ();
+ return;
+ }
+
+ for (tgt = 0; tgt < 0xf; tgt++)
+ {
+
+ if ((grub_ieee1275_set_address(op->ihandle, tgt, 0) == 0) &&
+ (grub_ieee1275_no_data_command (op->ihandle, &tur, &result) == 0) &&
+ (result == 0))
+ {
+
+ PRIxGRUB_UINT32_T, op->name, tgt);
+
+ add_disk (buf);
+ }
+ }
+}
+
+static void
+scan_sparc_sas_4cell (struct parent_dev *op)
+{
+ grub_uint16_t exp;
+ grub_uint8_t phy;
+ char *buf;
+
+ buf = grub_malloc (IEEE1275_MAX_PATH_LEN);
+
+ if (buf == NULL)
+ {
+ grub_error (GRUB_ERR_OUT_OF_MEMORY, "disk scan failure");
+ grub_print_error ();
+ return;
+ }
+
+ for (exp = 0; exp <= 0x100; exp+=0x100)
What is exp? And why exp <= 0x100; exp+=0x100? Could you add
a comment here or use constants?
Post by Eric Snowberg
+
Could you drop this empty line?
Post by Eric Snowberg
+ for (phy = 0; phy < 0x20; phy++)
Why 0x20? Constant? Or comment at least?
Post by Eric Snowberg
+ {
+ char *canon = NULL;
+
+ grub_snprintf (buf, IEEE1275_MAX_PATH_LEN, "p%" PRIxGRUB_UINT32_T ",0",
+ exp | phy);
+
+ canon = canonicalise_4cell_ua (op->ihandle, buf);
+
+ if (canon)
+ {
+ op->name, canon);
+
+ add_disk (buf);
+ grub_free (canon);
+ }
+ }
+
+ grub_free (buf);
+}
+
+static void
+scan_sparc_sas_disk (const char *parent)
+{
+ struct parent_dev *op;
+
+ op = open_parent (parent);
+
+ if ((op) && (op->address_cells == 4))
+ scan_sparc_sas_4cell (op);
+ else if ((op) && (op->address_cells == 2))
+ scan_sparc_sas_2cell (op);
+}
+
+static void
+iterate_devtree (const struct grub_ieee1275_devalias *alias)
+{
+ struct grub_ieee1275_devalias child;
+
+ if ((grub_strcmp (alias->type, "scsi-2") == 0) ||
+ (grub_strcmp (alias->type, "scsi-sas") == 0))
+ return scan_sparc_sas_disk (alias->path);
+
+ else if (grub_strcmp (alias->type, "nvme") == 0)
+ return scan_nvme_disk (alias->path);
+
+ else if (grub_strcmp (alias->type, "scsi-usb") == 0)
+ return scan_usb_disk (alias->path);
+
+ else if (grub_strcmp (alias->type, "block") == 0)
+ {
+ const char **bl = block_blacklist;
+
+ while (*bl != NULL)
+ {
+ if (grub_strstr (alias->path, *bl))
+ return;
+ bl++;
+ }
+
+ add_disk (alias->path);
+ return;
+ }
+
+ FOR_IEEE1275_DEVCHILDREN (alias->path, child)
+ iterate_devtree (&child);
+}
+
+static void
+unescape_devices (void)
Why?

In general I am happy with the changes. However, some
my comments for earlier version are still not addressed.
Please take a look at it and incorporate them. If you do
not agree with something just drop me a line.

Daniel
John Paul Adrian Glaubitz
2018-06-15 12:05:32 UTC
Permalink
Post by Daniel Kiper
Post by Eric Snowberg
+static int disks_enumerated = 0;
+static struct disk_dev *disk_devs = NULL;
+static struct parent_dev *parent_devs = NULL;
I would drop all these 0/NULL initializations.
Compiler will do work for you. I asked about
that in earlier comments.
Not sure about that though. I seem to remember that newer gcc versions
were quite nitpicky about that when building with -Werror and I had
to add a NULL initialization to get it to build.

Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer - ***@debian.org
`. `' Freie Universitaet Berlin - ***@physik.fu-berlin.de
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
Daniel Kiper
2018-06-15 12:14:36 UTC
Permalink
Post by John Paul Adrian Glaubitz
Post by Daniel Kiper
Post by Eric Snowberg
+static int disks_enumerated = 0;
+static struct disk_dev *disk_devs = NULL;
+static struct parent_dev *parent_devs = NULL;
I would drop all these 0/NULL initializations.
Compiler will do work for you. I asked about
that in earlier comments.
Not sure about that though. I seem to remember that newer gcc versions
were quite nitpicky about that when building with -Werror and I had
to add a NULL initialization to get it to build.
If this is true I am not going to insist. Sadly I have asked about
that in the earlier comments and I have not seen any reply WRT nor
changes to the code. So, that is why I am repeating my question.

Daniel
Eric Snowberg
2018-06-15 15:58:56 UTC
Permalink
Post by Daniel Kiper
Post by Eric Snowberg
Add a new disk driver called obdisk for IEEE1275 platforms. Currently
the only platform using this disk driver is SPARC, however other IEEE1275
platforms could start using it if they so choose. While the functionality
within the current IEEE1275 ofdisk driver may be suitable for PPC and x86, it
presented too many problems on SPARC hardware.
Within the old ofdisk, there is not a way to determine the true canonical
name for the disk. Within Open Boot, the same disk can have multiple names
but all reference the same disk. For example the same disk can be referenced
Also, when the LUN=0, it is legal to omit the ,0 from the device name. So with
the disk above, before taking into account the device aliases, there are 6 ways
to reference the same disk.
Then it is possible to have 0 .. n device aliases all representing the same disk.
Within this new driver the true canonical name is determined using the the
IEEE1275 encode-unit and decode-unit commands when address_cells == 4. This
will determine the true single canonical name for the device so multiple ihandles
are not opened for the same device. This is what frequently happens with the old
ofdisk driver. With some devices when they are opened multiple times it causes
the entire system to hang.
Another problem solved with this driver is devices that do not have a device
alias can be booted and used within GRUB. Within the old ofdisk, this was not
possible, unless it was the original boot device. All devices behind a SAS
or SCSI parent can be found. Within the old ofdisk, finding these disks
relied on there being an alias defined. The alias requirement is not
necessary with this new driver. It can also find devices behind a parent
after they have been hot-plugged. This is something that is not possible
with the old ofdisk driver.
The old ofdisk driver also incorrectly assumes that the device pointing to by a
device alias is in its true canonical form. This assumption is never made with
this new driver.
Another issue solved with this driver is that it properly caches the ihandle
for all open devices. The old ofdisk tries to do this by caching the last
opened ihandle. However this does not work properly because the layer above
does not use a consistent device name for the same disk when calling into the
driver. This is because the upper layer uses the bootpath value returned within
/chosen, other times it uses the device alias, and other times it uses the
value within grub.cfg. It does not have a way to figure out that these devices
are the same disk. This is not a problem with this new driver.
Due to the way GRUB repeatedly opens and closes the same disk. Caching the
ihandle is important on SPARC. Without caching, some SAS devices can take
15 - 20 minutes to get to the GRUB menu. This ihandle caching is not possible
without correctly having the canonical disk name.
When available, this driver also tries to use the deblocker #blocks and
a way of determining the disk size.
Finally and probably most importantly, this new driver is also capable of
seeing all partitions on a GPT disk. With the old driver, the GPT
partition table can not be read and only the first partition on the disk
can be seen.
---
- Addressed various coding style changes requested by Daniel Kipper
---
grub-core/Makefile.core.def | 1 +
grub-core/commands/nativedisk.c | 1 +
grub-core/disk/ieee1275/obdisk.c | 1106 ++++++++++++++++++++++++++++++++++++++
grub-core/kern/ieee1275/cmain.c | 3 +
grub-core/kern/ieee1275/init.c | 12 +-
include/grub/disk.h | 1 +
include/grub/ieee1275/ieee1275.h | 2 +
include/grub/ieee1275/obdisk.h | 25 +
8 files changed, 1150 insertions(+), 1 deletions(-)
create mode 100644 grub-core/disk/ieee1275/obdisk.c
create mode 100644 include/grub/ieee1275/obdisk.h
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index fc4767f..ab84aa4 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -292,6 +292,7 @@ kernel = {
sparc64_ieee1275 = kern/sparc64/cache.S;
sparc64_ieee1275 = kern/sparc64/dl.c;
sparc64_ieee1275 = kern/sparc64/ieee1275/ieee1275.c;
+ sparc64_ieee1275 = disk/ieee1275/obdisk.c;
arm = kern/arm/dl.c;
arm = kern/arm/dl_helper.c;
diff --git a/grub-core/commands/nativedisk.c b/grub-core/commands/nativedisk.c
index 2f56a87..2f2315d 100644
--- a/grub-core/commands/nativedisk.c
+++ b/grub-core/commands/nativedisk.c
@@ -66,6 +66,7 @@ get_uuid (const char *name, char **uuid, int getnative)
/* Firmware disks. */
diff --git a/grub-core/disk/ieee1275/obdisk.c b/grub-core/disk/ieee1275/obdisk.c
new file mode 100644
index 0000000..0bc37e6
--- /dev/null
+++ b/grub-core/disk/ieee1275/obdisk.c
@@ -0,0 +1,1106 @@
+/* obdisk.c - Open Boot disk access. */
+/*
+ * GRUB -- GRand Unified Bootloader
+ * Copyright (C) 2018 Free Software Foundation, Inc.
+ *
+ * GRUB is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * GRUB is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with GRUB. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/disk.h>
+#include <grub/env.h>
+#include <grub/i18n.h>
+#include <grub/kernel.h>
+#include <grub/list.h>
+#include <grub/misc.h>
+#include <grub/mm.h>
+#include <grub/scsicmd.h>
+#include <grub/time.h>
+#include <grub/ieee1275/ieee1275.h>
+#include <grub/ieee1275/obdisk.h>
+
+#define IEEE1275_DEV "ieee1275/"
+
+struct disk_dev
+{
+ struct disk_dev *next;
+ struct disk_dev **prev;
+ char *name;
+ char *raw_name;
+ char *grub_devpath;
+ char *grub_alias_devpath;
+ char *grub_decoded_devpath;
+ grub_ieee1275_ihandle_t ihandle;
+ grub_uint32_t block_size;
+ grub_uint64_t num_blocks;
+ unsigned int log_sector_size;
+ grub_uint32_t opened;
+ grub_uint32_t valid;
+ grub_uint32_t boot_dev;
+};
+
+struct parent_dev
+{
+ struct parent_dev *next;
+ struct parent_dev **prev;
+ char *name;
+ char *type;
+ grub_ieee1275_ihandle_t ihandle;
+ grub_uint32_t address_cells;
+};
+
+static struct grub_scsi_test_unit_ready tur =
+{
+ .opcode = grub_scsi_cmd_test_unit_ready,
+ .lun = 0,
+ .reserved1 = 0,
+ .reserved2 = 0,
+ .reserved3 = 0,
+ .control = 0,
+};
+
+static int disks_enumerated = 0;
+static struct disk_dev *disk_devs = NULL;
+static struct parent_dev *parent_devs = NULL;
I would drop all these 0/NULL initializations.
Compiler will do work for you. I asked about
that in earlier comments.
I thought I changed everything I could without getting the warning Adrian found. I’ll try to drop these.
Post by Daniel Kiper
Post by Eric Snowberg
+static const char *block_blacklist[] = {
+ /* Requires addition work in grub before being able to be used. */
s/addition/additional/?
ok
Post by Daniel Kiper
Post by Eric Snowberg
+ "/iscsi-hba",
+ /* This block device should never be used by grub. */
+ 0
+};
+
+#define STRCMP(a, b) ((a) && (b) && (grub_strcmp (a, b) == 0))
+
+static char *
+strip_ob_partition (char *path)
+{
+ char *sptr;
+
+ sptr = grub_strstr (path, ":");
+
+ if (sptr)
I saw that you have decided to use "src == NULL". Great! So, I would
expect that in cases like that you use "sptr != NULL". Could you fix
that here and below. Same applies to 0 comparison.
I missed that one, I’ll change it.
Post by Daniel Kiper
Post by Eric Snowberg
+ *sptr = '\0';
+
+ return path;
+}
+
+static char *
+replace_escaped_commas (char *src)
+{
+ char *iptr;
+
+ if (src == NULL)
+ return NULL;
+ for (iptr = src; *iptr; )
+ {
+ if ((*iptr == '\\') && (*(iptr + 1) == ','))
+ {
+ *iptr++ = '_';
+ *iptr++ = '_';
+ }
+ iptr++;
+ }
+
+ return src;
+}
+
+static int
+count_commas (const char *src)
+{
+ int count = 0;
+
+ for ( ; *src; src++)
+ if (*src == ',')
+ count++;
+
+ return count;
+}
+
+static void
+escape_commas (const char *src, char *dest)
I am confused by this play with commas. Could explain somewhere
where this commas are needed unescaped, escaped, replaced, etc.
Could not we simplify this somehow?
I’m open for recommendations.
Post by Daniel Kiper
If all of this functions are really needed I would put them in
that order in the file: escape_commas(), replace_escaped_commas(),
and finally count_commas().
Ok, I’ll change the order in the file.
Post by Daniel Kiper
Post by Eric Snowberg
+{
+ const char *iptr;
+
+ for (iptr = src; *iptr; )
+ {
+ if (*iptr == ',')
+ *dest++ ='\\';
+
+ *dest++ = *iptr++;
+ }
+
+ *dest = '\0';
+}
+
+static char *
+decode_grub_devname (const char *name)
+{
+ char *devpath = grub_malloc (grub_strlen (name) + 1);
+ char *p, c;
+
+ if (devpath == NULL)
+ return NULL;
+
+ /* Un-escape commas. */
Ugh... Another play with commas...
Post by Eric Snowberg
+ p = devpath;
+ while ((c = *name++) != '\0')
+ {
+ if (c == '\\' && *name == ',')
+ {
+ *p++ = ',';
+ name++;
+ }
+ else
+ *p++ = c;
+ }
+
+ *p++ = '\0';
+
+ return devpath;
+}
+
+static char *
+encode_grub_devname (const char *path)
+{
+ char *encoding, *optr;
+
+ if (path == NULL)
+ return NULL;
+
+ encoding = grub_malloc (sizeof (IEEE1275_DEV) + count_commas (path) +
+ grub_strlen (path) + 1);
+
+ if (encoding == NULL)
+ {
+ grub_print_error ();
+ return NULL;
+ }
+
+ optr = grub_stpcpy (encoding, IEEE1275_DEV);
+ escape_commas (path, optr);
+ return encoding;
+}
+
+static char *
+get_parent_devname (const char *devname)
+{
+ char *parent, *pptr;
+
+ parent = grub_strdup (devname);
+
+ if (parent == NULL)
+ {
+ grub_print_error ();
+ return NULL;
+ }
+
+ pptr = grub_strstr (parent, IEEE1275_DISK_ALIAS);
+
+ if (pptr)
+ *pptr = '\0';
+
+ return parent;
+}
+
+static void
+free_parent_dev (struct parent_dev *parent)
+{
+ if (parent)
+ {
+ grub_free (parent->name);
+ grub_free (parent->type);
+ grub_free (parent);
+ }
+}
+
+static struct parent_dev *
+init_parent (const char *parent)
+{
+ struct parent_dev *op;
+
+ op = grub_zalloc (sizeof (struct parent_dev));
+
+ if (op == NULL)
+ {
+ grub_print_error ();
+ return NULL;
+ }
+
+ op->name = grub_strdup (parent);
+ op->type = grub_malloc (IEEE1275_MAX_PROP_LEN);
+
+ if ((op->name == NULL) || (op->type == NULL))
+ {
+ grub_print_error ();
+ free_parent_dev (op);
+ return NULL;
+ }
+
+ return op;
+}
+
+static struct parent_dev *
+open_new_parent (const char *parent)
+{
+ struct parent_dev *op = init_parent(parent);
+ grub_ieee1275_ihandle_t ihandle;
+ grub_ieee1275_phandle_t phandle;
+ grub_uint32_t address_cells = 2;
+ grub_ssize_t actual = 0;
+
+ if (op == NULL)
+ return NULL;
+
+ grub_ieee1275_open (parent, &ihandle);
+
+ if (ihandle == 0)
+ {
+ grub_error (GRUB_ERR_BAD_DEVICE, "unable to open %s", parent);
+ grub_print_error ();
+ free_parent_dev (op);
+ return NULL;
+ }
+
+ if (grub_ieee1275_instance_to_package (ihandle, &phandle))
+ {
+ grub_error (GRUB_ERR_BAD_DEVICE, "unable to get parent %s", parent);
+ grub_print_error ();
+ free_parent_dev (op);
+ return NULL;
+ }
+
+ /*
+ * IEEE Std 1275-1994 page 110: A missing "address-cells" property
+ * signifies that the number of address cells is two. So ignore on error.
+ */
+ grub_ieee1275_get_integer_property (phandle, "#address-cells", &address_cells,
+ sizeof (address_cells), 0);
I have a feeling that you assume that grub_ieee1275_get_integer_property()
does not touch address_cells if it fails. I think that it is dangerous. Hence,
I would check for error and if it happens then assign 2 to address_cells.
Ok, I’ll change this.
Post by Daniel Kiper
Post by Eric Snowberg
+ grub_ieee1275_get_property (phandle, "device_type", op->type,
+ IEEE1275_MAX_PROP_LEN, &actual);
s/&actual/NULL/?
ok
Post by Daniel Kiper
Post by Eric Snowberg
+ op->ihandle = ihandle;
+ op->address_cells = address_cells;
+ return op;
+}
+
+static struct parent_dev *
+open_parent (const char *parent)
+{
+ struct parent_dev *op;
+
+ op = grub_named_list_find (GRUB_AS_NAMED_LIST (parent_devs), parent);
+
+ if (op == NULL)
+ {
+ op = open_new_parent (parent);
+
+ if (op)
+ grub_list_push (GRUB_AS_LIST_P (&parent_devs), GRUB_AS_LIST (op));
+ }
+
+ return op;
+}
+
+static void
+display_parents (void)
+{
+ struct parent_dev *parent;
+
+ grub_printf ("-------------------- PARENTS --------------------\n");
+
+ FOR_LIST_ELEMENTS (parent, parent_devs)
+ {
+ grub_printf ("name: %s\n", parent->name);
+ grub_printf ("type: %s\n", parent->type);
+ grub_printf ("address_cells %x\n", parent->address_cells);
+ }
+
+ grub_printf ("-------------------------------------------------\n");
+}
+
+static char *
+canonicalise_4cell_ua (grub_ieee1275_ihandle_t ihandle, char *unit_address)
+{
+ grub_uint32_t phy_lo, phy_hi, lun_lo, lun_hi;
+ int valid_phy = 0;
+ grub_size_t size;
+ char *canon = NULL;
+
+ valid_phy = grub_ieee1275_decode_unit4 (ihandle, unit_address,
+ grub_strlen (unit_address), &phy_lo,
+ &phy_hi, &lun_lo, &lun_hi);
+
+ if ((valid_phy == 0) && (phy_hi != 0xffffffff))
+ canon = grub_ieee1275_encode_uint4 (ihandle, phy_lo, phy_hi,
+ lun_lo, lun_hi, &size);
+
+ return canon;
+}
+
+static char *
+canonicalise_disk (const char *devname)
+{
+ char *canon, *parent;
+ struct parent_dev *op;
+
+ canon = grub_ieee1275_canonicalise_devname (devname);
+
+ if (canon == NULL)
+ {
+ /* This should not happen. */
+ grub_error (GRUB_ERR_BAD_DEVICE, "canonicalise devname failed");
+ grub_print_error ();
+ return NULL;
+ }
+
+ /* Don't try to open the parent of a virtual device. */
+ if (grub_strstr (canon, "virtual-devices"))
+ return canon;
+
+ parent = get_parent_devname (canon);
+
+ if (parent == NULL)
+ return NULL;
+
+ op = open_parent (parent);
+
+ /*
+ * Devices with 4 address cells can have many different types of addressing
+ * (phy, wwn, and target lun). Use the parents encode-unit / decode-unit
+ * to find the true canonical name.
+ */
+ if ((op) && (op->address_cells == 4))
+ {
+ char *unit_address, *real_unit_address, *real_canon;
+ grub_size_t real_unit_str_len;
+
+ unit_address = grub_strstr (canon, IEEE1275_DISK_ALIAS);
+ unit_address += grub_strlen (IEEE1275_DISK_ALIAS);
+
+ if (unit_address == NULL)
+ {
+ /*
+ * This should not be possible, but return the canonical name for
+ * the non-disk block device.
+ */
+ grub_free (parent);
+ return (canon);
+ }
+
+ real_unit_address = canonicalise_4cell_ua (op->ihandle, unit_address);
+
+ if (real_unit_address == NULL)
+ {
+ /*
+ * This is not an error, since this function could be called with a devalias
+ * containing a drive that isn't installed in the system.
+ */
+ grub_free (parent);
+ return NULL;
+ }
+
+ real_unit_str_len = grub_strlen (op->name) + sizeof (IEEE1275_DISK_ALIAS)
+ + grub_strlen (real_unit_address);
+
+ real_canon = grub_malloc (real_unit_str_len);
+
+ op->name, real_unit_address);
+
+ grub_free (canon);
+ canon = real_canon;
+ }
+
+ grub_free (parent);
+ return (canon);
+}
+
+static struct disk_dev *
+add_canon_disk (const char *cname)
+{
+ struct disk_dev *dev;
+
+ dev = grub_zalloc (sizeof (struct disk_dev));
+
+ if (dev == NULL)
+ goto failed;
+
+ if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_RAW_DEVNAMES))
+ {
+ /*
+ * Append :nolabel to the end of all SPARC disks.
+ * nolabel is mutually exclusive with all other
+ * arguments and allows a client program to open
+ * the entire (raw) disk. Any disk label is ignored.
+ */
+ dev->raw_name = grub_malloc (grub_strlen (cname) + sizeof (":nolabel"));
+
+ if (dev->raw_name == NULL)
+ goto failed;
+
+ grub_snprintf (dev->raw_name, grub_strlen (cname) + sizeof (":nolabel"),
+ "%s:nolabel", cname);
+ }
+
+ /*
+ * Don't use grub_ieee1275_encode_devname here, the devpath in grub.cfg doesn't
+ * understand device aliases, which the layer above sometimes sends us.
+ */
+ dev->grub_devpath = encode_grub_devname(cname);
+
+ if (dev->grub_devpath == NULL)
+ goto failed;
+
+ dev->name = grub_strdup (cname);
+
+ if (dev->name == NULL)
+ goto failed;
+
+ dev->valid = 1;
+ grub_list_push (GRUB_AS_LIST_P (&disk_devs), GRUB_AS_LIST (dev));
+ return dev;
+
+ grub_print_error ();
+
+ if (dev)
+ {
+ grub_free (dev->name);
+ grub_free (dev->grub_devpath);
+ grub_free (dev->raw_name);
+ }
+
+ grub_free (dev);
+ return NULL;
+}
+
+static grub_err_t
+add_disk (const char *path)
+{
+ grub_err_t ret = GRUB_ERR_NONE;
+ struct disk_dev *dev;
+ char *canon;
+
+ canon = canonicalise_disk (path);
+ dev = grub_named_list_find (GRUB_AS_NAMED_LIST (disk_devs), canon);
+
+ if ((canon != NULL) && (dev == NULL))
+ {
+ struct disk_dev *ob_device;
+
+ ob_device = add_canon_disk (canon);
+
+ if (ob_device == NULL)
+ ret = grub_error (GRUB_ERR_OUT_OF_MEMORY, "failure to add disk");
+ }
+ else if (dev)
+ dev->valid = 1;
What will happen if canon == NULL?
dev will always be equal to NULL in that case so nothing would happen other than the error being printed. But I supposed it would be better to have a final “else" after the "else if" and set ret = grub_error with GRUB_ERR_BAD_DEVICE.
Post by Daniel Kiper
Post by Eric Snowberg
+ grub_free (canon);
+ return (ret);
+}
+
+static grub_err_t
+grub_obdisk_read (grub_disk_t disk, grub_disk_addr_t sector,
+ grub_size_t size, char *dest)
+{
+ grub_err_t ret = GRUB_ERR_NONE;
+ struct disk_dev *dev;
+ unsigned long long pos;
+ grub_ssize_t result = 0;
+
+ if (disk->data == NULL)
+ return grub_error (GRUB_ERR_BAD_DEVICE, "invalid disk data");
+
+ dev = (struct disk_dev *)disk->data;
+ pos = sector << disk->log_sector_size;
+ grub_ieee1275_seek (dev->ihandle, pos, &result);
+
+ if (result < 0)
+ {
+ dev->opened = 0;
+ return grub_error (GRUB_ERR_READ_ERROR, "seek error, can't seek block %llu",
+ (long long) sector);
+ }
+
+ grub_ieee1275_read (dev->ihandle, dest, size << disk->log_sector_size,
+ &result);
+
+ if (result != (grub_ssize_t) (size << disk->log_sector_size))
+ {
+ dev->opened = 0;
+ return grub_error (GRUB_ERR_READ_ERROR, N_("failure reading sector 0x%llx "
+ "from `%s'"),
+ (unsigned long long) sector,
+ disk->name);
+ }
+ return ret;
+}
+
+static void
+grub_obdisk_close (grub_disk_t disk)
s/grub_obdisk_close/grub_obdisk_clear/?
It really is a close as far as the grub driver is concerned (grub_disk_dev). If you insist I’ll change it, but naming it clear doesn't make sense to me.
Post by Daniel Kiper
Post by Eric Snowberg
+{
+ grub_memset (disk, 0, sizeof (*disk));
+}
+
+static void
+scan_usb_disk (const char *parent)
+{
+ struct parent_dev *op;
+ grub_ssize_t result;
+
+ op = open_parent (parent);
+
+ if (op == NULL)
+ {
+ grub_error (GRUB_ERR_BAD_DEVICE, "unable to open %s", parent);
+ grub_print_error ();
+ return;
+ }
+
+ if ((grub_ieee1275_set_address (op->ihandle, 0, 0) == 0) &&
+ (grub_ieee1275_no_data_command (op->ihandle, &tur, &result) == 0) &&
+ (result == 0))
+ {
+ char *buf;
+
+ buf = grub_malloc (IEEE1275_MAX_PATH_LEN);
+
+ if (buf == NULL)
+ {
+ grub_error (GRUB_ERR_OUT_OF_MEMORY, "disk scan failure");
+ grub_print_error ();
+ return;
+ }
+
+ add_disk (buf);
+ grub_free (buf);
+ }
+}
+
+static void
+scan_nvme_disk (const char *path)
+{
+ char *buf;
+
+ buf = grub_malloc (IEEE1275_MAX_PATH_LEN);
+
+ if (buf == NULL)
+ {
+ grub_error (GRUB_ERR_OUT_OF_MEMORY, "disk scan failure");
+ grub_print_error ();
+ return;
+ }
+
+ add_disk (buf);
+ grub_free (buf);
+}
+
+static void
+scan_sparc_sas_2cell (struct parent_dev *op)
+{
+ grub_ssize_t result;
+ grub_uint8_t tgt;
+ char *buf;
+
+ buf = grub_malloc (IEEE1275_MAX_PATH_LEN);
+
+ if (buf == NULL)
+ {
+ grub_error (GRUB_ERR_OUT_OF_MEMORY, "disk scan failure");
+ grub_print_error ();
+ return;
+ }
+
+ for (tgt = 0; tgt < 0xf; tgt++)
+ {
+
+ if ((grub_ieee1275_set_address(op->ihandle, tgt, 0) == 0) &&
+ (grub_ieee1275_no_data_command (op->ihandle, &tur, &result) == 0) &&
+ (result == 0))
+ {
+
+ PRIxGRUB_UINT32_T, op->name, tgt);
+
+ add_disk (buf);
+ }
+ }
+}
+
+static void
+scan_sparc_sas_4cell (struct parent_dev *op)
+{
+ grub_uint16_t exp;
+ grub_uint8_t phy;
+ char *buf;
+
+ buf = grub_malloc (IEEE1275_MAX_PATH_LEN);
+
+ if (buf == NULL)
+ {
+ grub_error (GRUB_ERR_OUT_OF_MEMORY, "disk scan failure");
+ grub_print_error ();
+ return;
+ }
+
+ for (exp = 0; exp <= 0x100; exp+=0x100)
What is exp?
SAS expander
Post by Daniel Kiper
And why exp <= 0x100; exp+=0x100? Could you add
a comment here or use constants?
It is for dual port SAS disks. I’ll add a comment.
Post by Daniel Kiper
Post by Eric Snowberg
+
Could you drop this empty line?
ok
Post by Daniel Kiper
Post by Eric Snowberg
+ for (phy = 0; phy < 0x20; phy++)
Why 0x20? Constant? Or comment at least?
I’ll make this a constant since I suppose the max number of SAS phys could change in the future.
Post by Daniel Kiper
Post by Eric Snowberg
+ {
+ char *canon = NULL;
+
+ grub_snprintf (buf, IEEE1275_MAX_PATH_LEN, "p%" PRIxGRUB_UINT32_T ",0",
+ exp | phy);
+
+ canon = canonicalise_4cell_ua (op->ihandle, buf);
+
+ if (canon)
+ {
+ op->name, canon);
+
+ add_disk (buf);
+ grub_free (canon);
+ }
+ }
+
+ grub_free (buf);
+}
+
+static void
+scan_sparc_sas_disk (const char *parent)
+{
+ struct parent_dev *op;
+
+ op = open_parent (parent);
+
+ if ((op) && (op->address_cells == 4))
+ scan_sparc_sas_4cell (op);
+ else if ((op) && (op->address_cells == 2))
+ scan_sparc_sas_2cell (op);
+}
+
+static void
+iterate_devtree (const struct grub_ieee1275_devalias *alias)
+{
+ struct grub_ieee1275_devalias child;
+
+ if ((grub_strcmp (alias->type, "scsi-2") == 0) ||
+ (grub_strcmp (alias->type, "scsi-sas") == 0))
+ return scan_sparc_sas_disk (alias->path);
+
+ else if (grub_strcmp (alias->type, "nvme") == 0)
+ return scan_nvme_disk (alias->path);
+
+ else if (grub_strcmp (alias->type, "scsi-usb") == 0)
+ return scan_usb_disk (alias->path);
+
+ else if (grub_strcmp (alias->type, "block") == 0)
+ {
+ const char **bl = block_blacklist;
+
+ while (*bl != NULL)
+ {
+ if (grub_strstr (alias->path, *bl))
+ return;
+ bl++;
+ }
+
+ add_disk (alias->path);
+ return;
+ }
+
+ FOR_IEEE1275_DEVCHILDREN (alias->path, child)
+ iterate_devtree (&child);
+}
+
+static void
+unescape_devices (void)
Why?
Many SPARC disks contain a comma within the name. Code from various layers above this driver handle the comma differently. At times they will strip everything to the right of the comma. The solution I came up with here is self contained and will not impact generic grub2 code. So far it seems to work from all reports I've seen.
Post by Daniel Kiper
In general I am happy with the changes. However, some
my comments for earlier version are still not addressed.
Please take a look at it and incorporate them. If you do
not agree with something just drop me a line.
Daniel
Daniel Kiper
2018-06-21 16:58:22 UTC
Permalink
[...]
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
+static struct grub_scsi_test_unit_ready tur =
+{
+ .opcode = grub_scsi_cmd_test_unit_ready,
+ .lun = 0,
+ .reserved1 = 0,
+ .reserved2 = 0,
+ .reserved3 = 0,
+ .control = 0,
+};
+
+static int disks_enumerated = 0;
+static struct disk_dev *disk_devs = NULL;
+static struct parent_dev *parent_devs = NULL;
I would drop all these 0/NULL initializations.
Compiler will do work for you. I asked about
that in earlier comments.
I thought I changed everything I could without getting the warning
Adrian found. I’ll try to drop these.
Thanks.

[...]
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
+static char *
+replace_escaped_commas (char *src)
+{
+ char *iptr;
+
+ if (src == NULL)
+ return NULL;
+ for (iptr = src; *iptr; )
+ {
+ if ((*iptr == '\\') && (*(iptr + 1) == ','))
+ {
+ *iptr++ = '_';
+ *iptr++ = '_';
+ }
+ iptr++;
+ }
+
+ return src;
+}
+
+static int
+count_commas (const char *src)
+{
+ int count = 0;
+
+ for ( ; *src; src++)
+ if (*src == ',')
+ count++;
+
+ return count;
+}
+
+static void
+escape_commas (const char *src, char *dest)
I am confused by this play with commas. Could explain somewhere
where this commas are needed unescaped, escaped, replaced, etc.
Could not we simplify this somehow?
I’m open for recommendations.
Great! However, I need more info which layer need what WRT ",",
how often this conversions must be done, why you have chosen that
solution, etc. Then I will try to optimize solution a bit.

[...]
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
+static grub_err_t
+add_disk (const char *path)
+{
+ grub_err_t ret = GRUB_ERR_NONE;
+ struct disk_dev *dev;
+ char *canon;
+
+ canon = canonicalise_disk (path);
+ dev = grub_named_list_find (GRUB_AS_NAMED_LIST (disk_devs), canon);
+
+ if ((canon != NULL) && (dev == NULL))
+ {
+ struct disk_dev *ob_device;
+
+ ob_device = add_canon_disk (canon);
+
+ if (ob_device == NULL)
+ ret = grub_error (GRUB_ERR_OUT_OF_MEMORY, "failure to add disk");
+ }
+ else if (dev)
+ dev->valid = 1;
What will happen if canon == NULL?
dev will always be equal to NULL in that case so nothing would happen
other than the error being printed. But I supposed it would be better
to have a final “else" after the "else if" and set ret = grub_error
with GRUB_ERR_BAD_DEVICE.
Please do if you can.

[...]
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
+ grub_free (canon);
+ return (ret);
+}
+
+static grub_err_t
+grub_obdisk_read (grub_disk_t disk, grub_disk_addr_t sector,
+ grub_size_t size, char *dest)
+{
+ grub_err_t ret = GRUB_ERR_NONE;
+ struct disk_dev *dev;
+ unsigned long long pos;
+ grub_ssize_t result = 0;
+
+ if (disk->data == NULL)
+ return grub_error (GRUB_ERR_BAD_DEVICE, "invalid disk data");
+
+ dev = (struct disk_dev *)disk->data;
+ pos = sector << disk->log_sector_size;
+ grub_ieee1275_seek (dev->ihandle, pos, &result);
+
+ if (result < 0)
+ {
+ dev->opened = 0;
+ return grub_error (GRUB_ERR_READ_ERROR, "seek error, can't seek block %llu",
+ (long long) sector);
+ }
+
+ grub_ieee1275_read (dev->ihandle, dest, size << disk->log_sector_size,
+ &result);
+
+ if (result != (grub_ssize_t) (size << disk->log_sector_size))
+ {
+ dev->opened = 0;
+ return grub_error (GRUB_ERR_READ_ERROR, N_("failure reading sector 0x%llx "
+ "from `%s'"),
+ (unsigned long long) sector,
+ disk->name);
+ }
+ return ret;
+}
+
+static void
+grub_obdisk_close (grub_disk_t disk)
s/grub_obdisk_close/grub_obdisk_clear/?
It really is a close as far as the grub driver is concerned
(grub_disk_dev). If you insist I’ll change it, but naming it clear
doesn't make sense to me.
If similar functions in other drivers do just memset() or so and they
are named *close* then I am not going to insist. If it is not true then
I will ask you to do that change.

[...]
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
+static void
+iterate_devtree (const struct grub_ieee1275_devalias *alias)
+{
+ struct grub_ieee1275_devalias child;
+
+ if ((grub_strcmp (alias->type, "scsi-2") == 0) ||
+ (grub_strcmp (alias->type, "scsi-sas") == 0))
+ return scan_sparc_sas_disk (alias->path);
+
+ else if (grub_strcmp (alias->type, "nvme") == 0)
+ return scan_nvme_disk (alias->path);
+
+ else if (grub_strcmp (alias->type, "scsi-usb") == 0)
+ return scan_usb_disk (alias->path);
+
+ else if (grub_strcmp (alias->type, "block") == 0)
+ {
+ const char **bl = block_blacklist;
+
+ while (*bl != NULL)
+ {
+ if (grub_strstr (alias->path, *bl))
+ return;
+ bl++;
+ }
+
+ add_disk (alias->path);
+ return;
+ }
+
+ FOR_IEEE1275_DEVCHILDREN (alias->path, child)
+ iterate_devtree (&child);
+}
+
+static void
+unescape_devices (void)
Why?
Many SPARC disks contain a comma within the name. Code from various
layers above this driver handle the comma differently. At times they
will strip everything to the right of the comma. The solution I came
up with here is self contained and will not impact generic grub2 code.
So far it seems to work from all reports I've seen.
Great! Though I have feeling that there is still room for some
optimizations. At least we should try to do it...

Daniel
Eric Snowberg
2018-06-21 19:46:46 UTC
Permalink
Post by Daniel Kiper
[...]
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
+static struct grub_scsi_test_unit_ready tur =
+{
+ .opcode = grub_scsi_cmd_test_unit_ready,
+ .lun = 0,
+ .reserved1 = 0,
+ .reserved2 = 0,
+ .reserved3 = 0,
+ .control = 0,
+};
+
+static int disks_enumerated = 0;
+static struct disk_dev *disk_devs = NULL;
+static struct parent_dev *parent_devs = NULL;
I would drop all these 0/NULL initializations.
Compiler will do work for you. I asked about
that in earlier comments.
I thought I changed everything I could without getting the warning
Adrian found. I’ll try to drop these.
Thanks.
[...]
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
+static char *
+replace_escaped_commas (char *src)
+{
+ char *iptr;
+
+ if (src == NULL)
+ return NULL;
+ for (iptr = src; *iptr; )
+ {
+ if ((*iptr == '\\') && (*(iptr + 1) == ','))
+ {
+ *iptr++ = '_';
+ *iptr++ = '_';
+ }
+ iptr++;
+ }
+
+ return src;
+}
+
+static int
+count_commas (const char *src)
+{
+ int count = 0;
+
+ for ( ; *src; src++)
+ if (*src == ',')
+ count++;
+
+ return count;
+}
+
+static void
+escape_commas (const char *src, char *dest)
I am confused by this play with commas. Could explain somewhere
where this commas are needed unescaped, escaped, replaced, etc.
Could not we simplify this somehow?
I’m open for recommendations.
Great! However, I need more info which layer need what WRT ",”,
AFAIK all layers above expect it:
https://www.gnu.org/software/grub/manual/grub/grub.html#Device-syntax

Everything above this driver expects it to be escaped. Obviously when the driver talks to the actual hardware these devices can not have the escaped names.
Post by Daniel Kiper
how often this conversions must be done, why you have chosen that
solution, etc. Then I will try to optimize solution a bit.
Under normal circumstances it only takes place once per disk as they are enumerated. All disks are cached within this driver so it should not happen often. That is why I store both versions so I don’t have to go back and forth. Look at the current driver ofdisk. It has a function called compute_dev_path which does this conversion on every single open (grub_ofdisk_open). That does not happen with this new driver. IMHO this is a much more optimized solution than the current driver.
Post by Daniel Kiper
[...]
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
+static grub_err_t
+add_disk (const char *path)
+{
+ grub_err_t ret = GRUB_ERR_NONE;
+ struct disk_dev *dev;
+ char *canon;
+
+ canon = canonicalise_disk (path);
+ dev = grub_named_list_find (GRUB_AS_NAMED_LIST (disk_devs), canon);
+
+ if ((canon != NULL) && (dev == NULL))
+ {
+ struct disk_dev *ob_device;
+
+ ob_device = add_canon_disk (canon);
+
+ if (ob_device == NULL)
+ ret = grub_error (GRUB_ERR_OUT_OF_MEMORY, "failure to add disk");
+ }
+ else if (dev)
+ dev->valid = 1;
What will happen if canon == NULL?
dev will always be equal to NULL in that case so nothing would happen
other than the error being printed. But I supposed it would be better
to have a final “else" after the "else if" and set ret = grub_error
with GRUB_ERR_BAD_DEVICE.
Please do if you can.
I’ll take care of this.
Post by Daniel Kiper
[...]
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
+ grub_free (canon);
+ return (ret);
+}
+
+static grub_err_t
+grub_obdisk_read (grub_disk_t disk, grub_disk_addr_t sector,
+ grub_size_t size, char *dest)
+{
+ grub_err_t ret = GRUB_ERR_NONE;
+ struct disk_dev *dev;
+ unsigned long long pos;
+ grub_ssize_t result = 0;
+
+ if (disk->data == NULL)
+ return grub_error (GRUB_ERR_BAD_DEVICE, "invalid disk data");
+
+ dev = (struct disk_dev *)disk->data;
+ pos = sector << disk->log_sector_size;
+ grub_ieee1275_seek (dev->ihandle, pos, &result);
+
+ if (result < 0)
+ {
+ dev->opened = 0;
+ return grub_error (GRUB_ERR_READ_ERROR, "seek error, can't seek block %llu",
+ (long long) sector);
+ }
+
+ grub_ieee1275_read (dev->ihandle, dest, size << disk->log_sector_size,
+ &result);
+
+ if (result != (grub_ssize_t) (size << disk->log_sector_size))
+ {
+ dev->opened = 0;
+ return grub_error (GRUB_ERR_READ_ERROR, N_("failure reading sector 0x%llx "
+ "from `%s'"),
+ (unsigned long long) sector,
+ disk->name);
+ }
+ return ret;
+}
+
+static void
+grub_obdisk_close (grub_disk_t disk)
s/grub_obdisk_close/grub_obdisk_clear/?
It really is a close as far as the grub driver is concerned
(grub_disk_dev). If you insist I’ll change it, but naming it clear
doesn't make sense to me.
If similar functions in other drivers do just memset() or so and they
are named *close* then I am not going to insist. If it is not true then
I will ask you to do that change.
From what I can see _close seems like the standard name here. Some drivers such as efidisk just do a grub_dprintf and nothing more within its close.
Daniel Kiper
2018-07-16 13:51:17 UTC
Permalink
Sorry for late reply but I was busy with other stuff.
[...]
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
+static char *
+replace_escaped_commas (char *src)
+{
+ char *iptr;
+
+ if (src == NULL)
+ return NULL;
+ for (iptr = src; *iptr; )
+ {
+ if ((*iptr == '\\') && (*(iptr + 1) == ','))
+ {
+ *iptr++ = '_';
+ *iptr++ = '_';
+ }
+ iptr++;
+ }
+
+ return src;
+}
+
+static int
+count_commas (const char *src)
+{
+ int count = 0;
+
+ for ( ; *src; src++)
+ if (*src == ',')
+ count++;
+
+ return count;
+}
+
+static void
+escape_commas (const char *src, char *dest)
I am confused by this play with commas. Could explain somewhere
where this commas are needed unescaped, escaped, replaced, etc.
Could not we simplify this somehow?
I’m open for recommendations.
Great! However, I need more info which layer need what WRT ",”,
https://www.gnu.org/software/grub/manual/grub/grub.html#Device-syntax
Everything above this driver expects it to be escaped. Obviously when
Do you mean from the command line? If yes could you give an example with
proper escaping?
Post by Eric Snowberg
the driver talks to the actual hardware these devices can not have the
escaped names.
OK but this is not clear. So, please add a comment explaining it in
the code somewhere.
Post by Eric Snowberg
Post by Daniel Kiper
how often this conversions must be done, why you have chosen that
solution, etc. Then I will try to optimize solution a bit.
Under normal circumstances it only takes place once per disk as they
are enumerated. All disks are cached within this driver so it should
not happen often. That is why I store both versions so I don’t have
to go back and forth. Look at the current driver ofdisk. It has a
function called compute_dev_path which does this conversion on every
single open (grub_ofdisk_open). That does not happen with this new
driver. IMHO this is a much more optimized solution than the current
driver.
Then OK. However, this have to be explained somewhere in the code.
Additionally, I think that proper variable naming would help too,
e.g. name and name_esc. And I would do this:
- s/decode_grub_devname/unescape_grub_devnam/,
- s/encode_grub_devname/escape_grub_devname/,
- extract unscaping code to the unescape_commas() function;
even if it is called once; just for the completeness.

What is replace_escaped_commas()? Why do we need that function?

[...]
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
+ grub_free (canon);
+ return (ret);
+}
+
+static grub_err_t
+grub_obdisk_read (grub_disk_t disk, grub_disk_addr_t sector,
+ grub_size_t size, char *dest)
+{
+ grub_err_t ret = GRUB_ERR_NONE;
+ struct disk_dev *dev;
+ unsigned long long pos;
+ grub_ssize_t result = 0;
+
+ if (disk->data == NULL)
+ return grub_error (GRUB_ERR_BAD_DEVICE, "invalid disk data");
+
+ dev = (struct disk_dev *)disk->data;
+ pos = sector << disk->log_sector_size;
+ grub_ieee1275_seek (dev->ihandle, pos, &result);
+
+ if (result < 0)
+ {
+ dev->opened = 0;
+ return grub_error (GRUB_ERR_READ_ERROR, "seek error, can't seek block %llu",
+ (long long) sector);
+ }
+
+ grub_ieee1275_read (dev->ihandle, dest, size << disk->log_sector_size,
+ &result);
+
+ if (result != (grub_ssize_t) (size << disk->log_sector_size))
+ {
+ dev->opened = 0;
+ return grub_error (GRUB_ERR_READ_ERROR, N_("failure reading sector 0x%llx "
+ "from `%s'"),
+ (unsigned long long) sector,
+ disk->name);
+ }
+ return ret;
+}
+
+static void
+grub_obdisk_close (grub_disk_t disk)
s/grub_obdisk_close/grub_obdisk_clear/?
It really is a close as far as the grub driver is concerned
(grub_disk_dev). If you insist I’ll change it, but naming it clear
doesn't make sense to me.
If similar functions in other drivers do just memset() or so and they
are named *close* then I am not going to insist. If it is not true then
I will ask you to do that change.
From what I can see _close seems like the standard name here. Some
drivers such as efidisk just do a grub_dprintf and nothing more within
its close.
So, lets leave it as is.

Daniel
Eric Snowberg
2018-07-16 15:33:17 UTC
Permalink
Post by Daniel Kiper
Sorry for late reply but I was busy with other stuff.
[...]
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
+static char *
+replace_escaped_commas (char *src)
+{
+ char *iptr;
+
+ if (src == NULL)
+ return NULL;
+ for (iptr = src; *iptr; )
+ {
+ if ((*iptr == '\\') && (*(iptr + 1) == ','))
+ {
+ *iptr++ = '_';
+ *iptr++ = '_';
+ }
+ iptr++;
+ }
+
+ return src;
+}
+
+static int
+count_commas (const char *src)
+{
+ int count = 0;
+
+ for ( ; *src; src++)
+ if (*src == ',')
+ count++;
+
+ return count;
+}
+
+static void
+escape_commas (const char *src, char *dest)
I am confused by this play with commas. Could explain somewhere
where this commas are needed unescaped, escaped, replaced, etc.
Could not we simplify this somehow?
I’m open for recommendations.
Great! However, I need more info which layer need what WRT ",”,
https://www.gnu.org/software/grub/manual/grub/grub.html#Device-syntax
Everything above this driver expects it to be escaped. Obviously when
Do you mean from the command line?
This goes much further than the command line. For example, it is built right into the disk driver. Look at grub-core/kern/disk.c: 544

/* Return the location of the first ',', if any, which is not
escaped by a '\'. */
static const char *
find_part_sep (const char *name)
{
const char *p = name;
char c;

while ((c = *p++) != '\0')
{
if (c == '\\' && *p == ',')
p++;
else if (c == ',')
return p - 1;
}
return NULL;
}

When the obdisk driver discovers a disk, it must put the disk name in the proper format. Otherwise when grub_disk_open takes place later on, the wrong disk name will eventually get sent back to the obdisk driver.
Post by Daniel Kiper
If yes could you give an example with
proper escaping?
Post by Eric Snowberg
the driver talks to the actual hardware these devices can not have the
escaped names.
OK but this is not clear. So, please add a comment explaining it in
the code somewhere.
Ok
Post by Daniel Kiper
Post by Eric Snowberg
Post by Daniel Kiper
how often this conversions must be done, why you have chosen that
solution, etc. Then I will try to optimize solution a bit.
Under normal circumstances it only takes place once per disk as they
are enumerated. All disks are cached within this driver so it should
not happen often. That is why I store both versions so I don’t have
to go back and forth. Look at the current driver ofdisk. It has a
function called compute_dev_path which does this conversion on every
single open (grub_ofdisk_open). That does not happen with this new
driver. IMHO this is a much more optimized solution than the current
driver.
Then OK. However, this have to be explained somewhere in the code.
Additionally, I think that proper variable naming would help too,
- s/decode_grub_devname/unescape_grub_devnam/,
- s/encode_grub_devname/escape_grub_devname/,
- extract unscaping code to the unescape_commas() function;
even if it is called once; just for the completeness.
Ok
Post by Daniel Kiper
What is replace_escaped_commas()? Why do we need that function?
It is a convenience function for the end-user, so they can access a disk without having to understand all this escaping when they want to use one thru the shell.
Daniel Kiper
2018-07-17 13:38:57 UTC
Permalink
Post by Eric Snowberg
Post by Daniel Kiper
Sorry for late reply but I was busy with other stuff.
[...]
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
+static char *
+replace_escaped_commas (char *src)
+{
+ char *iptr;
+
+ if (src == NULL)
+ return NULL;
+ for (iptr = src; *iptr; )
+ {
+ if ((*iptr == '\\') && (*(iptr + 1) == ','))
+ {
+ *iptr++ = '_';
+ *iptr++ = '_';
+ }
+ iptr++;
+ }
+
+ return src;
+}
+
+static int
+count_commas (const char *src)
+{
+ int count = 0;
+
+ for ( ; *src; src++)
+ if (*src == ',')
+ count++;
+
+ return count;
+}
+
+static void
+escape_commas (const char *src, char *dest)
I am confused by this play with commas. Could explain somewhere
where this commas are needed unescaped, escaped, replaced, etc.
Could not we simplify this somehow?
I’m open for recommendations.
Great! However, I need more info which layer need what WRT ",”,
https://www.gnu.org/software/grub/manual/grub/grub.html#Device-syntax
Everything above this driver expects it to be escaped. Obviously when
Do you mean from the command line?
This goes much further than the command line. For example, it is
built right into the disk driver. Look at grub-core/kern/disk.c: 544
This is the last line of the file. So, I am not sure what exactly you mean.
Post by Eric Snowberg
/* Return the location of the first ',', if any, which is not
escaped by a '\'. */
static const char *
find_part_sep (const char *name)
{
const char *p = name;
char c;
while ((c = *p++) != '\0')
{
if (c == '\\' && *p == ',')
p++;
else if (c == ',')
return p - 1;
}
return NULL;
}
OK, this one.
Post by Eric Snowberg
When the obdisk driver discovers a disk, it must put the disk name in
the proper format. Otherwise when grub_disk_open takes place later
on, the wrong disk name will eventually get sent back to the obdisk
driver.
Then we need proper escaping. And AIUI your driver does that.
Post by Eric Snowberg
Post by Daniel Kiper
If yes could you give an example with
proper escaping?
Post by Eric Snowberg
the driver talks to the actual hardware these devices can not have the
escaped names.
OK but this is not clear. So, please add a comment explaining it in
the code somewhere.
Ok
Post by Daniel Kiper
Post by Eric Snowberg
Post by Daniel Kiper
how often this conversions must be done, why you have chosen that
solution, etc. Then I will try to optimize solution a bit.
Under normal circumstances it only takes place once per disk as they
are enumerated. All disks are cached within this driver so it should
not happen often. That is why I store both versions so I don’t have
to go back and forth. Look at the current driver ofdisk. It has a
function called compute_dev_path which does this conversion on every
single open (grub_ofdisk_open). That does not happen with this new
driver. IMHO this is a much more optimized solution than the current
driver.
Then OK. However, this have to be explained somewhere in the code.
Additionally, I think that proper variable naming would help too,
- s/decode_grub_devname/unescape_grub_devnam/,
- s/encode_grub_devname/escape_grub_devname/,
- extract unscaping code to the unescape_commas() function;
even if it is called once; just for the completeness.
Ok
Post by Daniel Kiper
What is replace_escaped_commas()? Why do we need that function?
It is a convenience function for the end-user, so they can access a
disk without having to understand all this escaping when they want to
use one thru the shell.
I think that this will introduce more confusion. I would like that
escaping of drive paths should be properly explained in GRUB docs.
Including why it is needed. And replace_escaped_commas() should be
dropped.

Daniel
Eric Snowberg
2018-07-17 15:59:33 UTC
Permalink
Post by Daniel Kiper
Post by Eric Snowberg
Post by Daniel Kiper
Sorry for late reply but I was busy with other stuff.
[...]
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
+static char *
+replace_escaped_commas (char *src)
+{
+ char *iptr;
+
+ if (src == NULL)
+ return NULL;
+ for (iptr = src; *iptr; )
+ {
+ if ((*iptr == '\\') && (*(iptr + 1) == ','))
+ {
+ *iptr++ = '_';
+ *iptr++ = '_';
+ }
+ iptr++;
+ }
+
+ return src;
+}
+
+static int
+count_commas (const char *src)
+{
+ int count = 0;
+
+ for ( ; *src; src++)
+ if (*src == ',')
+ count++;
+
+ return count;
+}
+
+static void
+escape_commas (const char *src, char *dest)
I am confused by this play with commas. Could explain somewhere
where this commas are needed unescaped, escaped, replaced, etc.
Could not we simplify this somehow?
I’m open for recommendations.
Great! However, I need more info which layer need what WRT ",”,
https://www.gnu.org/software/grub/manual/grub/grub.html#Device-syntax
Everything above this driver expects it to be escaped. Obviously when
Do you mean from the command line?
This goes much further than the command line. For example, it is
built right into the disk driver. Look at grub-core/kern/disk.c: 544
This is the last line of the file. So, I am not sure what exactly you mean.
Post by Eric Snowberg
/* Return the location of the first ',', if any, which is not
escaped by a '\'. */
static const char *
find_part_sep (const char *name)
{
const char *p = name;
char c;
while ((c = *p++) != '\0')
{
if (c == '\\' && *p == ',')
p++;
else if (c == ',')
return p - 1;
}
return NULL;
}
OK, this one.
Post by Eric Snowberg
When the obdisk driver discovers a disk, it must put the disk name in
the proper format. Otherwise when grub_disk_open takes place later
on, the wrong disk name will eventually get sent back to the obdisk
driver.
Then we need proper escaping. And AIUI your driver does that.
Post by Eric Snowberg
Post by Daniel Kiper
If yes could you give an example with
proper escaping?
Post by Eric Snowberg
the driver talks to the actual hardware these devices can not have the
escaped names.
OK but this is not clear. So, please add a comment explaining it in
the code somewhere.
Ok
Post by Daniel Kiper
Post by Eric Snowberg
Post by Daniel Kiper
how often this conversions must be done, why you have chosen that
solution, etc. Then I will try to optimize solution a bit.
Under normal circumstances it only takes place once per disk as they
are enumerated. All disks are cached within this driver so it should
not happen often. That is why I store both versions so I don’t have
to go back and forth. Look at the current driver ofdisk. It has a
function called compute_dev_path which does this conversion on every
single open (grub_ofdisk_open). That does not happen with this new
driver. IMHO this is a much more optimized solution than the current
driver.
Then OK. However, this have to be explained somewhere in the code.
Additionally, I think that proper variable naming would help too,
- s/decode_grub_devname/unescape_grub_devnam/,
- s/encode_grub_devname/escape_grub_devname/,
- extract unscaping code to the unescape_commas() function;
even if it is called once; just for the completeness.
Ok
Post by Daniel Kiper
What is replace_escaped_commas()? Why do we need that function?
It is a convenience function for the end-user, so they can access a
disk without having to understand all this escaping when they want to
use one thru the shell.
I think that this will introduce more confusion. I would like that
escaping of drive paths should be properly explained in GRUB docs.
Including why it is needed. And replace_escaped_commas() should be
dropped.
The confusion seems to be around what needs to be escaped and what doesn’t, as can be seen from the discussion within this email. This change makes it convenient for the end-user, since they will not need to understand any of this.

When function grub_obdisk_iterate (defined as .iterate within grub_disk_dev) gets called, it returns the disks the driver controls. As explained within the description of this patch, a single IEEE1275 disk can have over 6 names. When the .iterate function is called, only a single drive can be returned. If the disk that is to be returned contains \\, within the name, they are replaced with __. Then for example, the end-user will see something like this following a ls:

grub> ls
(ieee1275//***@308/***@1/***@0/***@1/***@3/***@0__0) (ieee1275//***@308/pc
***@1/***@0/***@1/***@3/***@0__0,gpt1) (ieee1275//***@306/***@1/***@0
/***@0__0) (ieee1275//***@306/***@1/***@0/***@0__0,gpt3) (ieee1275//pc
***@306/***@1/***@0/***@0__0,gpt2) (ieee1275//***@306/***@1/***@0/
***@0__0,gpt1)

The end-user can now type the disk name exactly as it is returned on the screen. Or they can escape any of the real disk names properly and the driver will understand it is the same disk. Do you really want this removed?
Daniel Kiper
2018-08-30 14:06:53 UTC
Permalink
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
Post by Daniel Kiper
Sorry for late reply but I was busy with other stuff.
[...]
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
+static char *
+replace_escaped_commas (char *src)
+{
+ char *iptr;
+
+ if (src == NULL)
+ return NULL;
+ for (iptr = src; *iptr; )
+ {
+ if ((*iptr == '\\') && (*(iptr + 1) == ','))
+ {
+ *iptr++ = '_';
+ *iptr++ = '_';
+ }
+ iptr++;
+ }
+
+ return src;
+}
+
+static int
+count_commas (const char *src)
+{
+ int count = 0;
+
+ for ( ; *src; src++)
+ if (*src == ',')
+ count++;
+
+ return count;
+}
+
+static void
+escape_commas (const char *src, char *dest)
I am confused by this play with commas. Could explain somewhere
where this commas are needed unescaped, escaped, replaced, etc.
Could not we simplify this somehow?
I’m open for recommendations.
Great! However, I need more info which layer need what WRT ",”,
https://www.gnu.org/software/grub/manual/grub/grub.html#Device-syntax
Everything above this driver expects it to be escaped. Obviously when
Do you mean from the command line?
This goes much further than the command line. For example, it is
built right into the disk driver. Look at grub-core/kern/disk.c: 544
This is the last line of the file. So, I am not sure what exactly you mean.
Post by Eric Snowberg
/* Return the location of the first ',', if any, which is not
escaped by a '\'. */
static const char *
find_part_sep (const char *name)
{
const char *p = name;
char c;
while ((c = *p++) != '\0')
{
if (c == '\\' && *p == ',')
p++;
else if (c == ',')
return p - 1;
}
return NULL;
}
OK, this one.
Post by Eric Snowberg
When the obdisk driver discovers a disk, it must put the disk name in
the proper format. Otherwise when grub_disk_open takes place later
on, the wrong disk name will eventually get sent back to the obdisk
driver.
Then we need proper escaping. And AIUI your driver does that.
Post by Eric Snowberg
Post by Daniel Kiper
If yes could you give an example with
proper escaping?
Post by Eric Snowberg
the driver talks to the actual hardware these devices can not have the
escaped names.
OK but this is not clear. So, please add a comment explaining it in
the code somewhere.
Ok
Post by Daniel Kiper
Post by Eric Snowberg
Post by Daniel Kiper
how often this conversions must be done, why you have chosen that
solution, etc. Then I will try to optimize solution a bit.
Under normal circumstances it only takes place once per disk as they
are enumerated. All disks are cached within this driver so it should
not happen often. That is why I store both versions so I don’t have
to go back and forth. Look at the current driver ofdisk. It has a
function called compute_dev_path which does this conversion on every
single open (grub_ofdisk_open). That does not happen with this new
driver. IMHO this is a much more optimized solution than the current
driver.
Then OK. However, this have to be explained somewhere in the code.
Additionally, I think that proper variable naming would help too,
- s/decode_grub_devname/unescape_grub_devnam/,
- s/encode_grub_devname/escape_grub_devname/,
- extract unscaping code to the unescape_commas() function;
even if it is called once; just for the completeness.
Ok
Post by Daniel Kiper
What is replace_escaped_commas()? Why do we need that function?
It is a convenience function for the end-user, so they can access a
disk without having to understand all this escaping when they want to
use one thru the shell.
I think that this will introduce more confusion. I would like that
escaping of drive paths should be properly explained in GRUB docs.
Including why it is needed. And replace_escaped_commas() should be
dropped.
The confusion seems to be around what needs to be escaped and what
doesn’t, as can be seen from the discussion within this email. This
change makes it convenient for the end-user, since they will not need
to understand any of this.
When function grub_obdisk_iterate (defined as .iterate within
grub_disk_dev) gets called, it returns the disks the driver controls.
As explained within the description of this patch, a single IEEE1275
disk can have over 6 names. When the .iterate function is called,
only a single drive can be returned. If the disk that is to be
returned contains \\, within the name, they are replaced with __.
Then for example, the end-user will see something like this following
grub> ls
The end-user can now type the disk name exactly as it is returned on
the screen. Or they can escape any of the real disk names properly
and the driver will understand it is the same disk. Do you really
want this removed?
After some thinking it seems to me that we should remove this "__"
feature. It introduces another specific escaping form. And IMO this will
make more confusion then it is worth. And what if disk path contains
"__"? Yeah, I know probably it is not the case right now but we should
be prepared... Though I am not against displaying properly escaped
disks and partitions paths, e.g.:

(ieee1275//***@308/***@1/***@0/***@1/***@3/***@0\\,0)

or

(ieee1275//***@308/***@1/***@0/***@1/***@3/***@0\\,0,gpt1)

etc.

Additionally, I think that you should update GRUB2 docs in relevant places
and add an info saying that disk paths containing "," should be properly
escaped.

Daniel
Eric Snowberg
2018-08-30 15:28:52 UTC
Permalink
Post by Daniel Kiper
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
Post by Daniel Kiper
Sorry for late reply but I was busy with other stuff.
[...]
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
+static char *
+replace_escaped_commas (char *src)
+{
+ char *iptr;
+
+ if (src == NULL)
+ return NULL;
+ for (iptr = src; *iptr; )
+ {
+ if ((*iptr == '\\') && (*(iptr + 1) == ','))
+ {
+ *iptr++ = '_';
+ *iptr++ = '_';
+ }
+ iptr++;
+ }
+
+ return src;
+}
+
+static int
+count_commas (const char *src)
+{
+ int count = 0;
+
+ for ( ; *src; src++)
+ if (*src == ',')
+ count++;
+
+ return count;
+}
+
+static void
+escape_commas (const char *src, char *dest)
I am confused by this play with commas. Could explain somewhere
where this commas are needed unescaped, escaped, replaced, etc.
Could not we simplify this somehow?
I’m open for recommendations.
Great! However, I need more info which layer need what WRT ",”,
https://www.gnu.org/software/grub/manual/grub/grub.html#Device-syntax
Everything above this driver expects it to be escaped. Obviously when
Do you mean from the command line?
This goes much further than the command line. For example, it is
built right into the disk driver. Look at grub-core/kern/disk.c: 544
This is the last line of the file. So, I am not sure what exactly you mean.
Post by Eric Snowberg
/* Return the location of the first ',', if any, which is not
escaped by a '\'. */
static const char *
find_part_sep (const char *name)
{
const char *p = name;
char c;
while ((c = *p++) != '\0')
{
if (c == '\\' && *p == ',')
p++;
else if (c == ',')
return p - 1;
}
return NULL;
}
OK, this one.
Post by Eric Snowberg
When the obdisk driver discovers a disk, it must put the disk name in
the proper format. Otherwise when grub_disk_open takes place later
on, the wrong disk name will eventually get sent back to the obdisk
driver.
Then we need proper escaping. And AIUI your driver does that.
Post by Eric Snowberg
Post by Daniel Kiper
If yes could you give an example with
proper escaping?
Post by Eric Snowberg
the driver talks to the actual hardware these devices can not have the
escaped names.
OK but this is not clear. So, please add a comment explaining it in
the code somewhere.
Ok
Post by Daniel Kiper
Post by Eric Snowberg
Post by Daniel Kiper
how often this conversions must be done, why you have chosen that
solution, etc. Then I will try to optimize solution a bit.
Under normal circumstances it only takes place once per disk as they
are enumerated. All disks are cached within this driver so it should
not happen often. That is why I store both versions so I don’t have
to go back and forth. Look at the current driver ofdisk. It has a
function called compute_dev_path which does this conversion on every
single open (grub_ofdisk_open). That does not happen with this new
driver. IMHO this is a much more optimized solution than the current
driver.
Then OK. However, this have to be explained somewhere in the code.
Additionally, I think that proper variable naming would help too,
- s/decode_grub_devname/unescape_grub_devnam/,
- s/encode_grub_devname/escape_grub_devname/,
- extract unscaping code to the unescape_commas() function;
even if it is called once; just for the completeness.
Ok
Post by Daniel Kiper
What is replace_escaped_commas()? Why do we need that function?
It is a convenience function for the end-user, so they can access a
disk without having to understand all this escaping when they want to
use one thru the shell.
I think that this will introduce more confusion. I would like that
escaping of drive paths should be properly explained in GRUB docs.
Including why it is needed. And replace_escaped_commas() should be
dropped.
The confusion seems to be around what needs to be escaped and what
doesn’t, as can be seen from the discussion within this email. This
change makes it convenient for the end-user, since they will not need
to understand any of this.
When function grub_obdisk_iterate (defined as .iterate within
grub_disk_dev) gets called, it returns the disks the driver controls.
As explained within the description of this patch, a single IEEE1275
disk can have over 6 names. When the .iterate function is called,
only a single drive can be returned. If the disk that is to be
returned contains \\, within the name, they are replaced with __.
Then for example, the end-user will see something like this following
grub> ls
The end-user can now type the disk name exactly as it is returned on
the screen. Or they can escape any of the real disk names properly
and the driver will understand it is the same disk. Do you really
want this removed?
After some thinking it seems to me that we should remove this "__"
feature. It introduces another specific escaping form. And IMO this will
make more confusion then it is worth. And what if disk path contains
"__”? Yeah, I know probably it is not the case right now but we should
be prepared…
Though I am not against displaying properly escaped
or
etc.
I don’t believe you have escaped the name properly above. Unless you are suggesting substituting ‘\\’ with “\\” before the item is displayed. If that is acceptable, would you accept this change instead?

+static char *
+replace_escaped_commas (char *src)
+{
+ char *iptr;
+
+ if (src == NULL)
+ return NULL;
+ for (iptr = src; *iptr; )
+ {
+ if ((*iptr == '\\') && (*(iptr + 1) == ','))
+ {
+ *iptr++ = '\';
+ *iptr++ = '\';
+ }
+ iptr++;
+ }
+
+ return src;
+}
Post by Daniel Kiper
Additionally, I think that you should update GRUB2 docs in relevant places
and add an info saying that disk paths containing "," should be properly
escaped.
It is already documented here:
https://www.gnu.org/software/grub/manual/grub/grub.html#Device-syntax
Daniel Kiper
2018-09-01 17:10:46 UTC
Permalink
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
Post by Daniel Kiper
Sorry for late reply but I was busy with other stuff.
[...]
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
+static char *
+replace_escaped_commas (char *src)
+{
+ char *iptr;
+
+ if (src == NULL)
+ return NULL;
+ for (iptr = src; *iptr; )
+ {
+ if ((*iptr == '\\') && (*(iptr + 1) == ','))
+ {
+ *iptr++ = '_';
+ *iptr++ = '_';
+ }
+ iptr++;
+ }
+
+ return src;
+}
+
+static int
+count_commas (const char *src)
+{
+ int count = 0;
+
+ for ( ; *src; src++)
+ if (*src == ',')
+ count++;
+
+ return count;
+}
+
+static void
+escape_commas (const char *src, char *dest)
I am confused by this play with commas. Could explain somewhere
where this commas are needed unescaped, escaped, replaced, etc.
Could not we simplify this somehow?
I’m open for recommendations.
Great! However, I need more info which layer need what WRT ",”,
https://www.gnu.org/software/grub/manual/grub/grub.html#Device-syntax
Everything above this driver expects it to be escaped. Obviously when
Do you mean from the command line?
This goes much further than the command line. For example, it is
built right into the disk driver. Look at grub-core/kern/disk.c: 544
This is the last line of the file. So, I am not sure what exactly you mean.
Post by Eric Snowberg
/* Return the location of the first ',', if any, which is not
escaped by a '\'. */
static const char *
find_part_sep (const char *name)
{
const char *p = name;
char c;
while ((c = *p++) != '\0')
{
if (c == '\\' && *p == ',')
p++;
else if (c == ',')
return p - 1;
}
return NULL;
}
OK, this one.
Post by Eric Snowberg
When the obdisk driver discovers a disk, it must put the disk name in
the proper format. Otherwise when grub_disk_open takes place later
on, the wrong disk name will eventually get sent back to the obdisk
driver.
Then we need proper escaping. And AIUI your driver does that.
Post by Eric Snowberg
Post by Daniel Kiper
If yes could you give an example with
proper escaping?
Post by Eric Snowberg
the driver talks to the actual hardware these devices can not have the
escaped names.
OK but this is not clear. So, please add a comment explaining it in
the code somewhere.
Ok
Post by Daniel Kiper
Post by Eric Snowberg
Post by Daniel Kiper
how often this conversions must be done, why you have chosen that
solution, etc. Then I will try to optimize solution a bit.
Under normal circumstances it only takes place once per disk as they
are enumerated. All disks are cached within this driver so it should
not happen often. That is why I store both versions so I don’t have
to go back and forth. Look at the current driver ofdisk. It has a
function called compute_dev_path which does this conversion on every
single open (grub_ofdisk_open). That does not happen with this new
driver. IMHO this is a much more optimized solution than the current
driver.
Then OK. However, this have to be explained somewhere in the code.
Additionally, I think that proper variable naming would help too,
- s/decode_grub_devname/unescape_grub_devnam/,
- s/encode_grub_devname/escape_grub_devname/,
- extract unscaping code to the unescape_commas() function;
even if it is called once; just for the completeness.
Ok
Post by Daniel Kiper
What is replace_escaped_commas()? Why do we need that function?
It is a convenience function for the end-user, so they can access a
disk without having to understand all this escaping when they want to
use one thru the shell.
I think that this will introduce more confusion. I would like that
escaping of drive paths should be properly explained in GRUB docs.
Including why it is needed. And replace_escaped_commas() should be
dropped.
The confusion seems to be around what needs to be escaped and what
doesn’t, as can be seen from the discussion within this email. This
change makes it convenient for the end-user, since they will not need
to understand any of this.
When function grub_obdisk_iterate (defined as .iterate within
grub_disk_dev) gets called, it returns the disks the driver controls.
As explained within the description of this patch, a single IEEE1275
disk can have over 6 names. When the .iterate function is called,
only a single drive can be returned. If the disk that is to be
returned contains \\, within the name, they are replaced with __.
Then for example, the end-user will see something like this following
grub> ls
The end-user can now type the disk name exactly as it is returned on
the screen. Or they can escape any of the real disk names properly
and the driver will understand it is the same disk. Do you really
want this removed?
After some thinking it seems to me that we should remove this "__"
feature. It introduces another specific escaping form. And IMO this will
make more confusion then it is worth. And what if disk path contains
"__”? Yeah, I know probably it is not the case right now but we should
be prepared…
Though I am not against displaying properly escaped
or
etc.
I don’t believe you have escaped the name properly above. Unless you
are suggesting substituting ‘\\’ with “\\” before the item is
I think that it is correct. If you use one '\' then after shell
processing you will get

(ieee1275//***@308/***@1/***@0/***@1/***@3/***@0,0)

or

(ieee1275//***@308/***@1/***@0/***@1/***@3/***@0,0,gpt1)

And I suppose that this is not what you want. So, you need two '\'.
This way the layer below will get after shell processing

(ieee1275//***@308/***@1/***@0/***@1/***@3/***@0\,0)

or

(ieee1275//***@308/***@1/***@0/***@1/***@3/***@0\,0,gpt1)

Then new driver should get

ieee1275//***@308/***@1/***@0/***@1/***@3/***@0,0

or

ieee1275//***@308/***@1/***@0/***@1/***@3/***@0\,0

if you really need escaped ',' in the path. However, I do not think so.
It seems to me that OF expects ',' as ','. Hence, I have a feeling that
we can reduce number of escaping/unescaping in the driver.

Am I right?
Post by Eric Snowberg
displayed. If that is acceptable, would you accept this change
instead?
+static char *
+replace_escaped_commas (char *src)
+{
+ char *iptr;
+
+ if (src == NULL)
+ return NULL;
+ for (iptr = src; *iptr; )
+ {
+ if ((*iptr == '\\') && (*(iptr + 1) == ','))
+ {
+ *iptr++ = '\';
+ *iptr++ = '\';
Plus

*iptr++ = ',';
Post by Eric Snowberg
+ }
+ iptr++;
+ }
+
+ return src;
+}
Post by Daniel Kiper
Additionally, I think that you should update GRUB2 docs in relevant places
and add an info saying that disk paths containing "," should be properly
escaped.
https://www.gnu.org/software/grub/manual/grub/grub.html#Device-syntax
OK but this does not discuss shell processing of '\' which is important
here. So, I think that doc should be updated.

Daniel
Eric Snowberg
2018-09-04 15:45:14 UTC
Permalink
Post by Daniel Kiper
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
Post by Daniel Kiper
Sorry for late reply but I was busy with other stuff.
[...]
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
Post by Daniel Kiper
Post by Eric Snowberg
+static char *
+replace_escaped_commas (char *src)
+{
+ char *iptr;
+
+ if (src == NULL)
+ return NULL;
+ for (iptr = src; *iptr; )
+ {
+ if ((*iptr == '\\') && (*(iptr + 1) == ','))
+ {
+ *iptr++ = '_';
+ *iptr++ = '_';
+ }
+ iptr++;
+ }
+
+ return src;
+}
+
+static int
+count_commas (const char *src)
+{
+ int count = 0;
+
+ for ( ; *src; src++)
+ if (*src == ',')
+ count++;
+
+ return count;
+}
+
+static void
+escape_commas (const char *src, char *dest)
I am confused by this play with commas. Could explain somewhere
where this commas are needed unescaped, escaped, replaced, etc.
Could not we simplify this somehow?
I’m open for recommendations.
Great! However, I need more info which layer need what WRT ",”,
https://www.gnu.org/software/grub/manual/grub/grub.html#Device-syntax
Everything above this driver expects it to be escaped. Obviously when
Do you mean from the command line?
This goes much further than the command line. For example, it is
built right into the disk driver. Look at grub-core/kern/disk.c: 544
This is the last line of the file. So, I am not sure what exactly you mean.
Post by Eric Snowberg
/* Return the location of the first ',', if any, which is not
escaped by a '\'. */
static const char *
find_part_sep (const char *name)
{
const char *p = name;
char c;
while ((c = *p++) != '\0')
{
if (c == '\\' && *p == ',')
p++;
else if (c == ',')
return p - 1;
}
return NULL;
}
OK, this one.
Post by Eric Snowberg
When the obdisk driver discovers a disk, it must put the disk name in
the proper format. Otherwise when grub_disk_open takes place later
on, the wrong disk name will eventually get sent back to the obdisk
driver.
Then we need proper escaping. And AIUI your driver does that.
Post by Eric Snowberg
Post by Daniel Kiper
If yes could you give an example with
proper escaping?
Post by Eric Snowberg
the driver talks to the actual hardware these devices can not have the
escaped names.
OK but this is not clear. So, please add a comment explaining it in
the code somewhere.
Ok
Post by Daniel Kiper
Post by Eric Snowberg
Post by Daniel Kiper
how often this conversions must be done, why you have chosen that
solution, etc. Then I will try to optimize solution a bit.
Under normal circumstances it only takes place once per disk as they
are enumerated. All disks are cached within this driver so it should
not happen often. That is why I store both versions so I don’t have
to go back and forth. Look at the current driver ofdisk. It has a
function called compute_dev_path which does this conversion on every
single open (grub_ofdisk_open). That does not happen with this new
driver. IMHO this is a much more optimized solution than the current
driver.
Then OK. However, this have to be explained somewhere in the code.
Additionally, I think that proper variable naming would help too,
- s/decode_grub_devname/unescape_grub_devnam/,
- s/encode_grub_devname/escape_grub_devname/,
- extract unscaping code to the unescape_commas() function;
even if it is called once; just for the completeness.
Ok
Post by Daniel Kiper
What is replace_escaped_commas()? Why do we need that function?
It is a convenience function for the end-user, so they can access a
disk without having to understand all this escaping when they want to
use one thru the shell.
I think that this will introduce more confusion. I would like that
escaping of drive paths should be properly explained in GRUB docs.
Including why it is needed. And replace_escaped_commas() should be
dropped.
The confusion seems to be around what needs to be escaped and what
doesn’t, as can be seen from the discussion within this email. This
change makes it convenient for the end-user, since they will not need
to understand any of this.
When function grub_obdisk_iterate (defined as .iterate within
grub_disk_dev) gets called, it returns the disks the driver controls.
As explained within the description of this patch, a single IEEE1275
disk can have over 6 names. When the .iterate function is called,
only a single drive can be returned. If the disk that is to be
returned contains \\, within the name, they are replaced with __.
Then for example, the end-user will see something like this following
grub> ls
The end-user can now type the disk name exactly as it is returned on
the screen. Or they can escape any of the real disk names properly
and the driver will understand it is the same disk. Do you really
want this removed?
After some thinking it seems to me that we should remove this "__"
feature. It introduces another specific escaping form. And IMO this will
make more confusion then it is worth. And what if disk path contains
"__”? Yeah, I know probably it is not the case right now but we should
be prepared…
Though I am not against displaying properly escaped
or
etc.
I don’t believe you have escaped the name properly above. Unless you
are suggesting substituting ‘\\’ with “\\” before the item is
I think that it is correct. If you use one '\' then after shell
processing you will get
or
And I suppose that this is not what you want. So, you need two '\'.
This way the layer below will get after shell processing
or
Then new driver should get
or
if you really need escaped ',' in the path.
For obdisk devices that are displayed thru the shell, I didn't want the escaped ‘,’ in the path. But you rejected my change that substituted it with __ instead. Therefore we are left with making this work with code above obdisk without changing it.
Post by Daniel Kiper
However, I do not think so.
It seems to me that OF expects ',' as ','. Hence, I have a feeling that
we can reduce number of escaping/unescaping in the driver.
Am I right?
No.

The driver provides the name of the device which is displayed in the shell. It must be in a format that will allow it to get back to the driver at a later time. If a ‘,’ is in the name, it must be escaped. Otherwise the code above the driver will trim off everything to the right of the ‘,’. Then the driver will be sent a device name that does not exist.

For example with this device:
/***@306/***@1/LSI,***@0/***@0,0,gpt3

If the ‘,’ is not escaped properly, either (obdisk or ofdisk) will be sent an open to device: /***@306/***@1/LSI. The upper level code will think everything to the right of the ‘,’ is the partition. Once the driver receives this open, it will fail, since it isn’t a valid device.
Daniel Kiper
2018-09-05 09:36:40 UTC
Permalink
[...]
Post by Eric Snowberg
Post by Daniel Kiper
I think that it is correct. If you use one '\' then after shell
processing you will get
or
And I suppose that this is not what you want. So, you need two '\'.
This way the layer below will get after shell processing
or
Then new driver should get
or
if you really need escaped ',' in the path.
For obdisk devices that are displayed thru the shell, I didn't want
the escaped ‘,’ in the path. But you rejected my change that
substituted it with __ instead. Therefore we are left with making
this work with code above obdisk without changing it.
Post by Daniel Kiper
However, I do not think so.
It seems to me that OF expects ',' as ','. Hence, I have a feeling that
we can reduce number of escaping/unescaping in the driver.
Am I right?
No.
The driver provides the name of the device which is displayed in the
shell. It must be in a format that will allow it to get back to the
driver at a later time. If a ‘,’ is in the name, it must be escaped.
Otherwise the code above the driver will trim off everything to the
right of the ‘,’. Then the driver will be sent a device name that does
not exist.
If the ‘,’ is not escaped properly, either (obdisk or ofdisk) will be
think everything to the right of the ‘,’ is the partition. Once the
driver receives this open, it will fail, since it isn’t a valid
device.
So, AIUI, the driver has to use device paths with escaped ',', e.g.

ieee1275//***@308/***@1/***@0/***@1/***@3/***@0\,0

I am OK with that.

Though we should think how to display such paths. We can show them as is
without additional escaping. And right now I think that it is preferred
form. Even GRUB doc shows paths with ',' that way. I know that then
user cannot just copy-and-paste the device path. However, I think that
we should update the doc in relevant place and say that he/she has to
take care about special characters for GRUB2 shell like '\' because shell
will eat them. So, they have to be properly escaped. This way everything
will be in line with common GRUB2 shell escaping rules and we will not
introduce exceptions which may make confusion in the future.

Daniel

Loading...