[PATCH] parser: Remove escape from the state transitions

[PATCH] parser: Remove escape from the state transitions

[Eric Snowberg]

Remove GRUB_PARSER_STATE_ESC with state GRUB_PARSER_STATE_TEXT from
the list of not allowed characters.

This fixes a problem where a properly escaped comma is in the disk path.

For example: /pci@306/pci@1/LSI,mrsas@0/disk@0: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//pci@306/pci@1
 001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,mrsas@0/di
 001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  sk@0: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//pci@306/pci@1/LSI,mrsas@0/disk@0:a

The hint above is not correct.  It should be:

hint: ieee1275//pci@306/pci@1/LSI\,mrsas@0/disk@0: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//pci@306/pci@1/LSI.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
 grub-core/kern/parser.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/grub-core/kern/parser.c b/grub-core/kern/parser.c
index 78175aa..be88baa 100644
--- a/grub-core/kern/parser.c
+++ b/grub-core/kern/parser.c
@@ -30,7 +30,6 @@ static struct grub_parser_state_transition state_transitions[] = {
   {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_QUOTE, '\'', 0},
   {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_DQUOTE, '\"', 0},
   {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_VAR, '$', 0},
-  {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_ESC, '\\', 0},
 
   {GRUB_PARSER_STATE_ESC, GRUB_PARSER_STATE_TEXT, 0, 1},
 
-- 
1.7.1

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

[Daniel Kiper]

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.
>
> This fixes a problem where a properly escaped comma is in the disk path.
>
> For example: /pci@306/pci@1/LSI,mrsas@0/disk@0: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//pci@306/pci@1
>  001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,mrsas@0/di
>  001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  sk@0: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//pci@306/pci@1/LSI,mrsas@0/disk@0:a
>
> The hint above is not correct.  It should be:
>
> hint: ieee1275//pci@306/pci@1/LSI\,mrsas@0/disk@0: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//pci@306/pci@1/LSI.
>
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
>  grub-core/kern/parser.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/grub-core/kern/parser.c b/grub-core/kern/parser.c
> index 78175aa..be88baa 100644
> --- a/grub-core/kern/parser.c
> +++ b/grub-core/kern/parser.c
> @@ -30,7 +30,6 @@ static struct grub_parser_state_transition state_transitions[] = {
>    {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_QUOTE, '\'', 0},
>    {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_DQUOTE, '\"', 0},
>    {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_VAR, '$', 0},
> -  {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_ESC, '\\', 0},

I have a feeling that this way you break parser for everybody. Could you
explain why it is valid for all? Or if issue is specific for SPARC only
please fix it in SPARC related code (e.g. store unescaped form in core.img).
I would prefer to not touch this file if we fix something only for one platform.

Daniel

PS Next time please CC on the patch Alex and Andrei too.

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

[Eric Snowberg]

> On Jun 2, 2017, at 4:41 AM, Daniel Kiper <daniel.kiper@oracle.com> 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.
>> 
>> This fixes a problem where a properly escaped comma is in the disk path.
>> 
>> For example: /pci@306/pci@1/LSI,mrsas@0/disk@0: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//pci@306/pci@1
>> 001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,mrsas@0/di
>> 001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  sk@0: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//pci@306/pci@1/LSI,mrsas@0/disk@0:a
>> 
>> The hint above is not correct.  It should be:
>> 
>> hint: ieee1275//pci@306/pci@1/LSI\,mrsas@0/disk@0: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//pci@306/pci@1/LSI.
>> 
>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
>> ---
>> grub-core/kern/parser.c |    1 -
>> 1 files changed, 0 insertions(+), 1 deletions(-)
>> 
>> diff --git a/grub-core/kern/parser.c b/grub-core/kern/parser.c
>> index 78175aa..be88baa 100644
>> --- a/grub-core/kern/parser.c
>> +++ b/grub-core/kern/parser.c
>> @@ -30,7 +30,6 @@ static struct grub_parser_state_transition state_transitions[] = {
>>   {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_QUOTE, '\'', 0},
>>   {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_DQUOTE, '\"', 0},
>>   {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_VAR, '$', 0},
>> -  {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_ESC, '\\', 0},
> 
> I have a feeling that this way you break parser for everybody.

Can you explain what would break on other platforms with this change?

> Could you explain why it is valid for all?

Any platform containing a device with a comma in it would have the problem I identified above.

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

[Daniel Kiper]

On Mon, Jun 05, 2017 at 08:29:47AM -0600, Eric Snowberg wrote:
> > On Jun 2, 2017, at 4:41 AM, Daniel Kiper <daniel.kiper@oracle.com> 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.
> >>
> >> This fixes a problem where a properly escaped comma is in the disk path.
> >>
> >> For example: /pci@306/pci@1/LSI,mrsas@0/disk@0: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//pci@306/pci@1
> >> 001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,mrsas@0/di
> >> 001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  sk@0: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//pci@306/pci@1/LSI,mrsas@0/disk@0:a
> >>
> >> The hint above is not correct.  It should be:
> >>
> >> hint: ieee1275//pci@306/pci@1/LSI\,mrsas@0/disk@0: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//pci@306/pci@1/LSI.
> >>
> >> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> >> ---
> >> grub-core/kern/parser.c |    1 -
> >> 1 files changed, 0 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/grub-core/kern/parser.c b/grub-core/kern/parser.c
> >> index 78175aa..be88baa 100644
> >> --- a/grub-core/kern/parser.c
> >> +++ b/grub-core/kern/parser.c
> >> @@ -30,7 +30,6 @@ static struct grub_parser_state_transition state_transitions[] = {
> >>   {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_QUOTE, '\'', 0},
> >>   {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_DQUOTE, '\"', 0},
> >>   {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_VAR, '$', 0},
> >> -  {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_ESC, '\\', 0},
> >
> > I have a feeling that this way you break parser for everybody.
>
> Can you explain what would break on other platforms with this change?

After your patch embedded config is parsed incorrectly, e.g. "set a=\f"
sets $a to "\f" instead of "f".

> > Could you explain why it is valid for all?
>
> Any platform containing a device with a comma in it would have the
> problem I identified above.

So, I think that you should store unescaped strings in core.img if it is required.

Daniel

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

[Eric Snowberg]

> On Jun 7, 2017, at 1:23 PM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> 
> On Mon, Jun 05, 2017 at 08:29:47AM -0600, Eric Snowberg wrote:
>>> On Jun 2, 2017, at 4:41 AM, Daniel Kiper <daniel.kiper@oracle.com> 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.
>>>> 
>>>> This fixes a problem where a properly escaped comma is in the disk path.
>>>> 
>>>> For example: /pci@306/pci@1/LSI,mrsas@0/disk@0: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//pci@306/pci@1
>>>> 001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,mrsas@0/di
>>>> 001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  sk@0: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//pci@306/pci@1/LSI,mrsas@0/disk@0:a
>>>> 
>>>> The hint above is not correct.  It should be:
>>>> 
>>>> hint: ieee1275//pci@306/pci@1/LSI\,mrsas@0/disk@0: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//pci@306/pci@1/LSI.
>>>> 
>>>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
>>>> ---
>>>> grub-core/kern/parser.c |    1 -
>>>> 1 files changed, 0 insertions(+), 1 deletions(-)
>>>> 
>>>> diff --git a/grub-core/kern/parser.c b/grub-core/kern/parser.c
>>>> index 78175aa..be88baa 100644
>>>> --- a/grub-core/kern/parser.c
>>>> +++ b/grub-core/kern/parser.c
>>>> @@ -30,7 +30,6 @@ static struct grub_parser_state_transition state_transitions[] = {
>>>>  {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_QUOTE, '\'', 0},
>>>>  {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_DQUOTE, '\"', 0},
>>>>  {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_VAR, '$', 0},
>>>> -  {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_ESC, '\\', 0},
>>> 
>>> I have a feeling that this way you break parser for everybody.
>> 
>> Can you explain what would break on other platforms with this change?
> 
> After your patch embedded config is parsed incorrectly, e.g. "set a=\f"
> sets $a to "\f" instead of "f”.

That is a useful config?

> 
>>> Could you explain why it is valid for all?
>> 
>> Any platform containing a device with a comma in it would have the
>> problem I identified above.
> 
> So, I think that you should store unescaped strings in core.img if it is required.

Throughout GRUB a comma is escaped unless it is before a partition number. If I store it unescaped in the core.img I would still have the same problem above when going into grub_disk_open.

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

[Daniel Kiper]

On Wed, Jun 07, 2017 at 02:48:04PM -0600, Eric Snowberg wrote:
> > On Jun 7, 2017, at 1:23 PM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> > On Mon, Jun 05, 2017 at 08:29:47AM -0600, Eric Snowberg wrote:
> >>> On Jun 2, 2017, at 4:41 AM, Daniel Kiper <daniel.kiper@oracle.com> 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.
> >>>>
> >>>> This fixes a problem where a properly escaped comma is in the disk path.
> >>>>
> >>>> For example: /pci@306/pci@1/LSI,mrsas@0/disk@0: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//pci@306/pci@1
> >>>> 001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,mrsas@0/di
> >>>> 001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  sk@0: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//pci@306/pci@1/LSI,mrsas@0/disk@0:a
> >>>>
> >>>> The hint above is not correct.  It should be:
> >>>>
> >>>> hint: ieee1275//pci@306/pci@1/LSI\,mrsas@0/disk@0: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//pci@306/pci@1/LSI.
> >>>>
> >>>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> >>>> ---
> >>>> grub-core/kern/parser.c |    1 -
> >>>> 1 files changed, 0 insertions(+), 1 deletions(-)
> >>>>
> >>>> diff --git a/grub-core/kern/parser.c b/grub-core/kern/parser.c
> >>>> index 78175aa..be88baa 100644
> >>>> --- a/grub-core/kern/parser.c
> >>>> +++ b/grub-core/kern/parser.c
> >>>> @@ -30,7 +30,6 @@ static struct grub_parser_state_transition state_transitions[] = {
> >>>>  {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_QUOTE, '\'', 0},
> >>>>  {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_DQUOTE, '\"', 0},
> >>>>  {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_VAR, '$', 0},
> >>>> -  {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_ESC, '\\', 0},
> >>>
> >>> I have a feeling that this way you break parser for everybody.
> >>
> >> Can you explain what would break on other platforms with this change?
> >
> > After your patch embedded config is parsed incorrectly, e.g. "set a=\f"
> > sets $a to "\f" instead of "f”.
>
> That is a useful config?

Sorry, I do not understand the question.

> >>> Could you explain why it is valid for all?
> >>
> >> Any platform containing a device with a comma in it would have the
> >> problem I identified above.
> >
> > So, I think that you should store unescaped strings in core.img if it is required.
>
> Throughout GRUB a comma is escaped unless it is before a partition number.
> If I store it unescaped in the core.img I would still have the same problem
> above when going into grub_disk_open.

I understand that this can be a problem for you and I am happy that you wish
to fix it. However, I do not accept breakage of the parser as a solution for
this problem. At least in such form (by the way, even if we assume that patch
is correct it seems to me incomplete after closer look). Sorry. So, I am looking
forward for another patch(es) which properly fixes the issue.

Daniel

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

[Eric Snowberg]

> On Jun 7, 2017, at 3:36 PM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> 
> On Wed, Jun 07, 2017 at 02:48:04PM -0600, Eric Snowberg wrote:
>>> On Jun 7, 2017, at 1:23 PM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>> On Mon, Jun 05, 2017 at 08:29:47AM -0600, Eric Snowberg wrote:
>>>>> On Jun 2, 2017, at 4:41 AM, Daniel Kiper <daniel.kiper@oracle.com> 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.
>>>>>> 
>>>>>> This fixes a problem where a properly escaped comma is in the disk path.
>>>>>> 
>>>>>> For example: /pci@306/pci@1/LSI,mrsas@0/disk@0: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//pci@306/pci@1
>>>>>> 001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,mrsas@0/di
>>>>>> 001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  sk@0: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//pci@306/pci@1/LSI,mrsas@0/disk@0:a
>>>>>> 
>>>>>> The hint above is not correct.  It should be:
>>>>>> 
>>>>>> hint: ieee1275//pci@306/pci@1/LSI\,mrsas@0/disk@0: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//pci@306/pci@1/LSI.
>>>>>> 
>>>>>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
>>>>>> ---
>>>>>> grub-core/kern/parser.c |    1 -
>>>>>> 1 files changed, 0 insertions(+), 1 deletions(-)
>>>>>> 
>>>>>> diff --git a/grub-core/kern/parser.c b/grub-core/kern/parser.c
>>>>>> index 78175aa..be88baa 100644
>>>>>> --- a/grub-core/kern/parser.c
>>>>>> +++ b/grub-core/kern/parser.c
>>>>>> @@ -30,7 +30,6 @@ static struct grub_parser_state_transition state_transitions[] = {
>>>>>> {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_QUOTE, '\'', 0},
>>>>>> {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_DQUOTE, '\"', 0},
>>>>>> {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_VAR, '$', 0},
>>>>>> -  {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_ESC, '\\', 0},
>>>>> 
>>>>> I have a feeling that this way you break parser for everybody.
>>>> 
>>>> Can you explain what would break on other platforms with this change?
>>> 
>>> After your patch embedded config is parsed incorrectly, e.g. "set a=\f"
>>> sets $a to "\f" instead of "f”.
>> 
>> That is a useful config?
> 
> Sorry, I do not understand the question.
> 
>>>>> Could you explain why it is valid for all?
>>>> 
>>>> Any platform containing a device with a comma in it would have the
>>>> problem I identified above.
>>> 
>>> So, I think that you should store unescaped strings in core.img if it is required.
>> 
>> Throughout GRUB a comma is escaped unless it is before a partition number.
>> If I store it unescaped in the core.img I would still have the same problem
>> above when going into grub_disk_open.
> 
> I understand that this can be a problem for you and I am happy that you wish
> to fix it. However, I do not accept breakage of the parser as a solution for
> this problem. At least in such form (by the way, even if we assume that patch
> is correct it seems to me incomplete after closer look). Sorry. So, I am looking
> forward for another patch(es) which properly fixes the issue.

Could you provide a recommendation on how to fix it then?

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

[Daniel Kiper]

On Wed, Jun 07, 2017 at 03:43:34PM -0600, Eric Snowberg wrote:
>
> > On Jun 7, 2017, at 3:36 PM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> >
> > On Wed, Jun 07, 2017 at 02:48:04PM -0600, Eric Snowberg wrote:
> >>> On Jun 7, 2017, at 1:23 PM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> >>> On Mon, Jun 05, 2017 at 08:29:47AM -0600, Eric Snowberg wrote:
> >>>>> On Jun 2, 2017, at 4:41 AM, Daniel Kiper <daniel.kiper@oracle.com> 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.
> >>>>>>
> >>>>>> This fixes a problem where a properly escaped comma is in the disk path.
> >>>>>>
> >>>>>> For example: /pci@306/pci@1/LSI,mrsas@0/disk@0: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//pci@306/pci@1
> >>>>>> 001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,mrsas@0/di
> >>>>>> 001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  sk@0: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//pci@306/pci@1/LSI,mrsas@0/disk@0:a
> >>>>>>
> >>>>>> The hint above is not correct.  It should be:
> >>>>>>
> >>>>>> hint: ieee1275//pci@306/pci@1/LSI\,mrsas@0/disk@0: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//pci@306/pci@1/LSI.
> >>>>>>
> >>>>>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> >>>>>> ---
> >>>>>> grub-core/kern/parser.c |    1 -
> >>>>>> 1 files changed, 0 insertions(+), 1 deletions(-)
> >>>>>>
> >>>>>> diff --git a/grub-core/kern/parser.c b/grub-core/kern/parser.c
> >>>>>> index 78175aa..be88baa 100644
> >>>>>> --- a/grub-core/kern/parser.c
> >>>>>> +++ b/grub-core/kern/parser.c
> >>>>>> @@ -30,7 +30,6 @@ static struct grub_parser_state_transition state_transitions[] = {
> >>>>>> {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_QUOTE, '\'', 0},
> >>>>>> {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_DQUOTE, '\"', 0},
> >>>>>> {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_VAR, '$', 0},
> >>>>>> -  {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_ESC, '\\', 0},
> >>>>>
> >>>>> I have a feeling that this way you break parser for everybody.
> >>>>
> >>>> Can you explain what would break on other platforms with this change?
> >>>
> >>> After your patch embedded config is parsed incorrectly, e.g. "set a=\f"
> >>> sets $a to "\f" instead of "f”.
> >>
> >> That is a useful config?
> >
> > Sorry, I do not understand the question.
> >
> >>>>> Could you explain why it is valid for all?
> >>>>
> >>>> Any platform containing a device with a comma in it would have the
> >>>> problem I identified above.
> >>>
> >>> So, I think that you should store unescaped strings in core.img if it is required.
> >>
> >> Throughout GRUB a comma is escaped unless it is before a partition number.
> >> If I store it unescaped in the core.img I would still have the same problem
> >> above when going into grub_disk_open.

Hmmm... Does it (un)escape paths?

> > I understand that this can be a problem for you and I am happy that you wish
> > to fix it. However, I do not accept breakage of the parser as a solution for
> > this problem. At least in such form (by the way, even if we assume that patch
> > is correct it seems to me incomplete after closer look). Sorry. So, I am looking
> > forward for another patch(es) which properly fixes the issue.
>
> Could you provide a recommendation on how to fix it then?

I think that we should store and use everywhere if possible unescaped
texts. Escaped versions should be used only if required, e.g. if we
pass something to a shell. On the other hand if we get as an input
escaped form we should unsecape it immediately. This way we avoid
a lot of problems coming from mixing escaped and unescaped forms
in various places.

Daniel

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

[Eric Snowberg]

> On Jun 13, 2017, at 6:59 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> 
> On Wed, Jun 07, 2017 at 03:43:34PM -0600, Eric Snowberg wrote:
>> 
>>> On Jun 7, 2017, at 3:36 PM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>> 
>>> On Wed, Jun 07, 2017 at 02:48:04PM -0600, Eric Snowberg wrote:
>>>>> On Jun 7, 2017, at 1:23 PM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>>>> On Mon, Jun 05, 2017 at 08:29:47AM -0600, Eric Snowberg wrote:
>>>>>>> On Jun 2, 2017, at 4:41 AM, Daniel Kiper <daniel.kiper@oracle.com> 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.
>>>>>>>> 
>>>>>>>> This fixes a problem where a properly escaped comma is in the disk path.
>>>>>>>> 
>>>>>>>> For example: /pci@306/pci@1/LSI,mrsas@0/disk@0: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//pci@306/pci@1
>>>>>>>> 001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,mrsas@0/di
>>>>>>>> 001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  sk@0: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//pci@306/pci@1/LSI,mrsas@0/disk@0:a
>>>>>>>> 
>>>>>>>> The hint above is not correct.  It should be:
>>>>>>>> 
>>>>>>>> hint: ieee1275//pci@306/pci@1/LSI\,mrsas@0/disk@0: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//pci@306/pci@1/LSI.
>>>>>>>> 
>>>>>>>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
>>>>>>>> ---
>>>>>>>> grub-core/kern/parser.c |    1 -
>>>>>>>> 1 files changed, 0 insertions(+), 1 deletions(-)
>>>>>>>> 
>>>>>>>> diff --git a/grub-core/kern/parser.c b/grub-core/kern/parser.c
>>>>>>>> index 78175aa..be88baa 100644
>>>>>>>> --- a/grub-core/kern/parser.c
>>>>>>>> +++ b/grub-core/kern/parser.c
>>>>>>>> @@ -30,7 +30,6 @@ static struct grub_parser_state_transition state_transitions[] = {
>>>>>>>> {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_QUOTE, '\'', 0},
>>>>>>>> {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_DQUOTE, '\"', 0},
>>>>>>>> {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_VAR, '$', 0},
>>>>>>>> -  {GRUB_PARSER_STATE_TEXT, GRUB_PARSER_STATE_ESC, '\\', 0},
>>>>>>> 
>>>>>>> I have a feeling that this way you break parser for everybody.
>>>>>> 
>>>>>> Can you explain what would break on other platforms with this change?
>>>>> 
>>>>> After your patch embedded config is parsed incorrectly, e.g. "set a=\f"
>>>>> sets $a to "\f" instead of "f”.
>>>> 
>>>> That is a useful config?
>>> 
>>> Sorry, I do not understand the question.
>>> 
>>>>>>> Could you explain why it is valid for all?
>>>>>> 
>>>>>> Any platform containing a device with a comma in it would have the
>>>>>> problem I identified above.
>>>>> 
>>>>> So, I think that you should store unescaped strings in core.img if it is required.
>>>> 
>>>> Throughout GRUB a comma is escaped unless it is before a partition number.
>>>> If I store it unescaped in the core.img I would still have the same problem
>>>> above when going into grub_disk_open.
> 
> Hmmm... Does it (un)escape paths?

My patch does not unescape paths, it leaves them alone.  I think there is some confusion here.  The example you provided above is currently broken in the upstream code.  You would need this patch to get the result you requested.

before patch:
“set a=\f” sets $a to ‘f'

after patch:
“set a=\f” sets $a to ‘\f'


> 
>>> I understand that this can be a problem for you and I am happy that you wish
>>> to fix it. However, I do not accept breakage of the parser as a solution for
>>> this problem. At least in such form (by the way, even if we assume that patch
>>> is correct it seems to me incomplete after closer look). Sorry. So, I am looking
>>> forward for another patch(es) which properly fixes the issue.
>> 
>> Could you provide a recommendation on how to fix it then?
> 
> I think that we should store and use everywhere if possible unescaped
> texts. Escaped versions should be used only if required, e.g. if we
> pass something to a shell. On the other hand if we get as an input
> escaped form we should unsecape it immediately. This way we avoid
> a lot of problems coming from mixing escaped and unescaped forms
> in various places.

But this isn’t the way GRUB was designed:

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)

There are many other places in the code that make this same assumption.  You want this changed now?  This will impact every platform.

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

[Daniel Kiper]

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: /pci@306/pci@1/LSI,mrsas@0/disk@0: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//pci@306/pci@1
>  001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,mrsas@0/di
>  001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  sk@0: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//pci@306/pci@1/LSI,mrsas@0/disk@0: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.

Daniel

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

[Eric Snowberg]

> On Oct 6, 2017, at 8:47 AM, Daniel Kiper <dkiper@net-space.pl> 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: /pci@306/pci@1/LSI,mrsas@0/disk@0: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//pci@306/pci@1
>> 001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,mrsas@0/di
>> 001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  sk@0: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//pci@306/pci@1/LSI,mrsas@0/disk@0: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.

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

[Daniel Kiper]

On Fri, Oct 06, 2017 at 09:37:54AM -0600, Eric Snowberg wrote:
> > On Oct 6, 2017, at 8:47 AM, Daniel Kiper <dkiper@net-space.pl> 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: /pci@306/pci@1/LSI,mrsas@0/disk@0: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//pci@306/pci@1
> >> 001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,mrsas@0/di
> >> 001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  sk@0: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//pci@306/pci@1/LSI,mrsas@0/disk@0: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?

Daniel

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

[Eric Snowberg]

> On Oct 9, 2017, at 5:48 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> 
> On Fri, Oct 06, 2017 at 09:37:54AM -0600, Eric Snowberg wrote:
>>> On Oct 6, 2017, at 8:47 AM, Daniel Kiper <dkiper@net-space.pl> 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: /pci@306/pci@1/LSI,mrsas@0/disk@0: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//pci@306/pci@1
>>>> 001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,mrsas@0/di
>>>> 001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  sk@0: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//pci@306/pci@1/LSI,mrsas@0/disk@0: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//pci@306/pci@1
001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,mrsas@0/di
001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  sk@0: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 \,.

So it changes the hint from:

ieee1275//pci@306/pci@1/LSI,mrsas@0/disk@0:a
to
ieee1275//pci@306/pci@1/LSI\,mrsas@0/disk@0: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//pci@306/pci@1/LSI.

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

[Daniel Kiper]

On Mon, Oct 09, 2017 at 09:23:44AM -0600, Eric Snowberg wrote:
> > On Oct 9, 2017, at 5:48 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> > On Fri, Oct 06, 2017 at 09:37:54AM -0600, Eric Snowberg wrote:
> >>> On Oct 6, 2017, at 8:47 AM, Daniel Kiper <dkiper@net-space.pl> 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: /pci@306/pci@1/LSI,mrsas@0/disk@0: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//pci@306/pci@1
> >>>> 001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,mrsas@0/di
> >>>> 001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  sk@0: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//pci@306/pci@1/LSI,mrsas@0/disk@0: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//pci@306/pci@1
> 001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,mrsas@0/di
> 001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  sk@0: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//pci@306/pci@1/LSI,mrsas@0/disk@0:a
> to
> ieee1275//pci@306/pci@1/LSI\,mrsas@0/disk@0: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//pci@306/pci@1/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.

Daniel

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

[Eric Snowberg]

> On Oct 12, 2017, at 5:43 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> 
> On Mon, Oct 09, 2017 at 09:23:44AM -0600, Eric Snowberg wrote:
>>> On Oct 9, 2017, at 5:48 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>> On Fri, Oct 06, 2017 at 09:37:54AM -0600, Eric Snowberg wrote:
>>>>> On Oct 6, 2017, at 8:47 AM, Daniel Kiper <dkiper@net-space.pl> 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: /pci@306/pci@1/LSI,mrsas@0/disk@0: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//pci@306/pci@1
>>>>>> 001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,mrsas@0/di
>>>>>> 001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  sk@0: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//pci@306/pci@1/LSI,mrsas@0/disk@0: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//pci@306/pci@1
>> 001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,mrsas@0/di
>> 001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  sk@0: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//pci@306/pci@1/LSI,mrsas@0/disk@0:a
>> to
>> ieee1275//pci@306/pci@1/LSI\,mrsas@0/disk@0: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//pci@306/pci@1/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?

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

[Daniel Kiper]

On Thu, Oct 12, 2017 at 08:38:51AM -0600, Eric Snowberg wrote:
> > On Oct 12, 2017, at 5:43 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> > On Mon, Oct 09, 2017 at 09:23:44AM -0600, Eric Snowberg wrote:
> >>> On Oct 9, 2017, at 5:48 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> >>> On Fri, Oct 06, 2017 at 09:37:54AM -0600, Eric Snowberg wrote:
> >>>>> On Oct 6, 2017, at 8:47 AM, Daniel Kiper <dkiper@net-space.pl> 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: /pci@306/pci@1/LSI,mrsas@0/disk@0: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//pci@306/pci@1
> >>>>>> 001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,mrsas@0/di
> >>>>>> 001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  sk@0: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//pci@306/pci@1/LSI,mrsas@0/disk@0: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//pci@306/pci@1
> >> 001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,mrsas@0/di
> >> 001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  sk@0: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//pci@306/pci@1/LSI,mrsas@0/disk@0:a
> >> to
> >> ieee1275//pci@306/pci@1/LSI\,mrsas@0/disk@0: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//pci@306/pci@1/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.

Daniel

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

[Eric Snowberg]

> On Oct 13, 2017, at 3:36 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> 
> On Thu, Oct 12, 2017 at 08:38:51AM -0600, Eric Snowberg wrote:
>>> On Oct 12, 2017, at 5:43 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>> On Mon, Oct 09, 2017 at 09:23:44AM -0600, Eric Snowberg wrote:
>>>>> On Oct 9, 2017, at 5:48 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>>>> On Fri, Oct 06, 2017 at 09:37:54AM -0600, Eric Snowberg wrote:
>>>>>>> On Oct 6, 2017, at 8:47 AM, Daniel Kiper <dkiper@net-space.pl> 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: /pci@306/pci@1/LSI,mrsas@0/disk@0: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//pci@306/pci@1
>>>>>>>> 001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,mrsas@0/di
>>>>>>>> 001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  sk@0: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//pci@306/pci@1/LSI,mrsas@0/disk@0: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//pci@306/pci@1
>>>> 001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,mrsas@0/di
>>>> 001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  sk@0: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//pci@306/pci@1/LSI,mrsas@0/disk@0:a
>>>> to
>>>> ieee1275//pci@306/pci@1/LSI\,mrsas@0/disk@0: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//pci@306/pci@1/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 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.

If you do not believe this is the case, please provide an example of how this disk can be quoted properly to work with both the parser and the disk driver during boot:

/pci@306/pci@1/LSI,mrsas@0/disk@0,0

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

[Daniel Kiper]

On Fri, Oct 13, 2017 at 08:53:23AM -0600, Eric Snowberg wrote:
> > On Oct 13, 2017, at 3:36 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> > On Thu, Oct 12, 2017 at 08:38:51AM -0600, Eric Snowberg wrote:
> >>> On Oct 12, 2017, at 5:43 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> >>> On Mon, Oct 09, 2017 at 09:23:44AM -0600, Eric Snowberg wrote:
> >>>>> On Oct 9, 2017, at 5:48 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> >>>>> On Fri, Oct 06, 2017 at 09:37:54AM -0600, Eric Snowberg wrote:
> >>>>>>> On Oct 6, 2017, at 8:47 AM, Daniel Kiper <dkiper@net-space.pl> 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: /pci@306/pci@1/LSI,mrsas@0/disk@0: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//pci@306/pci@1
> >>>>>>>> 001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,mrsas@0/di
> >>>>>>>> 001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  sk@0: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//pci@306/pci@1/LSI,mrsas@0/disk@0: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//pci@306/pci@1
> >>>> 001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,mrsas@0/di
> >>>> 001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  sk@0: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//pci@306/pci@1/LSI,mrsas@0/disk@0:a
> >>>> to
> >>>> ieee1275//pci@306/pci@1/LSI\,mrsas@0/disk@0: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//pci@306/pci@1/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:
>
> /pci@306/pci@1/LSI,mrsas@0/disk@0,0

I am a bit surprised that you are not able to quote a string but I will
give you some examples:

"/pci@306/pci@1/LSI\,mrsas@0/disk@0\,0"
'/pci@306/pci@1/LSI\,mrsas@0/disk@0\,0'
/pci@306/pci@1/LSI\\,mrsas@0/disk@0\\,0

Daniel

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

[Eric Snowberg]

> On Oct 16, 2017, at 2:44 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> 
> On Fri, Oct 13, 2017 at 08:53:23AM -0600, Eric Snowberg wrote:
>>> On Oct 13, 2017, at 3:36 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>> On Thu, Oct 12, 2017 at 08:38:51AM -0600, Eric Snowberg wrote:
>>>>> On Oct 12, 2017, at 5:43 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>>>> On Mon, Oct 09, 2017 at 09:23:44AM -0600, Eric Snowberg wrote:
>>>>>>> On Oct 9, 2017, at 5:48 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>>>>>> On Fri, Oct 06, 2017 at 09:37:54AM -0600, Eric Snowberg wrote:
>>>>>>>>> On Oct 6, 2017, at 8:47 AM, Daniel Kiper <dkiper@net-space.pl> 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: /pci@306/pci@1/LSI,mrsas@0/disk@0: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//pci@306/pci@1
>>>>>>>>>> 001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,mrsas@0/di
>>>>>>>>>> 001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  sk@0: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//pci@306/pci@1/LSI,mrsas@0/disk@0: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//pci@306/pci@1
>>>>>> 001e3d0: 2f4c 5349 5c2c 6d72 7361 7340 302f 6469  /LSI\,mrsas@0/di
>>>>>> 001e3e0: 736b 4030 3a61 200a 7365 7420 7072 6566  sk@0: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//pci@306/pci@1/LSI,mrsas@0/disk@0:a
>>>>>> to
>>>>>> ieee1275//pci@306/pci@1/LSI\,mrsas@0/disk@0: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//pci@306/pci@1/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:
>> 
>> /pci@306/pci@1/LSI,mrsas@0/disk@0,0
> 
> I am a bit surprised that you are not able to quote a string but I will
> give you some examples:
> 
> "/pci@306/pci@1/LSI\,mrsas@0/disk@0\,0"
> '/pci@306/pci@1/LSI\,mrsas@0/disk@0\,0’
> /pci@306/pci@1/LSI\\,mrsas@0/disk@0\\,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.

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

[Daniel Kiper]

On Mon, Oct 16, 2017 at 02:04:17PM -0600, Eric Snowberg wrote:
> > On Oct 16, 2017, at 2:44 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:

[...]

> > I am a bit surprised that you are not able to quote a string but I will
> > give you some examples:
> >
> > "/pci@306/pci@1/LSI\,mrsas@0/disk@0\,0"
> > '/pci@306/pci@1/LSI\,mrsas@0/disk@0\,0’
> > /pci@306/pci@1/LSI\\,mrsas@0/disk@0\\,0
>
> I just tried all three combinations above, none of which worked.
> All failed within the disk open during boot.

Really??? Have you tried __EXACTLY__ those listed above? At least
one should work. Try them with echo command. In all cases you should
see on the console: /pci@306/pci@1/LSI\,mrsas@0/disk@0\,0
No more no less.

> 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.

If you __REALLY__ tried strings listed above they should work. If they do
not then it means that search.fs_uuid further process hint somehow. Then
probably it is a bug and should be fixed.

Daniel

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

[Eric Snowberg]

> On Oct 17, 2017, at 6:53 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> 
> On Mon, Oct 16, 2017 at 02:04:17PM -0600, Eric Snowberg wrote:
>>> On Oct 16, 2017, at 2:44 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> 
> [...]
> 
>>> I am a bit surprised that you are not able to quote a string but I will
>>> give you some examples:
>>> 
>>> "/pci@306/pci@1/LSI\,mrsas@0/disk@0\,0"
>>> '/pci@306/pci@1/LSI\,mrsas@0/disk@0\,0’
>>> /pci@306/pci@1/LSI\\,mrsas@0/disk@0\\,0
>> 
>> I just tried all three combinations above, none of which worked.
>> All failed within the disk open during boot.
> 
> Really??? Have you tried __EXACTLY__ those listed above? At least
> one should work. Try them with echo command. In all cases you should
> see on the console: /pci@306/pci@1/LSI\,mrsas@0/disk@0\,0
> No more no less.

AFAIU the search.fs_uuid doesn’t show up in the console.  The console comes up too late in the boot. It is embedded into the core.img.   I verified they were quoted as you stated above with hexdump. I did need to add the necessary ieee1275/ in front of the disk name so they get routed to the correct driver. The open failed for each case.  Something is transforming them that I haven’t found yet.

I started looking at just having grub-install embed something like (,gpt1)/grub instead of using the uuid by default.  We could then pick up the disk name from OBPs /chosen/bootpath instead.

> 
>> 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.
> 
> If you __REALLY__ tried strings listed above they should work. If they do
> not then it means that search.fs_uuid further process hint somehow. Then
> probably it is a bug and should be fixed.
> 

I agree, one of many on SPARC.