Re: [PATCH v2] ieee1275: obdisk driver

[Daniel Kiper]

On Tue, Jul 17, 2018 at 09:59:33AM -0600, Eric Snowberg wrote:
> > On Jul 17, 2018, at 7:38 AM, Daniel Kiper <address@hidden> wrote:
> > On Mon, Jul 16, 2018 at 09:33:17AM -0600, Eric Snowberg wrote:
> >>> On Jul 16, 2018, at 7:51 AM, Daniel Kiper <address@hidden> wrote:
> >>>
> >>> 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?
> >>
> >> This goes much further than the command line.  For example, it is
> >> built right into the disk driver.  Look at grub-core/kern/disk.c: 544
> >
> > This is the last line of the file. So, I am not sure what exactly you mean.
> >
> >> /* Return the location of the first ',', if any, which is not
> >>   escaped by a '\'.  */
> >> static const char *
> >> find_part_sep (const char *name)
> >> {
> >>  const char *p = name;
> >>  char c;
> >>
> >>  while ((c = *p++) != '\0')
> >>    {
> >>      if (c == '\\' && *p == ',')
> >>        p++;
> >>      else if (c == ',')
> >>        return p - 1;
> >>    }
> >>  return NULL;
> >> }
> >
> > OK, this one.
> >
> >> When the obdisk driver discovers a disk, it must put the disk name in
> >> the proper format.  Otherwise when grub_disk_open takes place later
> >> on, the wrong disk name will eventually get sent back to the obdisk
> >> driver.
> >
> > Then we need proper escaping. And AIUI your driver does that.
> >
> >>> 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.
> >>
> >> Ok
> >>
> >>>
> >>>>> 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.
> >>
> >> Ok
> >>
> >>>
> >>> What is replace_escaped_commas()? Why do we need that function?
> >>
> >> It is a convenience function for the end-user, so they can access a
> >> disk without having to understand all this escaping when they want to
> >> use one thru the shell.
> >
> > I think that this will introduce more confusion. I would like that
> > escaping of drive paths should be properly explained in GRUB docs.
> > Including why it is needed. And replace_escaped_commas() should be
> > dropped.
>
> The confusion seems to be around what needs to be escaped and what
> doesn’t, as can be seen from the discussion within this email. This
> change makes it convenient for the end-user, since they will not need
> to understand any of this.
>
> When function grub_obdisk_iterate (defined as .iterate within
> grub_disk_dev) gets called, it returns the disks the driver controls.
> As explained within the description of this patch, a single IEEE1275
> disk can have over 6 names.  When the .iterate function is called,
> only a single drive can be returned.  If the disk that is to be
> returned contains \\, within the name, they are replaced with __.
> Then for example, the end-user will see something like this following
> a ls:
>
> grub> ls
> (ieee1275//address@hidden/address@hidden/address@hidden/address@hidden/address@hidden/address@hidden)
>  (ieee1275//address@hidden/pc
> address@hidden/address@hidden/address@hidden/address@hidden/address@hidden,gpt1)
>  (ieee1275//address@hidden/address@hidden/address@hidden
> /address@hidden) 
> (ieee1275//address@hidden/address@hidden/address@hidden/address@hidden,gpt3) 
> (ieee1275//pc
> address@hidden/address@hidden/address@hidden/address@hidden,gpt2) 
> (ieee1275//address@hidden/address@hidden/address@hidden/
> address@hidden,gpt1)
>
> The end-user can now type the disk name exactly as it is returned on
> the screen.  Or they can escape any of the real disk names properly
> and the driver will understand it is the same disk.  Do you really
> want this removed?

After some thinking it seems to me that we should remove this "__"
feature. It introduces another specific escaping form. And IMO this will
make more confusion then it is worth. And what if disk path contains
"__"? Yeah, I know probably it is not the case right now but we should
be prepared... Though I am not against displaying properly escaped
disks and partitions paths, e.g.:

(ieee1275//address@hidden/address@hidden/address@hidden/address@hidden/address@hidden/address@hidden,0)

or

(ieee1275//address@hidden/address@hidden/address@hidden/address@hidden/address@hidden/address@hidden,0,gpt1)

etc.

Additionally, I think that you should update GRUB2 docs in relevant places
and add an info saying that disk paths containing "," should be properly
escaped.

Daniel