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

[Konrad Rzeszutek Wilk <konrad@xxxxxxxxxx>]

On Thu, Apr 14, 2016 at 10:36:46AM -0600, Jan Beulich wrote:
> >>> 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?

B/c I thought you would want it this way.

I changed it to

466         if ( ret != -ENOSYS && ret != -EOPNOTSUPP )
467             copyback = 1;

But I don't think this what you meant by 'negating'? As in:

 copyback = !rc ?

But one of the subops returns the number of items and we definitly
want copyback=1 for that.
> 
> > +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?

Yes. In the earlier versions I only verified the name->name and
then later on made the copy. You pointed out that in effect I had
invalidated the earlier checks. Also you asked to make the 
name have a NUL padding - so I put that together - the verification
here checks the name for this. And since to verify I need to copy it in first..

I will change the name of the function to 'get_name'
> 
> > +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.

I can certainly unlock and then lock again (when adding
it to the list).

That has the positive aspect that in the later patches when
we do vzalloc for the payload we can do it without holding locks.
> 
> Jan
> 

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