> On Apr 11, 2018, at 4:29 AM, Daniel Kiper <daniel.ki...@oracle.com> wrote:
>
> On Thu, Apr 05, 2018 at 10:53:55AM -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 other IEEE1275 ofdisk driver may be suitable for PPC and x86, it
>
> s/other/current/?
>
>> +struct parent_dev
>> +{
>> + struct parent_dev *next;
>> + struct parent_dev **prev;
>> + /* canonical parent name */
>> + char *name;
>> + char *type;
>> + grub_ieee1275_ihandle_t ihandle;
>> + grub_uint32_t address_cells;
>> +};
>
> Could you align all members names in one column for these structures?
>
>> +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
>> +};
>
> I do not think that you need to set values to 0/NULL above.
> Compiler should do work for you.
>
>> +#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 *
>> +remove_escaped_commas (char *src)
>
> s/remove/replace/?
>
> Hmmm... Why do not really remove them?
>
>> +static char *
>> +decode_grub_devname (const char *name)
>> +{
>> + char *devpath = grub_malloc (grub_strlen (name) + 1);
>> + char *p, c;
>> +
>> + if (!devpath)
>
> Please be consistent. Use !devpath or devpath != NULL in entire file.
> Same applies to comparison with 0.
>
>> + return NULL;
>> +
>> + /* Un-escape commas. */
>
> Is not it related to the issue which you have tried to
> fix by the patch for shell parser?
Yes. Doing it here does not impact generic GRUB2 code.
>
>> +static char *
>> +encode_grub_devname (const char *path)
>> +{
>> + char *encoding, *optr;
>> +
>> + if (path == NULL)
>
> Yes, like this one please. If you like it/it is in line with other files of
> course...
>
>> + return NULL;
>> +
>> + encoding = grub_malloc (sizeof ("ieee1275/") + count_commas (path) +
>> + grub_strlen (path) + 1);
>> +
>> + if (encoding == NULL)
>
> Ditto.
>
>> + {
>> + grub_print_error ();
>> + return NULL;
>> + }
>> +
>> + optr = grub_stpcpy (encoding, "ieee1275/");
>
> You use "ieee1275/" in various places. Please define it as a constant.
>
>> +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, "/disk@");
>
> A constant please...
>
>> +static struct parent_dev *
>> +open_parent (const char *parent)
>> +{
>> + struct parent_dev *op;
>> +
>> + if ((op =
>> + grub_named_list_find (GRUB_AS_NAMED_LIST (parent_devs), parent)) ==
>> NULL)
>
> Oh no, please call grub_named_list_find() in separate line and then check op
> value.
>
>> + {
>> + op = open_new_parent (parent);
>> +
>> + if (op)
>> + grub_list_push (GRUB_AS_LIST_P (&parent_devs), GRUB_AS_LIST (op));
>
> Something is wrong with alignment.
>
>> +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);
>
> Values in one column?
>
>> +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) && (phy_hi != 0xffffffff))
>
> valid_phy == 0
>
>> +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;
>> +
>> + unit_address = grub_strstr (canon, "/disk@");
>> + unit_address += grub_strlen ("/disk@");
>> +
>> + 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_canon = grub_malloc (grub_strlen (op->name) + sizeof ("/disk@") +
>> + grub_strlen (real_unit_address));
>
> Please calculate and assign the size in separate line and use it in
> grub_malloc() and grub_snprintf().
>
>> +static struct disk_dev *
>> +add_canon_disk (const char *cname)
>> +{
>> + struct disk_dev *dev;
>> +
>> + dev = grub_zalloc (sizeof (struct disk_dev));
>> +
>> + if (!dev)
>> + 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:
>
> Use one space before each label.
>
>> +
>> +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 rval = GRUB_ERR_NONE;
>
> I would use ret instead of rval here and there.
>
>> + 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))
>
> s/ / /
>
>> + {
>> + 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 rval;
>> +}
>> +
>> +static void
>> +grub_obdisk_close (grub_disk_t disk)
>
> s/grub_obdisk_close/grub_obdisk_clear/?
>
>> +{
>> + disk->data = NULL;
>> + disk->id = 0;
>> + disk->total_sectors = 0;
>> + disk->log_sector_size = 0;
>
> grub_memset (disk, 0, sizeof (*disk));?
>
>> +static grub_err_t
>> +add_bootpath (void)
>> +{
>> + struct disk_dev *ob_device;
>> + grub_err_t rval = 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))
>
> grub_strcmp (type, "network") != 0
>
>> +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. */
>
> I do not like comments formated in that way because it is difficult to
> differentiate code from comments at first sight. I know that coding style
> says something different but I am going to change it. So, please adhere
> to Linux kernel comments style. Here and there.
I have used the GNU GRUB2 coding style here. Could the comment changes be done
in a separate patch once the coding style has changed? Possibly a script could
be created to change them all from the old format to the new?
>
>> + 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. */
>
> Ditto...
>
>> + 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");
>
> Could not you put all values in one column?
>
>> + }
>> +
>> + grub_printf ("-------------------------------------------------\n");
>> +}
>> +
>> +static void
>> +display_stats (void)
>> +{
>> + const char *debug = grub_env_get ("debug");
>> +
>> + if (! debug)
>
> Please be consistent...
>
>> +
>> +
>> +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,
>> + .next = 0
>
> Assignments in one column please... And drop 0 assignment.
> Compiler is your friend.
>
Thanks for your review, I’ll make all the other changes above in V2.
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
