[PATCH] Parse commandline in grub-xen

[PATCH] Parse commandline in grub-xen

[Olaf Hering]

If grub is used as the kernel in a Xen PV guest there is no way to pass
information into grub. This includes info like which disk should be used
first when searching for files.

Up to now the workaround for the host admin is to rebuild grub-xen every
time with grub-mkimage and include a custom script. Such step should be
avoided if possible, the distro provided grub-xen binary should be used.

With this change the command line (extra= in domU.cfg) will be evaluated
by grub. Each 'name=val' pair will be exported as shell variable, other
strings will be ignored. This makes it possible to provide a generic
grub-xen binary for PV guests. It is now up to the scripts in such
binary to interpret the variables as they see fit.

It should be noted that some variables may be set by grub itself,
overriding anything provided in the cmdline. This depends on the way
grub-xen is built, which modules are included.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 grub-core/kern/xen/init.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/grub-core/kern/xen/init.c b/grub-core/kern/xen/init.c
index 0559c03..1dbc01f 100644
--- a/grub-core/kern/xen/init.c
+++ b/grub-core/kern/xen/init.c
@@ -524,6 +524,77 @@ map_all_pages (void)
   grub_mm_init_region ((void *) heap_start, heap_end - heap_start);
 }
 
+/*
+ * Find all name=val pairs in the provided cmd_line and export them
+ * so that scripts can evaluate the variables for their own purpose.
+ */
+static void
+parse_cmdline (void)
+{
+  grub_size_t i;
+  char *p, *name, *val;
+  int found;
+
+  p = grub_malloc (MAX_GUEST_CMDLINE + 1);
+  if (!p)
+    return;
+
+  grub_memcpy (p, grub_xen_start_page_addr->cmd_line, MAX_GUEST_CMDLINE);
+  p[MAX_GUEST_CMDLINE] = '\0';
+
+  for (i = 0; i < MAX_GUEST_CMDLINE && p[i]; i++)
+    {
+      if (grub_isspace (p[i]))
+        continue;
+
+      name = &p[i];
+      found = 0;
+      do
+        {
+          if (grub_isspace (p[i]))
+            break;
+          if (p[i] == '=')
+            {
+              p[i] = '\0';
+              found = 1;
+              break;
+            }
+          if (!p[i + 1])
+            break;
+          i++;
+        }
+      while (i < MAX_GUEST_CMDLINE);
+
+      if (!found)
+        continue;
+
+      i++;
+      val = &p[i];
+      found = 0;
+      do
+        {
+          if (grub_isspace (p[i]))
+            {
+              p[i] = '\0';
+              found = 1;
+            }
+          if (!p[i + 1])
+            found = 1;
+          if (found)
+              break;
+          i++;
+        }
+      while (i < MAX_GUEST_CMDLINE);
+
+      if (!found)
+        continue;
+
+      grub_env_set (name, val);
+      grub_env_export (name);
+    }
+    grub_free (p);
+}
+
 extern char _end[];
 
 void
@@ -539,6 +610,8 @@ grub_machine_init (void)
 
   map_all_pages ();
 
+  parse_cmdline ();
+
   grub_console_init ();
 
   grub_tsc_init ();

Re: [PATCH] Parse commandline in grub-xen

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 3793 bytes --]

As said previously, allowing setting arbitrary variables from command line
will not be an accepted behavior. Also the code should be generic enough to
allow handling of other platforms if need be but surely without including
any unnecessary code for platforms that don't need it. Until there is an
agreement on how to handle the arguments, the only patch I would accept is
to remove the parser in ieee1275 code
On May 11, 2015 11:28 AM, "Olaf Hering" <olaf@aepfle.de> wrote:

