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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Thu, Aug 25, 2016 at 07:27:21AM -0600, Jan Beulich wrote:
> >>> On 20.08.16 at 00:43, <daniel.kiper@xxxxxxxxxx> 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.

Maybe we should include xen/stdbool.h in defs.h and move to it (defs.h)
all macros, types definitions, etc. from reloc.c and cmdline.c. This way
we can share more stuff without duplicating it in both files. Does it
make sense?

> > +/*
> > + * 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)
>
> ?

I can try it.

> > +/*
> > + * 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.

I tried to use following thing suggested by you earlier:
  static const char *delim_chars = &delim_chars_comma[1];

Sadly, it does not work because after building/linking by older
GCC/linker delim_chars contains static address and cannot be
properly relocated during runtime. Newer GCC/linkers are much
smarter and all references to delim_chars and finally to
&delim_chars_comma[1] are properly relocated during runtime.
However, there is a chance that your approach with #define
will work with all supported build environments.

> > +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.

Hmmm.... How come? Could you give an example?

Daniel

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