[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 2/4] Accept environment variables on the command line for Xen.



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=sda1 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" <arvidjaar@xxxxxxxxx> wrote:
Ð Thu, 12 Dec 2013 15:37:22 +0000
Colin Watson <cjwatson@xxxxxxxxxx> ÐÐÑÐÑ:

> * 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 Â<cjwatson@xxxxxxxxxx>
>
> + Â Â 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 Â<cjwatson@xxxxxxxxxx>
> +
> Â Â Â Add an option to exclude devices from search results.
>
> Â Â Â * 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)
> Â Â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 = pos;
> + Âwhile (*pos && !grub_isspace (*pos))
> + Â Âpos++;
> + Âif (*pos)
> + Â Â*pos++ = '\0';
> + Âreturn pos;
> +}
> +
> +static void
> +parse_command_line (void)
> +{
> + Âchar *cmd_line;
> + Âchar *pos, *word;
> +
> + Âcmd_line = 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] = '\0';
> + Âpos = cmd_line;
> + Âwhile ((pos = fetch_command_line_word (pos, &word)) != NULL)
> + Â Â{
> + Â Â Âchar *equals;
> +
> + Â Â Âequals = grub_strchr (word, '=');
> + Â Â Âif (!equals)
> + Â Â continue;
> +
> + Â Â Â*equals = '\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@xxxxxxx
https://lists.gnu.org/mailman/listinfo/grub-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.