> If grub is used as the kernel in a Xen PV guest there is no way to pass
> information into grub. This includes info like which disk should be used
> first when searching for files.
>
> Up to now the workaround for the host admin is to rebuild grub-xen every
> time with grub-mkimage and include a custom script. Such step should be
> avoided if possible, the distro provided grub-xen binary should be used.
>
> With this change the command line (extra= in domU.cfg) will be evaluated
> by grub. Each 'name=val' pair will be exported as shell variable, other
> strings will be ignored. This makes it possible to provide a generic
> grub-xen binary for PV guests. It is now up to the scripts in such
> binary to interpret the variables as they see fit.
>
> It should be noted that some variables may be set by grub itself,
> overriding anything provided in the cmdline. This depends on the way
> grub-xen is built, which modules are included.
>
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
>  grub-core/kern/xen/init.c | 73
> +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
>
> diff --git a/grub-core/kern/xen/init.c b/grub-core/kern/xen/init.c
> index 0559c03..1dbc01f 100644
> --- a/grub-core/kern/xen/init.c
> +++ b/grub-core/kern/xen/init.c
> @@ -524,6 +524,77 @@ map_all_pages (void)
>    grub_mm_init_region ((void *) heap_start, heap_end - heap_start);
>  }
>
> +/*
> + * Find all name=val pairs in the provided cmd_line and export them
> + * so that scripts can evaluate the variables for their own purpose.
> + */
> +static void
> +parse_cmdline (void)
> +{
> +  grub_size_t i;
> +  char *p, *name, *val;
> +  int found;
> +
> +  p = grub_malloc (MAX_GUEST_CMDLINE + 1);
> +  if (!p)
> +    return;
> +
> +  grub_memcpy (p, grub_xen_start_page_addr->cmd_line, MAX_GUEST_CMDLINE);
> +  p[MAX_GUEST_CMDLINE] = '\0';
> +
> +  for (i = 0; i < MAX_GUEST_CMDLINE && p[i]; i++)
> +    {
> +      if (grub_isspace (p[i]))
> +        continue;
> +
> +      name = &p[i];
> +      found = 0;
> +      do
> +        {
> +          if (grub_isspace (p[i]))
> +            break;
> +          if (p[i] == '=')
> +            {
> +              p[i] = '\0';
> +              found = 1;
> +              break;
> +            }
> +          if (!p[i + 1])
> +            break;
> +          i++;
> +        }
> +      while (i < MAX_GUEST_CMDLINE);
> +
> +      if (!found)
> +        continue;
> +
> +      i++;
> +      val = &p[i];
> +      found = 0;
> +      do
> +        {
> +          if (grub_isspace (p[i]))
> +            {
> +              p[i] = '\0';
> +              found = 1;
> +            }
> +          if (!p[i + 1])
> +            found = 1;
> +          if (found)
> +              break;
> +          i++;
> +        }
> +      while (i < MAX_GUEST_CMDLINE);
> +
> +      if (!found)
> +        continue;
> +
> +      grub_env_set (name, val);
> +      grub_env_export (name);
> +    }
> +    grub_free (p);
> +}
> +
>  extern char _end[];
>
>  void
> @@ -539,6 +610,8 @@ grub_machine_init (void)
>
>    map_all_pages ();
>
> +  parse_cmdline ();
> +
>    grub_console_init ();
>
>    grub_tsc_init ();
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 4854 bytes --]

Re: [PATCH] Parse commandline in grub-xen

[Olaf Hering]

On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:

> As said previously, allowing setting arbitrary variables from command line will
> not be an accepted behavior. Also the code should be generic enough to allow
> handling of other platforms if need be but surely without including any
> unnecessary code for platforms that don't need it. Until there is an agreement
> on how to handle the arguments, the only patch I would accept is to remove the
> parser in ieee1275 code

The function as is looks generic enough to be used by xen and ieee1275.
Not sure why they went for ';' as separator, I'm sure the firmware does
not require that.
I dont have an EFI capable box around, someone familiar with that could
make use of that function. I'm sure EFI has something like
/chosen/bootargs.

Futhermore what freedom would you like to hand out to those who
implement the scripts? In my testing some variables such as root= and
prefix= are overriden anyway.

Olaf

Re: [PATCH] Parse commandline in grub-xen

[Andrei Borzenkov]

On Mon, May 11, 2015 at 12:43 PM, Olaf Hering <olaf@aepfle.de> wrote:
> On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:
>
>> As said previously, allowing setting arbitrary variables from command line will
>> not be an accepted behavior. Also the code should be generic enough to allow
>> handling of other platforms if need be but surely without including any
>> unnecessary code for platforms that don't need it.

Each platform has own ways to pass arguments, so I'm not sure it makes
sense to really share this code. To this extent patch is OK as it is
specific to xen.

>                                                                                   Until there is an agreement
>> on how to handle the arguments, the only patch I would accept is to remove the
>> parser in ieee1275 code
>
> The function as is looks generic enough to be used by xen and ieee1275.
> Not sure why they went for ';' as separator, I'm sure the firmware does
> not require that.
> I dont have an EFI capable box around, someone familiar with that could
> make use of that function. I'm sure EFI has something like
> /chosen/bootargs.
>

EFI application gets analog of (argc, argv) directly. EFI arguments
are arbitrary binary blobs, but this is up to application to define
what is acceptable.

> Futhermore what freedom would you like to hand out to those who
> implement the scripts? In my testing some variables such as root= and
> prefix= are overriden anyway.
>

You do not know what variables will be used by modules/configuration files.

Re: [PATCH] Parse commandline in grub-xen

[Olaf Hering]

On Mon, May 11, Andrei Borzenkov wrote:

> On Mon, May 11, 2015 at 12:43 PM, Olaf Hering <olaf@aepfle.de> wrote:
> > Futhermore what freedom would you like to hand out to those who
> > implement the scripts? In my testing some variables such as root= and
> > prefix= are overriden anyway.
> You do not know what variables will be used by modules/configuration files.

Of course. But since kernel= and extra= is one unit it does not matter.
Who ever is in charge for both decides: the host admin.

Olaf

Re: [PATCH] Parse commandline in grub-xen

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 970 bytes --]

On May 11, 2015 12:41 PM, "Olaf Hering" <olaf@aepfle.de> wrote:
>
> On Mon, May 11, Andrei Borzenkov wrote:
>
> > On Mon, May 11, 2015 at 12:43 PM, Olaf Hering <olaf@aepfle.de> wrote:
> > > Futhermore what freedom would you like to hand out to those who
> > > implement the scripts? In my testing some variables such as root= and
> > > prefix= are overriden anyway.
> > You do not know what variables will be used by modules/configuration
files.
>
> Of course. But since kernel= and extra= is one unit it does not matter.
> Who ever is in charge for both decides: the host admin.
>
Do you really own all those installs to be able to speak on behalf of all
of them?
Also it might be even inadvertent. Think of separating namespaces as
hygiene. Bad hygiene might not kill you straight away but is still a bad
idea
> Olaf
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

[-- Attachment #2: Type: text/html, Size: 1353 bytes --]

Re: [PATCH] Parse commandline in grub-xen

[Olaf Hering]

On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:

> Do you really own all those installs to be able to speak on behalf of all of
> them?

Since there is zero upstream support for anything regarding grub xen
distros are forced to provide their own grub-xen binary for dom0. This
includes at least some script to do something useful within the very
first grub-xen. So in this sense its up to whoever creates such dom0
binary to handle variables as needed, and document the implemented
features.

Olaf

Re: [PATCH] Parse commandline in grub-xen

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 761 bytes --]

On May 11, 2015 1:23 PM, "Olaf Hering" <olaf@aepfle.de> wrote:
>
> On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:
>
> > Do you really own all those installs to be able to speak on behalf of
all of
> > them?
>
> Since there is zero upstream support for anything regarding grub xen
> distros are forced to provide their own grub-xen binary for dom0. This
> includes at least some script to do something useful within the very
> first grub-xen. So in this sense its up to whoever creates such dom0
> binary to handle variables as needed, and document the implemented
> features.
>
And your point is?
> Olaf
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

[-- Attachment #2: Type: text/html, Size: 1095 bytes --]

Re: [PATCH] Parse commandline in grub-xen

[Olaf Hering]

On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:

> 
> On May 11, 2015 1:23 PM, "Olaf Hering" <olaf@aepfle.de> wrote:
> >
> > On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:
> >
> > > Do you really own all those installs to be able to speak on behalf of all
> of
> > > them?
> >
> > Since there is zero upstream support for anything regarding grub xen
> > distros are forced to provide their own grub-xen binary for dom0. This
> > includes at least some script to do something useful within the very
> > first grub-xen. So in this sense its up to whoever creates such dom0
> > binary to handle variables as needed, and document the implemented
> > features.
> >
> And your point is?

That this patch gets in as is?

How do you envison a way to select a boot device, or set debug=all or
whatever a script may interpret?

Olaf

Re: [PATCH] Parse commandline in grub-xen

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1342 bytes --]

On May 11, 2015 1:52 PM, "Olaf Hering" <olaf@aepfle.de> wrote:
>
> On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:
>
> >
> > On May 11, 2015 1:23 PM, "Olaf Hering" <olaf@aepfle.de> wrote:
> > >
> > > On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:
> > >
> > > > Do you really own all those installs to be able to speak on behalf
of all
> > of
> > > > them?
> > >
> > > Since there is zero upstream support for anything regarding grub xen
> > > distros are forced to provide their own grub-xen binary for dom0. This
> > > includes at least some script to do something useful within the very
> > > first grub-xen. So in this sense its up to whoever creates such dom0
> > > binary to handle variables as needed, and document the implemented
> > > features.
> > >
> > And your point is?
>
> That this patch gets in as is?
>
Well your argument doesn't say at all why no prefix is better than
separating namespaces
> How do you envison a way to select a boot device, or set debug=all or
> whatever a script may interpret?
>
Setting debug is a good example of abuse that would be sneaking in.
Variables set from command line need to have a prefix to indicate where
they come from.
> Olaf
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

