Il 12/02/2013 21:49, Michael S. Tsirkin ha scritto: > On Tue, Feb 12, 2013 at 09:08:27PM +0100, Paolo Bonzini wrote: >> 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. > > That's virtqueue_add_buf_single, that's a separate matter. > Another option for _single is just two wrappers: > virtqueue_add_buf_in > virtqueue_add_buf_out
I like it less, but yes this one would be ok (no driver uses a variable for the enum parameter). >> 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). > > Yes it does, if add_sg does not have in/out at all there's no way to > request the impossible to/from mix. In your example above it does have it. I assume you meant ret = virtqueue_start_buf(vq, data, out, in, gfp); if (ret < 0) return ret; virtqueue_add_sg(vq, sg, out + in); virtqueue_end_buf(vq); >>>> 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. > > I'm pretty sure we can do this without duplication, if we want to. Indeed, if you remove the out/in arguments from _sg there is no duplication in virtio-blk. That's because it places data-out at the end and data-in at the beginning (so data is always after the request header and before the response footer). >> 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. > > Yea, maybe. I'm not the API guru here anyway, it's Rusty's street. Let's wait for him. Paolo _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization