Re: [PATCH v4 04/34] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.

[Konrad Rzeszutek Wilk <konrad@kernel.org>]

.. snip ..
> > +    case XEN_VERSION_OP_guest_handle:
> > +        *sz = ARRAY_SIZE(current->domain->handle);
> > +        break;
> > +
> > +    case XEN_VERSION_OP_commandline:
> > +        *sz = ARRAY_SIZE(saved_cmdline);
> > +        break;
> > +
> > +    default:
> > +        rc = -ENOSYS;
> > +    }
> > +
> > +    return rc;
> > +}
> > +
> > +/*
> > + * Similar to HYPERVISOR_xen_version but with a sane interface
> > + * (has a length, one can probe for the length) and with one less sub-ops:
> > + * missing XENVER_compile_info.
> > + */
> > +DO(version_op)(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg,
> > +               unsigned int len)
> > +{
> > +    union {
> > +        xen_version_op_val_t n;
> > +        xen_feature_info_t fi;
> > +    } u;
> 
> = {}; and you can forgo the explicit memset() below.

Done!
> 
> > +    ssize_t sz = 0;
> > +    const void *ptr = NULL;
> > +    int rc = xsm_version_op(XSM_OTHER, cmd);
> > +
> > +    /* We can safely return -EPERM! */
> > +    if ( rc )
> > +        return rc;
> > +
> > +    rc = size_of_subops_data(cmd, &sz);
> > +    if ( rc )
> > +        return rc;
> > +
> > +    /* Some of the subops may have no data. */
> > +    if ( !sz )
> > +        return 0;
> 
> Really? I would have thought it would be reasonable to assert that
> either sz != 0 after the rc != 0 return.

Commandline and guest_handle may be empty. Ah they aren't as
they are array.

ARRAY_SIZE(saved_commandline) is always 1024. Ugh.

.. snip..

> > +
> > +    if ( !rc )
> > +    {
> > +        ssize_t bytes;
> > +
> > +        if ( sz > len )
> > +            bytes = len;
> > +        else
> > +            bytes = sz;
> > +
> > +        if ( copy_to_guest(arg, ptr ? ptr : &u, bytes) )
> 
> Can be shortened to ptr ?: &u
> 
> > +            rc = -EFAULT;
> > +    }
> > +    if ( !rc )

         ^^^^^^^^^ - here
> > +    {
> > +        /*
> > +         * We return len (truncate) worth of data even if we fail.
> > +         */
> > +        if ( sz > len )
> > +            rc = -ENOBUFS;
> 
> This needs to be in the previous if() clause to avoid overriding -EFAULT
> with -ENOBUFS.

That is exactly why it is in its own 'if ( !rc )' - so it won't
overwrite -EFAULT. See above for 'here'


> 
> > +
> > +/*
> > + * The HYPERCALL_version_op has a set of sub-ops which mirror the
> > + * sub-ops of HYPERCALL_xen_version. However this hypercall differs
> > + * radically from the former:
> > + *  - It returns the amount of bytes returned.
> > + *  - It will return -XEN_EPERM if the guest is not permitted.
> > + *  - It will return the requested data in arg.
> > + *  - It requires an third argument (len) for the length of the
> > + *    arg. Naturally the arg has to fit the requested data otherwise
> > + *    -XEN_ENOBUFS is returned.
> > + *
> > + * It also offers an mechanism to probe for the amount of bytes an
> > + * sub-op will require. Having the arg have an NULL pointer will
> > + * return the number of bytes requested for the operation. Or an
> > + * negative value if an error is encountered.
> > + */
> > +
> > +typedef uint64_t xen_version_op_val_t;
> > +DEFINE_XEN_GUEST_HANDLE(xen_version_op_val_t);
> > +
> > +typedef unsigned char xen_version_op_buf_t[];
> > +DEFINE_XEN_GUEST_HANDLE(xen_version_op_buf_t);
> 
> Strictly speaking this should be a void* guest handle, as not all data
> is returned via this mechanism is unsigned char.

Done!
> 
> > +
> > +/* arg == version_op_val_t. Encoded as major:minor (31..16:15..0) */
> > +#define XEN_VERSION_OP_version      0
> > +
> > +/* arg == version_op_buf. */
> > +#define XEN_VERSION_OP_extraversion 1
> > +
> > +/* arg == version_op_buf */
> > +#define XEN_VERSION_OP_capabilities 3
> > +
> > +/* arg == version_op_buf */
> > +#define XEN_VERSION_OP_changeset 4
> 
> Might be worth stating that these return NUL terminated utf-8 strings?

Done!
> 
> ~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel