Re: [Qemu-devel] [PATCHv2 6/7] cleanup qemu_co_sendv(), qemu_co_recvv() and friends

[Michael Tokarev]

On 12.03.2012 17:30, Paolo Bonzini wrote:
> Il 11/03/2012 16:26, Michael Tokarev ha scritto:
>> Note that - I still hope - in the end there will be no sendv or
>> recv calls at all, only common sendv_recvv with is_write passed
>> as an argument from upper layer.  It will be easier to remove
>> that #define - just two lines of code instead of minimum 5 :)
> 
> This is what I don't really like in the second part of these patches.
> You are doing changes for the sake of other changes which are not
> upstream yet, for which there is no clear vision, and for which there is
> no clear benefit.

I already posted the example of this.  I can complete whole thing
and send it all in one huge chunk if you prefer that :)

> While I agree that there is a lot of duplicated code in block.c and
> block/*, I don't think that what we need is more parameters to the
> functions.  We have places where we need to know the request flags, for
> example, but the methods are already quite unwieldy and have a lot of
> arguments.  So I'm not sure that this kind of unification buys anything.

Which places are these?

Also, how about dropping nr_sectors?

If you need flags, well, the extra argument being
added can really be used for that if necessary.

> On top of this, your patches unify things that are similar but not quite
> identical, and you do not provide hints in the commit messages that you
> did so.  For example, qemu_co_recvv has handling for receiving 0, but
> qemu_co_sendv does not.

This is a bug, which I dind't notice, it shouldn't
be there.  Somehow I overlooked this difference, but
I really wondered how they're differ!  By using common
code here it becomes much more obvious (whith the bug
actually fixed).

I'll take a look at other similar things here and
in my block layer changes.

> Can you please separate the changes to make the argument order uniform?
>  Those should be easy to get in.

While at it I found another "library", iov.[ch], which
has another implementation of the same thing.  So that's
where it all should have been started.

I'll resend a v3 covering iov_* functions too.

Thank you!

/mjt