From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38201) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yrqgl-0004Lo-1E for qemu-devel@nongnu.org; Mon, 11 May 2015 12:36:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yrqgh-0001hi-W3 for qemu-devel@nongnu.org; Mon, 11 May 2015 12:36:18 -0400 Message-ID: <5550DA6E.9090701@redhat.com> Date: Mon, 11 May 2015 18:35:58 +0200 From: Max Reitz MIME-Version: 1.0 References: <1431105726-3682-1-git-send-email-kwolf@redhat.com> <1431105726-3682-13-git-send-email-kwolf@redhat.com> In-Reply-To: <1431105726-3682-13-git-send-email-kwolf@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 12/34] block: Allow specifying driver-specific options to reopen List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , qemu-block@nongnu.org Cc: armbru@redhat.com, qemu-devel@nongnu.org On 08.05.2015 19:21, Kevin Wolf wrote: > Signed-off-by: Kevin Wolf > --- > block.c | 42 +++++++++++++++++++++++++++++++++++++++--- > block/commit.c | 4 ++-- > include/block/block.h | 4 +++- > 3 files changed, 44 insertions(+), 6 deletions(-) > > diff --git a/block.c b/block.c > index 95dc51e..561cefd 100644 > --- a/block.c > +++ b/block.c > @@ -1584,6 +1584,9 @@ typedef struct BlockReopenQueueEntry { > * > * bs is the BlockDriverState to add to the reopen queue. > * > + * options contains the changed options for the associated bs > + * (the BlockReopenQueue takes the ownership) > + * > * flags contains the open flags for the associated bs > * > * returns a pointer to bs_queue, which is either the newly allocated > @@ -1591,18 +1594,28 @@ typedef struct BlockReopenQueueEntry { > * > */ > BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue, > - BlockDriverState *bs, int flags) > + BlockDriverState *bs, > + QDict *options, int flags) > { > assert(bs != NULL); > > BlockReopenQueueEntry *bs_entry; > BdrvChild *child; > + QDict *old_options; > > if (bs_queue == NULL) { > bs_queue = g_new0(BlockReopenQueue, 1); > QSIMPLEQ_INIT(bs_queue); > } > > + if (!options) { > + options = qdict_new(); > + } > + > + old_options = qdict_clone_shallow(bs->options); > + qdict_join(options, old_options, false); > + QDECREF(old_options); > + > /* bdrv_open() masks this flag out */ > flags &= ~BDRV_O_PROTOCOL; > > @@ -1614,13 +1627,15 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue, > } > > child_flags = child->role->inherit_flags(flags); > - bdrv_reopen_queue(bs_queue, child->bs, child_flags); > + /* TODO Pass down child flags (backing.*, extents.*, ...) */ > + bdrv_reopen_queue(bs_queue, child->bs, NULL, child_flags); > } > > bs_entry = g_new0(BlockReopenQueueEntry, 1); > QSIMPLEQ_INSERT_TAIL(bs_queue, bs_entry, entry); > > bs_entry->state.bs = bs; > + bs_entry->state.options = options; > bs_entry->state.flags = flags; > > return bs_queue; > @@ -1673,6 +1688,7 @@ cleanup: > if (ret && bs_entry->prepared) { > bdrv_reopen_abort(&bs_entry->state); > } > + QDECREF(bs_entry->state.options); > g_free(bs_entry); > } > g_free(bs_queue); > @@ -1685,7 +1701,7 @@ int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp) > { > int ret = -1; > Error *local_err = NULL; > - BlockReopenQueue *queue = bdrv_reopen_queue(NULL, bs, bdrv_flags); > + BlockReopenQueue *queue = bdrv_reopen_queue(NULL, bs, NULL, bdrv_flags); > > ret = bdrv_reopen_multiple(queue, &local_err); > if (local_err != NULL) { > @@ -1761,6 +1777,26 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, > goto error; > } > > + /* Options that are not handled are only okay if they are unchanged > + * compared to the old state. It is expected that some options are only > + * used for the initial open, but not reopen (e.g. filename) */ > + if (qdict_size(reopen_state->options)) { > + const QDictEntry *entry = qdict_first(reopen_state->options); > + > + do { > + QString *new_obj = qobject_to_qstring(entry->value); Maybe add an assert(new_obj)? If it's NULL, the next qstring_get_str() will dereference NULL which is basically just as good, but well... > + const char *new = qstring_get_str(new_obj); > + const char *old = qdict_get_try_str(reopen_state->bs->options, > + entry->key); > + > + if (!new != !old || strcmp(new, old)) { This form implies to me that you expect !new might be true. However, assuming that, we might get !new && !old, thus !new == !old and thus strcmp(NULL, NULL), which would be bad. I think !new will never be true. Therefore, I'd rather put this as "if (!old || strcmp(new, old))". > + error_setg(errp, "Cannot change the option '%s'", entry->key); > + ret = -EINVAL; Hm, well, so far this function generally returned -1 on error, but okay... I don't see anything truly wrong, though, so with or without assert() and the different condition: Reviewed-by: Max Reitz > + goto error; > + } > + } while ((entry = qdict_next(reopen_state->options, entry))); > + } > + > ret = 0; > > error: > diff --git a/block/commit.c b/block/commit.c > index cfa2bbe..2c07d12 100644 > --- a/block/commit.c > +++ b/block/commit.c > @@ -235,11 +235,11 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base, > > /* convert base & overlay_bs to r/w, if necessary */ > if (!(orig_base_flags & BDRV_O_RDWR)) { > - reopen_queue = bdrv_reopen_queue(reopen_queue, base, > + reopen_queue = bdrv_reopen_queue(reopen_queue, base, NULL, > orig_base_flags | BDRV_O_RDWR); > } > if (!(orig_overlay_flags & BDRV_O_RDWR)) { > - reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs, > + reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs, NULL, > orig_overlay_flags | BDRV_O_RDWR); > } > if (reopen_queue) { > diff --git a/include/block/block.h b/include/block/block.h > index 341a551..1287013 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -146,6 +146,7 @@ typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue; > typedef struct BDRVReopenState { > BlockDriverState *bs; > int flags; > + QDict *options; > void *opaque; > } BDRVReopenState; > > @@ -215,7 +216,8 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, > const char *reference, QDict *options, int flags, > BlockDriver *drv, Error **errp); > BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue, > - BlockDriverState *bs, int flags); > + BlockDriverState *bs, > + QDict *options, int flags); > int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp); > int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp); > int bdrv_reopen_prepare(BDRVReopenState *reopen_state,