Re: pvgrub2 legacy_configfile issues: /sbin/init exec error -8

[Andrei Borzenkov]

25.02.2017 02:22, Vladimir 'phcoder' Serbinenko пишет:
> On Sun, Feb 12, 2017, 11:52 Vladimir 'phcoder' Serbinenko <address@hidden>
> wrote:
> 
>>
>>
>> On Sun, 12 Feb 2017, 08:06 Andrei Borzenkov <address@hidden> wrote:
>>
>> 12.02.2017 00:07, Vladimir 'phcoder' Serbinenko пишет:
>> ...
>>>>
>>> For multiboot module old grub was passing module file name on module
>>> command line, so legacy_configfile should replicate this behaviour. But
>> in
>>> Linux case it shouldn't have ended up loading twice. We need additional
>>> code in legacy_initrd. Please try the attached patch
>>>
>> ...
>>>
>>>
>>> diff --git a/grub-core/commands/legacycfg.c
>> b/grub-core/commands/legacycfg.c
>>> index dd9d9f1..24546bf 100644
>>> --- a/grub-core/commands/legacycfg.c
>>> +++ b/grub-core/commands/legacycfg.c
>>> @@ -517,7 +517,7 @@ grub_cmd_legacy_initrd (struct grub_command *mycmd
>> __attribute__ ((unused)),
>>>  #endif
>>>                          );
>>>
>>> -      return cmd->func (cmd, argc, args);
>>> +      return cmd->func (cmd, argc ? argc - 1 : 0, args + 1);
>>
>> If the goal is to be compatible with legacy grub, it is not compatible
>> (nor was) - legacy grub "initrd" ignored all extra arguments to "initrd"
>> command while we attempt to load them as additional initrd components.
>>
>> Nor did legacy grub attempt to quote command line for multiboot kernel
>> or modules.
>>
>> Agreed on both. That's why I said that previous patch is wrong and I'm
>> working on new one
>>
> For first point, see attached patch.
> Second point isn't a problem as legacycfg will already split along spaces
> and hence quoting code will not be triggered.
> 

It will.

      while (*c)
        {
          if (*c == '\\' || *c == '\'' || *c == '"')
            *buf++ = '\\';
          *buf++ = *c;
          c++;
        }

In particular, this is a problem with Linux which does not interpret
these quotes at all (it only supports " .... " without any way to escape
nested `"'). That's something for post 2.02 now, but I still would like
to know what was the reason for this code in the first place.

> 
> diff --git a/grub-core/commands/legacycfg.c b/grub-core/commands/legacycfg.c
> index dd9d9f1..b32f3c7 100644
> --- a/grub-core/commands/legacycfg.c
> +++ b/grub-core/commands/legacycfg.c
> @@ -517,7 +517,7 @@ grub_cmd_legacy_initrd (struct grub_command *mycmd
> __attribute__ ((unused)),
> #endif
> );
> 
> - return cmd->func (cmd, argc, args);
> + return cmd->func (cmd, argc ? 1 : 0, args);
> }
> if (kernel_type == MULTIBOOT)
> {
> 

Looks OK as stop gap. @Andy, could you test it? 

I still do not understand why initrd is parsed as TYPE_FILE_NO_CONSUME,
nor why we handle multiboot kernel in legacy_initrd in the first place.
Legacy grub only supported Linux kernel with initrd command.

  switch (kernel_type)
    {
    case KERNEL_TYPE_LINUX:
    case KERNEL_TYPE_BIG_LINUX:
      if (! load_initrd (arg))
        return 1;
      break;

    default:
      errnum = ERR_NEED_LX_KERNEL;
      return 1;
    }

Although this probably does not really matter now.