Re: [Xen-devel] [PATCH v1 1/4] xen: add hypercall for getting parameters at runtime

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.03.19 at 16:44, <vliaskovitis@xxxxxxxx> wrote:
> On Mon, 2019-03-18 at 15:02 +0100, Jan Beulich wrote:
>> > > I'm also
>> > > unconvinced this is appropriate if the value is actually
>> > > a signed quantity.
>> > 
>> > isn't OPT_UINT only for unsigned integers?
>> 
>> You'll notice that there's no OPT_INT or OPT_SINT. OPT_UINT
>> may be slightly misleading as a name. Otoh we probably don't
>> have very many signed integer options.
> 
> I see. Assuming an unsigned parameter is problematic then.
> 
> Doesn't the .data.param section (between __param_start and __param_end)
> only include runtime parameters, of which the integer ones are all
> unsigned integers at the moment? Or would you like to see this
> functionality eventually used for all parameters, including non-runtime 
> as well?

Well, at the very least I expect restrictions to be called out very
clearly in the commit message. There not being any problematic
runtime parameters right now doesn't mean some might not
appear. And whether the problem would then be noticed is at
least unpredictable. IOW not dealing with the case right away
introduces a latent bug, and this shouldn't go unmentioned.

>> > with the new get_func prototype being:
>> > 
>> >             const char *(*get_func)(void);
>> > 
>> > returning a string with the current value of the parameter.
>> 
>> And where would the storage live for the string?
> 
> I was thinking these custom functions would return static strings, as
> loglvl_str() does, and these would eventually be copied into the user
> provided values[] buffer via snprintf in get_params().

Please don't limit your thinking to the few runtime parameters we
currently have. At least to me it is quite obvious that there can
easily be cases where static strings won't work.

>> > e.g. the function for loglvl, guest_loglvl could return output
>> > using
>> > loglvl_str() :  "Nothing", "Errors", "Errors and warnings" etc.
>> > 
>> > An alternative prototype could be:
>> > 
>> >             int (*get_func)(char *output);
>> > 
>> > if we want the function to write the current parameter value into a
>> > caller-provided buffer, and possibly return error codes.
>> 
>> And how would the callee know how much space there is?
> 
> yes, an extra size argument would be needed from the caller here.
> 
> Let me know if either of these or a different approach is preferred.

Of the two, I clearly prefer the latter. Then again me having pointed
out the shortcoming doesn't mean we (and hence you for this patch)
absolutely have to deal with custom parameters. Input from others
would certainly be helpful here.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel