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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Fri, May 27, 2016 at 03:33:49AM -0600, Jan Beulich wrote:
> >>> On 25.05.16 at 23:36, <daniel.kiper@xxxxxxxxxx> wrote:
> > On Wed, May 25, 2016 at 04:33:54AM -0600, Jan Beulich wrote:
> >> >>> On 15.04.16 at 14:33, <daniel.kiper@xxxxxxxxxx> wrote:

[...]

> >> > +/*
> >> > + * Compiler is not able to optimize regular strlen()
> >> > + * if argument is well known string during build.
> >> > + * Hence, introduce optimized strlen_opt().
> >> > + */
> >> > +#define strlen_opt(s) (sizeof(s) - 1)
> >>
> >> Do we really care in this code?
> >
> > Not to strongly but why not?
>
> Keep things as readable as possible. In fact I wouldn't mind hard
> coded literal numbers for the string lengths, if they sit right next
> to the respective string literal.

As separate variable? Does it pays? I prefer standard strlen() call
instead of that.

> >> > +static int strtoi(const char *s, const char *stop, const char **next)
> >> > +{
> >> > +    int base = 10, i, ores = 0, res = 0;
> >>
> >> You don't even handle a '-' on the numbers here, so all the variables
> >
> > Yep, however, ...
> >
> >> and the function return type should be unsigned int afaict. And the
> >> function name perhaps be strtoui().
> >
> > ... we return -1 in case of error.
>
> Which - having looked at some of the callers - could easily be
> UINT_MAX as it seems.

Here it looks safe.

> >> > +static u8 skip_realmode(const char *cmdline)
> >> > +{
> >> > +    return !!find_opt(cmdline, "no-real-mode", 0) || !!find_opt(cmdline,
> > "tboot=", 1);
> >>
> >> The || makes the two !! pointless.
> >>
> >> Also please settle on which type you want to use for boolean
> >> (find_opt()'s last parameter is "int", yet here you use "u8"), and
> >
> > Could be u8.
> >
> >> perhaps make yourself a bool_t.
> >
> > I do not think it make sense here.
>
> I think it makes as much or as little sense as having NULL available.

:-)))

> >> > +        /*
> >> > +         * Increment c outside of strtoi() because otherwise some
> >> > +         * compiler may complain with following message:
> >> > +         * warning: operation on ‘c’ may be undefined.
> >> > +         */
> >> > +        ++c;
> >> > +        tmp = strtoi(c, "x", &c);
> >>
> >> The comment is pointless - the operation is firmly undefined if you
> >> put it in the strtoi() invocation.
> >
> > In terms of C spec you are right. However, it is quite surprising that older
> > GCC complains and newer ones do not. Should not we investigate this?
>
> Actually I think I was wrong here. A function call like func(c++, c)

Because argument evaluation order is undefined in C. Am I correct?

> would be undefined, but func(c++, &c) isn't. So I guess if there are
> compiler versions getting this wrong, then you should just disregard
> my comment.

By the way, here is quite good description of these problems:
  http://en.cppreference.com/w/c/language/eval_order

> >> > +        pushl   $sym_phys(early_boot_opts)
> >> > +        pushl   MB_cmdline(%ebx)
> >> >          call    cmdline_parse_early
> >> > +        add     $8,%esp             /* Remove cmdline_parse_early() 
> >> > args from stack. */
> >>
> >> I don't think such a comment is really useful (seems like I overlooked
> >> a similar one in an earlier patch, on the reloc() invocation).
> >
> > This thing is quite obvious but I do not think that this comment hurts.
>
> It may not really hurt, but it draws needless attention to something
> that is to b expected after any function call getting arguments
> passed on the stack. You could, btw., make cmdline_parse_early
> a stdcall function, so you wouldn't have to do that adjustment
> here.

If it is acceptable by you then I can do that.

Daniel

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