Re: [Qemu-block] [PATCH 03/16] blkverify: Convert s->test_file to BdrvChild

[Alberto Garcia]

On Thu 17 Sep 2015 03:48:07 PM CEST, Kevin Wolf wrote:

> @@ -151,7 +151,7 @@ static void blkverify_close(BlockDriverState *bs)
>  {
>  BDRVBlkverifyState *s = bs->opaque;
>  
> -bdrv_unref(s->test_file);
> +bdrv_unref_child(bs, s->test_file);
>  s->test_file = NULL;
>  }

You are using bdrv_unref_child() here whereas in quorum you kept
bdrv_unref().

In principle both seem correct because if you don't detach the children
in the driver's close function then bdrv_close() will take care of doing
it, but is there a reason why you are using different methods?

Berto

Re: [Qemu-block] [PATCH 02/16] vmdk: Use BdrvChild instead of BDS for references to extents

[Alberto Garcia]

On Thu 17 Sep 2015 03:48:06 PM CEST, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf 
> ---
>  block/vmdk.c | 99 
> +++-
>  1 file changed, 51 insertions(+), 48 deletions(-)

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [PATCH 04/16] quorum: Convert to BdrvChild

[Alberto Garcia]

On Thu 17 Sep 2015 03:48:08 PM CEST, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf 
> ---
>  block/quorum.c | 63 
> ++

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [PATCH 05/16] block: Convert bs->file to BdrvChild

[Kevin Wolf]

Am 22.09.2015 um 20:36 hat Max Reitz geschrieben:
> On 17.09.2015 15:48, Kevin Wolf wrote:
> > This patch removes the temporary duplication between bs->file and
> > bs->file_child by converting everything to BdrvChild.
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >  block.c   | 61 
> > ++
> >  block/blkdebug.c  | 32 
> >  block/blkverify.c | 33 ++---
> >  block/bochs.c |  8 +++---
> >  block/cloop.c | 10 
> >  block/dmg.c   | 20 +++
> >  block/io.c| 50 +++---
> >  block/parallels.c | 38 +++--
> >  block/qapi.c  |  2 +-
> >  block/qcow.c  | 42 +---
> >  block/qcow2-cache.c   | 11 +
> >  block/qcow2-cluster.c | 38 +
> >  block/qcow2-refcount.c| 45 ++
> >  block/qcow2-snapshot.c| 30 ---
> >  block/qcow2.c | 62 
> > ---
> >  block/qed-table.c |  4 +--
> >  block/qed.c   | 32 
> >  block/raw_bsd.c   | 40 +++---
> >  block/snapshot.c  | 12 -
> >  block/vdi.c   | 17 +++--
> >  block/vhdx-log.c  | 25 ++-
> >  block/vhdx.c  | 36 ++-
> >  block/vmdk.c  | 27 +++--
> >  block/vpc.c   | 34 ++
> >  include/block/block.h |  8 +-
> >  include/block/block_int.h |  3 +--
> >  26 files changed, 377 insertions(+), 343 deletions(-)
> 
> Reviewed-by: Max Reitz 
> 
> > diff --git a/block.c b/block.c
> > index 2e9e5e2..93d713b 100644
> > --- a/block.c
> > +++ b/block.c
> 
> [...]
> 
> > @@ -1929,6 +1925,11 @@ void bdrv_close(BlockDriverState *bs)
> >  bdrv_unref(backing_hd);
> >  }
> >  
> > +if (bs->file != NULL) {
> > +bdrv_unref(bs->file->bs);
> 
> I think we can use bdrv_unref_child(bs->file) here. I'd personally like
> it more, at least (because using bdrv_unref() and relying on the loop
> below is basically what the comment inside of the loop advises against).
> 
> Yes, I know, eventually, we want to drop this block anyway and let the
> loop below handle everything using bdrv_unref_child(). But when we do
> that conversion, it'll be obvious to drop a bdrv_unref_child() call,
> whereas dropping bdrv_unref() alone isn't so obvious.

Seems safe to do, and reads a bit nicer, so I'll change the patch
accordingly. Thanks.

Kevin

> > +bs->file = NULL;
> > +}
> > +
> >  QLIST_FOREACH_SAFE(child, >children, next, next) {
> >  /* TODO Remove bdrv_unref() from drivers' close function and 
> > use
> >   * bdrv_unref_child() here */
> > @@ -1953,11 +1954,6 @@ void bdrv_close(BlockDriverState *bs)
> >  bs->options = NULL;
> >  QDECREF(bs->full_open_options);
> >  bs->full_open_options = NULL;
> > -
> > -if (bs->file != NULL) {
> > -bdrv_unref(bs->file);
> > -bs->file = NULL;
> > -}
> >  }
> >  
> >  if (bs->blk) {
> 




pgprNUNzQR4Gl.pgp
Description: PGP signature

Re: [Qemu-block] [PATCH 03/16] blkverify: Convert s->test_file to BdrvChild

[Alberto Garcia]

On Wed 23 Sep 2015 03:58:23 PM CEST, Kevin Wolf wrote:

>> > @@ -151,7 +151,7 @@ static void blkverify_close(BlockDriverState *bs)
>> >  {
>> >  BDRVBlkverifyState *s = bs->opaque;
>> >  
>> > -bdrv_unref(s->test_file);
>> > +bdrv_unref_child(bs, s->test_file);
>> >  s->test_file = NULL;
>> >  }
>> 
>> You are using bdrv_unref_child() here whereas in quorum you kept
>> bdrv_unref().
>> 
>> In principle both seem correct because if you don't detach the
>> children in the driver's close function then bdrv_close() will take
>> care of doing it, but is there a reason why you are using different
>> methods?
>
> Because consistency is overrated? Or simply carelessness?

Ah ok :-)

> VMDK uses bdrv_unref_child() as well, so I guess quorum is the one
> that should be changed?

You can keep my R-b if you do so.

Berto

Re: [Qemu-block] [PATCH 03/16] blkverify: Convert s->test_file to BdrvChild

[Alberto Garcia]

On Thu 17 Sep 2015 03:48:07 PM CEST, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf 
> ---
>  block/blkverify.c | 41 +
>  1 file changed, 21 insertions(+), 20 deletions(-)
>

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [PATCH 01/16] block: Introduce BDS.file_child

[Alberto Garcia]

On Thu 17 Sep 2015 03:48:05 PM CEST, Kevin Wolf wrote:
> Store the BdrvChild for bs->file. At this point, bs->file_child->bs just
> duplicates the bs->file pointer. Later, it will completely replace it.
>
> Signed-off-by: Kevin Wolf 
> ---

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [PATCH 03/16] blkverify: Convert s->test_file to BdrvChild

[Kevin Wolf]

Am 23.09.2015 um 15:01 hat Alberto Garcia geschrieben:
> On Thu 17 Sep 2015 03:48:07 PM CEST, Kevin Wolf wrote:
> 
> > @@ -151,7 +151,7 @@ static void blkverify_close(BlockDriverState *bs)
> >  {
> >  BDRVBlkverifyState *s = bs->opaque;
> >  
> > -bdrv_unref(s->test_file);
> > +bdrv_unref_child(bs, s->test_file);
> >  s->test_file = NULL;
> >  }
> 
> You are using bdrv_unref_child() here whereas in quorum you kept
> bdrv_unref().
> 
> In principle both seem correct because if you don't detach the children
> in the driver's close function then bdrv_close() will take care of doing
> it, but is there a reason why you are using different methods?

Because consistency is overrated? Or simply carelessness?

In the end (not in this series), I'd like to remove all of this from the
close functions (as the comment in block.c says) and let it all be
handled in bdrv_close(). But until then, we should stay reasonably
consistent indeed.

VMDK uses bdrv_unref_child() as well, so I guess quorum is the one that
should be changed?

Kevin

