> On Oct 16, 2017, at 2:44 AM, Daniel Kiper <daniel.ki...@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.ki...@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.ki...@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.ki...@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 <dki...@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. _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
