From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53282) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XS1Lc-0001vk-BR for qemu-devel@nongnu.org; Thu, 11 Sep 2014 06:11:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XS1LX-0007Gq-5q for qemu-devel@nongnu.org; Thu, 11 Sep 2014 06:11:28 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50310) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XS1LW-0007Gm-VW for qemu-devel@nongnu.org; Thu, 11 Sep 2014 06:11:23 -0400 From: Markus Armbruster References: <1410336832-22160-1-git-send-email-armbru@redhat.com> <1410336832-22160-3-git-send-email-armbru@redhat.com> <20140910113417.GA22376@irqsave.net> Date: Thu, 11 Sep 2014 12:11:17 +0200 In-Reply-To: <20140910113417.GA22376@irqsave.net> (=?utf-8?Q?=22Beno=C3=AE?= =?utf-8?Q?t?= Canet"'s message of "Wed, 10 Sep 2014 13:34:17 +0200") Message-ID: <87vbousbzu.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 02/23] block: New BlockBackend List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?utf-8?Q?Beno=C3=AEt?= Canet Cc: kwolf@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com Beno=C3=AEt Canet writes: > The Wednesday 10 Sep 2014 10:13:31 (+0200), Markus Armbruster wrote : >> A block device consists of a frontend device model and a backend. >>=20 >> A block backend has a tree of block drivers doing the actual work. >> The tree is managed by the block layer. >>=20 >> We currently use a single abstraction BlockDriverState both for tree >> nodes and the backend as a whole. Drawbacks: >>=20 >> * 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. >>=20 >> * 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. >>=20 >> 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. >>=20 >> 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. >>=20 >> 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. >>=20 >> BlockBackend is reference-counted. Its reference count never exceeds >> one so far, but that's going to change. >>=20 >> Signed-off-by: Markus Armbruster >> --- >> block/Makefile.objs | 2 +- >> block/block-backend.c | 110 ++++++++++++++++++++++++++++++++++= +++++++ >> blockdev.c | 10 +++- >> hw/block/xen_disk.c | 11 +++++ >> include/qemu/typedefs.h | 1 + >> include/sysemu/block-backend.h | 26 ++++++++++ >> qemu-img.c | 46 +++++++++++++++++ >> qemu-io.c | 8 +++ >> qemu-nbd.c | 3 +- >> 9 files changed, 214 insertions(+), 3 deletions(-) >> create mode 100644 block/block-backend.c >> create mode 100644 include/sysemu/block-backend.h >>=20 >> diff --git a/block/Makefile.objs b/block/Makefile.objs >> index f45f939..a70140b 100644 >> --- a/block/Makefile.objs >> +++ b/block/Makefile.objs >> @@ -5,7 +5,7 @@ block-obj-y +=3D qed-check.o >> block-obj-$(CONFIG_VHDX) +=3D vhdx.o vhdx-endian.o vhdx-log.o >> block-obj-$(CONFIG_QUORUM) +=3D quorum.o >> block-obj-y +=3D parallels.o blkdebug.o blkverify.o >> -block-obj-y +=3D snapshot.o qapi.o >> +block-obj-y +=3D block-backend.o snapshot.o qapi.o >> block-obj-$(CONFIG_WIN32) +=3D raw-win32.o win32-aio.o >> block-obj-$(CONFIG_POSIX) +=3D raw-posix.o >> block-obj-$(CONFIG_LINUX_AIO) +=3D linux-aio.o >> diff --git a/block/block-backend.c b/block/block-backend.c >> new file mode 100644 >> index 0000000..833f7d9 >> --- /dev/null >> +++ b/block/block-backend.c >> @@ -0,0 +1,110 @@ >> +/* >> + * QEMU Block backends >> + * >> + * Copyright (C) 2014 Red Hat, Inc. >> + * >> + * Authors: >> + * Markus Armbruster , >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2 or >> + * later. See the COPYING file in the top-level directory. >> + */ >> + >> +#include "sysemu/block-backend.h" >> +#include "block/block_int.h" >> + >> +struct BlockBackend { >> + char *name; >> + int refcnt; >> + QTAILQ_ENTRY(BlockBackend) link; /* for blk_backends */ >> +}; >> + >> +static QTAILQ_HEAD(, BlockBackend) blk_backends =3D >> + QTAILQ_HEAD_INITIALIZER(blk_backends); >> + >> +/** >> + * blk_new: >> + * @name: name, must not be %NULL or empty >> + * @errp: return location for an error to be set on failure, or %NULL >> + * >> + * Create a new BlockBackend, with a reference count of one. Fail if >> + * @name already exists. >> + * >> + * Returns: the BlockBackend on success, %NULL on failure >> + */ >> +BlockBackend *blk_new(const char *name, Error **errp) > > I am responding for the easy part first. > > So here the blockbackend is identified by a name > >> +{ >> + BlockBackend *blk =3D g_new0(BlockBackend, 1); >> + >> + assert(name && name[0]); >> + if (blk_by_name(name)) { > >> + error_setg(errp, "Device with id '%s' already exists", name); > > But here is it an id or a name ? > Do we need to make a choice everywhere in the code between id and name ? If we can agree on a convention to use within the block layer, I'll be happy to follow it. Right now, we mix "id", "device", "device name" freely. My patch mimicks existing usage: "id" in QemuOpts and some error messages, "device name" and its abbreviated variations in the code, the schema, and some other error messages. >> + return NULL; >> + } >> + blk->name =3D g_strdup(name); >> + blk->refcnt =3D 1; >> + QTAILQ_INSERT_TAIL(&blk_backends, blk, link); >> + return blk; >> +} >> + >> +static void blk_delete(BlockBackend *blk) >> +{ >> + assert(!blk->refcnt); >> + QTAILQ_REMOVE(&blk_backends, blk, link); >> + g_free(blk->name); >> + g_free(blk); >> +} >> + >> +/** >> + * 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. > > 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 ? Kevin already explained this one. [...]