From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Vladimir 'phcoder' Serbinenko" Subject: Re: [PATCH 2/4] Accept environment variables on the command line for Xen. Date: Thu, 12 Dec 2013 17:13:13 +0100 Message-ID: References: <20131212153643.GA1431@riva.ucam.org> <20131212153722.GC1431@riva.ucam.org> <20131212194805.5d81a2e7@opensuse.site> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7845651709179264357==" Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Vr8t2-0002Rh-1p for xen-devel@lists.xenproject.org; Thu, 12 Dec 2013 16:13:16 +0000 Received: by mail-we0-f174.google.com with SMTP id q58so658492wes.33 for ; Thu, 12 Dec 2013 08:13:13 -0800 (PST) In-Reply-To: <20131212194805.5d81a2e7@opensuse.site> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: The development of GNU GRUB , xen-devel List-Id: xen-devel@lists.xenproject.org --===============7845651709179264357== Content-Type: multipart/alternative; boundary=001a11c3473a61def704ed589fe0 --001a11c3473a61def704ed589fe0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable I was about to post similar remarks. I'm on phone so will tell just the key points. - Most of GRUB platforms have command lines. I.a xen, efi, loongson, ieee1275, arc. - Many platforms add their own command line arguments even if user didn't ask for it. Those shouldn't interfere. In your patch root=3Dsda1 from xen will either be lost or will clobber GRUB root. - IEEE1275 code already looks into command line. If it does sth sane it could be used as base, otherwise it should be replaced with sth common. - Perhaps we should use some prefix to avoid unintended clobbering. - There should be a way to emulate pvgrub1 behaviour which took config name from commandline. Perhaps pvgrub2 should even do it by default (by using legacy_configfile) On Dec 12, 2013 4:48 PM, "Andrey Borzenkov" wrote: > =D0=92 Thu, 12 Dec 2013 15:37:22 +0000 > Colin Watson =D0=BF=D0=B8=D1=88=D0=B5=D1=82: > > > * grub-core/kern/xen/init.c (fetch_command_line_word): New function. > > (parse_command_line): Likewise. > > (grub_machine_init): Call parse_command_line. > > --- > > ChangeLog | 8 ++++++++ > > grub-core/kern/xen/init.c | 44 > ++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 52 insertions(+) > > > > diff --git a/ChangeLog b/ChangeLog > > index 766fe4b..fc86601 100644 > > --- a/ChangeLog > > +++ b/ChangeLog > > @@ -1,5 +1,13 @@ > > 2013-12-12 Colin Watson > > > > + Accept environment variables on the command line for Xen. > > + > > Thank you! > > It may make sense to generalize it to other archs. At least EFI > definitely comes in mind. In this case platform specific part is only > to fetch parameter string(s). What about > > - move fetch_command_line_word and parse_command_line to separate file > that is included for specific platforms only > - make each platform that can accept fetch parameters in platform > specific way and call common code (parse_command_line) > > Does it make sense? > > I will add EFI part then later. > > > + * grub-core/kern/xen/init.c (fetch_command_line_word): New > function. > > + (parse_command_line): Likewise. > > + (grub_machine_init): Call parse_command_line. > > + > > +2013-12-12 Colin Watson > > + > > Add an option to exclude devices from search results. > > > > * grub-core/commands/search.c (struct search_ctx): Add excludes a= nd > > diff --git a/grub-core/kern/xen/init.c b/grub-core/kern/xen/init.c > > index 1d8eaec..eb9b8b3 100644 > > --- a/grub-core/kern/xen/init.c > > +++ b/grub-core/kern/xen/init.c > > @@ -525,6 +525,48 @@ map_all_pages (void) > > grub_mm_init_region ((void *) heap_start, heap_end - heap_start); > > } > > > > +static char * > > + (char *pos, char **word) > > +{ > > + while (grub_isspace (*pos)) > > + pos++; > > + > > + if (!*pos) > > + return NULL; > > + > > + *word =3D pos; > > + while (*pos && !grub_isspace (*pos)) > > + pos++; > > + if (*pos) > > + *pos++ =3D '\0'; > > + return pos; > > +} > > + > > +static void > > +parse_command_line (void) > > +{ > > + char *cmd_line; > > + char *pos, *word; > > + > > + cmd_line =3D grub_malloc (MAX_GUEST_CMDLINE + 1); > > + grub_memcpy (cmd_line, grub_xen_start_page_addr->cmd_line, > > + MAX_GUEST_CMDLINE); > > + cmd_line[MAX_GUEST_CMDLINE] =3D '\0'; > > + pos =3D cmd_line; > > + while ((pos =3D fetch_command_line_word (pos, &word)) !=3D NULL) > > + { > > + char *equals; > > + > > + equals =3D grub_strchr (word, '=3D'); > > + if (!equals) > > + continue; > > + > > + *equals =3D '\0'; > > + grub_env_set (word, equals + 1); > > + } > > + grub_free (cmd_line); > > +} > > + > > extern char _end[]; > > > > void > > @@ -547,6 +589,8 @@ grub_machine_init (void) > > grub_xendisk_init (); > > > > grub_boot_init (); > > + > > + parse_command_line (); > > } > > > > void > > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel > --001a11c3473a61def704ed589fe0 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

