Re: [Xen-devel] [PATCH v4 1/4] xen: add hypercall for reading runtime parameters

From: "Jan Beulich" <JBeulich@suse.com>
To: "Vasilis Liaskovitis" <vliaskovitis@suse.com>
Cc: Juergen Gross <jgross@suse.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	wei.liu2@citrix.com, George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	Anthony Perard <anthony.perard@citrix.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Daniel de Graaf <dgdegra@tycho.nsa.gov>
Subject: Re: [Xen-devel] [PATCH v4 1/4] xen: add hypercall for reading runtime parameters
Date: Fri, 07 Jun 2019 07:49:48 -0600	[thread overview]
Message-ID: <5CFA6B7C0200007800236491@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <20190528145416.16918-2-vliaskovitis@suse.com>

>>> On 28.05.19 at 16:54, <vliaskovitis@suse.com> wrote:
> +int runtime_get_params(const char *cmdline, char *values,
> +                      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;
> +
> +    while ( !rc )
> +    {
> +        /* Skip whitespace. */
> +        while ( isspace(*p) )
> +            p++;
> +        if ( *p == '\0' )
> +            break;
> +
> +        /* Grab the next whitespace-delimited option. */
> +        q = optkey = opt;
> +        while ( !isspace(*p) && (*p != '\0') )
> +        {
> +            if ( (q - opt) < (sizeof(opt) - 1) ) /* avoid overflow */
> +                *q++ = *p;
> +            else return -ENOMEM;
> +            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);
> +                break;
> +
> +            case OPT_UINT:
> +            case OPT_SIZE:
> +                /* Signed integer parameters are not supported yet.
> +                 * While there are no runtime signed integer parameters
> +                 * at the moment, adding one and trying to get its value
> +                 * with the current implementation will output the wrong
> +                 * value.
> +                 */

Comment style.

> +                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;
> +            }
> +
> +            if ( len < 0 )
> +                rc = len;
> +            else if ( len < maxlen - bufpos )
> +                /* if output was not truncated update buffer position. */

Again. It's pretty odd to have a full stop (which isn't mandatory, but
I appreciate it being there) yet start with a lower case letter (which
isn't permitted).

> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -466,9 +466,9 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>              copyback = 1;
>          break;
>  
> +#define XEN_PARAMETER_MAX_SIZE 1023
>      case XEN_SYSCTL_set_parameter:
>      {
> -#define XEN_SET_PARAMETER_MAX_SIZE 1023
>          char *params;
>  
>          if ( op->u.set_parameter.pad[0] || op->u.set_parameter.pad[1] ||
> @@ -477,7 +477,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>              ret = -EINVAL;
>              break;
>          }
> -        if ( op->u.set_parameter.size > XEN_SET_PARAMETER_MAX_SIZE )
> +        if ( op->u.set_parameter.size > XEN_PARAMETER_MAX_SIZE )
>          {
>              ret = -E2BIG;
>              break;
> @@ -501,6 +501,54 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>  
>          break;
>      }
> +    case XEN_SYSCTL_get_parameter:

Blank line between non-fall-through case blocks please.

These are all issues which could easily be fixed while committing,
and other than those I'm now fine with the patch. However, I'm
still not overly happy with the restrictions named, some of which
derive from certain interface decisions (like using ASCII strings
as return values). I'd therefore like to have at least one other
maintainer's opinion as to whether to indeed go with this interface
before giving my ack.

Jan

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