Re: [PATCH v5 14/16] x86/boot: implement early command line parser in C

From: "Jan Beulich" <JBeulich@suse.com>
To: Daniel Kiper <daniel.kiper@oracle.com>
Cc: Juergen Gross <JGross@suse.com>,
	sstabellini@kernel.org, andrew.cooper3@citrix.com,
	cardoe@cardoe.com, pgnet.dev@gmail.com, ning.sun@intel.com,
	david.vrabel@citrix.com, xen-devel@lists.xenproject.org,
	qiaowei.ren@intel.com, gang.wei@intel.com, fu.wei@linaro.org
Subject: Re: [PATCH v5 14/16] x86/boot: implement early command line parser in C
Date: Thu, 25 Aug 2016 07:27:21 -0600	[thread overview]
Message-ID: <57BF0E59020000780010903D@prv-mh.provo.novell.com> (raw)
In-Reply-To: <1471646606-28519-15-git-send-email-daniel.kiper@oracle.com>

>>> On 20.08.16 at 00:43, <daniel.kiper@oracle.com> wrote:
> +#define NULL	((void *)0)
> +
> +#define __packed	__attribute__((__packed__))
> +#define __stdcall	__attribute__((__stdcall__))
> +
> +#define max(x,y) ({ \
> +        const typeof(x) _x = (x);       \
> +        const typeof(y) _y = (y);       \
> +        (void) (&_x == &_y);            \
> +        _x > _y ? _x : _y; })

Now that you add a second instance of (some of) these, please
move them to a new (local to this directory) header, e.g. defs.h.

> +#define tolower(c) ((c) | 0x20)
> +
> +typedef unsigned char bool_t;

_Bool and bool please and ...

> +typedef unsigned char u8;
> +typedef unsigned short u16;
> +typedef unsigned int size_t;
> +
> +#define FALSE		0
> +#define TRUE		1

... these replaced by true and false. In fact I see no reason why
you couldn't include xen/stdbool.h here now that it can be more
generally used.

> +/*
> + * Space and TAB are obvious delimiters. However, I am
> + * adding "\n" and "\r" here too. Just in case when
> + * crazy bootloader/user puts them somewhere.
> + */
> +static const char delim_chars_comma[] = ", \n\r\t";
> +static const char delim_chars[] = " \n\r\t";

I realize it's minor, but why two arrays instead of

#define delim_chars (delim_chars_comma + 1)

?

> +/*
> + * static const char *delim_chars = &delim_chars_comma[1];
> + *
> + * Older compilers, e.g. gcc version 4.1.2 20061115 (prerelease) (Debian 4.1.1-21),
> + * put &delim_chars_comma[1] directly into *delim_chars. This means that the address
> + * in *delim_chars is not properly updated during runtime. Newer compilers are much
> + * smarter and build fully relocatable code even if above shown construct is used.
> + * However, define delim_chars[] separately to properly build Xen code on
> + * older systems.
> + */

I have to admit that I don't really understand what you want to
say with this comment.

> +static unsigned int strtoui(const char *s, const char *stop, const char **next)
> +{
> +    char l;
> +    unsigned int base = 10, ores = 0, res = 0;
> +
> +    if ( *s == '0' )
> +      base = (tolower(*++s) == 'x') ? (++s, 16) : 8;
> +
> +    for ( ; *s != '\0'; ++s )
> +    {
> +        if ( stop && strchr(stop, *s) )
> +            goto out;
> +
> +        if ( *s < '0' || (*s > '7' && base == 8) )
> +        {
> +            res = UINT_MAX;
> +            goto out;
> +        }
> +
> +        l = tolower(*s);
> +
> +        if ( *s > '9' && (base != 16 || l < 'a' || l > 'f') )
> +        {
> +            res = UINT_MAX;
> +            goto out;
> +        }
> +
> +        res *= base;
> +        res += (l >= 'a') ? (l - 'a' + 10) : (*s - '0');
> +
> +        if ( ores > res )
> +        {
> +            res = UINT_MAX;
> +            goto out;
> +        }

Without having spent time to try and find an example, it feels like this
check won't catch all possible overflow conditions. If you care about
overflow, please make sure you catch all cases.

> --- a/xen/arch/x86/boot/trampoline.S
> +++ b/xen/arch/x86/boot/trampoline.S
> @@ -220,8 +220,20 @@ trampoline_boot_cpu_entry:
>          /* Jump to the common bootstrap entry point. */
>          jmp     trampoline_protmode_entry
>  
> +#include "video.h"
> +
> +/* Keep in sync with cmdline.c:early_boot_opts_t type! */
> +early_boot_opts:
>  skip_realmode:
>          .byte   0
> +opt_edd:
> +        .byte   0                               /* edd=on/off/skipmbr */
> +opt_edid:
> +        .byte   0                               /* EDID parsing option (force/no/default). */
> +GLOBAL(boot_vid_mode)
> +        .word   VIDEO_80x25                     /* If we don't run at all, assume basic video mode 3 at 80x25. */
> +vesa_size:
> +        .word   0,0,0                           /* width x depth x height */

While I don't mind you using the packed attribute on the C variant,
please insert a padding byte here and there to make the four words
aligned, and add an alignment directive to make the whole structure
at least word aligned.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel