qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Paolo Bonzini <pbonzini@redhat.com>
To: Michael Tokarev <mjt@tls.msk.ru>
Cc: qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCHv2 6/7] cleanup qemu_co_sendv(), qemu_co_recvv() and friends
Date: Mon, 12 Mar 2012 14:30:14 +0100	[thread overview]
Message-ID: <4F5DFA66.80900@redhat.com> (raw)
In-Reply-To: <4F5CC41E.1080106@msgid.tls.msk.ru>

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.

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.

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.

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

Paolo

  reply	other threads:[~2012-03-12 13:30 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-03-11  1:49 [Qemu-devel] [PATCHv2 0/7] cleanup/consolidate some iovec functions Michael Tokarev
2012-03-11  1:49 ` [Qemu-devel] [PATCHv2 1/7] Consolidate qemu_iovec_memset{, _skip}() into single, simplified function Michael Tokarev
2012-03-12 13:55   ` Kevin Wolf
2012-03-11  1:49 ` [Qemu-devel] [PATCHv2 2/7] allow qemu_iovec_from_buffer() to specify offset from which to start copying Michael Tokarev
2012-03-11  1:49 ` [Qemu-devel] [PATCHv2 3/7] consolidate qemu_iovec_copy() and qemu_iovec_concat() and make them consistent Michael Tokarev
2012-03-11 14:59   ` Paolo Bonzini
2012-03-11  1:49 ` [Qemu-devel] [PATCHv2 4/7] change prototypes of qemu_sendv() and qemu_recvv() Michael Tokarev
2012-03-11  1:49 ` [Qemu-devel] [PATCHv2 5/7] Export qemu_sendv_recvv() and use it in " Michael Tokarev
2012-03-11 15:00   ` Paolo Bonzini
2012-03-11 15:22     ` Michael Tokarev
2012-03-11  1:49 ` [Qemu-devel] [PATCHv2 6/7] cleanup qemu_co_sendv(), qemu_co_recvv() and friends Michael Tokarev
2012-03-11 15:01   ` Paolo Bonzini
2012-03-11 15:26     ` Michael Tokarev
2012-03-12 13:30       ` Paolo Bonzini [this message]
2012-03-12 16:29         ` Michael Tokarev
2012-03-12 16:50           ` Paolo Bonzini
2012-03-11  1:49 ` [Qemu-devel] [PATCHv2 7/7] rewrite and comment qemu_sendv_recvv() Michael Tokarev
2012-03-11  2:11 ` [Qemu-devel] [PATCHv2 0/7] cleanup/consolidate some iovec functions Michael Tokarev
2012-03-11 15:06 ` Paolo Bonzini

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4F5DFA66.80900@redhat.com \
    --to=pbonzini@redhat.com \
    --cc=mjt@tls.msk.ru \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).