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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.06.16 at 10:15, <daniel.kiper@xxxxxxxxxx> wrote:
> 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.

Variable? I said literal numbers. As in

        strncmp(str, "xyz", 3);

From such code it is visible at the first glance what the 3 stands for,
and is imo better readable than

        strncmp(str, "xyz", strlen("xyz"));

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

There are two possible issues, which would need checking (which
I only thought of after having written that reply):
- Do all gcc versions we care about support stdcall?
- What's the disposition of those asm() stubs on the callee side?
  (Remember that Andrew had asked for them to get dropped?)

Jan


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