Re: [PATCH v2] ieee1275: obdisk driver

[Daniel Kiper]

On Fri, Jun 15, 2018 at 09:58:56AM -0600, Eric Snowberg wrote:
> > On Jun 15, 2018, at 6:02 AM, Daniel Kiper <address@hidden> 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