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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

> +int runtime_get_params(const char *cmdline, char *values,

I don't think "cmdline" is the correct term here ("opts" would
be a possible better name), but then again this keeps it
nicely in line with the actual cmdline parsing function.

> +                      size_t maxlen)
> +{
> +    char opt[128], *optkey, *q, *val = values;
> +    const char *p = cmdline;
> +    const struct kernel_param *param;
> +    int rc = 0, len = 0;
> +    size_t bufpos = 0;
> +    uint64_t param_int;
> +
> +    if (!values)

Style (missing blanks). But the question is whether this conditional
is useful at all. The only caller passes a non-NULL value anyway.
Otherwise, why wouldn't you check cmdline as well?

> +        return -EFAULT;
> +
> +    for ( ; ; )
> +    {
> +        /* Skip whitespace. */
> +        while ( isspace(*p) )
> +            p++;
> +        if ( *p == '\0' )
> +            break;
> +
> +        /* Grab the next whitespace-delimited option. */
> +        q = optkey = opt;
> +        while ( (*p != ' ') && (*p != '\0') )

You've used isspace() above - why not here?

> +        {
> +            if ( (q - opt) < (sizeof(opt)-1) ) /* avoid overflow */

Missing blanks (around binary operator).

I also think that you should return an error upon buffer
overflow, instead of potentially returning the wrong option's
value.

> +                *q++ = *p;
> +            p++;
> +        }
> +        *q = '\0';
> +
> +        for ( param = __param_start; param < __param_end; param++ )
> +        {
> +            if ( strcmp(param->name, optkey) )
> +                continue;
> +
> +            switch ( param->type )
> +            {
> +            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.

> +                break;
> +            case OPT_UINT:

I think (hope) I've said so on v1 already: Please have blank lines
between individual case blocks.

> +            case OPT_SIZE:
> +                get_integer_param(param, &param_int);
> +                len = snprintf(val + bufpos, maxlen - bufpos,
> +                               "%"PRIu64" ", param_int);
> +                break;
> +            case OPT_BOOL:
> +                get_integer_param(param, &param_int);
> +                len = snprintf(val + bufpos, maxlen - bufpos, "%s ",
> +                               param_int ? "true" : "false");
> +                break;
> +            case OPT_CUSTOM:
> +                /* Custom parameters are not supported yet. */
> +                rc = -EINVAL;
> +                break;
> +            default:
> +                BUG();
> +                break;

Please be consistent as to whether you add "break;" in a default
case positioned last in a switch(). Personally I'd prefer them to be
there, but I won't insist. All I'd like to see is no mixed style within
a single patch at least.

> +            }
> +
> +            if (len < 0)
> +                rc = len;
> +            else if (len < maxlen - bufpos)
> +            /* if output was not truncated update buffer position */
> +                bufpos += (size_t) len;
> +            else if (len > 0)
> +                rc = -ENOMEM;

Various style issues here: Missing blanks inside if(), malformed and
improperly indented comment. I also don't see the need for the
cast, and if it was needed there would be a stray blank there.

> +            break;
> +        }
> +
> +        /* no parameter was matched */
> +        if ( param >= __param_end )
> +        {
> +            rc = -EINVAL;
> +        }

Stray braces.

> +        if (rc)
> +            break;

Missing blanks again. But perhaps better make the loop
"while ( !rc )" anyway, or "do { ... } while ( !rc );".

> @@ -501,6 +501,57 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
> u_sysctl)
>  
>          break;
>      }
> +    case XEN_SYSCTL_get_parameter:

As per above, blank line above here please.

> +    {
> +        char *params;
> +        char *values;

Put both on the same line?

> +        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_PARAMETER_MAX_SIZE )
> +        {
> +            ret = -E2BIG;
> +            break;
> +        }
> +        params = xmalloc_bytes(op->u.get_parameter.size + 1);
> +        if ( !params )
> +        {
> +            ret = -ENOMEM;
> +            break;
> +        }
> +
> +        values = xmalloc_bytes(XEN_PARAMETER_MAX_SIZE);
> +        if ( !values )
> +        {
> +            xfree(params);
> +            ret = -ENOMEM;
> +            break;
> +        }
> +
> +        else if ( copy_from_guest(params, op->u.get_parameter.params,
> +                             op->u.get_parameter.size) )

The "else" is pointless here with the "break;" above, but if you
were to keep it, then there should be no blank line ahead of it.
The second line is also insufficiently indented.

> +            ret = -EFAULT;
> +        else
> +        {
> +            params[op->u.set_parameter.size] = 0;
> +            ret = runtime_get_params(params, values, XEN_PARAMETER_MAX_SIZE);
> +
> +            if ( !ret && copy_to_guest(op->u.get_parameter.values, values,
> +                               strlen(values)) )

Indentation again.

> +            {
> +                ret = -EFAULT;
> +            }

Stray braces again.

Jan


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