> On Sep 1, 2018, at 11:10 AM, Daniel Kiper <daniel.ki...@oracle.com> wrote:
>
> 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.
For obdisk devices that are displayed thru the shell, I didn't want the escaped
‘,’ in the path. But you rejected my change that substituted it with __
instead. Therefore we are left with making this work with code above obdisk
without changing it.
> 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?
No.
The driver provides the name of the device which is displayed in the shell. It
must be in a format that will allow it to get back to the driver at a later
time. If a ‘,’ is in the name, it must be escaped. Otherwise the code above
the driver will trim off everything to the right of the ‘,’. Then the driver
will be sent a device name that does not exist.
For example with this device:
/pci@306/pci@1/LSI,mrsas@0/disk@0,0,gpt3
If the ‘,’ is not escaped properly, either (obdisk or ofdisk) will be sent an
open to device: /pci@306/pci@1/LSI. The upper level code will think everything
to the right of the ‘,’ is the partition. Once the driver receives this open,
it will fail, since it isn’t a valid device.
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
