[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] parser: Remove escape from the state transitions
From: |
Daniel Kiper |
Subject: |
Re: [PATCH] parser: Remove escape from the state transitions |
Date: |
Fri, 13 Oct 2017 11:36:57 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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.
Daniel
- Re: [PATCH] parser: Remove escape from the state transitions, Daniel Kiper, 2017/10/06
- Re: [PATCH] parser: Remove escape from the state transitions, Eric Snowberg, 2017/10/06
- Re: [PATCH] parser: Remove escape from the state transitions, Daniel Kiper, 2017/10/09
- Re: [PATCH] parser: Remove escape from the state transitions, Eric Snowberg, 2017/10/09
- Re: [PATCH] parser: Remove escape from the state transitions, Daniel Kiper, 2017/10/12
- Re: [PATCH] parser: Remove escape from the state transitions, Eric Snowberg, 2017/10/12
- Re: [PATCH] parser: Remove escape from the state transitions,
Daniel Kiper <=
- Re: [PATCH] parser: Remove escape from the state transitions, Eric Snowberg, 2017/10/13
- Re: [PATCH] parser: Remove escape from the state transitions, Daniel Kiper, 2017/10/16
- Re: [PATCH] parser: Remove escape from the state transitions, Eric Snowberg, 2017/10/16
- Re: [PATCH] parser: Remove escape from the state transitions, Daniel Kiper, 2017/10/17
- Re: [PATCH] parser: Remove escape from the state transitions, Eric Snowberg, 2017/10/17