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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 25.05.16 at 23:36, <daniel.kiper@oracle.com> wrote:
> On Wed, May 25, 2016 at 04:33:54AM -0600, Jan Beulich wrote:
>> >>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote:
>> > --- /dev/null
>> > +++ b/xen/arch/x86/boot/cmdline.c
>> > @@ -0,0 +1,357 @@
>> > +/*
>> > + * Copyright (c) 2015, 2016 Oracle and/or its affiliates. All rights reserved.
>> > + *      Daniel Kiper <daniel.kiper@oracle.com>
>> > + *
>> > + * This program is free software; you can redistribute it and/or modify
>> > + * it under the terms of the GNU General Public License as published by
>> > + * the Free Software Foundation; either version 2 of the License, or
>> > + * (at your option) any later version.
>> > + *
>> > + * This program is distributed in the hope that it will be useful,
>> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> > + * GNU General Public License for more details.
>> > + *
>> > + * You should have received a copy of the GNU General Public License along
>> > + * with this program.  If not, see <http://www.gnu.org/licenses/>.
>> > + *
>> > + * strlen(), strncmp(), strspn() and strcspn() were copied from
>> > + * Linux kernel source (linux/lib/string.c).
>>
>> Any reason you can't just #include ".../common/string.c" here?
> 
> I am confused. Sometimes you request to reduce number of such strange
> includes and sometimes you ask me to do something contrary? So, what
> is the rule behind these requests?

The rule is "whatever fits best for the current purpose". Such
"strange" includes should be avoided if their purpose can be
fulfilled by other means. Them being avoided at the price of
quite a bit of code duplication, otoh, doesn't seem well advised
to me.

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

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

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

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

Jan

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