Re: [Xen-devel] [PATCH v4] xen: Allow a default compiled-in command line using Kconfig

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.03.17 at 18:36, <blackskygg@xxxxxxxxx> wrote:
> 2017-03-10 23:03 GMT+08:00 Jan Beulich <JBeulich@xxxxxxxx>:
>>>>> On 09.03.17 at 04:13, <blackskygg@xxxxxxxxx> wrote:
>>> If CMDLINE is set, the cmdline_parse() routine will append the bootloader
>>> command line to this string, forming the complete command line
>>> before parsing.
>>
>> I disagree to making it behave like this: There's no need to
>> concatenate both, simply parse them one after the other. The
>> built-in one is well known (and hence also has no need to be in
>> saved_cmdline). That'll avoid reducing the space available for
>> user specified options.
>>
> 
> I did so because I do think the embedded one should be in
> saved_cmdline, too, but your suggestion
> sounds quite reasonable.
> Then I really have to introduce a wrapper to the original
> cmdline_parse(). (Recursion seems not suitable for
> this, since a new variable indicating the depth is inevitable, making
> the parameter list even longer).

I don't see why recursion is not suitable. All you need is a proper
guard to avoid recursing more than you really want.

>>> @@ -1566,14 +1557,14 @@ void __init noreturn __start_xen(unsigned long 
>>> mbi_p)
>>>
>>>      /* Grab the DOM0 command line. */
>>>      cmdline = (char *)(mod[0].string ? __va(mod[0].string) : NULL);
>>> -    if ( (cmdline != NULL) || (kextra != NULL) )
>>> +    if ( (cmdline != NULL) || strlen(kextra) )
>>
>> Is there any reason why kextra can't come out as NULL if unset,
>> avoiding the need to touch the code here? That would also avoid
>> making kextra a static variable.
>>
> 
> In the original code, kextra is a pointer to a suffix of the original
> cmdline. It's
> orphaned from cmdline by turning the first blank in " -- " into a '\0'.
> But now since the dom0 options can appear in both CONFIG_CMDLINE and
> the bootloader
> cmdline, there must be some place (an array) to hold the concatenated
> dom0 option string.
> So if we want to avoid modifying this piece of code, I can only come
> up with two solutions:
> 1.  Define a new array in this function and let kextra point to it.
> Set kextra to NULL if the array is empty.
>      But I think this is too awkward.
>  2. Define a new array,  say, opt_dom0_options[],  in kernel.c, and
> return the pointer to this array back to
>      the caller when cmdline_parse() is invoked, or return NULL if the
> array is empty.
> What do you say?

Having thought about it again, I'm actually no longer of the opinion
that hard coding a Dom0 command line (portion) in the hypervisor
is sensible at all. With that eliminated, this discussion aspect is moot
afaict.

Jan


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