* [PATCH v2] ieee1275: obdisk driver
@ 2018-05-30 23:55 Eric Snowberg
2018-06-15 12:02 ` Daniel Kiper
0 siblings, 1 reply; 16+ messages in thread
From: Eric Snowberg @ 2018-05-30 23:55 UTC (permalink / raw)
To: grub-devel; +Cc: daniel.kiper, eric.snowberg, glaubitz
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:
/pci@302/pci@2/pci@0/pci@17/LSI,sas@0/disk@w5000cca02f037d6d,0
It can also be referenced by its PHY identifier using this form:
/pci@302/pci@2/pci@0/pci@17/LSI,sas@0/disk@p0
It can also be referenced by its Target identifier using this form:
/pci@302/pci@2/pci@0/pci@17/LSI,sas@0/disk@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 <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-memory@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/disk@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/disk@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 /pci@301/pci@1/scsi@0/disk@p3
+ * disk2 /pci@301/pci@1/scsi@0/disk@p2
+ * disk1 /pci@301/pci@1/scsi@0/disk@p1
+ * disk /pci@301/pci@1/scsi@0/disk@p0
+ * disk0 /pci@301/pci@1/scsi@0/disk@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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ieee1275: obdisk driver
2018-05-30 23:55 [PATCH v2] ieee1275: obdisk driver Eric Snowberg
@ 2018-06-15 12:02 ` Daniel Kiper
2018-06-15 12:05 ` John Paul Adrian Glaubitz
2018-06-15 15:58 ` Eric Snowberg
0 siblings, 2 replies; 16+ messages in thread
From: Daniel Kiper @ 2018-06-15 12:02 UTC (permalink / raw)
To: Eric Snowberg; +Cc: grub-devel, glaubitz
On Wed, May 30, 2018 at 04:55:22PM -0700, Eric Snowberg wrote:
> 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:
>
> /pci@302/pci@2/pci@0/pci@17/LSI,sas@0/disk@w5000cca02f037d6d,0
>
> It can also be referenced by its PHY identifier using this form:
>
> /pci@302/pci@2/pci@0/pci@17/LSI,sas@0/disk@p0
>
> It can also be referenced by its Target identifier using this form:
>
> /pci@302/pci@2/pci@0/pci@17/LSI,sas@0/disk@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 <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;
I would drop all these 0/NULL initializations.
Compiler will do work for you. I asked about
that in earlier comments.
> +static const char *block_blacklist[] = {
> + /* Requires addition work in grub before being able to be used. */
s/addition/additional/?
> + "/iscsi-hba",
> + /* This block device should never be used by grub. */
> + "/reboot-memory@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)
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.
> + *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().
> +{
> + 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...
> + 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.
> + grub_ieee1275_get_property (phandle, "device_type", op->type,
> + IEEE1275_MAX_PROP_LEN, &actual);
s/&actual/NULL/?
> + 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;
What will happen if canon == NULL?
> + 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/?
> +{
> + 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/disk@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/disk@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)
What is exp? And why exp <= 0x100; exp+=0x100? Could you add
a comment here or use constants?
> +
Could you drop this empty line?
> + for (phy = 0; phy < 0x20; phy++)
Why 0x20? Constant? Or comment at least?
> + {
> + 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)
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ieee1275: obdisk driver
2018-06-15 12:02 ` Daniel Kiper
@ 2018-06-15 12:05 ` John Paul Adrian Glaubitz
2018-06-15 12:14 ` Daniel Kiper
2018-06-15 15:58 ` Eric Snowberg
1 sibling, 1 reply; 16+ messages in thread
From: John Paul Adrian Glaubitz @ 2018-06-15 12:05 UTC (permalink / raw)
To: Daniel Kiper, Eric Snowberg; +Cc: grub-devel
On 06/15/2018 02:02 PM, Daniel Kiper wrote:
>> +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 - glaubitz@debian.org
`. `' Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ieee1275: obdisk driver
2018-06-15 12:05 ` John Paul Adrian Glaubitz
@ 2018-06-15 12:14 ` Daniel Kiper
0 siblings, 0 replies; 16+ messages in thread
From: Daniel Kiper @ 2018-06-15 12:14 UTC (permalink / raw)
To: John Paul Adrian Glaubitz; +Cc: Eric Snowberg, grub-devel
On Fri, Jun 15, 2018 at 02:05:32PM +0200, John Paul Adrian Glaubitz wrote:
> On 06/15/2018 02:02 PM, Daniel Kiper wrote:
> >> +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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ieee1275: obdisk driver
2018-06-15 12:02 ` Daniel Kiper
2018-06-15 12:05 ` John Paul Adrian Glaubitz
@ 2018-06-15 15:58 ` Eric Snowberg
2018-06-21 16:58 ` Daniel Kiper
1 sibling, 1 reply; 16+ messages in thread
From: Eric Snowberg @ 2018-06-15 15:58 UTC (permalink / raw)
To: The development of GNU GRUB; +Cc: John Paul Adrian Glaubitz, Daniel Kiper
> On Jun 15, 2018, at 6:02 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>
> On Wed, May 30, 2018 at 04:55:22PM -0700, Eric Snowberg wrote:
>> 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:
>>
>> /pci@302/pci@2/pci@0/pci@17/LSI,sas@0/disk@w5000cca02f037d6d,0
>>
>> It can also be referenced by its PHY identifier using this form:
>>
>> /pci@302/pci@2/pci@0/pci@17/LSI,sas@0/disk@p0
>>
>> It can also be referenced by its Target identifier using this form:
>>
>> /pci@302/pci@2/pci@0/pci@17/LSI,sas@0/disk@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 <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;
>
> 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.
>
>> +static const char *block_blacklist[] = {
>> + /* Requires addition work in grub before being able to be used. */
>
> s/addition/additional/?
ok
>
>> + "/iscsi-hba",
>> + /* This block device should never be used by grub. */
>> + "/reboot-memory@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)
>
> 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.
>
>> + *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.
>
> 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.
>
>> +{
>> + 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...
>
>> + 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.
>
>> + grub_ieee1275_get_property (phandle, "device_type", op->type,
>> + IEEE1275_MAX_PROP_LEN, &actual);
>
> s/&actual/NULL/?
ok
>
>> + 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;
>
> 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.
>
>> + 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.
>
>> +{
>> + 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/disk@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/disk@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)
>
> What is exp?
SAS expander
> 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.
>
>> +
>
> Could you drop this empty line?
ok
>
>> + 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.
>
>> + {
>> + 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)
>
> 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.
>
> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ieee1275: obdisk driver
2018-06-15 15:58 ` Eric Snowberg
@ 2018-06-21 16:58 ` Daniel Kiper
2018-06-21 19:46 ` Eric Snowberg
0 siblings, 1 reply; 16+ messages in thread
From: Daniel Kiper @ 2018-06-21 16:58 UTC (permalink / raw)
To: Eric Snowberg; +Cc: The development of GNU GRUB, John Paul Adrian Glaubitz
On Fri, Jun 15, 2018 at 09:58:56AM -0600, Eric Snowberg wrote:
> > On Jun 15, 2018, at 6:02 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> > On Wed, May 30, 2018 at 04:55:22PM -0700, Eric Snowberg wrote:
[...]
> >> +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.
[...]
> >> +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.
[...]
> >> +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.
[...]
> >> + 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.
[...]
> >> +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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ieee1275: obdisk driver
2018-06-21 16:58 ` Daniel Kiper
@ 2018-06-21 19:46 ` Eric Snowberg
2018-07-16 13:51 ` Daniel Kiper
0 siblings, 1 reply; 16+ messages in thread
From: Eric Snowberg @ 2018-06-21 19:46 UTC (permalink / raw)
To: The development of GNU GRUB; +Cc: John Paul Adrian Glaubitz, Daniel Kiper
> On Jun 21, 2018, at 10:58 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>
> On Fri, Jun 15, 2018 at 09:58:56AM -0600, Eric Snowberg wrote:
>>> On Jun 15, 2018, at 6:02 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>> On Wed, May 30, 2018 at 04:55:22PM -0700, Eric Snowberg wrote:
>
> [...]
>
>>>> +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.
>
> [...]
>
>>>> +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.
> 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.
>
> [...]
>
>>>> +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.
>
> [...]
>
>>>> + 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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ieee1275: obdisk driver
2018-06-21 19:46 ` Eric Snowberg
@ 2018-07-16 13:51 ` Daniel Kiper
2018-07-16 15:33 ` Eric Snowberg
0 siblings, 1 reply; 16+ messages in thread
From: Daniel Kiper @ 2018-07-16 13:51 UTC (permalink / raw)
To: Eric Snowberg; +Cc: The development of GNU GRUB, John Paul Adrian Glaubitz
Sorry for late reply but I was busy with other stuff.
On Thu, Jun 21, 2018 at 01:46:46PM -0600, Eric Snowberg wrote:
> > On Jun 21, 2018, at 10:58 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> > On Fri, Jun 15, 2018 at 09:58:56AM -0600, Eric Snowberg wrote:
> >>> On Jun 15, 2018, at 6:02 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> >>> On Wed, May 30, 2018 at 04:55:22PM -0700, Eric Snowberg wrote:
[...]
> >>>> +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
Do you mean from the command line? If yes could you give an example with
proper escaping?
> 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.
> > 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?
[...]
> >>>> + 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ieee1275: obdisk driver
2018-07-16 13:51 ` Daniel Kiper
@ 2018-07-16 15:33 ` Eric Snowberg
2018-07-17 13:38 ` Daniel Kiper
0 siblings, 1 reply; 16+ messages in thread
From: Eric Snowberg @ 2018-07-16 15:33 UTC (permalink / raw)
To: The development of GNU GRUB; +Cc: John Paul Adrian Glaubitz, Daniel Kiper
> On Jul 16, 2018, at 7:51 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>
> Sorry for late reply but I was busy with other stuff.
>
> On Thu, Jun 21, 2018 at 01:46:46PM -0600, Eric Snowberg wrote:
>>> On Jun 21, 2018, at 10:58 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>> On Fri, Jun 15, 2018 at 09:58:56AM -0600, Eric Snowberg wrote:
>>>>> On Jun 15, 2018, at 6:02 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>>>> On Wed, May 30, 2018 at 04:55:22PM -0700, Eric Snowberg wrote:
>
> [...]
>
>>>>>> +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
>
> 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.
> If yes could you give an example with
> proper escaping?
>
>> 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
>
>>> 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.
Ok
>
> 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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ieee1275: obdisk driver
2018-07-16 15:33 ` Eric Snowberg
@ 2018-07-17 13:38 ` Daniel Kiper
2018-07-17 15:59 ` Eric Snowberg
0 siblings, 1 reply; 16+ messages in thread
From: Daniel Kiper @ 2018-07-17 13:38 UTC (permalink / raw)
To: Eric Snowberg; +Cc: The development of GNU GRUB, John Paul Adrian Glaubitz
On Mon, Jul 16, 2018 at 09:33:17AM -0600, Eric Snowberg wrote:
> > On Jul 16, 2018, at 7:51 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> >
> > Sorry for late reply but I was busy with other stuff.
> >
> > On Thu, Jun 21, 2018 at 01:46:46PM -0600, Eric Snowberg wrote:
> >>> On Jun 21, 2018, at 10:58 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> >>> On Fri, Jun 15, 2018 at 09:58:56AM -0600, Eric Snowberg wrote:
> >>>>> On Jun 15, 2018, at 6:02 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> >>>>> On Wed, May 30, 2018 at 04:55:22PM -0700, Eric Snowberg wrote:
> >
> > [...]
> >
> >>>>>> +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
> >
> > 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.
> /* 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.
> 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.
> > If yes could you give an example with
> > proper escaping?
> >
> >> 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
>
> >
> >>> 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.
>
> Ok
>
> >
> > 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ieee1275: obdisk driver
2018-07-17 13:38 ` Daniel Kiper
@ 2018-07-17 15:59 ` Eric Snowberg
2018-08-30 14:06 ` Daniel Kiper
0 siblings, 1 reply; 16+ messages in thread
From: Eric Snowberg @ 2018-07-17 15:59 UTC (permalink / raw)
To: The development of GNU GRUB; +Cc: John Paul Adrian Glaubitz, Daniel Kiper
> On Jul 17, 2018, at 7:38 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>
> On Mon, Jul 16, 2018 at 09:33:17AM -0600, Eric Snowberg wrote:
>>> On Jul 16, 2018, at 7:51 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>>
>>> Sorry for late reply but I was busy with other stuff.
>>>
>>> On Thu, Jun 21, 2018 at 01:46:46PM -0600, Eric Snowberg wrote:
>>>>> On Jun 21, 2018, at 10:58 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>>>> On Fri, Jun 15, 2018 at 09:58:56AM -0600, Eric Snowberg wrote:
>>>>>>> On Jun 15, 2018, at 6:02 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>>>>>> On Wed, May 30, 2018 at 04:55:22PM -0700, Eric Snowberg wrote:
>>>
>>> [...]
>>>
>>>>>>>> +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
>>>
>>> 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.
>
>> /* 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.
>
>> 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.
>
>>> If yes could you give an example with
>>> proper escaping?
>>>
>>>> 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
>>
>>>
>>>>> 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.
>>
>> Ok
>>
>>>
>>> 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//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0__0) (ieee1275//pci@308/pc
i@1/usb@0/hub@1/storage@3/disk@0__0,gpt1) (ieee1275//pci@306/pci@1/LSI__mrsas@0
/disk@0__0) (ieee1275//pci@306/pci@1/LSI__mrsas@0/disk@0__0,gpt3) (ieee1275//pc
i@306/pci@1/LSI__mrsas@0/disk@0__0,gpt2) (ieee1275//pci@306/pci@1/LSI__mrsas@0/
disk@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?
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ieee1275: obdisk driver
2018-07-17 15:59 ` Eric Snowberg
@ 2018-08-30 14:06 ` Daniel Kiper
2018-08-30 15:28 ` Eric Snowberg
0 siblings, 1 reply; 16+ messages in thread
From: Daniel Kiper @ 2018-08-30 14:06 UTC (permalink / raw)
To: Eric Snowberg; +Cc: The development of GNU GRUB, John Paul Adrian Glaubitz
On Tue, Jul 17, 2018 at 09:59:33AM -0600, Eric Snowberg wrote:
> > On Jul 17, 2018, at 7:38 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> > On Mon, Jul 16, 2018 at 09:33:17AM -0600, Eric Snowberg wrote:
> >>> On Jul 16, 2018, at 7:51 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> >>>
> >>> Sorry for late reply but I was busy with other stuff.
> >>>
> >>> On Thu, Jun 21, 2018 at 01:46:46PM -0600, Eric Snowberg wrote:
> >>>>> On Jun 21, 2018, at 10:58 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> >>>>> On Fri, Jun 15, 2018 at 09:58:56AM -0600, Eric Snowberg wrote:
> >>>>>>> On Jun 15, 2018, at 6:02 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> >>>>>>> On Wed, May 30, 2018 at 04:55:22PM -0700, Eric Snowberg wrote:
> >>>
> >>> [...]
> >>>
> >>>>>>>> +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
> >>>
> >>> 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.
> >
> >> /* 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.
> >
> >> 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.
> >
> >>> If yes could you give an example with
> >>> proper escaping?
> >>>
> >>>> 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
> >>
> >>>
> >>>>> 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.
> >>
> >> Ok
> >>
> >>>
> >>> 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//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0__0) (ieee1275//pci@308/pc
> i@1/usb@0/hub@1/storage@3/disk@0__0,gpt1) (ieee1275//pci@306/pci@1/LSI__mrsas@0
> /disk@0__0) (ieee1275//pci@306/pci@1/LSI__mrsas@0/disk@0__0,gpt3) (ieee1275//pc
> i@306/pci@1/LSI__mrsas@0/disk@0__0,gpt2) (ieee1275//pci@306/pci@1/LSI__mrsas@0/
> disk@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?
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//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0\\,0)
or
(ieee1275//pci@308/pci@1/usb@0/hub@1/storage@3/disk@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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ieee1275: obdisk driver
2018-08-30 14:06 ` Daniel Kiper
@ 2018-08-30 15:28 ` Eric Snowberg
2018-09-01 17:10 ` Daniel Kiper
0 siblings, 1 reply; 16+ messages in thread
From: Eric Snowberg @ 2018-08-30 15:28 UTC (permalink / raw)
To: The development of GNU GRUB; +Cc: John Paul Adrian Glaubitz, Daniel Kiper
> On Aug 30, 2018, at 8:06 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>
> On Tue, Jul 17, 2018 at 09:59:33AM -0600, Eric Snowberg wrote:
>>> On Jul 17, 2018, at 7:38 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>> On Mon, Jul 16, 2018 at 09:33:17AM -0600, Eric Snowberg wrote:
>>>>> On Jul 16, 2018, at 7:51 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>>>>
>>>>> Sorry for late reply but I was busy with other stuff.
>>>>>
>>>>> On Thu, Jun 21, 2018 at 01:46:46PM -0600, Eric Snowberg wrote:
>>>>>>> On Jun 21, 2018, at 10:58 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>>>>>> On Fri, Jun 15, 2018 at 09:58:56AM -0600, Eric Snowberg wrote:
>>>>>>>>> On Jun 15, 2018, at 6:02 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>>>>>>>> On Wed, May 30, 2018 at 04:55:22PM -0700, Eric Snowberg wrote:
>>>>>
>>>>> [...]
>>>>>
>>>>>>>>>> +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
>>>>>
>>>>> 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.
>>>
>>>> /* 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.
>>>
>>>> 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.
>>>
>>>>> If yes could you give an example with
>>>>> proper escaping?
>>>>>
>>>>>> 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
>>>>
>>>>>
>>>>>>> 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.
>>>>
>>>> Ok
>>>>
>>>>>
>>>>> 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//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0__0) (ieee1275//pci@308/pc
>> i@1/usb@0/hub@1/storage@3/disk@0__0,gpt1) (ieee1275//pci@306/pci@1/LSI__mrsas@0
>> /disk@0__0) (ieee1275//pci@306/pci@1/LSI__mrsas@0/disk@0__0,gpt3) (ieee1275//pc
>> i@306/pci@1/LSI__mrsas@0/disk@0__0,gpt2) (ieee1275//pci@306/pci@1/LSI__mrsas@0/
>> disk@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?
>
> 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//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0\\,0)
>
> or
>
> (ieee1275//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0\\,0,gpt1)
>
> 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;
+}
>
> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ieee1275: obdisk driver
2018-08-30 15:28 ` Eric Snowberg
@ 2018-09-01 17:10 ` Daniel Kiper
2018-09-04 15:45 ` Eric Snowberg
0 siblings, 1 reply; 16+ messages in thread
From: Daniel Kiper @ 2018-09-01 17:10 UTC (permalink / raw)
To: Eric Snowberg; +Cc: The development of GNU GRUB, John Paul Adrian Glaubitz
On Thu, Aug 30, 2018 at 09:28:52AM -0600, Eric Snowberg wrote:
> > On Aug 30, 2018, at 8:06 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> > On Tue, Jul 17, 2018 at 09:59:33AM -0600, Eric Snowberg wrote:
> >>> On Jul 17, 2018, at 7:38 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> >>> On Mon, Jul 16, 2018 at 09:33:17AM -0600, Eric Snowberg wrote:
> >>>>> On Jul 16, 2018, at 7:51 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> >>>>>
> >>>>> Sorry for late reply but I was busy with other stuff.
> >>>>>
> >>>>> On Thu, Jun 21, 2018 at 01:46:46PM -0600, Eric Snowberg wrote:
> >>>>>>> On Jun 21, 2018, at 10:58 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> >>>>>>> On Fri, Jun 15, 2018 at 09:58:56AM -0600, Eric Snowberg wrote:
> >>>>>>>>> On Jun 15, 2018, at 6:02 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> >>>>>>>>> On Wed, May 30, 2018 at 04:55:22PM -0700, Eric Snowberg wrote:
> >>>>>
> >>>>> [...]
> >>>>>
> >>>>>>>>>> +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
> >>>>>
> >>>>> 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.
> >>>
> >>>> /* 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.
> >>>
> >>>> 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.
> >>>
> >>>>> If yes could you give an example with
> >>>>> proper escaping?
> >>>>>
> >>>>>> 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
> >>>>
> >>>>>
> >>>>>>> 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.
> >>>>
> >>>> Ok
> >>>>
> >>>>>
> >>>>> 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//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0__0) (ieee1275//pci@308/pc
> >> i@1/usb@0/hub@1/storage@3/disk@0__0,gpt1) (ieee1275//pci@306/pci@1/LSI__mrsas@0
> >> /disk@0__0) (ieee1275//pci@306/pci@1/LSI__mrsas@0/disk@0__0,gpt3) (ieee1275//pc
> >> i@306/pci@1/LSI__mrsas@0/disk@0__0,gpt2) (ieee1275//pci@306/pci@1/LSI__mrsas@0/
> >> disk@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?
> >
> > 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//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0\\,0)
> >
> > or
> >
> > (ieee1275//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0\\,0,gpt1)
> >
> > 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//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0,0)
or
(ieee1275//pci@308/pci@1/usb@0/hub@1/storage@3/disk@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//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0\,0)
or
(ieee1275//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0\,0,gpt1)
Then new driver should get
ieee1275//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0,0
or
ieee1275//pci@308/pci@1/usb@0/hub@1/storage@3/disk@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?
> 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++ = ',';
> + }
> + iptr++;
> + }
> +
> + return src;
> +}
>
>
> >
> > 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
OK but this does not discuss shell processing of '\' which is important
here. So, I think that doc should be updated.
Daniel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ieee1275: obdisk driver
2018-09-01 17:10 ` Daniel Kiper
@ 2018-09-04 15:45 ` Eric Snowberg
2018-09-05 9:36 ` Daniel Kiper
0 siblings, 1 reply; 16+ messages in thread
From: Eric Snowberg @ 2018-09-04 15:45 UTC (permalink / raw)
To: The development of GNU GRUB; +Cc: John Paul Adrian Glaubitz, Daniel Kiper
> On Sep 1, 2018, at 11:10 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>
> On Thu, Aug 30, 2018 at 09:28:52AM -0600, Eric Snowberg wrote:
>>> On Aug 30, 2018, at 8:06 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>> On Tue, Jul 17, 2018 at 09:59:33AM -0600, Eric Snowberg wrote:
>>>>> On Jul 17, 2018, at 7:38 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>>>> On Mon, Jul 16, 2018 at 09:33:17AM -0600, Eric Snowberg wrote:
>>>>>>> On Jul 16, 2018, at 7:51 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>>>>>>
>>>>>>> Sorry for late reply but I was busy with other stuff.
>>>>>>>
>>>>>>> On Thu, Jun 21, 2018 at 01:46:46PM -0600, Eric Snowberg wrote:
>>>>>>>>> On Jun 21, 2018, at 10:58 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>>>>>>>> On Fri, Jun 15, 2018 at 09:58:56AM -0600, Eric Snowberg wrote:
>>>>>>>>>>> On Jun 15, 2018, at 6:02 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>>>>>>>>>> On Wed, May 30, 2018 at 04:55:22PM -0700, Eric Snowberg wrote:
>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>>>>>> +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
>>>>>>>
>>>>>>> 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.
>>>>>
>>>>>> /* 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.
>>>>>
>>>>>> 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.
>>>>>
>>>>>>> If yes could you give an example with
>>>>>>> proper escaping?
>>>>>>>
>>>>>>>> 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
>>>>>>
>>>>>>>
>>>>>>>>> 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.
>>>>>>
>>>>>> Ok
>>>>>>
>>>>>>>
>>>>>>> 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//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0__0) (ieee1275//pci@308/pc
>>>> i@1/usb@0/hub@1/storage@3/disk@0__0,gpt1) (ieee1275//pci@306/pci@1/LSI__mrsas@0
>>>> /disk@0__0) (ieee1275//pci@306/pci@1/LSI__mrsas@0/disk@0__0,gpt3) (ieee1275//pc
>>>> i@306/pci@1/LSI__mrsas@0/disk@0__0,gpt2) (ieee1275//pci@306/pci@1/LSI__mrsas@0/
>>>> disk@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?
>>>
>>> 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//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0\\,0)
>>>
>>> or
>>>
>>> (ieee1275//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0\\,0,gpt1)
>>>
>>> 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//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0,0)
>
> or
>
> (ieee1275//pci@308/pci@1/usb@0/hub@1/storage@3/disk@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//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0\,0)
>
> or
>
> (ieee1275//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0\,0,gpt1)
>
> Then new driver should get
>
> ieee1275//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0,0
>
> or
>
> ieee1275//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0\,0
>
> 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.
> 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:
/pci@306/pci@1/LSI,mrsas@0/disk@0,0,gpt3
If the ‘,’ is not escaped properly, either (obdisk or ofdisk) will be sent an open to device: /pci@306/pci@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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] ieee1275: obdisk driver
2018-09-04 15:45 ` Eric Snowberg
@ 2018-09-05 9:36 ` Daniel Kiper
0 siblings, 0 replies; 16+ messages in thread
From: Daniel Kiper @ 2018-09-05 9:36 UTC (permalink / raw)
To: Eric Snowberg; +Cc: The development of GNU GRUB, John Paul Adrian Glaubitz
On Tue, Sep 04, 2018 at 09:45:14AM -0600, Eric Snowberg wrote:
> > On Sep 1, 2018, at 11:10 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
[...]
> > I think that it is correct. If you use one '\' then after shell
> > processing you will get
> >
> > (ieee1275//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0,0)
> >
> > or
> >
> > (ieee1275//pci@308/pci@1/usb@0/hub@1/storage@3/disk@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//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0\,0)
> >
> > or
> >
> > (ieee1275//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0\,0,gpt1)
> >
> > Then new driver should get
> >
> > ieee1275//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0,0
> >
> > or
> >
> > ieee1275//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0\,0
> >
> > 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.
>
> > 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: /pci@306/pci@1/LSI,mrsas@0/disk@0,0,gpt3
>
> If the ‘,’ is not escaped properly, either (obdisk or ofdisk) will be
> sent an open to device: /pci@306/pci@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.
So, AIUI, the driver has to use device paths with escaped ',', e.g.
ieee1275//pci@308/pci@1/usb@0/hub@1/storage@3/disk@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
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-09-05 9:36 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-30 23:55 [PATCH v2] ieee1275: obdisk driver Eric Snowberg
2018-06-15 12:02 ` Daniel Kiper
2018-06-15 12:05 ` John Paul Adrian Glaubitz
2018-06-15 12:14 ` Daniel Kiper
2018-06-15 15:58 ` Eric Snowberg
2018-06-21 16:58 ` Daniel Kiper
2018-06-21 19:46 ` Eric Snowberg
2018-07-16 13:51 ` Daniel Kiper
2018-07-16 15:33 ` Eric Snowberg
2018-07-17 13:38 ` Daniel Kiper
2018-07-17 15:59 ` Eric Snowberg
2018-08-30 14:06 ` Daniel Kiper
2018-08-30 15:28 ` Eric Snowberg
2018-09-01 17:10 ` Daniel Kiper
2018-09-04 15:45 ` Eric Snowberg
2018-09-05 9:36 ` Daniel Kiper
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.