[-- Attachment #2: Type: text/html, Size: 1925 bytes --]

Re: [PATCH] Parse commandline in grub-xen

[Olaf Hering]

On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:

> Well your argument doesn't say at all why no prefix is better than separating
> namespaces

Who is supposed to parse the prefix?
And at what point in time?  Early, later, never?

> > How do you envison a way to select a boot device, or set debug=all or
> > whatever a script may interpret?
> Setting debug is a good example of abuse that would be sneaking in. Variables
> set from command line need to have a prefix to indicate where they come from.

Why is that abuse?
When should debug=all become active from your POV?  Early, later, never?

Olaf

Re: [PATCH] Parse commandline in grub-xen

[Andrei Borzenkov]

В Mon, 11 May 2015 13:51:48 +0200
Olaf Hering <olaf@aepfle.de> пишет:

> On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:
> 
> > 
> > On May 11, 2015 1:23 PM, "Olaf Hering" <olaf@aepfle.de> wrote:
> > >
> > > On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:
> > >
> > > > Do you really own all those installs to be able to speak on behalf of all
> > of
> > > > them?
> > >
> > > Since there is zero upstream support for anything regarding grub xen
> > > distros are forced to provide their own grub-xen binary for dom0. This
> > > includes at least some script to do something useful within the very
> > > first grub-xen. So in this sense its up to whoever creates such dom0
> > > binary to handle variables as needed, and document the implemented
> > > features.
> > >
> > And your point is?
> 
> That this patch gets in as is?
> 
> How do you envison a way to select a boot device, or set debug=all or
> whatever a script may interpret?

Either by allowing ${grub.arg.XXX} (not sure if current grammar accepts
it) or by adding getarg command, something like

getarg --name debug --set debug

You do not control what arguments grub gets - end use (admin) controls
it. You cannot force end user to actually strictly comply with what you
expect. As example, grub.cfg you recently submitted has

if [ -n "hddev" ]

without initializing it first. So administrator setting this variable
will unintentionally change behavior of script.

Re: [PATCH] Parse commandline in grub-xen

[Olaf Hering]

On Mon, May 11, Andrei Borzenkov wrote:

> Either by allowing ${grub.arg.XXX} (not sure if current grammar accepts
> it) or by adding getarg command, something like
> 
> getarg --name debug --set debug

What would such format buy us?

> You do not control what arguments grub gets - end use (admin) controls
> it. You cannot force end user to actually strictly comply with what you
> expect. As example, grub.cfg you recently submitted has
> 
> if [ -n "hddev" ]
> 
> without initializing it first. So administrator setting this variable
> will unintentionally change behavior of script.

That should have been hdcfg of hddev, thanks for spotting it.

The use of uninitialized vars has to be caught by the script author no?
Code like that is valid IMO:

set localvar=""
if [ -n "${whatever}" ];then
  echo "do whatever implies"
  set localvar="val"
fi
if [ -n "${localvar}" ];then
  echo "do whatever local things"
fi


Olaf

Re: [PATCH] Parse commandline in grub-xen

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1222 bytes --]

On May 11, 2015 2:16 PM, "Olaf Hering" <olaf@aepfle.de> wrote:
>
> On Mon, May 11, Andrei Borzenkov wrote:
>
> > Either by allowing ${grub.arg.XXX} (not sure if current grammar accepts
> > it) or by adding getarg command, something like
> >
> > getarg --name debug --set debug
>
> What would such format buy us?
>
> > You do not control what arguments grub gets - end use (admin) controls
> > it. You cannot force end user to actually strictly comply with what you
> > expect. As example, grub.cfg you recently submitted has
> >
> > if [ -n "hddev" ]
> >
> > without initializing it first. So administrator setting this variable
> > will unintentionally change behavior of script.
>
> That should have been hdcfg of hddev, thanks for spotting it.
>
> The use of uninitialized vars has to be caught by the script author no?
It never works this way
> Code like that is valid IMO:
>
> set localvar=""
> if [ -n "${whatever}" ];then
>   echo "do whatever implies"
>   set localvar="val"
> fi
> if [ -n "${localvar}" ];then
>   echo "do whatever local things"
> fi
>
>
> Olaf
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

[-- Attachment #2: Type: text/html, Size: 1842 bytes --]

Re: [PATCH] Parse commandline in grub-xen

[Olaf Hering]

On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:

> On May 11, 2015 2:16 PM, "Olaf Hering" <olaf@aepfle.de> wrote:
> > The use of uninitialized vars has to be caught by the script author no?
> It never works this way

So? How does it work then? Because thats what I will propose to my
package maintainer later this week...

Olaf

> > Code like that is valid IMO:
> >
> > set localvar=""
> > if [ -n "${whatever}" ];then
> >   echo "do whatever implies"
> >   set localvar="val"
> > fi
> > if [ -n "${localvar}" ];then
> >   echo "do whatever local things"
> > fi

Re: [PATCH] Parse commandline in grub-xen

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1010 bytes --]

