Re: [PATCH v2] ieee1275: obdisk driver

[Daniel Kiper]

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 <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 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