Re: [PATCH v2] kexec-tools: Fix option/argument parsing

Simon Horman
2010-05-19T00:37:23Z

On Fri, May 14, 2010 at 02:15:09PM +1000, Matt Evans wrote:
> Hi,
>
> v2 changes:
> - Fixed ARM's multiline KEXEC_ALL_OPTIONS #define
> - Removed opterr=0 from main()'s getopts_long() scan, and handle
> unknown options in main() (as it now knows about all allowed opts).
> Previously it relied on the file_type->load()er parsing & complaining
> about unknown options; if the loader didn't parse them, no complaint.
>
> Tested on x86, ppc and compile-tested on my new crisv32-axis-linux-gnu
> crosscompiler :-)
>
>
> Cheers,
>
>
> Matt
>
>
> ---
> The argument parsing is currently a bit broken as main()'s getopt_long()
> knows nothing about either the architecture-specific options or, even
> more specifically, the architecture-and-loader-specific options.
>
> This patch introduces new #defines for all architectures,
> KEXEC_ALL_OPTIONS and KEXEC_ALL_OPT_STR. These contain all possible
> options for a given build, and the getopt_long() passes in main() and
> arch_process_options() will now recognise arch- and loader-specific
> options; these will not be re-ordered in argv[], there is no confusion
> over which argv[] entry is the kernel filename, and using '--opt=foo' and
> '--opt foo' both work.
>
> All architectures have command line options (and #define OPT_BLAHs)
> consolidated into their include/arch/option.h files. x86_64 builds
> parts of i386/ as well, so now both share a single option.h file (with
> a symlink).
>
> Signed-off-by: Matt Evans <matt@ozlabs.org>
> ---

[snip]

> diff --git a/kexec/arch/sh/include/arch/options.h b/kexec/arch/sh/include/arch/options.h
> index e02960d..4bdd6a3 100644
> --- a/kexec/arch/sh/include/arch/options.h
> +++ b/kexec/arch/sh/include/arch/options.h
> @@ -7,16 +7,36 @@
> #define OPT_NBSD_HOWTO (OPT_ARCH_MAX+3)
> #define OPT_NBSD_MROOT (OPT_ARCH_MAX+4)
>
> -
> +/* Options relevant to the architecture (excluding loader-specific ones): */
> #define KEXEC_ARCH_OPTIONS \
> KEXEC_OPTIONS \
> {"command-line", 1, 0, OPT_APPEND}, \
> {"append", 1, 0, OPT_APPEND}, \
> {"empty-zero", 1, 0, OPT_APPEND}, \
> {"howto", 1, 0, OPT_NBSD_HOWTO}, \
> - {"miniroot", 1, 0, OPT_NBSD_MROOT}, \
> -
> + {"miniroot", 1, 0, OPT_NBSD_MROOT},
> +/* These options seem to be loader-specific rather than cris-specific, so
> + * ought to be moved to KEXEC_ALL_OPTIONS below and parsed in the relevant

The line above has a trailing space.

> + * loader, e.g. kexec-netbsd-sh.c
> + */
>
> #define KEXEC_ARCH_OPT_STR KEXEC_OPT_STR ""
>
> +/* The following two #defines list ALL of the options added by all of the
> + * architecture's loaders.
> + * o main() uses this complete list to scan for its options, ignoring
> + * arch-specific/loader-specific ones.
> + * o Then, arch_process_options() uses this complete list to scan for its
> + * options, ignoring general/loader-specific ones.
> + * o Then, the file_type[n].load re-scans for options, using
> + * KEXEC_ARCH_OPTIONS plus its loader-specific options subset.
> + * Any unrecognised options cause an error here.
> + *
> + * This is done so that main()'s/arch_process_options()'s getopt_long() calls
> + * don't choose a kernel filename from random arguments to options they don't
> + * recognise -- as they now recognise (if not act upon) all possible options.
> + */
> +#define KEXEC_ALL_OPTIONS KEXEC_ARCH_OPTIONS
> +#define KEXEC_ALL_OPT_STR KEXEC_ARCH_OPT_STR
> +
> #endif /* KEXEC_ARCH_SH_OPTIONS_H */

[snip]

> diff --git a/kexec/arch/x86_64/include/arch/options.h b/kexec/arch/x86_64/include/arch/options.h
> deleted file mode 100644
> index 75065b9..0000000
> --- a/kexec/arch/x86_64/include/arch/options.h
> +++ /dev/null
> @@ -1,22 +0,0 @@
> -#ifndef KEXEC_ARCH_X86_64_OPTIONS_H
> -#define KEXEC_ARCH_X86_64_OPTIONS_H
> -
> -#define OPT_RESET_VGA (OPT_MAX+0)
> -#define OPT_SERIAL (OPT_MAX+1)
> -#define OPT_SERIAL_BAUD (OPT_MAX+2)
> -#define OPT_CONSOLE_VGA (OPT_MAX+3)
> -#define OPT_CONSOLE_SERIAL (OPT_MAX+4)
> -#define OPT_ARCH_MAX (OPT_MAX+5)
> -
> -#define KEXEC_ARCH_OPTIONS \
> - KEXEC_OPTIONS \
> - { "reset-vga", 0, 0, OPT_RESET_VGA }, \
> - { "serial", 1, 0, OPT_SERIAL }, \
> - { "serial-baud", 1, 0, OPT_SERIAL_BAUD }, \
> - { "console-vga", 0, 0, OPT_CONSOLE_VGA }, \
> - { "console-serial", 0, 0, OPT_CONSOLE_SERIAL }, \
> -
> -#define KEXEC_ARCH_OPT_STR KEXEC_OPT_STR ""
> -
> -#endif /* KEXEC_ARCH_X86_64_OPTIONS_H */
> -
> diff --git a/kexec/arch/x86_64/include/arch/options.h b/kexec/arch/x86_64/include/arch/options.h
> new file mode 120000
> index 0000000..047b0f9
> --- /dev/null
> +++ b/kexec/arch/x86_64/include/arch/options.h
> @@ -0,0 +1 @@
> +../../../i386/include/arch/options.h
> \ No newline at end of file

The fragment above seems to a) belong in the previous fragment and
b) be bogus. In any case x86_64 fails to build.

kexec/kexec.c: In function 'main':
kexec/kexec.c:1061: error: 'KEXEC_ALL_OPTIONS' undeclared (first use in this function)
...

[snip]