On Thu, Aug 30, 2018 at 09:28:52AM -0600, Eric Snowberg wrote:
> > On Aug 30, 2018, at 8:06 AM, Daniel Kiper <daniel.ki...@oracle.com> wrote:
> > On Tue, Jul 17, 2018 at 09:59:33AM -0600, Eric Snowberg wrote:
> >>> On Jul 17, 2018, at 7:38 AM, Daniel Kiper <daniel.ki...@oracle.com> wrote:
> >>> On Mon, Jul 16, 2018 at 09:33:17AM -0600, Eric Snowberg wrote:
> >>>>> On Jul 16, 2018, at 7:51 AM, Daniel Kiper <daniel.ki...@oracle.com>
> >>>>> 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 <daniel.ki...@oracle.com>
> >>>>>>> wrote:
> >>>>>>> On Fri, Jun 15, 2018 at 09:58:56AM -0600, Eric Snowberg wrote:
> >>>>>>>>> On Jun 15, 2018, at 6:02 AM, Daniel Kiper <daniel.ki...@oracle.com>
> >>>>>>>>> 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//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0__0)
> >> (ieee1275//pci@308/pc
> >> i@1/usb@0/hub@1/storage@3/disk@0__0,gpt1)
> >> (ieee1275//pci@306/pci@1/LSI__mrsas@0
> >> /disk@0__0) (ieee1275//pci@306/pci@1/LSI__mrsas@0/disk@0__0,gpt3)
> >> (ieee1275//pc
> >> i@306/pci@1/LSI__mrsas@0/disk@0__0,gpt2)
> >> (ieee1275//pci@306/pci@1/LSI__mrsas@0/
> >> disk@0__0,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//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0\\,0)
> >
> > or
> >
> > (ieee1275//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0\\,0,gpt1)
> >
> > etc.
>
> I don’t believe you have escaped the name properly above. Unless you
> are suggesting substituting ‘\\’ with “\\” before the item is
I think that it is correct. If you use one '\' then after shell
processing you will get
(ieee1275//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0,0)
or
(ieee1275//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0,0,gpt1)
And I suppose that this is not what you want. So, you need two '\'.
This way the layer below will get after shell processing
(ieee1275//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0\,0)
or
(ieee1275//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0\,0,gpt1)
Then new driver should get
ieee1275//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0,0
or
ieee1275//pci@308/pci@1/usb@0/hub@1/storage@3/disk@0\,0
if you really need escaped ',' in the path. However, I do not think so.
It seems to me that OF expects ',' as ','. Hence, I have a feeling that
we can reduce number of escaping/unescaping in the driver.
Am I right?
> displayed. If that is acceptable, would you accept this change
> instead?
>
> +static char *
> +replace_escaped_commas (char *src)
> +{
> + char *iptr;
> +
> + if (src == NULL)
> + return NULL;
> + for (iptr = src; *iptr; )
> + {
> + if ((*iptr == '\\') && (*(iptr + 1) == ','))
> + {
> + *iptr++ = '\';
> + *iptr++ = '\';
Plus
*iptr++ = ',';
> + }
> + iptr++;
> + }
> +
> + return src;
> +}
>
>
> >
> > 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.
> >
>
> It is already documented here:
> https://www.gnu.org/software/grub/manual/grub/grub.html#Device-syntax
OK but this does not discuss shell processing of '\' which is important
here. So, I think that doc should be updated.
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
