[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 02/23] block: New BlockBackend
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 02/23] block: New BlockBackend |
Date: |
Thu, 11 Sep 2014 12:11:17 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
Benoît Canet <address@hidden> 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.
>>
>> 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 <address@hidden>
>> ---
>> 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
>>
>> 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 += qed-check.o
>> block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o
>> block-obj-$(CONFIG_QUORUM) += quorum.o
>> block-obj-y += parallels.o blkdebug.o blkverify.o
>> -block-obj-y += snapshot.o qapi.o
>> +block-obj-y += block-backend.o snapshot.o qapi.o
>> block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o
>> block-obj-$(CONFIG_POSIX) += raw-posix.o
>> block-obj-$(CONFIG_LINUX_AIO) += 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 <address@hidden>,
>> + *
>> + * 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 =
>> + 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 = 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 = g_strdup(name);
>> + blk->refcnt = 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.
[...]
[Qemu-devel] [PATCH 05/23] block: Make BlockBackend own its BlockDriverState, Markus Armbruster, 2014/09/10
[Qemu-devel] [PATCH 10/23] block: Eliminate DriveInfo member bdrv, use blk_by_legacy_dinfo(), Markus Armbruster, 2014/09/10