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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.03.19 at 14:34, <vliaskovitis@xxxxxxxx> wrote:
> On Wed, 2019-03-13 at 17:35 +0100, Jan Beulich wrote:
>> > > > On 06.03.19 at 13:58, <vliaskovitis@xxxxxxxx> wrote:
>> > +        found = false;
>> 
>> I don't think you need this variable here, or if so, it shouldn't
>> be boolean: Either you mean to support returning data for
>> multiple matching options (there are no runtime ones at
>> present, but we at least used to have multiple-instance
>> boot time ones), in which case you may need to invent a
>> means to disambiguate them. Or you don't, in which case
>> you could bail from the loop once you've found a match.
> 
> Multiple matching options was not my intention. Yes, the loop going
> through the parameters should be exited on a match, but I still need to
> detect if there was no parameter matched at all for the current option,
> in order to return -EINVAL in that case. This can be detected from the
> return value of strcmp, instead of using a boolean variable. 

From the return value of strcmp()? I don't think so, because you
may have run past all table entries. Instead it's that property
that you can use, i.e. checking whether ...

>> > +        for ( param = start; param < end; param++ )

... the loop end condition was hit.

>> > +        {
>> > +
>> > +            if ( strcmp(param->name, optkey) )
>> > +                continue;
>> > +
>> > +            found = true;
>> > +            switch ( param->type )
>> > +            {
>> > +            case OPT_STR:
>> > +                len = snprintf((char*)val, sizeof(values), "%s ",
>> 
>> Here and below, sizeof(values) is wrong in two ways: For one
>> it's sizeof(char*), not sizeof(char[nn]). And then you need to
>> subtract the amount of space consumed already.
> 
> thanks, a basic mistake. I think the caller will need to pass the size
> of the values[] buffer as an argument to the function, or I can always
> assume a XEN_PARAMETER_SIZE max size for the values[] buffer. The
> former sounds better to me. 

Indeed.

>> > +                               (char*)param->par.var);
>> > +                val += len;
>> > +                break;
>> > +            case OPT_UINT:
>> > +                get_integer_param(param, &param_int);
>> > +                len = snprintf((char*)val, sizeof(values), "%lu ",
>> > param_int);
>> 
>> This is going to fail to build on 32-bit Arm. 
> 
> I will change the formatting to %"PRIu64"
> 
> 
>> 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.

>> > +                               param_int ? "true" : "false");
>> > +                val += len;
>> > +                break;
>> > +            case OPT_SIZE:
>> > +            case OPT_CUSTOM:
>> > +                rc = -EINVAL;
>> > +                break;
>> 
>> I can see why custom parameters can't be dealt with yet,
>> but why also size ones? As to custom ones - four out of the
>> seven runtime parameters we currently have are custom
>> ones, so this limits the utility of the change quite a bit.
> 
> From what I understand OPT_SIZE can be handled similarly to OPT_UINT,
> printing an unsigned integer in the output.

Right.

> For OPT_CUSTOM, what about adding a get_func function pointer to their
> definition in kernel_param, which would be used for reading the current
> value? So the union par of struct kernel_param would now have a struct
> with 2 function pointers for the custom parameters, something like:
> 
>     union {
>         void *var;
>         struct {
>             int (*func)(const char *);
>             const char *(*get_func)(void);
>         } custom;
>     } par;
> 
> 
> 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?

> 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?

> The first option looks simpler to me.

Simpler is often, but not always better.

>> > @@ -199,6 +303,11 @@ int runtime_parse(const char *line)
>> >      return parse_params(line, __param_start, __param_end);
>> >  }
>> >  
>> > +int runtime_get_parameter(const char *line, char *values)
>> > +{
>> > +    return get_params(line, values, __param_start, __param_end);
>> > +}
>> 
>> I don't see the need for this wrapper.
> 
> I think I had trouble accessing __param_start, __param_end directly 
> from do_sysctl in xen/common/sysctl.c , that's why this was needed. But
> perhaps I missed something obvious.

You certainly shouldn't access them from sysctl.c. You should
simply make get_params() access them instead of what its
only caller passes, at which point the function should be
renamed to it only caller's name.

Jan


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