I was about to post similar remarks. I'm on phone so will tell just = the key points.
- Most of GRUB platforms have command lines. I.a xen, efi, loongson, ieee12= 75, arc.
- Many platforms add their own command line arguments even if user didn'= ;t ask for it. Those shouldn't interfere. In your patch root=3Dsda1 fro= m xen will either be lost or will clobber GRUB root.
- IEEE1275 code already looks into command line. If it does sth sane it cou= ld be used as base, otherwise it should be replaced with sth common.
- Perhaps we should use some prefix to avoid unintended clobbering.
- There should be a way to emulate pvgrub1 behaviour which took config name= from commandline. Perhaps pvgrub2 should even do it by default (by using l= egacy_configfile)

On Dec 12, 2013 4:48 PM, "Andrey Borzenkov&= quot; <arvidjaar@gmail.com>= ; wrote:
=D0=92 Thu, 12 Dec 2013 15:37:22 +0000
Colin Watson <cjwatson@ubuntu.com= > =D0=BF=D0=B8=D1=88=D0=B5=D1=82:

> * grub-core/kern/xen/init.c (fetch_command_line_word): New function. > (parse_command_line): Likewise.
> (grub_machine_init): Call parse_command_line.
> ---
> =C2=A0ChangeLog =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 | =C2=A08 ++++++++
> =C2=A0grub-core/kern/xen/init.c | 44 +++++++++++++++++++++++++++++++++= +++++++++++
> =C2=A02 files changed, 52 insertions(+)
>
> diff --git a/ChangeLog b/ChangeLog
> index 766fe4b..fc86601 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,13 @@
> =C2=A02013-12-12 =C2=A0Colin Watson =C2=A0<cjwatson@ubuntu.com>
>
> + =C2=A0 =C2=A0 Accept environment variables on the command line for X= en.
> +

Thank you!

It may make sense to generalize it to other archs. At least EFI
definitely comes in mind. In this case platform specific part is only
to fetch parameter string(s). What about

- move fetch_command_line_word and parse_command_line to separate file
=C2=A0 that is included for specific platforms only
- make each platform that can accept fetch parameters in platform
=C2=A0 specific way and call common code (parse_command_line)

Does it make sense?

I will add EFI part then later.

> + =C2=A0 =C2=A0 * grub-core/kern/xen/init.c (fetch_command_line_word):= New function.
> + =C2=A0 =C2=A0 (parse_command_line): Likewise.
> + =C2=A0 =C2=A0 (grub_machine_init): Call parse_command_line.
> +
> +2013-12-12 =C2=A0Colin Watson =C2=A0<cjwatson@ubuntu.com>
> +
> =C2=A0 =C2=A0 =C2=A0 Add an option to exclude devices from search resu= lts.
>
> =C2=A0 =C2=A0 =C2=A0 * grub-core/commands/search.c (struct search_ctx)= : Add excludes and
> diff --git a/grub-core/kern/xen/init.c b/grub-core/kern/xen/init.c
> index 1d8eaec..eb9b8b3 100644
> --- a/grub-core/kern/xen/init.c
> +++ b/grub-core/kern/xen/init.c
> @@ -525,6 +525,48 @@ map_all_pages (void)
> =C2=A0 =C2=A0grub_mm_init_region ((void *) heap_start, heap_end - heap= _start);
> =C2=A0}
>
> +static char *
> + (char *pos, char **word)
> +{
> + =C2=A0while (grub_isspace (*pos))
> + =C2=A0 =C2=A0pos++;
> +
> + =C2=A0if (!*pos)
> + =C2=A0 =C2=A0return NULL;
> +
> + =C2=A0*word =3D pos;
> + =C2=A0while (*pos && !grub_isspace (*pos))
> + =C2=A0 =C2=A0pos++;
> + =C2=A0if (*pos)
> + =C2=A0 =C2=A0*pos++ =3D '\0';
> + =C2=A0return pos;
> +}
> +
> +static void
> +parse_command_line (void)
> +{
> + =C2=A0char *cmd_line;
> + =C2=A0char *pos, *word;
> +
> + =C2=A0cmd_line =3D grub_malloc (MAX_GUEST_CMDLINE + 1);
> + =C2=A0grub_memcpy (cmd_line, grub_xen_start_page_addr->cmd_line,<= br> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0MAX_GUEST_CMDLINE);
> + =C2=A0cmd_line[MAX_GUEST_CMDLINE] =3D '\0';
> + =C2=A0pos =3D cmd_line;
> + =C2=A0while ((pos =3D fetch_command_line_word (pos, &word)) !=3D= NULL)
> + =C2=A0 =C2=A0{
> + =C2=A0 =C2=A0 =C2=A0char *equals;
> +
> + =C2=A0 =C2=A0 =C2=A0equals =3D grub_strchr (word, '=3D'); > + =C2=A0 =C2=A0 =C2=A0if (!equals)
> + =C2=A0 =C2=A0 continue;
> +
> + =C2=A0 =C2=A0 =C2=A0*equals =3D '\0';
> + =C2=A0 =C2=A0 =C2=A0grub_env_set (word, equals + 1);
> + =C2=A0 =C2=A0}
> + =C2=A0grub_free (cmd_line);
> +}
> +
> =C2=A0extern char _end[];
>
> =C2=A0void
> @@ -547,6 +589,8 @@ grub_machine_init (void)
> =C2=A0 =C2=A0grub_xendisk_init ();
>
> =C2=A0 =C2=A0grub_boot_init ();
> +
> + =C2=A0parse_command_line ();
> =C2=A0}
>
> =C2=A0void


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
--001a11c3473a61def704ed589fe0-- --===============7845651709179264357== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============7845651709179264357==--