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

[Vasilis Liaskovitis <vliaskovitis@suse.com>]

Sorry for the delay, I was on a long vacation.

On Fri, 2019-04-05 at 17:01 +0200, Jan Beulich wrote:On 22.03.19 at
20:28, <vliaskovitis@suse.com> 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.

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. 

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.

If correct formatting for all possible types is a requirement, the
get_function may be needed. Or we could separate integer from unsigned
integer parameters with an additional OPT_INT parameter type. I don't
know if either of these is desirable or simply overkill to implement
just for this command.

I 'll send a v3 when there is agreement on the above.

[...]

> > +            {
> > +            case OPT_STR:
> > +                len = snprintf(val + bufpos, maxlen - bufpos, "%s
> > ",
> > +                               (char*)param->par.var);
> 
> What you return here is the value as set into the buffer when the
> option was parsed, and before that string was actually consumed.
> Is this really what's interesting to the user? I'd rather expect them
> to be after what is actually in effect.
> 
> Since we've decided against a "get" per-option hook, I consider it
> acceptable this way as long as the behavior is spelled out amongst
> the limitations.
> 

I 'll add this limitation to the commit message in the next version.

thanks,
- Vasilis


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