Re: [Qemu-devel] [PATCH 02/23] block: New BlockBackend

["Benoît Canet" <benoit.canet@irqsave.net>]

The Wednesday 10 Sep 2014 à 13:44:17 (+0200), Kevin Wolf wrote :
> Am 10.09.2014 um 13:34 hat Benoît Canet geschrieben:
> > The Wednesday 10 Sep 2014 à 10:13:31 (+0200), Markus Armbruster wrote :
> > > A block device consists of a frontend device model and a backend.
> > > 
> > > A block backend has a tree of block drivers doing the actual work.
> > > The tree is managed by the block layer.
> > > 
> > > We currently use a single abstraction BlockDriverState both for tree
> > > nodes and the backend as a whole.  Drawbacks:
> > > 
> > > * Its API includes both stuff that makes sense only at the block
> > >   backend level (root of the tree) and stuff that's only for use
> > >   within the block layer.  This makes the API bigger and more complex
> > >   than necessary.  Moreover, it's not obvious which interfaces are
> > >   meant for device models, and which really aren't.
> > > 
> > > * Since device models keep a reference to their backend, the backend
> > >   object can't just be destroyed.  But for media change, we need to
> > >   replace the tree.  Our solution is to make the BlockDriverState
> > >   generic, with actual driver state in a separate object, pointed to
> > >   by member opaque.  That lets us replace the tree by deinitializing
> > >   and reinitializing its root.  This special need of the root makes
> > >   the data structure awkward everywhere in the tree.
> > > 
> > > The general plan is to separate the APIs into "block backend", for use
> > > by device models, monitor and whatever other code dealing with block
> > > backends, and "block driver", for use by the block layer and whatever
> > > other code (if any) dealing with trees and tree nodes.
> > > 
> > > Code dealing with block backends, device models in particular, should
> > > become completely oblivious of BlockDriverState.  This should let us
> > > clean up both APIs, and the tree data structures.
> > > 
> > > This commit is a first step.  It creates a minimal "block backend"
> > > API: type BlockBackend and functions to create, destroy and find them.
> > > BlockBackend objects are created and destroyed, but not yet used for
> > > anything; that'll come shortly.
> > > 
> > > BlockBackend is reference-counted.  Its reference count never exceeds
> > > one so far, but that's going to change.
> > > 
> > > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> 
> > > +/**
> > > + * blk_ref:
> > > + *
> > > + * Increment @blk's reference count.
> > > + */
> > > +void blk_ref(BlockBackend *blk)
> > > +{
> > 
> > if blk_unref you take care of doing
> > +    if (blk) {
> > to make sur the user does not pass a NULL pointer.
> > Transforming blk into a NULL pointer is not a side effect
> > of blk_unref so this test is designed to prevent a user
> > brain damage.
> 
> Not really, I'd rather consider it a convenience feature, just like
> you're allowed to call free(NULL) or bdrv_unref(NULL) without having a
> check for != NULL everywhere. This will be handy especially in error
> paths.

ok I see the spirit of it.

Benoit
> 
> > If the user can be brain damaged to pass a NULL to blk_unref he
> > could be equally stupid passing a NULL to blk_ref.
> > Why not adding the same test here ?
> 
> Whereas in blk_ref() it really wouldn't make any sense.
> 
> Kevin
>