Re: [Xen-devel] [PATCH v8.1 04/27] xen/xsplice: Hypervisor implementation of XEN_XSPLICE_op

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 04/14/16 12:05 AM >>>
> @@ -460,6 +461,11 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
> u_sysctl)
>         ret = tmem_control(&op->u.tmem_op);
>         break;
> 
> +    case XEN_SYSCTL_xsplice_op:
> +        ret = xsplice_op(&op->u.xsplice);
> +        copyback = (ret == -ENOSYS || ret == -EOPNOTSUPP) ? 0 : 1;

Why use a conditional expression here when its condition already is a boolean 
one
just needing negating?

> +static int verify_name(const xen_xsplice_name_t *name, char *n)
> +{
> +    if ( !name->size || name->size > XEN_XSPLICE_NAME_SIZE )
> +        return -EINVAL;
> +
> +    if ( name->pad[0] || name->pad[1] || name->pad[2] )
> +        return -EINVAL;
> +
> +    if ( !guest_handle_okay(name->name, name->size) )
> +        return -EINVAL;
> +
> +    if ( __copy_from_guest(n, name->name, name->size) )
> +        return -EFAULT;

Is there a particular reason why you open code copy_from_guest() here? And
considering that you now also read the string here, isn't the function name
somewhat off?

> +static int xsplice_upload(xen_sysctl_xsplice_upload_t *upload)
> +{
> +    struct payload *data = NULL, *found;
> +    char n[XEN_XSPLICE_NAME_SIZE];
> +    int rc;
> +
> +    rc = verify_payload(upload, n);
> +    if ( rc )
> +        return rc;
> +
> +    spin_lock(&payload_lock);
> +
> +    found = find_payload(n);
> +    if ( IS_ERR(found) )
> +    {
> +        rc = PTR_ERR(found);
> +        goto out;
> +    }
> +    else if ( found )
> +    {
> +        rc = -EEXIST;
> +        goto out;
> +    }
> +
> +    data = xzalloc(struct payload);

I generally advocate for not doing allocations with locks held, and I don't 
think
it would severely complicate the code here doing so.

Jan


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