On May 11, 2015 3:34 PM, "Olaf Hering" <olaf@aepfle.de> wrote:
>
> On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:
>
> > On May 11, 2015 2:16 PM, "Olaf Hering" <olaf@aepfle.de> wrote:
> > > The use of uninitialized vars has to be caught by the script author
no?
> > It never works this way
>
> So? How does it work then? Because thats what I will propose to my
> package maintainer later this week...
>
Never rely on people checking their scripts for anything. If you can make
introducing bugs into scripts by adding prefix then do so. I fail to
understand your stubbornness for a bad design
> Olaf
>
> > > Code like that is valid IMO:
> > >
> > > set localvar=""
> > > if [ -n "${whatever}" ];then
> > >   echo "do whatever implies"
> > >   set localvar="val"
> > > fi
> > > if [ -n "${localvar}" ];then
> > >   echo "do whatever local things"
> > > fi
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

[-- Attachment #2: Type: text/html, Size: 1612 bytes --]

Re: [PATCH] Parse commandline in grub-xen

[Andrei Borzenkov]

В Mon, 11 May 2015 14:15:54 +0200
Olaf Hering <olaf@aepfle.de> пишет:

> On Mon, May 11, Andrei Borzenkov wrote:
> 
> > Either by allowing ${grub.arg.XXX} (not sure if current grammar accepts
> > it) or by adding getarg command, something like
> > 
> > getarg --name debug --set debug
> 
> What would such format buy us?

Make it possible for you, as script author, have control over which
arguments and how are used.

Re: [PATCH] Parse commandline in grub-xen

[Olaf Hering]

On Mon, May 11, Andrei Borzenkov wrote:

> В Mon, 11 May 2015 14:15:54 +0200
> Olaf Hering <olaf@aepfle.de> пишет:
> 
> > On Mon, May 11, Andrei Borzenkov wrote:
> > 
> > > Either by allowing ${grub.arg.XXX} (not sure if current grammar accepts
> > > it) or by adding getarg command, something like
> > > 
> > > getarg --name debug --set debug
> > 
> > What would such format buy us?
> 
> Make it possible for you, as script author, have control over which
> arguments and how are used.

'if [ -n "${name}" ]' is simpler than 
'getarg --name name --set name;if [ -n "${name}"]'.
Not sure if 'if $cmd' is supposed to work, but either way your proposal
looks strange.

And regarding $debug itself, with my change its set very early before
any script runs. Perhaps that is useful to debug early issues.


Olaf

Re: [PATCH] Parse commandline in grub-xen

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1167 bytes --]

On May 11, 2015 7:08 PM, "Olaf Hering" <olaf@aepfle.de> wrote:
>
> On Mon, May 11, Andrei Borzenkov wrote:
>
> > В Mon, 11 May 2015 14:15:54 +0200
> > Olaf Hering <olaf@aepfle.de> пишет:
> >
> > > On Mon, May 11, Andrei Borzenkov wrote:
> > >
> > > > Either by allowing ${grub.arg.XXX} (not sure if current grammar
accepts
> > > > it) or by adding getarg command, something like
> > > >
> > > > getarg --name debug --set debug
> > >
> > > What would such format buy us?
> >
> > Make it possible for you, as script author, have control over which
> > arguments and how are used.
>
> 'if [ -n "${name}" ]' is simpler than
> 'getarg --name name --set name;if [ -n "${name}"]'.
> Not sure if 'if $cmd' is supposed to work, but either way your proposal
> looks strange.
>
> And regarding $debug itself, with my change its set very early before
> any script runs. Perhaps that is useful to debug early issues.
>
Even if it is, it shouldn't be a side effect of bad design
>
> Olaf
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

[-- Attachment #2: Type: text/html, Size: 1754 bytes --]

Re: [PATCH] Parse commandline in grub-xen

[Olaf Hering]

On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:

> On May 11, 2015 7:08 PM, "Olaf Hering" <olaf@aepfle.de> wrote:
> > And regarding $debug itself, with my change its set very early before
> > any script runs. Perhaps that is useful to debug early issues.
> >
> Even if it is, it shouldn't be a side effect of bad design

This does not answer the question when debug=X is supposed to be
activated.

Olaf

Re: [PATCH] Parse commandline in grub-xen

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 675 bytes --]

On May 12, 2015 10:06 AM, "Olaf Hering" <olaf@aepfle.de> wrote:
>
> On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:
>
> > On May 11, 2015 7:08 PM, "Olaf Hering" <olaf@aepfle.de> wrote:
> > > And regarding $debug itself, with my change its set very early before
> > > any script runs. Perhaps that is useful to debug early issues.
> > >
> > Even if it is, it shouldn't be a side effect of bad design
>
> This does not answer the question when debug=X is supposed to be
> activated.
>
Which is a different problem
> Olaf
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

