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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.04.19 at 21:05, <vliaskovitis@xxxxxxxx> wrote:
> On Fri, 2019-04-05 at 17:01 +0200, Jan Beulich wrote:On 22.03.19 at
> 20:28, <vliaskovitis@xxxxxxxx> wrote:
>> > Limitations:
>> > - Custom runtime parameters (OPT_CUSTOM) are not supported yet.
>> > - For integer parameters (OPT_UINT), only unsigned parameters are
>> > printed
>> > correctly.
>> 
>> For this latter case I wonder whether it wouldn't be better to
>> return back the raw binary value, allowing the caller to format
>> it in suitable ways (e.g. both signed and unsigned, or dec and
>> hex).
> 
> Currently the caller is oblivious to the parameters and their types,
> and all retrieved values are printed together in a single string (see
> patch 4/4). I'd like to keep it like that for simplicity. Otherwise I
> think the caller has to find the types of requested parameters to
> produce the right formatting. Actually, since OPT_UINT is used for both
> signed and unsigned integer parameters, case-by-case parameter
> formatting would be required to do this, and the libx* callers do not
> have that knowledge. A "get_" per-parameter function pointer, which
> would handle formatting for each parameter, be it int, uint, string or
> custom, seems cleaner to me than leaving it to the caller.

I disagree. The caller requires no knowledge of the actual format.
It could easily format the string twice (once assuming it might be a
signed quantity, and another time assuming it might be an unsigned
one). All it would need to know is the width of the binary
representation.

> By the way, the current implementation searches in [__param_start
> __param_end), which are only the runtime parameters, not all
> parameters, correct? So the series should be renamed to "Support for
> reading runtime-only hypervisor parameters". The command is useful for
> checking parameters that can be changed at runtime after all. 

Yes, such renaming would seem desirable.

> I believe there are currently no signed integer runtime parameters. So
> alternatively we could add a warning to the commit message and/or to
> the code stating that signed integer runtime parameters, if added,
> would not be printed correctly at the moment. This would gloss over
> rather than solve the issue.

That's an option. As for other aspects, I don't heavily mind cases
not getting dealt with right away which have no practical use right
now, as long as the restrictions are clearly spelled out, such that
no-one will be surprised when trying to add a runtime option beyond
what the code is able to deal with.

Jan



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