Re: [Xen-devel] [PATCH v2 21/23] x86/boot: implement early command line parser in C

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.09.15 at 19:03, <daniel.kiper@xxxxxxxxxx> wrote:
> On Thu, Aug 27, 2015 at 06:43:39AM -0600, Jan Beulich wrote:
>> >>> On 20.07.15 at 16:29, <daniel.kiper@xxxxxxxxxx> wrote:
>> > +#define __packed  __attribute__((__packed__))
>> > +#define __text            __attribute__((__section__(".text")))
>> > +#define __used            __attribute__((__used__))
>>
>> Likely better to include compiler.h instead.
> 
> As I know you do not like to include such headers in early C files
> because it makes code fragile and it looks strange. I agree with you
> to some extent. So, I decided to define needed constants ourselves.
> Whatever we do we should be consistent. Hence, if we include compiler.h
> here we should do the same in reloc.c too if it is required.

I disagree - reloc.c serves a completely different purpose, and
hence may have different considerations applied when it comes
to what to (not) include.

>> > +#define max(x,y) ({ \
>> > +        const typeof(x) _x = (x);       \
>> > +        const typeof(y) _y = (y);       \
>> > +        (void) (&_x == &_y);            \
>> > +        _x > _y ? _x : _y; })
>>
>> I also wonder whether -imacros .../xen/kernel.h wouldn't be a better
>> approach here. Please really think hard on how to avoid duplications
>> like these.
> 
> Ditto. So, what is your decision? Include or define? If include then
> we should think how to generate relevant dependencies automatically.

I think the question should rather be whether we can't make cmdline.c
build the normal way, not the reloc.c one.

>> > +#define strlen_static(s) (sizeof(s) - 1)
>>
>> What is this good for? A decent compiler should be able to deal with
>> strlen("..."). Plus your macro is longer that what it tries to "abbreviate".
> 
> I thought that it is true but it is not. Sadly, without this binary is 
> bigger... :-(((

Perhaps as a result of some (missing) option(s)?

> However, you are right that the name could be better.

Or just drop the macro.

>> > +static const char empty_chars[] __text = " \n\r\t";
>>
>> What is empty about them? DYM blank or (white) space or separator
>> or delimiter? I also wonder whether \n and \r are actually usefully here,
> 
> Yep, delimiter or something like that looks better.
> 
>> as they should (if at all) only end the line.
> 
> Yes, I included them just in case and they should not appear in command 
> line.

If you mean to retain characters in that array that aren't obviously
needed, please explain such in a comment so people won't have to
guess whether to delete them, or will know it is (relatively) safe to
delete them in case their presence causes problems. But even better
would be to just have the obvious blank characters (space and tab)
there.

>> > +static void __cdecl __used cmdline_parse_early(const char *cmdline, 
> early_boot_opts_t *ebo)
>>
>> I don't see the point of the __cdecl, and (as said before) dislike the
>> static __used pair.
> 
> Are you sure that without __cdecl compiler will not try to optimize
> cmdline_parse_early() call and try to pass arguments using registers
> or anything else not conforming to cdecl calling convention?

Yes, I am - the moment you drop the "static". At that point the
compiler has no choice, unless it is being told on the command line
to use a different calling convention

Jan


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