[-- Attachment #2: Type: text/html, Size: 1076 bytes --]

Re: [PATCH] Parse commandline in grub-xen

[Olaf Hering]

On Tue, May 12, Vladimir 'phcoder' Serbinenko wrote:

> 
> On May 12, 2015 10:06 AM, "Olaf Hering" <olaf@aepfle.de> wrote:
> >
> > On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:
> >
> > > On May 11, 2015 7:08 PM, "Olaf Hering" <olaf@aepfle.de> wrote:
> > > > And regarding $debug itself, with my change its set very early before
> > > > any script runs. Perhaps that is useful to debug early issues.
> > > >
> > > Even if it is, it shouldn't be a side effect of bad design
> >
> > This does not answer the question when debug=X is supposed to be
> > activated.
> >
> Which is a different problem

Not at all. Should the parser which iterates over the cmd_line string
set debug= once it finds it? Should there be another helper who sets it?

Olaf

Re: [PATCH] Parse commandline in grub-xen

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]

On May 12, 2015 11:10 AM, "Olaf Hering" <olaf@aepfle.de> wrote:
>
> On Tue, May 12, Vladimir 'phcoder' Serbinenko wrote:
>
> >
> > On May 12, 2015 10:06 AM, "Olaf Hering" <olaf@aepfle.de> wrote:
> > >
> > > On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:
> > >
> > > > On May 11, 2015 7:08 PM, "Olaf Hering" <olaf@aepfle.de> wrote:
> > > > > And regarding $debug itself, with my change its set very early
before
> > > > > any script runs. Perhaps that is useful to debug early issues.
> > > > >
> > > > Even if it is, it shouldn't be a side effect of bad design
> > >
> > > This does not answer the question when debug=X is supposed to be
> > > activated.
> > >
> > Which is a different problem
>
> Not at all. Should the parser which iterates over the cmd_line string
> set debug= once it finds it?
No
> Should there be another helper who sets it?
No
>
> Olaf
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

[-- Attachment #2: Type: text/html, Size: 1641 bytes --]

Re: [PATCH] Parse commandline in grub-xen

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 2019 bytes --]

On May 11, 2015 2:04 PM, "Andrei Borzenkov" <arvidjaar@gmail.com> wrote:
>
> В Mon, 11 May 2015 13:51:48 +0200
> Olaf Hering <olaf@aepfle.de> пишет:
>
> > On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:
> >
> > >
> > > On May 11, 2015 1:23 PM, "Olaf Hering" <olaf@aepfle.de> wrote:
> > > >
> > > > On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:
> > > >
> > > > > Do you really own all those installs to be able to speak on
behalf of all
> > > of
> > > > > them?
> > > >
> > > > Since there is zero upstream support for anything regarding grub xen
> > > > distros are forced to provide their own grub-xen binary for dom0.
This
> > > > includes at least some script to do something useful within the very
> > > > first grub-xen. So in this sense its up to whoever creates such dom0
> > > > binary to handle variables as needed, and document the implemented
> > > > features.
> > > >
> > > And your point is?
> >
> > That this patch gets in as is?
> >
> > How do you envison a way to select a boot device, or set debug=all or
> > whatever a script may interpret?
>
> Either by allowing ${grub.arg.XXX} (not sure if current grammar accepts
> it) or by adding getarg command, something like
>
I prefer the former. But probably fwarg. Would be a better prefix. This way
one can do:
extra="grub.root=xvda1"
And then get it as fwarg.grub.root whereas root for Linux would be
fwarg.root and thus avoiding any conflict
> getarg --name debug --set debug
>
> You do not control what arguments grub gets - end use (admin) controls
> it. You cannot force end user to actually strictly comply with what you
> expect. As example, grub.cfg you recently submitted has
>
> if [ -n "hddev" ]
>
> without initializing it first. So administrator setting this variable
> will unintentionally change behavior of script.
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

