Re: [PATCH v2] ieee1275: obdisk driver

[Eric Snowberg]

> On Jun 21, 2018, at 10:58 AM, Daniel Kiper <address@hidden> wrote:
> 
> 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 ",”,

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.