Re: [Qemu-block] [PATCH 10/16] block/io: Make bdrv_requests_pending() public

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf 
> ---
>  block/io.c| 2 +-
>  include/block/block_int.h | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Max Reitz 

In addition: Shouldn't we iterate over all children in that function
instead of just file and backing?

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 14/16] blockjob: Store device name at job creation

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> Some block jobs change the block device graph on completion. This means
> that the device that owns the job and originally was addressed with its
> device name may no longer be what the corresponding BlockBackend points
> to.
> 
> Previously, the effects of bdrv_swap() ensured that the job was (at
> least partially) transferred to the target image. Events that contain
> the device name could still use bdrv_get_device_name(job->bs) and get
> the same result.
> 
> After removing bdrv_swap(), this won't work any more. Instead, save the
> device name at job creation and use that copy for the events.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  blockjob.c   | 8 +---
>  include/block/blockjob.h | 8 
>  2 files changed, 13 insertions(+), 3 deletions(-)

Why not %s/bdrv_get_device_name(job->bs)/job->id/?

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 16/16] block: Remove bdrv_swap()

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> bdrv_swap() is unused now. Remove it and all functions that have
> no other users than bdrv_swap(). In particular, this removes the
> .bdrv_rebind callbacks from block drivers.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c   | 155 
> +-
>  block/qed.c   |   7 ---
>  block/vvfat.c |   7 ---
>  include/block/block_int.h |   1 -
>  include/qemu/queue.h  |   6 --
>  5 files changed, 1 inse

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 11/16] block-backend: Add blk_set_bs()

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> It allows changing the BlockDriverState that a BlockBackend points to.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/block-backend.c | 16 
>  include/block/block_int.h |  2 ++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index c2e8732..a2afcff 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -239,6 +239,22 @@ BlockDriverState *blk_bs(BlockBackend *blk)
>  }
>  
>  /*
> + * Changes the BlockDriverState attached to @blk
> + */
> +void blk_set_bs(BlockBackend *blk, BlockDriverState *bs)
> +{
> +bdrv_ref(bs);
> +
> +if (blk->bs) {
> +blk->bs->blk = NULL;
> +bdrv_unref(blk->bs);
> +}
> +
> +blk->bs = bs;
> +bs->blk = blk;
> +}
> +
> +/*

As discussed in the BlockBackend and media thread, you should probably
assert(bs->blk == NULL), and I'd rather have this implemented as
blk_remove_bs(blk); blk_insert_bs(blk, bs);, but that's probably
something that won't happen until I do so myself.

Max

>   * Return @blk's DriveInfo if any, else null.
>   */
>  DriveInfo *blk_legacy_dinfo(BlockBackend *blk)
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 4598101..cfcae52 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -659,6 +659,8 @@ void backup_start(BlockDriverState *bs, BlockDriverState 
> *target,
>BlockCompletionFunc *cb, void *opaque,
>Error **errp);
>  
> +void blk_set_bs(BlockBackend *blk, BlockDriverState *bs);
> +
>  void blk_dev_change_media_cb(BlockBackend *blk, bool load);
>  bool blk_dev_has_removable_media(BlockBackend *blk);
>  void blk_dev_eject_request(BlockBackend *blk, bool force);
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 09/16] block: Split bdrv_move_feature_fields()

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> After bdrv_swap(), some fields must be moved back to their original BDS
> to compensate for the effects that a swap of the contents of the objects
> has while keeping the old addresses. Other fields must be moved back
> because they should logically be moved and must stay on top
> 
> When replacing bdrv_swap() with operations changing the pointers in the
> parents, we only need the latter and must avoid swapping the former.
> Split the function accordingly.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c | 12 
>  1 file changed, 12 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 743f82e..7930f3c 100644
> --- a/block.c
> +++ b/block.c
> @@ -1984,6 +1984,8 @@ static void bdrv_rebind(BlockDriverState *bs)
>  }
>  }
>  
> +/* Fields that need to stay with the top-level BDS, no matter whether the
> + * address of the top-level BDS stays the same or not. */
>  static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
>   BlockDriverState *bs_src)
>  {
> @@ -2019,7 +2021,13 @@ static void bdrv_move_feature_fields(BlockDriverState 
> *bs_dest,
>  
>  /* dirty bitmap */
>  bs_dest->dirty_bitmaps  = bs_src->dirty_bitmaps;
> +}
>  
> +/* Fields that only need to be swapped if the contents of BDSes is swapped
> + * rather than pointers being changed in the parents. */
> +static void bdrv_move_reference_fields(BlockDriverState *bs_dest,
> +   BlockDriverState *bs_src)
> +{
>  /* reference count */
>  bs_dest->refcnt = bs_src->refcnt;
>  

I'm not sure whether the op blockers should be moved in this function...
I think they should be moved in bdrv_move_feasture_fields(), because
they generally depend on the position within the node graph and not on
the BDS itself, don't they?

Max

> @@ -2090,6 +2098,10 @@ void bdrv_swap(BlockDriverState *bs_new, 
> BlockDriverState *bs_old)
>  bdrv_move_feature_fields(bs_old, bs_new);
>  bdrv_move_feature_fields(bs_new, );
>  
> +bdrv_move_reference_fields(, bs_old);
> +bdrv_move_reference_fields(bs_old, bs_new);
> +bdrv_move_reference_fields(bs_new, );
> +
>  /* bs_new must remain unattached */
>  assert(!bs_new->blk);
>  
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 12/16] block: Introduce parents list

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf 
> ---
>  block.c   | 3 +++
>  include/block/block_int.h | 2 ++
>  2 files changed, 5 insertions(+)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 13/16] block: Implement bdrv_append() without bdrv_swap()

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> Remember all parent nodes and just change the pointers there instead of
> swapping the contents of the BlockDriverState.
> 
> Handling of snapshot=on must be moved further down in bdrv_open()
> because *pbs (which is the bs pointer in the BlockBackend) must already
> be set before bdrv_append() is called. Otherwise bdrv_append() changes
> the BB's pointer to the temporary snapshot, but bdrv_open() overwrites
> it with the read-only original image.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c | 109 
> +++-
>  1 file changed, 81 insertions(+), 28 deletions(-)
> 
> diff --git a/block.c b/block.c
> index c196f83..98fc17c 100644
> --- a/block.c
> +++ b/block.c
> @@ -1516,15 +1516,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
> const char *filename,
>  
>  bdrv_refresh_filename(bs);
>  
> -/* For snapshot=on, create a temporary qcow2 overlay. bs points to the
> - * temporary snapshot afterwards. */
> -if (snapshot_flags) {
> -ret = bdrv_append_temp_snapshot(bs, snapshot_flags, _err);
> -if (local_err) {
> -goto close_and_fail;
> -}
> -}
> -
>  /* Check if any unknown options were used */
>  if (options && (qdict_size(options) != 0)) {
>  const QDictEntry *entry = qdict_first(options);
> @@ -1556,6 +1547,16 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
> const char *filename,
>  
>  QDECREF(options);
>  *pbs = bs;
> +
> +/* For snapshot=on, create a temporary qcow2 overlay. bs points to the
> + * temporary snapshot afterwards. */
> +if (snapshot_flags) {
> +ret = bdrv_append_temp_snapshot(bs, snapshot_flags, _err);
> +if (local_err) {
> +goto close_and_fail;
> +}
> +}
> +
>  return 0;
>  
>  fail:
> @@ -1999,20 +2000,6 @@ static void bdrv_move_feature_fields(BlockDriverState 
> *bs_dest,
>  
>  bs_dest->enable_write_cache = bs_src->enable_write_cache;
>  
> -/* i/o throttled req */
> -bs_dest->throttle_state = bs_src->throttle_state,
> -bs_dest->io_limits_enabled  = bs_src->io_limits_enabled;
> -bs_dest->pending_reqs[0]= bs_src->pending_reqs[0];
> -bs_dest->pending_reqs[1]= bs_src->pending_reqs[1];
> -bs_dest->throttled_reqs[0]  = bs_src->throttled_reqs[0];
> -bs_dest->throttled_reqs[1]  = bs_src->throttled_reqs[1];
> -memcpy(_dest->round_robin,
> -   _src->round_robin,
> -   sizeof(bs_dest->round_robin));
> -memcpy(_dest->throttle_timers,
> -   _src->throttle_timers,
> -   sizeof(ThrottleTimers));
> -
>  /* r/w error */
>  bs_dest->on_read_error  = bs_src->on_read_error;
>  bs_dest->on_write_error = bs_src->on_write_error;
> @@ -2026,10 +2013,25 @@ static void bdrv_move_feature_fields(BlockDriverState 
> *bs_dest,
>  }
>  
>  /* Fields that only need to be swapped if the contents of BDSes is swapped
> - * rather than pointers being changed in the parents. */
> + * rather than pointers being changed in the parents, and throttling fields
> + * because only bdrv_swap() messes with internals of throttling. */
>  static void bdrv_move_reference_fields(BlockDriverState *bs_dest,
> BlockDriverState *bs_src)
>  {
> +/* i/o throttled req */
> +bs_dest->throttle_state = bs_src->throttle_state,
> +bs_dest->io_limits_enabled  = bs_src->io_limits_enabled;
> +bs_dest->pending_reqs[0]= bs_src->pending_reqs[0];
> +bs_dest->pending_reqs[1]= bs_src->pending_reqs[1];
> +bs_dest->throttled_reqs[0]  = bs_src->throttled_reqs[0];
> +bs_dest->throttled_reqs[1]  = bs_src->throttled_reqs[1];
> +memcpy(_dest->round_robin,
> +   _src->round_robin,
> +   sizeof(bs_dest->round_robin));
> +memcpy(_dest->throttle_timers,
> +   _src->throttle_timers,
> +   sizeof(ThrottleTimers));
> +
>  /* reference count */
>  bs_dest->refcnt = bs_src->refcnt;
>  
> @@ -2155,6 +2157,42 @@ void bdrv_swap(BlockDriverState *bs_new, 
> BlockDriverState *bs_old)
>  bdrv_rebind(bs_old);
>  }
>  
> +static void change_parent_backing_link(BlockDriverState *from,
> +   BlockDriverState *to)
> +{
> +BdrvChild *c, *next;
> +
> +QLIST_FOREACH_SAFE(c, >parents, next_parent, next) {
> +assert(c->role != _backing);
> +c->bs = to;
> +QLIST_REMOVE(c, next_parent);
> +QLIST_INSERT_HEAD(>parents, c, next_parent);

This drops a reference from the parent BDS to @from, and adds a new one
from the parent BDS to @to. However, this is not reflected here.

> +}
> +if (from->blk) {
> +blk_set_bs(from->blk, to);
> +if (!to->device_list.tqe_prev) {
> +QTAILQ_INSERT_BEFORE(from, to, device_list);
> +}
> +QTAILQ_REMOVE(_states, from, 

Re: [Qemu-block] [PATCH 15/16] block: Add and use bdrv_replace_in_backing_chain()

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> This cleans up the mess we left behind in the mirror code after the
> previous patch. Instead of using bdrv_swap(), just change pointers.
> 
> The interface change of the mirror job that callers must consider is
> that after job completion, their local BDS pointers still point to the
> same node now. qemu-img must change its code accordingly (which makes it
> easier to understand); the other callers stays unchanged because after
> completion they don't do anything with the BDS, but just with the job,
> and the job is still owned by the source BDS.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c   | 32 +++-
>  block/mirror.c| 23 +++
>  include/block/block.h |  4 +++-
>  qemu-img.c| 16 
>  4 files changed, 49 insertions(+), 26 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 98fc17c..7c21659 100644
> --- a/block.c
> +++ b/block.c
> @@ -1095,7 +1095,7 @@ static BdrvChild *bdrv_attach_child(BlockDriverState 
> *parent_bs,
>  return child;
>  }
>  
> -void bdrv_detach_child(BdrvChild *child)
> +static void bdrv_detach_child(BdrvChild *child)
>  {
>  QLIST_REMOVE(child, next);
>  QLIST_REMOVE(child, next_parent);
> @@ -2228,6 +2228,36 @@ void bdrv_append(BlockDriverState *bs_new, 
> BlockDriverState *bs_top)
>  bdrv_unref(bs_new);
>  }
>  
> +void bdrv_replace_in_backing_chain(BlockDriverState *old, BlockDriverState 
> *new)
> +{
> +assert(!bdrv_requests_pending(old));
> +assert(!bdrv_requests_pending(new));
> +
> +bdrv_ref(old);
> +
> +if (old->blk) {
> +/* As long as these fields aren't in BlockBackend, but in the 
> top-level
> + * BlockDriverState, it's not possible for a BDS to have two BBs.
> + *
> + * We really want to copy the fields from old to new, but we go for a
> + * swap instead so that pointers aren't duplicated and cause trouble.
> + * (Also, bdrv_swap() used to do the same.) */
> +assert(!new->blk);
> +swap_feature_fields(old, new);
> +}
> +change_parent_backing_link(old, new);
> +
> +/* Change backing files if a previously independent node is added to the
> + * chain. For active commit, we replace top by its own (indirect) backing
> + * file and don't do anything here so we don't build a loop. */
> +if (new->backing == NULL && !bdrv_chain_contains(backing_bs(old), new)) {
> +bdrv_set_backing_hd(new, backing_bs(old));
> +bdrv_set_backing_hd(old, NULL);

Wouldn't we want @old to keep its backing file?

Then bdrv_append() would basically be a special case of this function,
with it additionally decrementing the refcount of @bs_new.

Max

> +}
> +
> +bdrv_unref(old);
> +}
> +
>  static void bdrv_delete(BlockDriverState *bs)
>  {
>  assert(!bs->job);



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v5 00/38] blockdev: BlockBackend and media

[Wen Congyang]

On 09/22/2015 11:09 PM, Max Reitz wrote:
> On 22.09.2015 16:45, Kevin Wolf wrote:
>> Am 18.09.2015 um 17:22 hat Max Reitz geschrieben:
>>> This series reworks a lot regarding BlockBackend and media. Basically,
>>> it allows empty BlockBackends, that is BBs without a BDS tree.
>>>
>>> Before this series, empty drives are represented by a BlockBackend with
>>> an empty BDS attached to it (a BDS with a NULL driver). However, now we
>>> have BlockBackends, thus an empty drive should be represented by a
>>> BlockBackend without any BDS tree attached to it. This is what this
>>> series does.
>>
>> Patches 1-16 and 18: Reviewed-by: Kevin Wolf 
>>
>> I had comments for 17 and 19-21, and can't seem to find a base commit
>> that patch 22 applies cleanly to, so I'm stopping the review here,
>> waiting for another rebase or instructions how to apply the series.
> 
> 16a1b6e97c2a2919fd296db4bea2f9da2ad3cc4d was my base, if that was the
> question. ;-)

It can be applied to the upstream qemu very easily:
s/qmp_marshal_input_/qmp_marshal_/

> 
> Thanks for reviewing!
> 
> Max
> 

Re: [Qemu-block] [PATCH v6 3/4] block: add a 'blockdev-snapshot' QMP command

[Alberto Garcia]

On Tue 22 Sep 2015 06:38:27 PM CEST, Max Reitz wrote:

>> +.name   = "blockdev-snapshot",
>> +.args_type  = "node:s,overlay:s",
>> +.mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot,
>
> As of 7fad30f06eb6aa57aaa8f3d264288f24ae7646f0, this needs to be
> qmp_marshal_blockdev_snapshot.

Sure, that needs to be part of the next rebase, otherwise it won't
build.

Berto