[-- Attachment #2: Type: text/html, Size: 2844 bytes --]

Re: [PATCH] Parse commandline in grub-xen

[Andrei Borzenkov]

В Mon, 11 May 2015 14:55:34 +0200
"Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> пишет:

> On May 11, 2015 2:04 PM, "Andrei Borzenkov" <arvidjaar@gmail.com> wrote:
> >
> > В Mon, 11 May 2015 13:51:48 +0200
> > Olaf Hering <olaf@aepfle.de> пишет:
> >
> > > On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:
> > >
> > > >
> > > > On May 11, 2015 1:23 PM, "Olaf Hering" <olaf@aepfle.de> wrote:
> > > > >
> > > > > On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:
> > > > >
> > > > > > Do you really own all those installs to be able to speak on
> behalf of all
> > > > of
> > > > > > them?
> > > > >
> > > > > Since there is zero upstream support for anything regarding grub xen
> > > > > distros are forced to provide their own grub-xen binary for dom0.
> This
> > > > > includes at least some script to do something useful within the very
> > > > > first grub-xen. So in this sense its up to whoever creates such dom0
> > > > > binary to handle variables as needed, and document the implemented
> > > > > features.
> > > > >
> > > > And your point is?
> > >
> > > That this patch gets in as is?
> > >
> > > How do you envison a way to select a boot device, or set debug=all or
> > > whatever a script may interpret?
> >
> > Either by allowing ${grub.arg.XXX} (not sure if current grammar accepts
> > it) or by adding getarg command, something like
> >
> I prefer the former. But probably fwarg. Would be a better prefix. This way
> one can do:
> extra="grub.root=xvda1"
> And then get it as fwarg.grub.root whereas root for Linux would be
> fwarg.root and thus avoiding any conflict

Using it as variable name means change to parser with possible side
effects; also it means it will go into core for every platform, even if
platform itself does not support such variable passing, increasing its
size. Having it as external command does not require any core changes,
will be used only when needed and could be modeled after getopt e.g.
allowing loop over arguments.

> > getarg --name debug --set debug
> >
> > You do not control what arguments grub gets - end use (admin) controls
> > it. You cannot force end user to actually strictly comply with what you
> > expect. As example, grub.cfg you recently submitted has
> >
> > if [ -n "hddev" ]
> >
> > without initializing it first. So administrator setting this variable
> > will unintentionally change behavior of script.
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Parse commandline in grub-xen

[Andrei Borzenkov]

В Mon, 11 May 2015 19:14:14 +0200
"Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> пишет:

> On May 11, 2015 6:52 PM, "Andrei Borzenkov" <arvidjaar@gmail.com> wrote:
> >
> > В Mon, 11 May 2015 14:55:34 +0200
> > "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> пишет:
> >
> > > On May 11, 2015 2:04 PM, "Andrei Borzenkov" <arvidjaar@gmail.com> wrote:
> > > >
> > > > В Mon, 11 May 2015 13:51:48 +0200
> > > > Olaf Hering <olaf@aepfle.de> пишет:
> > > >
> > > > > On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:
> > > > >
> > > > > >
> > > > > > On May 11, 2015 1:23 PM, "Olaf Hering" <olaf@aepfle.de> wrote:
> > > > > > >
> > > > > > > On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:
> > > > > > >
> > > > > > > > Do you really own all those installs to be able to speak on
> > > behalf of all
> > > > > > of
> > > > > > > > them?
> > > > > > >
> > > > > > > Since there is zero upstream support for anything regarding
> grub xen
> > > > > > > distros are forced to provide their own grub-xen binary for
> dom0.
> > > This
> > > > > > > includes at least some script to do something useful within the
> very
> > > > > > > first grub-xen. So in this sense its up to whoever creates such
> dom0
> > > > > > > binary to handle variables as needed, and document the
> implemented
> > > > > > > features.
> > > > > > >
> > > > > > And your point is?
> > > > >
> > > > > That this patch gets in as is?
> > > > >
> > > > > How do you envison a way to select a boot device, or set debug=all
> or
> > > > > whatever a script may interpret?
> > > >
> > > > Either by allowing ${grub.arg.XXX} (not sure if current grammar
> accepts
> > > > it) or by adding getarg command, something like
> > > >
> > > I prefer the former. But probably fwarg. Would be a better prefix. This
> way
> > > one can do:
> > > extra="grub.root=xvda1"
> > > And then get it as fwarg.grub.root whereas root for Linux would be
> > > fwarg.root and thus avoiding any conflict
> >
> > Using it as variable name means change to parser with possible side
> > effects; also it means it will go into core for every platform, even if
> > platform itself does not support such variable passing, increasing its
> > size.
> We can link it in only for the affected platforms and call from
> grub-machine-init. Also we don't need the full parser

Having different syntax for different platforms is really no-go.

> >Having it as external command does not require any core changes,
> > will be used only when needed and could be modeled after getopt e.g.
> > allowing loop over arguments.
> >
> > > > getarg --name debug --set debug
> > > >

Re: [PATCH] Parse commandline in grub-xen

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 3250 bytes --]

