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.
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
