Re: [PATCH v9 04/27] xen/xsplice: Hypervisor implementation of XEN_XSPLICE_op

["Jan Beulich" <JBeulich@suse.com>]

>>> On 27.04.16 at 15:47, <konrad@kernel.org> wrote:
> On Wed, Apr 27, 2016 at 12:51:34AM -0600, Jan Beulich wrote:
>> >>> On 26.04.16 at 19:50, <konrad.wilk@oracle.com> wrote:
>> > On Tue, Apr 26, 2016 at 04:21:10AM -0600, Jan Beulich wrote:
>> >> I also wonder whether the code wouldn't be easier to read if you
>> >> used just a sequence of if()/else if() here, without any goto-s.
>> > 
>> > But I do need to free(data) and unlock the spinlock - so having
>> > a common code to pass through makes sense.
>> > 
>> > Unless you mean have an condition on if ( !rc ), and do the normal path?
>> > Like so:
>> > 
>> >     rc = verify_payload(upload, n);
>> >     if ( rc )
>> >         return rc;
>> > 
>> >     data = xzalloc(struct payload);
>> > 
>> >     spin_lock(&payload_lock);
>> > 
>> >     found = find_payload(n);
>> >     if ( IS_ERR(found) )
>> >         rc = PTR_ERR(found);
>> >     else if ( found )
>> >         rc = -EEXIST;
>> > 
>> >     if ( !rc && !data )
>> 
>> This can just be "else if ( !data )" afaict.
> 
> But then we "lose"

I don't understand what you're trying to tell me. But it looks like I also
don't need to understand it, since ...

> But it is neater than what it has now.
> The final product ends up being:
> 
>     rc = verify_payload(upload, n);
>     if ( rc )
>         return rc;
> 
>     data = xzalloc(struct payload);
>     raw_data = vmalloc(upload->size);
> 
>     spin_lock(&payload_lock);
> 
>     found = find_payload(n);
>     if ( IS_ERR(found) )
>         rc = PTR_ERR(found);
>     else if ( found )
>         rc = -EEXIST;
>     else if ( !data || !raw_data )
>         rc = -ENOMEM;
>     else if ( __copy_from_guest(raw_data, upload->payload, upload->size) )
>         rc = -EFAULT;
>     else

... this is what I was hoping for.

>> As I have tried to express by saying "I also wonder", and as this
>> clearly is a matter of taste to some degree, I'm not insisting on
>> that alternative code flow. What I'd really like to ask for is
>> consistency though: While in the patch here you use
>> 
>>     if ( ... )
>>     {
>>         rc = ...;
>>         goto ...;
>>     }
>> 
>> patch 11 introduces an instance of the alternative
>> 
>>     rc = -E...;
>>     if ( ... )
>>         goto ...;
>> 
>> Similarly (see above) you should aim at consistency between
>> if/else-if chains or chains of just if-s, when each of them ends in an
>> unconditional goto (or return, continue, or break, taking a more
>> general perspective). Not mixing styles helps avoid (possibly silent)
>> questions by readers along the lines of "Is there a reason it's done
>> one way here and another way a few lines down?"
> 
> Different authors, different matter of taste - as you saw with
> the sizeof and this one - Ross and me write code differently.
> 
> How do you and Andrew deal with this one?

Simply by making code additions fit existing (surrounding) style
(and that's not specific to being between Andrew and me).

Jan


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