On May 11, 2015 8:46 PM, "Andrei Borzenkov" <arvidjaar@gmail.com> wrote:
>
> В Mon, 11 May 2015 19:14:14 +0200
> "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> пишет:
>
> > On May 11, 2015 6:52 PM, "Andrei Borzenkov" <arvidjaar@gmail.com> wrote:
> > >
> > > В Mon, 11 May 2015 14:55:34 +0200
> > > "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> пишет:
> > >
> > > > On May 11, 2015 2:04 PM, "Andrei Borzenkov" <arvidjaar@gmail.com>
wrote:
> > > > >
> > > > > В Mon, 11 May 2015 13:51:48 +0200
> > > > > Olaf Hering <olaf@aepfle.de> пишет:
> > > > >
> > > > > > On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:
> > > > > >
> > > > > > >
> > > > > > > On May 11, 2015 1:23 PM, "Olaf Hering" <olaf@aepfle.de> wrote:
> > > > > > > >
> > > > > > > > On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:
> > > > > > > >
> > > > > > > > > Do you really own all those installs to be able to speak
on
> > > > behalf of all
> > > > > > > of
> > > > > > > > > them?
> > > > > > > >
> > > > > > > > Since there is zero upstream support for anything regarding
> > grub xen
> > > > > > > > distros are forced to provide their own grub-xen binary for
> > dom0.
> > > > This
> > > > > > > > includes at least some script to do something useful within
the
> > very
> > > > > > > > first grub-xen. So in this sense its up to whoever creates
such
> > dom0
> > > > > > > > binary to handle variables as needed, and document the
> > implemented
> > > > > > > > features.
> > > > > > > >
> > > > > > > And your point is?
> > > > > >
> > > > > > That this patch gets in as is?
> > > > > >
> > > > > > How do you envison a way to select a boot device, or set
debug=all
> > or
> > > > > > whatever a script may interpret?
> > > > >
> > > > > Either by allowing ${grub.arg.XXX} (not sure if current grammar
> > accepts
> > > > > it) or by adding getarg command, something like
> > > > >
> > > > I prefer the former. But probably fwarg. Would be a better prefix.
This
> > way
> > > > one can do:
> > > > extra="grub.root=xvda1"
> > > > And then get it as fwarg.grub.root whereas root for Linux would be
> > > > fwarg.root and thus avoiding any conflict
> > >
> > > Using it as variable name means change to parser with possible side
> > > effects; also it means it will go into core for every platform, even
if
> > > platform itself does not support such variable passing, increasing its
> > > size.
> > We can link it in only for the affected platforms and call from
> > grub-machine-init. Also we don't need the full parser
>
> Having different syntax for different platforms is really no-go.
>
Agreed. I was just referring to the fact that the common syntax can be
simple one and that we can put those functions in separate file and link
only in select platforms
> > >Having it as external command does not require any core changes,
> > > will be used only when needed and could be modeled after getopt e.g.
> > > allowing loop over arguments.
> > >
> > > > > getarg --name debug --set debug
> > > > >
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

[-- Attachment #2: Type: text/html, Size: 5104 bytes --]

Re: [PATCH] Parse commandline in grub-xen

[Michael Chang]

On Mon, May 11, 2015 at 01:01:43PM +0200, Olaf Hering wrote:
> On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:
> 
> > Do you really own all those installs to be able to speak on behalf of all of
> > them?
> 
> Since there is zero upstream support for anything regarding grub xen
> distros are forced to provide their own grub-xen binary for dom0. This

I'd like to hear from upstream whether they consider to provide host
tools for setting up xen pv loaders from xen domU config? The
grub-mkstandalone has done part of the job but is not automated for the
config.

The extra= can be passing to the pvgrub2's config by the utils and is
from difference layer.

Thanks,
Michael

Re: [PATCH] Parse commandline in grub-xen

[Andrei Borzenkov]

В Mon, 11 May 2015 20:01:29 +0800
Michael Chang <mchang@suse.com> пишет:

> On Mon, May 11, 2015 at 01:01:43PM +0200, Olaf Hering wrote:
> > On Mon, May 11, Vladimir 'phcoder' Serbinenko wrote:
> > 
> > > Do you really own all those installs to be able to speak on behalf of all of
> > > them?
> > 
> > Since there is zero upstream support for anything regarding grub xen
> > distros are forced to provide their own grub-xen binary for dom0. This
> 
> I'd like to hear from upstream whether they consider to provide host
> tools for setting up xen pv loaders from xen domU config? The
> grub-mkstandalone has done part of the job but is not automated for the
> config.
> 

Actually I do not see much difference between using fixed path for
second stage bootloader and using fixed path for second stage
configuration from administrator PoV. In both cases you run
grub-mkconfig from domU to update *config*.

Using second stage config directly does make domU grub.cfg dependent on
dom0 grub version, so support for chainloading would be useful. Someone
needs to address comments in the last review.

> The extra= can be passing to the pvgrub2's config by the utils and is
> from difference layer.
> 
> Thanks,
> Michael
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel