All of lore.kernel.org
 help / color / mirror / Atom feed
From: Fam Zheng <famz@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	qemu-block@nongnu.org, jcody@redhat.com, qemu-devel@nongnu.org,
	mreitz@redhat.com, stefanha@redhat.com
Subject: Re: [Qemu-devel] [PATCH 00/17] block: Convert common I/O path to BdrvChild
Date: Wed, 22 Jun 2016 16:37:47 +0800	[thread overview]
Message-ID: <20160622083747.GA17040@ad.usersys.redhat.com> (raw)
In-Reply-To: <20160621113146.GE4520@noname.redhat.com>

On Tue, 06/21 13:31, Kevin Wolf wrote:
> Am 21.06.2016 um 13:01 hat Paolo Bonzini geschrieben:
> > On 21/06/2016 12:56, Kevin Wolf wrote:
> > > Am 21.06.2016 um 11:47 hat Paolo Bonzini geschrieben:
> > >> I still fail to understand what is the rationale for this change.  The
> > >> API is weird; you read from a disk, not from an edge, and in fact the
> > >> first thing all the APIs do is dereference the BdrvChild...
> > >>
> > >> The assertions are nice, but I'm not sure it's a good idea to design a
> > >> whole API around them.
> > > 
> > > Do you see a problem with such an API, though? If there is no reason not
> > > to have the advantages, as small as they may seem, why not take them?
> > 
> > I don't see a reason not to take them; I don't see any red flags, but
> > there are some yellow flags (the kinda weird API) that I don't
> > understand and I hope you can explain.
> > 
> > Thinking more about it, it's perfectly possible that this is just a
> > combination of block/io.c's growth by accretion and the well-known fact
> > "naming pseudo-OOP member functions in C sucks".

I think this interface makes a lot of sense to me - I have been looking forward
to this model since three or four years ago, just without knowing ahead that
the object will be called BdrvChild, and that they'll occupy the first
parameter of bdrv_* API. I'm glad we are here today.

> > 
> > In other words, if you sell me this as "let's add some member functions
> > to BdrvChild and use them", I can buy it.  Perhaps the only thing to do
> > then is to rename functions and design a consistent naming.

And so I agree on renaming functions. :)

Fam

> 
> Hm, I never thought about it this way, but I think it actually makes
> sense.
> 
> As we want to represent a graph where both nodes and edges can have
> attributes and methods, OOP-wise both of them are objects, namely BDS
> and BdrvChild.
> 
> So we have some BDS A that has a Child B, and Child B in turn has a
> BDS C. What we used to do is that A asks B for the node it points to
> (C), and then directly calls a method of C. After the conversion, A
> calls a method of B, which in turn forwards the request by calling a
> method of C, which is much more straightforward and ideally even allows
> the node that B points to to remain private (we're not quite there,
> though).

  reply	other threads:[~2016-06-22  8:38 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-06-21  9:21 [Qemu-devel] [PATCH 00/17] block: Convert common I/O path to BdrvChild Kevin Wolf
2016-06-21  9:21 ` [Qemu-devel] [PATCH 01/17] vvfat: Use BdrvChild for s->qcow Kevin Wolf
2016-06-22 16:54   ` Max Reitz
2016-06-21  9:21 ` [Qemu-devel] [PATCH 02/17] blkreplay: Convert to byte-based I/O Kevin Wolf
2016-06-22 17:03   ` Max Reitz
2016-06-22 17:15   ` Eric Blake
2016-06-21  9:21 ` [Qemu-devel] [PATCH 03/17] vhdx: Some more BlockBackend use in vhdx_create() Kevin Wolf
2016-06-22 17:08   ` Max Reitz
2016-06-21  9:21 ` [Qemu-devel] [PATCH 04/17] block: Convert bdrv_co_readv() to BdrvChild Kevin Wolf
2016-06-25 15:07   ` Max Reitz
2016-06-21  9:21 ` [Qemu-devel] [PATCH 05/17] block: Convert bdrv_co_writev() " Kevin Wolf
2016-06-25 15:14   ` Max Reitz
2016-06-27  8:56     ` Kevin Wolf
2016-06-21  9:21 ` [Qemu-devel] [PATCH 06/17] block: Convert bdrv_aio_readv() " Kevin Wolf
2016-06-25 15:16   ` Max Reitz
2016-06-21  9:21 ` [Qemu-devel] [PATCH 07/17] block: Convert bdrv_aio_writev() " Kevin Wolf
2016-06-25 15:20   ` Max Reitz
2016-06-21  9:21 ` [Qemu-devel] [PATCH 08/17] block: Convert bdrv_co_do_readv/writev " Kevin Wolf
2016-06-25 15:26   ` Max Reitz
2016-06-21  9:21 ` [Qemu-devel] [PATCH 09/17] block: Move bdrv_commit() to block/commit.c Kevin Wolf
2016-06-24 15:37   ` Eric Blake
2016-06-21  9:21 ` [Qemu-devel] [PATCH 10/17] block: Use BlockBackend for I/O in bdrv_commit() Kevin Wolf
2016-06-25 15:34   ` Max Reitz
2016-06-21  9:21 ` [Qemu-devel] [PATCH 11/17] block: Convert bdrv_read() to BdrvChild Kevin Wolf
2016-06-27 13:24   ` Max Reitz
2016-06-21  9:21 ` [Qemu-devel] [PATCH 12/17] block: Convert bdrv_write() " Kevin Wolf
2016-06-27 13:44   ` Max Reitz
2016-06-27 13:47     ` Max Reitz
2016-06-29 12:11       ` [Qemu-devel] [PATCH v2 " Kevin Wolf
2016-06-29 15:22         ` Max Reitz
2016-06-29 15:33           ` Kevin Wolf
2016-06-29 15:37             ` Max Reitz
2016-06-21  9:21 ` [Qemu-devel] [PATCH 13/17] block: Convert bdrv_pread(v) " Kevin Wolf
2016-06-27 14:55   ` Max Reitz
2016-06-21  9:21 ` [Qemu-devel] [PATCH 14/17] block: Convert bdrv_pwrite(v/_sync) " Kevin Wolf
2016-06-27 15:07   ` Max Reitz
2016-06-21  9:21 ` [Qemu-devel] [PATCH 15/17] block: Convert bdrv_pwrite_zeroes() " Kevin Wolf
2016-06-27 15:12   ` Max Reitz
2016-06-21  9:21 ` [Qemu-devel] [PATCH 16/17] block: Convert bdrv_prwv_co() " Kevin Wolf
2016-06-27 15:19   ` Max Reitz
2016-06-21  9:21 ` [Qemu-devel] [PATCH 17/17] block: Convert bdrv_co_preadv/pwritev " Kevin Wolf
2016-06-21  9:47 ` [Qemu-devel] [PATCH 00/17] block: Convert common I/O path " Paolo Bonzini
2016-06-21 10:56   ` Kevin Wolf
2016-06-21 11:01     ` Paolo Bonzini
2016-06-21 11:31       ` Kevin Wolf
2016-06-22  8:37         ` Fam Zheng [this message]
2016-06-28 13:28 ` Stefan Hajnoczi
2016-06-29 11:58   ` Kevin Wolf

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=20160622083747.GA17040@ad.usersys.redhat.com \
    --to=famz@redhat.com \
    --cc=jcody@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=mreitz@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=stefanha@redhat.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.