Re: [PATCH 1/9] virtio: add functions for piecewise addition of buffers

[Paolo Bonzini <pbonzini@redhat.com>]

Il 12/02/2013 19:23, Michael S. Tsirkin ha scritto:
> On Tue, Feb 12, 2013 at 07:04:27PM +0100, Paolo Bonzini wrote:
>>>> Perhaps, but 3 or 4 arguments (in/out/nsg or in/out/nsg_in/nsg_out) just
>>>> for this are definitely too many and make the API harder to use.
>>>>
>>>> You have to find a balance.  Having actually used the API, the
>>>> possibility of mixing in/out buffers by mistake never even occurred to
>>>> me, much less happened in practice, so I didn't consider it a problem.
>>>> Mixing in/out buffers in a single call wasn't a necessity, either.
>>>
>>> It is useful for virtqueue_add_buf implementation.
>>
>>         ret = virtqueue_start_buf(vq, data, out + in, !!out + !!in,
>> 				  gfp);
>>         if (ret < 0)
>>                 return ret;
>>
>>         if (out)
>>                 virtqueue_add_sg(vq, sg, out, DMA_TO_DEVICE);
>>         if (in)
>>                 virtqueue_add_sg(vq, sg + out, in, DMA_FROM_DEVICE);
>>
>>         virtqueue_end_buf(vq);
>> 	return 0;
>>
>> How can it be simpler and easier to understand than that?
> 
> Like this:
> 
>          ret = virtqueue_start_buf(vq, data, in, out, gfp);
>          if (ret < 0)
>                  return ret;
>  
>          virtqueue_add_sg(vq, sg, in, out);
>  
>          virtqueue_end_buf(vq);

It's out/in, not in/out... I know you wrote it in a hurry, but it kind
of shows that the new API is easier to use.  Check out patch 8, it's  a
real improvement in readability.

Plus you haven't solved the problem of alternating to/from-device
elements (which is also harder to spot with in/out than with the enum).

And no one else would use add_sg with in != 0 && out != 0, so you'd be
favoring one caller over all the others.  If you did this instead:

         virtqueue_add_sg(vq, sg, in + out);

it would really look like a hack IMHO.

>>> Basically the more consistent the interface is with virtqueue_add_buf,
>>> the better.
>>
>> The interface is consistent with virtqueue_add_buf_single, where out/in
>> clearly doesn't make sense.
> 
> Hmm, we could make virtqueue_add_buf_single consistent by giving it 'bool in'.

But is it "bool in" or "bool out"?

>> virtqueue_add_buf and virtqueue_add_sg are very different, despite the
>> similar name.
> 
> True. The similarity is between _start and _add_buf.
> And this is confusing too. Maybe this means
> _start and _add_sg should be renamed.

Maybe.  If you have any suggestions it's fine.

BTW I tried using out/in for start_buf, and the code in virtio-blk gets
messier, it has to do all the math twice.  Perhaps we just need to
acknowledge that the API is different and thus the optimal choice of
arguments is different.  C doesn't have keyword arguments, there not
much that we can do.

Paolo