Re: [PATCH] ieee1275: obdisk driver

[Eric Snowberg]

> On Apr 11, 2018, at 4:29 AM, Daniel Kiper <address@hidden> 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. */
>> +  "/address@hidden",
>> +  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                    
>> /address@hidden/address@hidden/address@hidden/address@hidden
>> +     disk2                    
>> /address@hidden/address@hidden/address@hidden/address@hidden
>> +     disk1                    
>> /address@hidden/address@hidden/address@hidden/address@hidden
>> +     disk                     
>> /address@hidden/address@hidden/address@hidden/address@hidden
>> +     disk0                    
>> /address@hidden/address@hidden/address@hidden/address@hidden
>> +
>> +     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.