|
[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, ¶m_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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |