[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



thanks for the review.

On Wed, 2019-03-13 at 17:35 +0100, Jan Beulich wrote:
> > > > On 06.03.19 at 13:58, <vliaskovitis@xxxxxxxx> wrote:
> > +static int get_params(const char *cmdline, char *values,
> > +                      const struct kernel_param *start,
> > +                      const struct kernel_param *end)
> > +{
> > +    char opt[128], *optkey, *q;
> > +    const char *p = cmdline, *val = values;
> 
> Why is val a pointer to const char? There are several casts below
> because of this, and all these casts should go away.

yes, it should be a pointer to char, and those casts are not needed, I
sent out an older version by mistake.

> 
> > +    const struct kernel_param *param;
> > +    int len, rc = 0;
> > +    uint64_t param_int;
> > +    bool found;
> > +
> > +    if (!values)
> > +        return -EFAULT;
> > +
> > +    for ( ; ; )
> > +    {
> > +        /* Skip whitespace. */
> > +        while ( *p == ' ' )
> > +            p++;
> 
> For a user exposed interface I think it would be better to at least
> use isspace() here.

ok

> 
> > +        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. 

> 
> > +        for ( param = start; param < end; param++ )
> > +        {
> > +
> > +            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. 

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

> 
> 
> > +                               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.

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.

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.  The first
option looks simpler to me.

The new custom function would have to be implemented of course for the
4 existing and any future custom parameters.

> 
> > +            default:
> > +                BUG();
> > +                break;
> > +            }
> > +        }
> > +
> > +        if ( !found )
> > +        {
> > +            printk("get-parameters: parameter \"%s\" unknown!\n",
> > optkey);
> 
> I don't think logging of a message is appropriate here.

ok

> 
> > @@ -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.

> 
> > --- a/xen/common/sysctl.c
> > +++ b/xen/common/sysctl.c
> > @@ -501,6 +501,51 @@ long
> > do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> >  
> >          break;
> >      }
> > +    case XEN_SYSCTL_get_parameter:
> > +    {
> > +#define XEN_GET_PARAMETER_MAX_SIZE 1023
> 
> May I suggest to re-use XEN_SET_PARAMETER_MAX_SIZE,
> perhaps after renaming to XEN_PARAMETER_MAX_SIZE?

ok

> 
> > +        char *params;
> > +        char *values;
> > +
> > +        if ( op->u.get_parameter.pad[0] || op-
> > >u.get_parameter.pad[1] ||
> > +             op->u.get_parameter.pad[2] )
> > +        {
> > +            ret = -EINVAL;
> > +            break;
> > +        }
> > +        if ( op->u.get_parameter.size > XEN_GET_PARAMETER_MAX_SIZE
> > )
> > +        {
> > +            ret = -E2BIG;
> > +            break;
> > +        }
> > +        params = xmalloc_bytes(op->u.get_parameter.size + 1);
> > +        values = xmalloc_bytes(XEN_GET_PARAMETER_MAX_SIZE);
> > +        if ( !params )
> > +        {
> > +            ret = -ENOMEM;
> > +            break;
> > +        }
> 
> What if params is non-NULL, but values is NULL? Also here and in one
> instance below you leak memory on the error paths.

I 'll fix these, thanks.

> 
> > +        if ( copy_from_guest(params, op->u.get_parameter.params,
> > +                             op->u.get_parameter.size) )
> > +            ret = -EFAULT;
> > +        else
> > +        {
> > +            params[op->u.get_parameter.size] = 0;
> 
> Even if this is a sysctl, please use array_access_nospec() here.

ok

> 
> > +            ret = runtime_get(params, values);
> > +
> > +            if ( copy_to_guest(op->u.get_parameter.values, values,
> > +                               strlen(values)) )
> 
> You should only copy when ret is zero.

right, thanks.

I 'll send a v2 after a few points above are cleared further.

thanks,

- Vasilis




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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.