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

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Konrad Rzeszutek Wilk <konrad@xxxxxxxxxx> 04/15/16 4:29 AM >>>
>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;

That's fine too.

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

What I mean with "negating" was

copyback = (ret != -ENOSYS && ret != -EOPNOTSUPP);

>> > +    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 would create a race again afaict. Instead what I have been trying to hint
at is that the allocation should be done before taking the lock, freeing the 
object
again if in the end it turned out it's not going to be needed. Hence the 
referral to
possibly complicating the code.

Jan


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