Re: [PATCH] parser: Remove escape from the state transitions

[Eric Snowberg]

> On Oct 16, 2017, at 2:44 AM, Daniel Kiper <address@hidden> wrote:
> 
> On Fri, Oct 13, 2017 at 08:53:23AM -0600, Eric Snowberg wrote:
>>> On Oct 13, 2017, at 3:36 AM, Daniel Kiper <address@hidden> wrote:
>>> On Thu, Oct 12, 2017 at 08:38:51AM -0600, Eric Snowberg wrote:
>>>>> On Oct 12, 2017, at 5:43 AM, Daniel Kiper <address@hidden> wrote:
>>>>> On Mon, Oct 09, 2017 at 09:23:44AM -0600, Eric Snowberg wrote:
>>>>>>> On Oct 9, 2017, at 5:48 AM, Daniel Kiper <address@hidden> wrote:
>>>>>>> On Fri, Oct 06, 2017 at 09:37:54AM -0600, Eric Snowberg wrote:
>>>>>>>>> On Oct 6, 2017, at 8:47 AM, Daniel Kiper <address@hidden> wrote:
>>>>>>>>> On Tue, May 30, 2017 at 04:07:52PM -0600, Eric Snowberg wrote:
>>>>>>>>>> Remove GRUB_PARSER_STATE_ESC with state GRUB_PARSER_STATE_TEXT from
>>>>>>>>>> the list of not allowed characters.
>>>>>>>>> 
>>>>>>>>> Once again, NACK for this patch. I explained why earlier but...
>>>>>>>>> 
>>>>>>>>>> This fixes a problem where a properly escaped comma is in the disk 
>>>>>>>>>> path.
>>>>>>>>>> 
>>>>>>>>>> For example: 
>>>>>>>>>> /address@hidden/address@hidden/LSI,address@hidden/address@hidden:a
>>>>>>>>>> 
>>>>>>>>>> During grub install, the search.fs_uuid is correctly stored in the
>>>>>>>>>> core.img.
>>>>>>>>>> 
>>>>>>>>>> As seen here:
>>>>>>>>>> 
>>>>>>>>>> 001e380: 7365 6172 6368 2e66 735f 7575 6964 2039  search.fs_uuid 9
>>>>>>>>>> 001e390: 6462 6137 6333 362d 6431 6432 2d34 6163  dba7c36-d1d2-4ac
>>>>>>>>>> 001e3a0: 642d 6135 3037 2d30 3634 6132 3462 3538  d-a507-064a24b58
>>>>>>>>>> 001e3b0: 3666 3420 726f 6f74 2069 6565 6531 3237  6f4 root ieee127
>>>>>>>>>> 001e3c0: 352f 2f70 6369 4033 3036 2f70 6369 4031  
>>>>>>>>>> 5//address@hidden/address@hidden
>>>>>>>>>> 001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  
>>>>>>>>>> /LSI\,address@hidden/di
>>>>>>>>>> 001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  address@hidden:a 
>>>>>>>>>> .set pref
>>>>>>>>>> 001e3f0: 6978 3d28 2472 6f6f 7429 272f 6772 7562  ix=($root)'/grub
>>>>>>>>>> 001e400: 3227 0a00 0000 0000 0000 0003 0000 0010  2'..............
>>>>>>>>>> 001e410: 2f67 7275 6232 0000                      /grub2..
>>>>>>>>>> 
>>>>>>>>>> Before this change the following args would be sent to
>>>>>>>>>> grub_cmd_do_search:
>>>>>>>>>> 
>>>>>>>>>> key: 9dba7c36-d1d2-4acd-a507-064a24b586f4
>>>>>>>>>> var: root
>>>>>>>>>> hint: 
>>>>>>>>>> ieee1275//address@hidden/address@hidden/LSI,address@hidden/address@hidden:a
>>>>>>>>> 
>>>>>>>>> ...because hint should be quoted in core.img using double quotes or 
>>>>>>>>> even single quotes...
>>>>>>>>> Or every control char should be escaped. Normal shell rules apply 
>>>>>>>>> here.
>>>>>>>> 
>>>>>>>> Hints are written during the install into the core.img. Once the system
>>>>>>>> boots, the parser is used to retrieve information from the core.img.
>>>>>>>> Currently the parser will strip double quotes, single quotes and 
>>>>>>>> escapes.
>>>>>>>> So I don’t understand how you recommend fixing this then.
>>>>>>> 
>>>>>>> Could you send me or point a script which creates embedded config for 
>>>>>>> you?
>>>>>> 
>>>>>> There is no script.
>>>>>> 
>>>>>> As I explained in the patch.  If your boot device name has a comma, which
>>>>>> it does with a Megaraid, you can not boot GRUB.
>>>>>> 
>>>>>> Install as follows:
>>>>>> 
>>>>>> $ grub-install —force /dev/sda1
>>>>>> 
>>>>>> By default it creates a core.img with what I provided in the git comment:
>>>>>> 
>>>>>> 001e380: 7365 6172 6368 2e66 735f 7575 6964 2039  search.fs_uuid 9
>>>>>> 001e390: 6462 6137 6333 362d 6431 6432 2d34 6163  dba7c36-d1d2-4ac
>>>>>> 001e3a0: 642d 6135 3037 2d30 3634 6132 3462 3538  d-a507-064a24b58
>>>>>> 001e3b0: 3666 3420 726f 6f74 2069 6565 6531 3237  6f4 root ieee127
>>>>>> 001e3c0: 352f 2f70 6369 4033 3036 2f70 6369 4031  
>>>>>> 5//address@hidden/address@hidden
>>>>>> 001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,address@hidden/di
>>>>>> 001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  address@hidden:a .set 
>>>>>> pref
>>>>>> 001e3f0: 6978 3d28 2472 6f6f 7429 272f 6772 7562  ix=($root)'/grub
>>>>>> 001e400: 3227 0a00 0000 0000 0000 0003 0000 0010  2'..............
>>>>>> 001e410: 2f67 7275 6232 0000                      /grub2..
>>>>>> 
>>>>>> As you can see, everything is escaped as GRUB expects.
>>>>>> 
>>>>>> Now during boot, the parser is used.  Without my patch, it will strip 
>>>>>> the \,.
>>>>> 
>>>>> AIUI you mean it strips '\'. If yes then it is correct behavior. And it
>>>>> should stay as is. If you wish to leave '\' you have to quote hint.
>>>>> Hence probably you have to fiddle with grub-install code or whatever
>>>>> creates the hint.Or the hint consumer code to properly consume ',' alone.
>>>>> 
>>>>>> So it changes the hint from:
>>>>>> 
>>>>>> ieee1275//address@hidden/address@hidden/LSI,address@hidden/address@hidden:a
>>>>>> to
>>>>>> ieee1275//address@hidden/address@hidden/LSI\,address@hidden/address@hidden:a
>>>>>> 
>>>>>> Later on, when it tries to use this disk, it incorrectly truncates
>>>>>> the device name since the comma isn’t escaped and tries to do the
>>>>>> grub_disk_open with ieee1275//address@hidden/address@hidden/LSI.
>>>>> 
>>>>> I am not sure who strips everything after the ','. Whoever it is it is
>>>>> not the parser for sure. There is a chance that you should look for
>>>>> problem here.
>>>> 
>>>> As I pointed out in the past, this is what strips everything after the
>>>> ‘,’ during boot. This is called after the parser has stripped the ‘\’.
>>>> 
>>>> grub-core/kern/disk.c
>>>> 
>>>> /* Return the location of the first ',', if any, which is not
>>>> escaped by a '\'.  */
>>>> static const char *
>>>> find_part_sep (const char *name)
>>>> 
>>>> Changing this would impact every platform. Also, it was my understanding
>>>> that disks were to follow this encoding style for commas.  Since it is
>>>> an easy way to find the disk partition. Your recommending this be changed 
>>>> now?
>>>> And you would approve such a patch?
>>> 
>>> Nope, I told you that you should check where it happens. And if it is done 
>>> by
>>> purpose then you should not touch it. And it looks that it is. So, as I told
>>> you earlier you have to quote the hint. Otherwise, '\' will be always 
>>> stripped
>>> by the parser. This is its normal behavior if string is not quoted.
>> 
>> It seems like there are two parts of GRUB that are not compatible with one
> 
> I do not think so.
> 
>> another if a disk name contains a comma.  There is a parser which strips
>> the escaped commas and there is the base disk driver that expects them.
> 
> It does this because string is not quoted. I am not sure why are you
> surprised here. Once again: just quote the string properly and your
> problem is gone.
> 
>> If you do not believe this is the case, please provide an example of how
> 
> I have never ever said that I do not believe this is the case. I say
> that your fix is unacceptable and you should find other way to fix
> the issue. Even I said how to fix it.
> 
>> this disk can be quoted properly to work with both the parser and the
>> disk driver during boot:
>> 
>> /address@hidden/address@hidden/LSI,address@hidden/address@hidden,0
> 
> I am a bit surprised that you are not able to quote a string but I will
> give you some examples:
> 
> "/address@hidden/address@hidden/LSI\,address@hidden/address@hidden,0"
> '/address@hidden/address@hidden/LSI\,address@hidden/address@hidden,0’
> /address@hidden/address@hidden/LSI\\,address@hidden/address@hidden,0


I just tried all three combinations above, none of which worked. All failed 
within the disk open during boot.

I’m moving on with my other patches now. If I get time in the future I’ll look 
at solving this a different way.  But for the moment, trying to boot with 
search.fs_uuid does not work if a disk contains a comma.