Re: [Qemu-devel] [PATCH v2 01/37] blockdev: Allow creation of BDS trees without BB

[Max Reitz]

On 2015-03-04 at 08:39, Kevin Wolf wrote:
> Am 09.02.2015 um 18:11 hat Max Reitz geschrieben:
> > If the "id" field is missing from the options given to blockdev-add,
> > just omit the BlockBackend and create the BlockDriverState tree alone.
> >
> > However, if "id" is missing, "node-name" must be specified; otherwise,
> > the BDS tree would no longer be accessible.
> >
> > Signed-off-by: Max Reitz <address@hidden>
> > ---
> >   blockdev.c                 | 44 +++++++++++++++++++++++++++++++-------------
> >   qapi/block-core.json       | 13 +++++++++----
> >   tests/qemu-iotests/087     |  2 +-
> >   tests/qemu-iotests/087.out |  4 ++--
> >   4 files changed, 43 insertions(+), 20 deletions(-)
> >
> > diff --git a/blockdev.c b/blockdev.c
> > index 6eedcf5..6d67c80 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -2822,17 +2822,12 @@ out:
> >   void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
> >   {
> >       QmpOutputVisitor *ov = qmp_output_visitor_new();
> > -    BlockBackend *blk;
> > +    BlockDriverState *bs;
> > +    BlockBackend *blk = NULL;
> >       QObject *obj;
> >       QDict *qdict;
> >       Error *local_err = NULL;
> >   
> > -    /* Require an ID in the top level */
> > -    if (!options->has_id) {
> > -        error_setg(errp, "Block device needs an ID");
> > -        goto fail;
> > -    }
> > -
> >       /* TODO Sort it out in raw-posix and drive_new(): Reject aio=native with
> >        * cache.direct=false instead of silently switching to aio=threads, except
> >        * when called from drive_new().
> > @@ -2860,14 +2855,37 @@ void qmp_blockdev_add(BlockdevOptions *options, Error 
> > **errp)
> >   
> >       qdict_flatten(qdict);
> >   
> > -    blk = blockdev_init(NULL, qdict, &local_err);
> > -    if (local_err) {
> > -        error_propagate(errp, local_err);
> > -        goto fail;
> > +    if (options->has_id) {
> > +        blk = blockdev_init(NULL, qdict, &local_err);
> > +        if (local_err) {
> > +            error_propagate(errp, local_err);
> > +            goto fail;
> > +        }
> > +
> > +        bs = blk_bs(blk);
> > +    } else {
> > +        int ret;
> > +
> > +        if (!qdict_get_try_str(qdict, "node-name")) {
> > +            error_setg(errp, "'id' and/or 'node-name' need to be specified for 
> > "
> > +                       "the root node");
> > +            goto fail;
> > +        }
> > +
> > +        bs = NULL;
> > +        ret = bdrv_open(&bs, NULL, NULL, qdict, BDRV_O_RDWR | BDRV_O_CACHE_WB,
> > +                        NULL, errp);
> Now all the qdict entries that aren't parsed by bdrv_open() but
> converted into flags by blockdev_init() are broken if you don't give an
> id. This includes read-only, discard, the cache options - in other
> words, enough to make this "support" completely useless.
>
> I guess I need to dig out my cache mode series and get it ready to be
> merged. Once this is in, the parsing of the other necessary options
> should be easy to move into bdrv_open().
>
> Unless you have a good reason why we should merge this patch in its
> hardly functional state now, I would consider those conversions a
> dependency of this one.

Because I will not send out another version of this series for the next 
two weeks, I'll just give an overview of where I'm currently at (for 
each of the patches you replied to):

We need this patch before "block: Make bdrv_is_inserted() recursive", 
because that patch makes the problems apparent which are fixed in my 
bdrv_close_all() series (071 fails, because flushing the qcow2 metadata 
suddenly no longer results in an error; this is because 
bdrv_is_inserted() now returns false because the file BDS under qcow2 
has been closed (because it has a BB), so it's no longer inserted). 
Therefore, we need patch 2 before said patch to work around the problem 
(until it's fixed by the respective series, which depends on this one, 
however).

I could probably split up patch 22 and pull the first part up front, 
along with patch 23, but this would require work, which I don't deem 
necessary. As I said before, in my opinion this patch does not break any 
existing valid usage of blockdev-add, it's just that there are some use 
cases we want to have after this patch that do not work yet. But they 
will be added in a later patch in this series, so I don't find this an 
issue.

Therefore, for this patch, I'll add a note into the commit message 
stating that there will be a follow-up patch allowing options that are 
not parsed by bdrv_open() (like caching) to be given.

Max