Re: [Qemu-block] [PATCH] error: Avoid redudant error_propagate() usage

2016-06-09 Thread Eduardo Habkost 
On Thu, Jun 09, 2016 at 02:54:51PM -0600, Eric Blake wrote:
> On 06/09/2016 02:47 PM, Eduardo Habkost wrote:
> > On Thu, Jun 09, 2016 at 05:21:34PM -0300, Eduardo Habkost wrote:
> >> This patch simplifies code that uses a local_err variable just to 
> >> immediately
> >> use it for an error_propagate() call.
> > 
> > Please ignore this one. I found a better way to tell Coccinelle
> > to do that. Updated Coccinelle patch is below, but I will send
> > the actual patch tomorrow.
> 
> Cool.  Worth sticking these in scripts/coccinelle/ as part of your
> commit so that we can rerun them later (and refer to them for ideas when
> writing new scripts)?

I didn't know about scripts/coccinelle. I will do!

-- 
Eduardo

Re: [Qemu-block] [PATCH] error: Avoid redudant error_propagate() usage

2016-06-09 Thread Eric Blake 
On 06/09/2016 02:21 PM, Eduardo Habkost wrote:
> This patch simplifies code that uses a local_err variable just to immediately
> use it for an error_propagate() call.
> 
> Done using the following Coccinelle patch:
> 

> +++ b/block.c
> @@ -353,7 +353,6 @@ out:
>  int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
>  {
>  BlockDriver *drv;
> -Error *local_err = NULL;
>  int ret;
>  
>  drv = bdrv_find_protocol(filename, true, errp);
> @@ -361,8 +360,7 @@ int bdrv_create_file(const char *filename, QemuOpts 
> *opts, Error **errp)
>  return -ENOENT;
>  }
>  
> -ret = bdrv_create(drv, filename, opts, _err);
> -error_propagate(errp, local_err);
> +ret = bdrv_create(drv, filename, opts, errp);
>  return ret;

And I _know_ there's a Coccinelle recipe for further shortening this
into 'return bdrv_create(...)' (since it was part of the tutorial class
at last year's KVM Forum) - again, I don't know the actual syntax to use
to get it, but it shouldn't be too hard to find in a web search.  Fine
as yet another followup patch.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH] error: Avoid redudant error_propagate() usage

2016-06-09 Thread Eric Blake 
On 06/09/2016 02:47 PM, Eduardo Habkost wrote:
> On Thu, Jun 09, 2016 at 05:21:34PM -0300, Eduardo Habkost wrote:
>> This patch simplifies code that uses a local_err variable just to immediately
>> use it for an error_propagate() call.
> 
> Please ignore this one. I found a better way to tell Coccinelle
> to do that. Updated Coccinelle patch is below, but I will send
> the actual patch tomorrow.

Cool.  Worth sticking these in scripts/coccinelle/ as part of your
commit so that we can rerun them later (and refer to them for ideas when
writing new scripts)?

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH] error: Avoid redudant error_propagate() usage

2016-06-09 Thread Eduardo Habkost 
This patch simplifies code that uses a local_err variable just to immediately
use it for an error_propagate() call.

Done using the following Coccinelle patch:

  @@
  expression R;
  expression list ARGS;
  type T;
  identifier F1, F2;
  identifier LOCAL_ERR;
  identifier ERRP;
  idexpression V;
  typedef Error;
  @@
   T F1(..., Error **ERRP)
   {
  (
   // this slice guarantees that the code won't be changed
   // if LOCAL_ERR is used elsewhere in the function
   // (i.e. if it appears 3+ times in the function body)
   ...
   Error *LOCAL_ERR;
   ...
   LOCAL_ERR
   ...
   LOCAL_ERR
   ...
   LOCAL_ERR
   ...
  |
   ...
  -Error *LOCAL_ERR;
   ...
  (
  -F2(ARGS, _ERR);
  -error_propagate(ERRP, LOCAL_ERR);
  +F2(ARGS, ERRP);
  |
  -V = F2(ARGS, _ERR);
  -error_propagate(ERRP, LOCAL_ERR);
  +V = F2(ARGS, ERRP);
  )
   ...
  )
   }

Signed-off-by: Eduardo Habkost 
---
In the end, I found a way to avoid matching cases where local_err
is used elsewhere in the function.
---
 block.c|  4 +---
 block/raw-posix.c  |  8 ++--
 block/raw_bsd.c|  4 +---
 blockdev.c | 16 +---
 hw/s390x/s390-virtio-ccw.c |  5 +
 hw/s390x/virtio-ccw.c  | 28 +++-
 target-i386/cpu.c  |  4 +---
 7 files changed, 18 insertions(+), 51 deletions(-)

diff --git a/block.c b/block.c
index ecca55a..5cc83e5 100644
--- a/block.c
+++ b/block.c
@@ -353,7 +353,6 @@ out:
 int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
 {
 BlockDriver *drv;
-Error *local_err = NULL;
 int ret;
 
 drv = bdrv_find_protocol(filename, true, errp);
@@ -361,8 +360,7 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, 
Error **errp)
 return -ENOENT;
 }
 
-ret = bdrv_create(drv, filename, opts, _err);
-error_propagate(errp, local_err);
+ret = bdrv_create(drv, filename, opts, errp);
 return ret;
 }
 
diff --git a/block/raw-posix.c b/block/raw-posix.c
index cb663d8..d7397bf 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -582,12 +582,10 @@ static int raw_open(BlockDriverState *bs, QDict *options, 
int flags,
 Error **errp)
 {
 BDRVRawState *s = bs->opaque;
-Error *local_err = NULL;
 int ret;
 
 s->type = FTYPE_FILE;
-ret = raw_open_common(bs, options, flags, 0, _err);
-error_propagate(errp, local_err);
+ret = raw_open_common(bs, options, flags, 0, errp);
 return ret;
 }
 
@@ -2442,14 +2440,12 @@ static int cdrom_open(BlockDriverState *bs, QDict 
*options, int flags,
   Error **errp)
 {
 BDRVRawState *s = bs->opaque;
-Error *local_err = NULL;
 int ret;
 
 s->type = FTYPE_CD;
 
 /* open will not fail even if no CD is inserted, so add O_NONBLOCK */
-ret = raw_open_common(bs, options, flags, O_NONBLOCK, _err);
-error_propagate(errp, local_err);
+ret = raw_open_common(bs, options, flags, O_NONBLOCK, errp);
 return ret;
 }
 
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 5af11b6..b51ac98 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -190,11 +190,9 @@ static int raw_has_zero_init(BlockDriverState *bs)
 
 static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
 {
-Error *local_err = NULL;
 int ret;
 
-ret = bdrv_create_file(filename, opts, _err);
-error_propagate(errp, local_err);
+ret = bdrv_create_file(filename, opts, errp);
 return ret;
 }
 
diff --git a/blockdev.c b/blockdev.c
index 028dba3..3b6d242 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3654,7 +3654,6 @@ void qmp_blockdev_mirror(const char *device, const char 
*target,
 BlockBackend *blk;
 BlockDriverState *target_bs;
 AioContext *aio_context;
-Error *local_err = NULL;
 
 blk = blk_by_name(device);
 if (!blk) {
@@ -3678,16 +3677,11 @@ void qmp_blockdev_mirror(const char *device, const char 
*target,
 
 bdrv_set_aio_context(target_bs, aio_context);
 
-blockdev_mirror_common(bs, target_bs,
-   has_replaces, replaces, sync,
-   has_speed, speed,
-   has_granularity, granularity,
-   has_buf_size, buf_size,
-   has_on_source_error, on_source_error,
-   has_on_target_error, on_target_error,
-   true, true,
-   _err);
-error_propagate(errp, local_err);
+blockdev_mirror_common(bs, target_bs, has_replaces, replaces, sync,
+   has_speed, speed, has_granularity, granularity,
+   has_buf_size, buf_size, has_on_source_error,
+   on_source_error, has_on_target_error,
+   on_target_error, true, true, errp);
 
 aio_context_release(aio_context);
 }

Re: [Qemu-block] [Qemu-devel] [PATCH] error: Remove NULL checks on error_propagate() calls

2016-06-09 Thread Eduardo Habkost 
On Thu, Jun 09, 2016 at 01:37:23PM -0600, Eric Blake wrote:
[...]
> 
> Hmm - it seems like in most of the cases where the ONLY thing done in
> the if (local_err) block is to propagate the error, we should instead be
> directly assigning to errp instead of wasting a local variable.  At this
> point, my review is repetitive enough that I'll stop looking, and leave
> it up to you and Markus whether to attempt a more ambitious Coccinelle
> script.

If it happens immediately before the function end or a return
statement it should be easy, but it would still require some
manual work to remove the unused variable declaration. Probably
easier to do that in a follow-up patch.

It's harder (impossible?) to make Coccinelle avoid matching if
local_err is used somewhere else in the function. But it's
probably doable with some manual work, in a follow-up patch.

-- 
Eduardo

[Qemu-block] [PATCH] error: Remove NULL checks on error_propagate() calls

2016-06-09 Thread Eduardo Habkost 
error_propagate() already ignores local_err==NULL, so there's no
need to check it before calling.

Done using the following Coccinelle patch:

  @@
  identifier L;
  expression E;
  @@
  -if (L) {
   error_propagate(E, L);
  -}

Signed-off-by: Eduardo Habkost 
---
 block.c   | 20 +---
 block/qcow2.c |  4 +---
 block/quorum.c|  4 +---
 block/raw-posix.c | 16 
 block/raw_bsd.c   |  4 +---
 block/snapshot.c  |  4 +---
 blockdev.c| 12 +++-
 bootdevice.c  |  4 +---
 dump.c|  4 +---
 hw/ide/qdev.c |  4 +---
 hw/net/ne2000-isa.c   |  4 +---
 hw/s390x/virtio-ccw.c | 28 +++-
 hw/usb/dev-storage.c  |  4 +---
 qga/commands-win32.c  |  8 ++--
 qom/object.c  |  4 +---
 15 files changed, 31 insertions(+), 93 deletions(-)

diff --git a/block.c b/block.c
index f54bc25..ecca55a 100644
--- a/block.c
+++ b/block.c
@@ -301,9 +301,7 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque)
 assert(cco->drv);
 
 ret = cco->drv->bdrv_create(cco->filename, cco->opts, _err);
-if (local_err) {
-error_propagate(>err, local_err);
-}
+error_propagate(>err, local_err);
 cco->ret = ret;
 }
 
@@ -364,9 +362,7 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, 
Error **errp)
 }
 
 ret = bdrv_create(drv, filename, opts, _err);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 return ret;
 }
 
@@ -1760,18 +1756,14 @@ fail:
 QDECREF(options);
 bs->options = NULL;
 bdrv_unref(bs);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 return NULL;
 
 close_and_fail:
 bdrv_unref(bs);
 QDECREF(snapshot_options);
 QDECREF(options);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 return NULL;
 }
 
@@ -3591,9 +3583,7 @@ void bdrv_img_create(const char *filename, const char 
*fmt,
 out:
 qemu_opts_del(opts);
 qemu_opts_free(create_opts);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 }
 
 AioContext *bdrv_get_aio_context(BlockDriverState *bs)
diff --git a/block/qcow2.c b/block/qcow2.c
index 6f5fb81..4504846 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2394,9 +2394,7 @@ static int qcow2_create(const char *filename, QemuOpts 
*opts, Error **errp)
 ret = qcow2_create2(filename, size, backing_file, backing_fmt, flags,
 cluster_size, prealloc, opts, version, refcount_order,
 _err);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 
 finish:
 g_free(backing_file);
diff --git a/block/quorum.c b/block/quorum.c
index ec6f3b9..331b726 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -971,9 +971,7 @@ close_exit:
 exit:
 qemu_opts_del(opts);
 /* propagate error */
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 return ret;
 }
 
diff --git a/block/raw-posix.c b/block/raw-posix.c
index ce2e20f..cb663d8 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -587,9 +587,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, 
int flags,
 
 s->type = FTYPE_FILE;
 ret = raw_open_common(bs, options, flags, 0, _err);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 return ret;
 }
 
@@ -2239,9 +2237,7 @@ hdev_open_Mac_error:
 
 ret = raw_open_common(bs, options, flags, 0, _err);
 if (ret < 0) {
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 #if defined(__APPLE__) && defined(__MACH__)
 if (*bsd_path) {
 filename = bsd_path;
@@ -2453,9 +2449,7 @@ static int cdrom_open(BlockDriverState *bs, QDict 
*options, int flags,
 
 /* open will not fail even if no CD is inserted, so add O_NONBLOCK */
 ret = raw_open_common(bs, options, flags, O_NONBLOCK, _err);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 return ret;
 }
 
@@ -2573,9 +2567,7 @@ static int cdrom_open(BlockDriverState *bs, QDict 
*options, int flags,
 
 ret = raw_open_common(bs, options, flags, 0, _err);
 if (ret) {
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 return ret;
 }
 
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index b1d5237..5af11b6 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -194,9 +194,7 @@ static int raw_create(const char *filename, QemuOpts *opts, 
Error **errp)
 int ret;
 
 ret =

Re: [Qemu-block] [Qemu-devel] [PATCH] error: Remove NULL checks on error_propagate() calls

2016-06-09 Thread Eduardo Habkost 
On Thu, Jun 09, 2016 at 02:11:23PM -0600, Eric Blake wrote:
> On 06/09/2016 01:50 PM, Eduardo Habkost wrote:
> > On Thu, Jun 09, 2016 at 01:37:23PM -0600, Eric Blake wrote:
> > [...]
> >>
> >> Hmm - it seems like in most of the cases where the ONLY thing done in
> >> the if (local_err) block is to propagate the error, we should instead be
> >> directly assigning to errp instead of wasting a local variable.  At this
> >> point, my review is repetitive enough that I'll stop looking, and leave
> >> it up to you and Markus whether to attempt a more ambitious Coccinelle
> >> script.
> > 
> > If it happens immediately before the function end or a return
> > statement it should be easy, but it would still require some
> > manual work to remove the unused variable declaration. Probably
> > easier to do that in a follow-up patch.
> 
> I think Coccinelle can be used to eliminate unused local variables, but
> don't know the recipe off-hand; maybe a web search will turn up something?

I found something called "when constraints", but the
documentation isn't very clear.

There's an example here:
http://coccinelle.lip6.fr/rules/unused.cocci

> 
> > 
> > It's harder (impossible?) to make Coccinelle avoid matching if
> > local_err is used somewhere else in the function. But it's
> > probably doable with some manual work, in a follow-up patch.
> 
> I don't know - Coccinelle is rather powerful, and there may indeed be a
> way to flag conditions for a variable that is not used anywhere except
> in the lines mentioned in the recipe, vs. a variable that can also be
> used in the ... portion of the recipe.

I found a hackish way to do that. Sending a follow-up patch in 1
minute.

-- 
Eduardo

Re: [Qemu-block] [Qemu-devel] [PATCH] error: Remove NULL checks on error_propagate() calls

2016-06-09 Thread Eric Blake 
On 06/09/2016 01:50 PM, Eduardo Habkost wrote:
> On Thu, Jun 09, 2016 at 01:37:23PM -0600, Eric Blake wrote:
> [...]
>>
>> Hmm - it seems like in most of the cases where the ONLY thing done in
>> the if (local_err) block is to propagate the error, we should instead be
>> directly assigning to errp instead of wasting a local variable.  At this
>> point, my review is repetitive enough that I'll stop looking, and leave
>> it up to you and Markus whether to attempt a more ambitious Coccinelle
>> script.
> 
> If it happens immediately before the function end or a return
> statement it should be easy, but it would still require some
> manual work to remove the unused variable declaration. Probably
> easier to do that in a follow-up patch.

I think Coccinelle can be used to eliminate unused local variables, but
don't know the recipe off-hand; maybe a web search will turn up something?

> 
> It's harder (impossible?) to make Coccinelle avoid matching if
> local_err is used somewhere else in the function. But it's
> probably doable with some manual work, in a follow-up patch.

I don't know - Coccinelle is rather powerful, and there may indeed be a
way to flag conditions for a variable that is not used anywhere except
in the lines mentioned in the recipe, vs. a variable that can also be
used in the ... portion of the recipe.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH] error: Remove NULL checks on error_propagate() calls

2016-06-09 Thread Eric Blake 
On 06/09/2016 11:40 AM, Eduardo Habkost wrote:
> error_propagate() already ignores local_err==NULL, so there's no
> need to check it before calling.
> 
> Done using the following Coccinelle patch:
> 
>   @@
>   identifier L;
>   expression E;
>   @@
>   -if (L) {
>error_propagate(E, L);
>   -}
> 
> Signed-off-by: Eduardo Habkost 
> ---

> diff --git a/block.c b/block.c
> index f54bc25..ecca55a 100644
> --- a/block.c
> +++ b/block.c
> @@ -301,9 +301,7 @@ static void coroutine_fn bdrv_create_co_entry(void 
> *opaque)
>  assert(cco->drv);
>  
>  ret = cco->drv->bdrv_create(cco->filename, cco->opts, _err);
> -if (local_err) {
> -error_propagate(>err, local_err);
> -}
> +error_propagate(>err, local_err);
>  cco->ret = ret;
>  }

This is a case where we can go one step further:

assert(cco->drv);
cco->ret = cco->drv->bdrv_create(cco->filename, cco->opts, >err);

and ditch local_err and ret altogether.

>  
> @@ -364,9 +362,7 @@ int bdrv_create_file(const char *filename, QemuOpts 
> *opts, Error **errp)
>  }
>  
>  ret = bdrv_create(drv, filename, opts, _err);
> -if (local_err) {
> -error_propagate(errp, local_err);
> -}
> +error_propagate(errp, local_err);
>  return ret;
>  }

Another place where we could ditch local_err, and directly call

return bdrv_create(drv, filename, opts, errp);

Of course, unless you tweak the Coccinelle script to do that cleanup
automatically, or improve the commit message to mention that you did
further cleanups beyond Coccinelle, then it would belong better as a
followup patch, leaving this conversion to be fine to commit as is.

>  
> @@ -1760,18 +1756,14 @@ fail:
>  QDECREF(options);
>  bs->options = NULL;
>  bdrv_unref(bs);
> -if (local_err) {
> -error_propagate(errp, local_err);
> -}
> +error_propagate(errp, local_err);
>  return NULL;

This one's fine, along with any others I don't comment on.

> +++ b/block/qcow2.c
> @@ -2394,9 +2394,7 @@ static int qcow2_create(const char *filename, QemuOpts 
> *opts, Error **errp)
>  ret = qcow2_create2(filename, size, backing_file, backing_fmt, flags,
>  cluster_size, prealloc, opts, version, 
> refcount_order,
>  _err);
> -if (local_err) {
> -error_propagate(errp, local_err);
> -}
> +error_propagate(errp, local_err);

Another case where we could ditch local_err, OR, we could do:

 prealloc = qapi_enum_parse(..., _err);
 if (local_err) {
-error_propagate(errp, local_err);
 ret = -EINVAL;
 goto finish;
 }
...
 ret = qcow2_create2(..., _err);
-if (local_err) {
-error_propagate(errp, local_err);
-}

 finish:
+error_propagate(errp, local_err);

> +++ b/block/raw-posix.c
> @@ -587,9 +587,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, 
> int flags,
>  
>  s->type = FTYPE_FILE;
>  ret = raw_open_common(bs, options, flags, 0, _err);
> -if (local_err) {
> -error_propagate(errp, local_err);
> -}
> +error_propagate(errp, local_err);
>  return ret;
>  }

Another case where we could ditch local_err.

> @@ -2453,9 +2449,7 @@ static int cdrom_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  
>  /* open will not fail even if no CD is inserted, so add O_NONBLOCK */
>  ret = raw_open_common(bs, options, flags, O_NONBLOCK, _err);
> -if (local_err) {
> -error_propagate(errp, local_err);
> -}
> +error_propagate(errp, local_err);
>  return ret;

and another.

>  }
>  
> @@ -2573,9 +2567,7 @@ static int cdrom_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  
>  ret = raw_open_common(bs, options, flags, 0, _err);
>  if (ret) {
> -if (local_err) {
> -error_propagate(errp, local_err);
> -}
> +error_propagate(errp, local_err);
>  return ret;
>  }

and another

>  
> diff --git a/block/raw_bsd.c b/block/raw_bsd.c
> index b1d5237..5af11b6 100644
> --- a/block/raw_bsd.c
> +++ b/block/raw_bsd.c
> @@ -194,9 +194,7 @@ static int raw_create(const char *filename, QemuOpts 
> *opts, Error **errp)
>  int ret;
>  
>  ret = bdrv_create_file(filename, opts, _err);
> -if (local_err) {
> -error_propagate(errp, local_err);
> -}
> +error_propagate(errp, local_err);
>  return ret;

and another

>  }
>  
> diff --git a/block/snapshot.c b/block/snapshot.c
> index da89d2b..bf5c2ca 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -358,9 +358,7 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState 
> *bs,
>  ret = bdrv_snapshot_load_tmp(bs, NULL, id_or_name, _err);
>  }
>  
> -if (local_err) {
> -error_propagate(errp, local_err);
> -}
> +error_propagate(errp, local_err);
>  
>  return ret;

and another

>  }
> diff --git a/blockdev.c b/blockdev.c
> index 7fd515a..028dba3 100644
> ---

Re: [Qemu-block] [Qemu-devel] coroutines: block: Co-routine re-entered recursively when migrating disk with iothreads

2016-06-09 Thread Jason J. Herne 

On 06/09/2016 12:31 PM, Stefan Hajnoczi wrote:

On Mon, May 23, 2016 at 7:54 PM, Jason J. Herne
 wrote:

Libvirt migration command:
virsh migrate --live --persistent --copy-storage-all --migrate-disks vdb
kvm1 qemu+ssh://dev1/system


I guess that this is the same scenario as a manual drive_mirror +
migrate test I have been doing.  I also get the "Co-routine re-entered
recursively" error message.

I've CCed you on an (unfinished) patch that solves the abort.  Please
test it and let me know!



Stefan, Yes!! This patch fixes the problem in our environment :) Thank 
you for digging into it and finding a solution. We'll patiently await a 
completed patch.


--
-- Jason J. Herne (jjhe...@linux.vnet.ibm.com)

[Qemu-block] [PATCH v5 03/15] qapi: Add parameter to visit_end_*

2016-06-09 Thread Eric Blake 
Rather than making the dealloc visitor track of stack of pointers
remembered during visit_start_* in order to free them during
visit_end_*, it's a lot easier to just make all callers pass the
same pointer to visit_end_*.  The generated code has access to the
same pointer, while all other users are doing virtual walks and
can pass NULL.  The dealloc visitor is then greatly simplified.

All three visit_end_*() functions intentionally take a void**,
even though the visit_start_*() functions differ between void**,
GenericList**, and GenericAlternate**.  This is done for several
reasons: when doing a virtual walk, passing NULL doesn't care
what the type is, but when doing a generated walk, we already
have to cast the caller's specific FOO* to call visit_start,
while using void** lets us use visit_end without a cast. Also,
an upcoming patch will add a clone visitor that wants to use
the same implementation for all three visit_end callbacks,
which is made easier if all three share the same signature.

For visitors with already track per-object state (the QMP visitors
via a stack, and the string visitors which do not allow nesting),
add an assertion that the caller is indeed passing the same
pointer to paired calls.

Signed-off-by: Eric Blake 

---
v5: add assertions in stack-based visitors
v4: hoist earlier in series, document why void** is used, update
docs/
v3: new patch
---
 include/qapi/visitor.h  | 32 
 include/qapi/visitor-impl.h |  6 +++---
 scripts/qapi-commands.py|  4 ++--
 scripts/qapi-event.py   |  2 +-
 scripts/qapi-visit.py   |  8 +++
 qapi/qapi-visit-core.c  | 12 +--
 block/crypto.c  |  4 ++--
 hw/ppc/spapr_drc.c  |  4 ++--
 hw/virtio/virtio-balloon.c  |  4 ++--
 qapi/opts-visitor.c |  4 ++--
 qapi/qapi-dealloc-visitor.c | 47 +++--
 qapi/qmp-input-visitor.c| 11 ++
 qapi/qmp-output-visitor.c   | 23 
 qapi/string-input-visitor.c |  7 +-
 qapi/string-output-visitor.c|  5 -
 qom/object.c|  2 +-
 qom/object_interfaces.c |  4 ++--
 tests/test-qmp-input-visitor.c  |  2 +-
 tests/test-qmp-output-visitor.c |  2 +-
 docs/qapi-code-gen.txt  |  8 +++
 20 files changed, 86 insertions(+), 105 deletions(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 4d12167..25d0cc2 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -193,12 +193,12 @@
  *  goto outlist;
  *  }
  * outlist:
- *  visit_end_list(v);
+ *  visit_end_list(v, NULL);
  *  if (!err) {
  *  visit_check_struct(v, );
  *  }
  * outobj:
- *  visit_end_struct(v);
+ *  visit_end_struct(v, NULL);
  * out:
  *  error_propagate(errp, err);
  *  ...clean up v...
@@ -242,8 +242,8 @@ typedef struct GenericAlternate {
  * After visit_start_struct() succeeds, the caller may visit its
  * members one after the other, passing the member's name and address
  * within the struct.  Finally, visit_end_struct() needs to be called
- * to clean up, even if intermediate visits fail.  See the examples
- * above.
+ * with the same @obj to clean up, even if intermediate visits fail.
+ * See the examples above.
  *
  * FIXME Should this be named visit_start_object, since it is also
  * used for QAPI unions, and maps to JSON objects?
@@ -267,12 +267,14 @@ void visit_check_struct(Visitor *v, Error **errp);
 /*
  * Complete an object visit started earlier.
  *
+ * @obj must match what was passed to the paired visit_start_struct().
+ *
  * Must be called after any successful use of visit_start_struct(),
  * even if intermediate processing was skipped due to errors, to allow
  * the backend to release any resources.  Destroying the visitor early
  * behaves as if this was implicitly called.
  */
-void visit_end_struct(Visitor *v);
+void visit_end_struct(Visitor *v, void **obj);


 /*** Visiting lists ***/
@@ -299,8 +301,9 @@ void visit_end_struct(Visitor *v);
  * visit (where @obj is NULL) uses other means.  For each list
  * element, call the appropriate visit_type_FOO() with name set to
  * NULL and obj set to the address of the value member of the list
- * element.  Finally, visit_end_list() needs to be called to clean up,
- * even if intermediate visits fail.  See the examples above.
+ * element.  Finally, visit_end_list() needs to be called with the
+ * same @list to clean up, even if intermediate visits fail.  See the
+ * examples above.
  */
 void visit_start_list(Visitor *v, const char *name, GenericList **list,
   size_t size, Error **errp);
@@ -324,12 +327,14 @@ GenericList *visit_next_list(Visitor *v, GenericList 
*tail, size_t size);
 /*
  * Complete a list visit started earlier.
  *
+ * @list must match what was passed to the paired visit_start_list().
+ *
  * Must be called after any successful use of visit_start_list(), even

[Qemu-block] [PATCH v5 09/15] qmp-output-visitor: Favor new visit_free() function

2016-06-09 Thread Eric Blake 
Now that we have a polymorphic visit_free(), we no longer need
qmp_output_visitor_cleanup(); however, we still need to
expose the subtype for qmp_output_get_qobject().

Signed-off-by: Eric Blake 

---
v5: blank line after declaration
v4: new patch
---
 include/qapi/qmp-output-visitor.h |  1 -
 block/qapi.c  |  2 +-
 blockdev.c|  2 +-
 qapi/qmp-output-visitor.c | 14 --
 qemu-img.c|  6 +++---
 qom/qom-qobject.c |  2 +-
 replay/replay-input.c |  2 +-
 tests/check-qnull.c   |  2 +-
 tests/test-qmp-output-visitor.c   |  2 +-
 util/qemu-sockets.c   |  2 +-
 10 files changed, 14 insertions(+), 21 deletions(-)

diff --git a/include/qapi/qmp-output-visitor.h 
b/include/qapi/qmp-output-visitor.h
index 2266770..29c9a2e 100644
--- a/include/qapi/qmp-output-visitor.h
+++ b/include/qapi/qmp-output-visitor.h
@@ -20,7 +20,6 @@
 typedef struct QmpOutputVisitor QmpOutputVisitor;

 QmpOutputVisitor *qmp_output_visitor_new(void);
-void qmp_output_visitor_cleanup(QmpOutputVisitor *v);

 QObject *qmp_output_get_qobject(QmpOutputVisitor *v);
 Visitor *qmp_output_get_visitor(QmpOutputVisitor *v);
diff --git a/block/qapi.c b/block/qapi.c
index 5594f74..41fe4f9 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -699,7 +699,7 @@ void bdrv_image_info_specific_dump(fprintf_function 
func_fprintf, void *f,
 assert(qobject_type(obj) == QTYPE_QDICT);
 data = qdict_get(qobject_to_qdict(obj), "data");
 dump_qobject(func_fprintf, f, 1, data);
-qmp_output_visitor_cleanup(ov);
+visit_free(qmp_output_get_visitor(ov));
 }

 void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
diff --git a/blockdev.c b/blockdev.c
index 7fd515a..1437dd7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4013,7 +4013,7 @@ void qmp_blockdev_add(BlockdevOptions *options, Error 
**errp)
 }

 fail:
-qmp_output_visitor_cleanup(ov);
+visit_free(qmp_output_get_visitor(ov));
 }

 void qmp_x_blockdev_del(bool has_id, const char *id,
diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index 5f0035c..3d12623 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -217,21 +217,15 @@ Visitor *qmp_output_get_visitor(QmpOutputVisitor *v)
 static void qmp_output_free(Visitor *v)
 {
 QmpOutputVisitor *qov = to_qov(v);
-
-qmp_output_visitor_cleanup(qov);
-}
-
-void qmp_output_visitor_cleanup(QmpOutputVisitor *v)
-{
 QStackEntry *e, *tmp;

-QTAILQ_FOREACH_SAFE(e, >stack, node, tmp) {
-QTAILQ_REMOVE(>stack, e, node);
+QTAILQ_FOREACH_SAFE(e, >stack, node, tmp) {
+QTAILQ_REMOVE(>stack, e, node);
 g_free(e);
 }

-qobject_decref(v->root);
-g_free(v);
+qobject_decref(qov->root);
+g_free(qov);
 }

 QmpOutputVisitor *qmp_output_visitor_new(void)
diff --git a/qemu-img.c b/qemu-img.c
index c4a8647..4a58578 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -493,7 +493,7 @@ static void dump_json_image_check(ImageCheck *check, bool 
quiet)
 assert(str != NULL);
 qprintf(quiet, "%s\n", qstring_get_str(str));
 qobject_decref(obj);
-qmp_output_visitor_cleanup(ov);
+visit_free(qmp_output_get_visitor(ov));
 QDECREF(str);
 }

@@ -2183,7 +2183,7 @@ static void dump_json_image_info_list(ImageInfoList *list)
 assert(str != NULL);
 printf("%s\n", qstring_get_str(str));
 qobject_decref(obj);
-qmp_output_visitor_cleanup(ov);
+visit_free(qmp_output_get_visitor(ov));
 QDECREF(str);
 }

@@ -2199,7 +2199,7 @@ static void dump_json_image_info(ImageInfo *info)
 assert(str != NULL);
 printf("%s\n", qstring_get_str(str));
 qobject_decref(obj);
-qmp_output_visitor_cleanup(ov);
+visit_free(qmp_output_get_visitor(ov));
 QDECREF(str);
 }

diff --git a/qom/qom-qobject.c b/qom/qom-qobject.c
index c3c9188..6714565 100644
--- a/qom/qom-qobject.c
+++ b/qom/qom-qobject.c
@@ -41,6 +41,6 @@ QObject *object_property_get_qobject(Object *obj, const char 
*name,
 ret = qmp_output_get_qobject(qov);
 }
 error_propagate(errp, local_err);
-qmp_output_visitor_cleanup(qov);
+visit_free(qmp_output_get_visitor(qov));
 return ret;
 }
diff --git a/replay/replay-input.c b/replay/replay-input.c
index 7b6fa93..9cbb4c1 100644
--- a/replay/replay-input.c
+++ b/replay/replay-input.c
@@ -31,7 +31,7 @@ static InputEvent *qapi_clone_InputEvent(InputEvent *src)
 ov = qmp_output_get_visitor(qov);
 visit_type_InputEvent(ov, NULL, , _abort);
 obj = qmp_output_get_qobject(qov);
-qmp_output_visitor_cleanup(qov);
+visit_free(ov);
 if (!obj) {
 return NULL;
 }
diff --git a/tests/check-qnull.c b/tests/check-qnull.c
index d0412e4..6310bf7 100644
--- a/tests/check-qnull.c
+++ b/tests/check-qnull.c
@@ -58,7 +58,7 @@ static void qnull_visit_test(void)
 obj = qmp_output_get_qobject(qov);
 g_assert(obj == _);
 qobject_decref(obj);
-

[Qemu-block] [PATCH v5 05/15] opts-visitor: Favor new visit_free() function

2016-06-09 Thread Eric Blake 
Now that we have a polymorphic visit_free(), we no longer need
opts_visitor_cleanup(); which in turn means we no longer need
to return a subtype from opts_visitor_new() nor a public upcast
function.

Signed-off-by: Eric Blake 

---
v5: blank line after declaration
v4: new patch
---
 include/qapi/opts-visitor.h |  4 +---
 block/crypto.c  | 30 ++
 hmp.c   |  8 
 hw/acpi/core.c  |  8 
 net/net.c   |  5 ++---
 numa.c  |  6 +++---
 qapi/opts-visitor.c | 26 ++
 qom/object_interfaces.c |  8 
 tests/test-opts-visitor.c   |  9 -
 9 files changed, 42 insertions(+), 62 deletions(-)

diff --git a/include/qapi/opts-visitor.h b/include/qapi/opts-visitor.h
index ae1bf7c..6462c96 100644
--- a/include/qapi/opts-visitor.h
+++ b/include/qapi/opts-visitor.h
@@ -35,8 +35,6 @@ typedef struct OptsVisitor OptsVisitor;
  * QTypes.  It also requires a non-null list argument to
  * visit_start_list().
  */
-OptsVisitor *opts_visitor_new(const QemuOpts *opts);
-void opts_visitor_cleanup(OptsVisitor *nv);
-Visitor *opts_get_visitor(OptsVisitor *nv);
+Visitor *opts_visitor_new(const QemuOpts *opts);

 #endif
diff --git a/block/crypto.c b/block/crypto.c
index 5f0ab4d..9bb55d3 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -193,17 +193,16 @@ block_crypto_open_opts_init(QCryptoBlockFormat format,
 QemuOpts *opts,
 Error **errp)
 {
-OptsVisitor *ov;
+Visitor *v;
 QCryptoBlockOpenOptions *ret = NULL;
 Error *local_err = NULL;

 ret = g_new0(QCryptoBlockOpenOptions, 1);
 ret->format = format;

-ov = opts_visitor_new(opts);
+v = opts_visitor_new(opts);

-visit_start_struct(opts_get_visitor(ov),
-   NULL, NULL, 0, _err);
+visit_start_struct(v, NULL, NULL, 0, _err);
 if (local_err) {
 goto out;
 }
@@ -211,7 +210,7 @@ block_crypto_open_opts_init(QCryptoBlockFormat format,
 switch (format) {
 case Q_CRYPTO_BLOCK_FORMAT_LUKS:
 visit_type_QCryptoBlockOptionsLUKS_members(
-opts_get_visitor(ov), >u.luks, _err);
+v, >u.luks, _err);
 break;

 default:
@@ -219,10 +218,10 @@ block_crypto_open_opts_init(QCryptoBlockFormat format,
 break;
 }
 if (!local_err) {
-visit_check_struct(opts_get_visitor(ov), _err);
+visit_check_struct(v, _err);
 }

-visit_end_struct(opts_get_visitor(ov), NULL);
+visit_end_struct(v, NULL);

  out:
 if (local_err) {
@@ -230,7 +229,7 @@ block_crypto_open_opts_init(QCryptoBlockFormat format,
 qapi_free_QCryptoBlockOpenOptions(ret);
 ret = NULL;
 }
-opts_visitor_cleanup(ov);
+visit_free(v);
 return ret;
 }

@@ -240,17 +239,16 @@ block_crypto_create_opts_init(QCryptoBlockFormat format,
   QemuOpts *opts,
   Error **errp)
 {
-OptsVisitor *ov;
+Visitor *v;
 QCryptoBlockCreateOptions *ret = NULL;
 Error *local_err = NULL;

 ret = g_new0(QCryptoBlockCreateOptions, 1);
 ret->format = format;

-ov = opts_visitor_new(opts);
+v = opts_visitor_new(opts);

-visit_start_struct(opts_get_visitor(ov),
-   NULL, NULL, 0, _err);
+visit_start_struct(v, NULL, NULL, 0, _err);
 if (local_err) {
 goto out;
 }
@@ -258,7 +256,7 @@ block_crypto_create_opts_init(QCryptoBlockFormat format,
 switch (format) {
 case Q_CRYPTO_BLOCK_FORMAT_LUKS:
 visit_type_QCryptoBlockCreateOptionsLUKS_members(
-opts_get_visitor(ov), >u.luks, _err);
+v, >u.luks, _err);
 break;

 default:
@@ -266,10 +264,10 @@ block_crypto_create_opts_init(QCryptoBlockFormat format,
 break;
 }
 if (!local_err) {
-visit_check_struct(opts_get_visitor(ov), _err);
+visit_check_struct(v, _err);
 }

-visit_end_struct(opts_get_visitor(ov), NULL);
+visit_end_struct(v, NULL);

  out:
 if (local_err) {
@@ -277,7 +275,7 @@ block_crypto_create_opts_init(QCryptoBlockFormat format,
 qapi_free_QCryptoBlockCreateOptions(ret);
 ret = NULL;
 }
-opts_visitor_cleanup(ov);
+visit_free(v);
 return ret;
 }

diff --git a/hmp.c b/hmp.c
index a4b1d3d..7578ee6 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1718,7 +1718,7 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
 {
 Error *err = NULL;
 QemuOpts *opts;
-OptsVisitor *ov;
+Visitor *v;
 Object *obj = NULL;

 opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, );
@@ -1727,9 +1727,9 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
 return;
 }

-ov = opts_visitor_new(opts);
-obj = user_creatable_add(qdict, opts_get_visitor(ov), );
-opts_visitor_cleanup(ov);
+v = opts_visitor_new(opts);
+obj =

[Qemu-block] [PATCH v5 02/15] qemu-img: Don't leak errors when outputting JSON

2016-06-09 Thread Eric Blake 
If our JSON output ever encounters an error, we would just silently
leak the error object.  Instead, assert that our usage won't fail.

Signed-off-by: Eric Blake 

---
v5: commit message wording tweak
v4: new patch (split out from v3 14/18)
---
 qemu-img.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 251386b..c4a8647 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -483,12 +483,11 @@ fail:

 static void dump_json_image_check(ImageCheck *check, bool quiet)
 {
-Error *local_err = NULL;
 QString *str;
 QmpOutputVisitor *ov = qmp_output_visitor_new();
 QObject *obj;
 visit_type_ImageCheck(qmp_output_get_visitor(ov), NULL, ,
-  _err);
+  _abort);
 obj = qmp_output_get_qobject(ov);
 str = qobject_to_json_pretty(obj);
 assert(str != NULL);
@@ -2174,12 +2173,11 @@ static void dump_snapshots(BlockDriverState *bs)

 static void dump_json_image_info_list(ImageInfoList *list)
 {
-Error *local_err = NULL;
 QString *str;
 QmpOutputVisitor *ov = qmp_output_visitor_new();
 QObject *obj;
 visit_type_ImageInfoList(qmp_output_get_visitor(ov), NULL, ,
- _err);
+ _abort);
 obj = qmp_output_get_qobject(ov);
 str = qobject_to_json_pretty(obj);
 assert(str != NULL);
@@ -2191,11 +2189,11 @@ static void dump_json_image_info_list(ImageInfoList 
*list)

 static void dump_json_image_info(ImageInfo *info)
 {
-Error *local_err = NULL;
 QString *str;
 QmpOutputVisitor *ov = qmp_output_visitor_new();
 QObject *obj;
-visit_type_ImageInfo(qmp_output_get_visitor(ov), NULL, , _err);
+visit_type_ImageInfo(qmp_output_get_visitor(ov), NULL, ,
+ _abort);
 obj = qmp_output_get_qobject(ov);
 str = qobject_to_json_pretty(obj);
 assert(str != NULL);
-- 
2.5.5

Re: [Qemu-block] [Qemu-devel] [PATCH v4 21/28] qstring: Add qstring_wrap_str()

2016-06-09 Thread Eric Blake 
On 06/02/2016 09:21 AM, Markus Armbruster wrote:
> Eric Blake  writes:
> 
>> Several spots in the code malloc a string, then wrap it in a
>> QString, which has to duplicate the input.  Adding a new
>> constructor with transfer semantics makes this pattern more
>> efficient, comparable to the just-added transfer semantics to
>> go from QString back to raw string.  Use the new
>> qstring_wrap_str() where it makes sense.
>>
>> The new test still passes under valgrind.
>>
>> Signed-off-by: Eric Blake 
>>

>> +++ b/qobject/qstring.c
>> @@ -66,6 +66,31 @@ QString *qstring_from_str(const char *str)
>>  return qstring_from_substr(str, 0, strlen(str) - 1);
>>  }
>>
>> +/**
>> + * qstring_wrap_str(): Create a new QString around a malloc'd C string
>> + *
>> + * Returns a strong reference, and caller must not use @str any more.
>> + * @str may be NULL, in which case the QString will be "".
> 
> I'm not fond of conflating null pointers and "" (see also the trouble
> with visit_type_str()).  For what it's worth, qstring_from_str(NULL)
> crashes.  Can we reject null?

Probably. I'll add an assert, and update the interface to match if
things still pass with my usage.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2] blockdev: clarify error on attempt to open locked tray

2016-06-09 Thread Kevin Wolf 
Am 08.06.2016 um 19:56 hat Colin Lord geschrieben:
> When opening a device with a locked tray, gives an error explaining the
> device tray is locked and that the user should wait and try again. This
> is less confusing than the previous error, which simply stated that the
> tray was locked.
> 
> Signed-off-by: Colin Lord 

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [PATCH v2 0/5] qcow2: Implement .bdrv_co_preadv/pwritev

2016-06-09 Thread Kevin Wolf 
Am 06.06.2016 um 16:59 hat Kevin Wolf geschrieben:
> This series converts qcow2 to the byte-based I/O interfaces. This simplifies
> the code by removing many unit conversions, and in the unlikely case of actual
> unaligned requests, it even makes the driver work more efficiently by avoiding
> read-modify-write.

Applied to the block branch.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v5 01/11] qdict: implement a qdict_crumple method for un-flattening a dict

2016-06-09 Thread Daniel P. Berrange 
On Thu, Jun 09, 2016 at 03:20:47PM +0200, Markus Armbruster wrote:
> I apologize for the lateness of this review.
> 
> "Daniel P. Berrange"  writes:
> 
> > The qdict_flatten() method will take a dict whose elements are
> > further nested dicts/lists and flatten them by concatenating
> > keys.
> >
> > The qdict_crumple() method aims to do the reverse, taking a flat
> > qdict, and turning it into a set of nested dicts/lists. It will
> > apply nesting based on the key name, with a '.' indicating a
> > new level in the hierarchy. If the keys in the nested structure
> > are all numeric, it will create a list, otherwise it will create
> > a dict.
> >
> > If the keys are a mixture of numeric and non-numeric, or the
> > numeric keys are not in strictly ascending order, an error will
> > be reported.
> >
> > As an example, a flat dict containing
> >
> >  {
> >'foo.0.bar': 'one',
> >'foo.0.wizz': '1',
> >'foo.1.bar': 'two',
> >'foo.1.wizz': '2'
> >  }
> >
> > will get turned into a dict with one element 'foo' whose
> > value is a list. The list elements will each in turn be
> > dicts.
> >
> >  {
> >'foo': [
> >  { 'bar': 'one', 'wizz': '1' },
> >  { 'bar': 'two', 'wizz': '2' }
> >],
> >  }
> >
> > If the key is intended to contain a literal '.', then it must
> > be escaped as '..'. ie a flat dict
> >
> >   {
> >  'foo..bar': 'wizz',
> >  'bar.foo..bar': 'eek',
> >  'bar.hello': 'world'
> >   }
> >
> > Will end up as
> >
> >   {
> >  'foo.bar': 'wizz',
> >  'bar': {
> > 'foo.bar': 'eek',
> > 'hello': 'world'
> >  }
> >   }
> >
> > The intent of this function is that it allows a set of QemuOpts
> > to be turned into a nested data structure that mirrors the nesting
> > used when the same object is defined over QMP.
> >
> > Signed-off-by: Daniel P. Berrange 
> 
> Not an objection to your patch, just an observation: we're digging
> ourselves ever deeper into the QemuOpts / QDict hole.  We've pushed
> QemuOpts far beyond its original purpose "parse key=value,... option
> arguments".  In my opinion, we'd better parse straight to QAPI-generated
> structs.

NB we're not actually digging the hole any deeper here. The BlockDev
code already fully dug the hole. This code is just generalizing
the code for dealing with the hole (intentionally designed to
100% mirror the syntax that BlockDev already defined), so that we
get consistent handling across the codebase, rather than having
many separate subtley different holes :-)  I have patches that
I've not posted yet, which start to convert some of the blockdev
code over to use this method.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

Re: [Qemu-block] [Qemu-devel] [PATCH v5 01/11] qdict: implement a qdict_crumple method for un-flattening a dict

2016-06-09 Thread Markus Armbruster 
I apologize for the lateness of this review.

"Daniel P. Berrange"  writes:

> The qdict_flatten() method will take a dict whose elements are
> further nested dicts/lists and flatten them by concatenating
> keys.
>
> The qdict_crumple() method aims to do the reverse, taking a flat
> qdict, and turning it into a set of nested dicts/lists. It will
> apply nesting based on the key name, with a '.' indicating a
> new level in the hierarchy. If the keys in the nested structure
> are all numeric, it will create a list, otherwise it will create
> a dict.
>
> If the keys are a mixture of numeric and non-numeric, or the
> numeric keys are not in strictly ascending order, an error will
> be reported.
>
> As an example, a flat dict containing
>
>  {
>'foo.0.bar': 'one',
>'foo.0.wizz': '1',
>'foo.1.bar': 'two',
>'foo.1.wizz': '2'
>  }
>
> will get turned into a dict with one element 'foo' whose
> value is a list. The list elements will each in turn be
> dicts.
>
>  {
>'foo': [
>  { 'bar': 'one', 'wizz': '1' },
>  { 'bar': 'two', 'wizz': '2' }
>],
>  }
>
> If the key is intended to contain a literal '.', then it must
> be escaped as '..'. ie a flat dict
>
>   {
>  'foo..bar': 'wizz',
>  'bar.foo..bar': 'eek',
>  'bar.hello': 'world'
>   }
>
> Will end up as
>
>   {
>  'foo.bar': 'wizz',
>  'bar': {
> 'foo.bar': 'eek',
> 'hello': 'world'
>  }
>   }
>
> The intent of this function is that it allows a set of QemuOpts
> to be turned into a nested data structure that mirrors the nesting
> used when the same object is defined over QMP.
>
> Signed-off-by: Daniel P. Berrange 

Not an objection to your patch, just an observation: we're digging
ourselves ever deeper into the QemuOpts / QDict hole.  We've pushed
QemuOpts far beyond its original purpose "parse key=value,... option
arguments".  In my opinion, we'd better parse straight to QAPI-generated
structs.

> ---
>  include/qapi/qmp/qdict.h |   1 +
>  qobject/qdict.c  | 282 
> +++
>  tests/check-qdict.c  | 229 ++
>  3 files changed, 512 insertions(+)
>
> diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
> index 71b8eb0..8a3ac13 100644
> --- a/include/qapi/qmp/qdict.h
> +++ b/include/qapi/qmp/qdict.h
> @@ -73,6 +73,7 @@ void qdict_flatten(QDict *qdict);
>  void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start);
>  void qdict_array_split(QDict *src, QList **dst);
>  int qdict_array_entries(QDict *src, const char *subqdict);
> +QObject *qdict_crumple(QDict *src, bool recursive, Error **errp);
>  
>  void qdict_join(QDict *dest, QDict *src, bool overwrite);
>  
> diff --git a/qobject/qdict.c b/qobject/qdict.c
> index 60f158c..ad6a563 100644
> --- a/qobject/qdict.c
> +++ b/qobject/qdict.c
> @@ -17,6 +17,7 @@
>  #include "qapi/qmp/qbool.h"
>  #include "qapi/qmp/qstring.h"
>  #include "qapi/qmp/qobject.h"
> +#include "qapi/error.h"
>  #include "qemu/queue.h"
>  #include "qemu-common.h"
>  #include "qemu/cutils.h"
> @@ -683,6 +684,287 @@ void qdict_array_split(QDict *src, QList **dst)
>  }
>  }
>  
> +
> +/**
> + * qdict_split_flat_key:
> + * @key: the key string to split
> + * @prefix: non-NULL pointer to hold extracted prefix
> + * @suffix: non-NULL pointer to hold extracted suffix
> + *
> + * Given a flattened key such as 'foo.0.bar', split it
> + * into two parts at the first '.' separator. Allows
> + * double dot ('..') to escape the normal separator.
> + *
> + * eg
> + *'foo.0.bar' -> prefix='foo' and suffix='0.bar'
> + *'foo..0.bar' -> prefix='foo.0' and suffix='bar'
> + *
> + * The '..' sequence will be unescaped in the returned
> + * 'prefix' string. The 'suffix' string will be left
> + * in escaped format, so it can be fed back into the
> + * qdict_split_flat_key() key as the input later.

Why is the suffix strdup'ed then?

> + *
> + * The caller is responsible for freeing the strings
> + * returned in @prefix and @suffix using g_free().

Wrap comment lines around column 70, please.

> + */
> +static void qdict_split_flat_key(const char *key, char **prefix, char 
> **suffix)
> +{
> +const char *separator;
> +size_t i, j;
> +
> +/* Find first '.' separator, but if there is a pair '..'
> + * that acts as an escape, so skip over '..' */
> +separator = NULL;
> +do {
> +if (separator) {
> +separator += 2;
> +} else {
> +separator = key;
> +}
> +separator = strchr(separator, '.');
> +} while (separator && separator[1] == '.');
> +
> +if (separator) {
> +*prefix = g_strndup(key,
> +separator - key);
> +*suffix = g_strdup(separator + 1);
> +} else {
> +*prefix = g_strdup(key);
> +*suffix = NULL;
> +}
> +
> +/* Unescape the '..' sequence into '.' */
> +for (i = 0, j = 0;

Re: [Qemu-block] [PATCH v2 2/3] block/mirror: Fix target backing BDS

2016-06-09 Thread Nir Soffer 
On Thu, Jun 9, 2016 at 11:58 AM, Kevin Wolf  wrote:
> Am 08.06.2016 um 17:39 hat Nir Soffer geschrieben:
>> On Wed, Jun 8, 2016 at 12:32 PM, Kevin Wolf  wrote:
>> > Am 06.06.2016 um 16:42 hat Max Reitz geschrieben:
>> >> Currently, we are trying to move the backing BDS from the source to the
>> >> target in bdrv_replace_in_backing_chain() which is called from
>> >> mirror_exit(). However, mirror_complete() already tries to open the
>> >> target's backing chain with a call to bdrv_open_backing_file().
>> >>
>> >> First, we should only set the target's backing BDS once. Second, the
>> >> mirroring block job has a better idea of what to set it to than the
>> >> generic code in bdrv_replace_in_backing_chain() (in fact, the latter's
>> >> conditions on when to move the backing BDS from source to target are not
>> >> really correct).
>> >>
>> >> Therefore, remove that code from bdrv_replace_in_backing_chain() and
>> >> leave it to mirror_complete().
>> >>
>> >> However, mirror_complete() in turn pursues a questionable strategy by
>> >> employing bdrv_open_backing_file(): On the one hand, because this may
>> >> open the wrong backing file with drive-mirror in "existing" mode, or
>> >> because it will not override a possibly wrong backing file in the
>> >> blockdev-mirror case.
>> >>
>> >> On the other hand, we want to reuse the existing backing chain of the
>> >> source instead of opening everything anew, because the latter results in
>> >> having multiple BDSs for a single physical file and thus potentially
>> >> concurrent access which we should try to avoid.
>> >
>> > Careful, this "wrong" backing file might actually be intended!
>> >
>> > Consider a case where you want to move an image with its whole backing
>> > chain to different storage. In that case, you would copy all of the
>> > backing files (cp is good enough, they are read-only), create the
>> > destination image which already points at the copied backing chain, and
>> > then mirror in "existing" mode.
>> >
>> > The intention is obviously that after the job completion the new backing
>> > chain is used and not the old one.
>> >
>> > I know that such cases were discussed when mirroring was introduced, I'm
>> > not sure whether it's actually used. We need some input there:
>> >
>> > Eric, can you tell us whether libvirt makes use of such a setup?
>> >
>> > Nir, I'm not sure who is the right person in oVirt these days, but do
>> > you either know yourself whether oVirt requires this to work, or do you
>> > know who else would know?
>>
>> I'm the right person, thanks for keeping me in the loop.
>>
>> What you describe is how we migrate a disk from one storage to another:
>>
>> 1. Create a vm snapshot
>> 2. Create a volume on the destination storage for the snapshot
>> 3. Start mirroring from the source snapshot to the destination snapshot
>> using libvirt virDomainBlockCopy:
>> https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainBlockCopy
>
> With VIR_DOMAIN_BLOCK_COPY_SHALLOW set, right? (That is, sync=top in QMP
> speech.)

Yes, actually we use:

VIR_DOMAIN_BLOCK_COPY_SHALLOW | VIR_DOMAIN_BLOCK_COPY_REUSE_EXT

>> 4. Copy the reset of the chain from source to destination using qemu-img 
>> convert
>> 5. Pivot to the new chain using libvirt virDomainBlockJobAbort
>> 
>> https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainBlockJobAbort
>> 6. Remove the old chain
>>
>> source and target can be files or block device, and we plan to support also
>> rbd and gluster volumes as target, maybe also as source.
>
> Thanks, Nir, we should then do our best not to break it.
>
> Max, maybe we can add a qemu-iotests case that does the exact same thing
> as oVirt does?
>
> Kevin

Re: [Qemu-block] coroutines: block: Co-routine re-entered recursively when migrating disk with iothreads

2016-06-09 Thread Paolo Bonzini 
> BTW this doesn't mean that the blockjobs AioContext following problem
> is solved.  AioContexts can still change if the guest resets the
> virtio device, for example.
> 
> I suspect we'll still hit undefined behavior when that happens.

I agree... :/

Paolo

Re: [Qemu-block] coroutines: block: Co-routine re-entered recursively when migrating disk with iothreads

2016-06-09 Thread Stefan Hajnoczi 
On Thu, Jun 9, 2016 at 9:47 AM, Stefan Hajnoczi  wrote:
> On Thu, Jun 9, 2016 at 9:25 AM, Paolo Bonzini  wrote:
>> On 09/06/2016 09:35, Stefan Hajnoczi wrote:
>>>
>>> if (s->dataplane) {
>>> virtio_blk_data_plane_stop(s->dataplane);
>>> }
>>>
>>> virtio_save(vdev, f);
>>> }
>>>
>>> We reach the situation I described where BDS AioContext changes but
>>> mirror_run() is still in the IOThread AioContext.
>>
>> Why is the virtio_blk_data_plane_stop necessary?  It used to sync
>> between vring and virtio state, but that's not required anymore---and we
>> know that the virtqueues are quiescent at this point...
>
> I agree.  It doesn't seem necessary anymore unless I'm missing a race
> condition with ioeventfd.
>
> It was introduced so that the savevm command (without live migration!) works:
>
> commit 10a06fd65f667a972848ebbbcac11bdba931b544
> Author: Pavel Butsykin 
> Date:   Mon Oct 26 14:42:57 2015 +0300
>
> virtio: sync the dataplane vring state to the virtqueue before virtio_save
>
> We can remove it now.

BTW this doesn't mean that the blockjobs AioContext following problem
is solved.  AioContexts can still change if the guest resets the
virtio device, for example.

I suspect we'll still hit undefined behavior when that happens.

Stefan

Re: [Qemu-block] coroutines: block: Co-routine re-entered recursively when migrating disk with iothreads

2016-06-09 Thread Paolo Bonzini 


On 09/06/2016 09:35, Stefan Hajnoczi wrote:
> 
> if (s->dataplane) {
> virtio_blk_data_plane_stop(s->dataplane);
> }
> 
> virtio_save(vdev, f);
> }
> 
> We reach the situation I described where BDS AioContext changes but
> mirror_run() is still in the IOThread AioContext.

Why is the virtio_blk_data_plane_stop necessary?  It used to sync
between vring and virtio state, but that's not required anymore---and we
know that the virtqueues are quiescent at this point...

Paolo

[Qemu-block] [PATCH 03/15] blockjob: Add block_job_get()

2016-06-09 Thread Alberto Garcia 
Currently the way to look for a specific block job is to iterate the
list manually using block_job_next().

Since we want to be able to identify a job primarily by its ID it
makes sense to have a function that does just that.

Signed-off-by: Alberto Garcia 
---
 blockjob.c   | 13 +
 include/block/blockjob.h | 10 ++
 2 files changed, 23 insertions(+)

diff --git a/blockjob.c b/blockjob.c
index 4d42987..a4a1caf 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -61,6 +61,19 @@ BlockJob *block_job_next(BlockJob *job)
 return QLIST_NEXT(job, job_list);
 }
 
+BlockJob *block_job_get(const char *id)
+{
+BlockJob *job;
+
+QLIST_FOREACH(job, _jobs, job_list) {
+if (!strcmp(id, job->id)) {
+return job;
+}
+}
+
+return NULL;
+}
+
 void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
int64_t speed, BlockCompletionFunc *cb,
void *opaque, Error **errp)
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 1533006..46d2af2 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -191,6 +191,16 @@ struct BlockJob {
 BlockJob *block_job_next(BlockJob *job);
 
 /**
+ * block_job_get:
+ * @id: The id of the block job.
+ *
+ * Get the block job identified by @id (which must not be %NULL).
+ *
+ * Returns the requested job, or %NULL if it doesn't exist.
+ */
+BlockJob *block_job_get(const char *id);
+
+/**
  * block_job_create:
  * @job_type: The class object for the newly-created job.
  * @bs: The block
-- 
2.8.1

[Qemu-block] [PATCH 14/15] blockjob: Add 'id' parameter to 'block-job-complete'

2016-06-09 Thread Alberto Garcia 
This patch allows the 'block-job-complete' command to identify the
job by either its ID or its device name. The latter becomes now
optional since the ID alone is enough to identify the job.

The HMP 'block_job_complete' command remains unchanged.

Signed-off-by: Alberto Garcia 
---
 blockdev.c   | 7 +--
 hmp.c| 2 +-
 qapi/block-core.json | 7 ++-
 qmp-commands.hx  | 2 +-
 4 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 97e5fef..63135d1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3819,10 +3819,13 @@ void qmp_block_job_resume(bool has_id, const char *id,
 aio_context_release(aio_context);
 }
 
-void qmp_block_job_complete(const char *device, Error **errp)
+void qmp_block_job_complete(bool has_id, const char *id,
+bool has_device, const char *device, Error **errp)
 {
 AioContext *aio_context;
-BlockJob *job = find_block_job(NULL, device, _context, errp);
+BlockJob *job = find_block_job(has_id ? id : NULL,
+   has_device ? device : NULL,
+   _context, errp);
 
 if (!job) {
 return;
diff --git a/hmp.c b/hmp.c
index dae1735..a2548f2 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1535,7 +1535,7 @@ void hmp_block_job_complete(Monitor *mon, const QDict 
*qdict)
 Error *error = NULL;
 const char *device = qdict_get_str(qdict, "device");
 
-qmp_block_job_complete(device, );
+qmp_block_job_complete(false, NULL, true, device, );
 
 hmp_handle_error(mon, );
 }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0efa73d..f15e62f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1593,6 +1593,11 @@
 #
 # A cancelled or paused job cannot be completed.
 #
+# The job must be identified with the @id or @device parameters, but
+# only one of them must be set.
+#
+# @id: #optional the job identifier. (Since 2.7)
+#
 # @device: the device name
 #
 # Returns: Nothing on success
@@ -1600,7 +1605,7 @@
 #
 # Since: 1.3
 ##
-{ 'command': 'block-job-complete', 'data': { 'device': 'str' } }
+{ 'command': 'block-job-complete', 'data': { '*id': 'str', '*device': 'str' } }
 
 ##
 # @BlockdevDiscardOptions
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 716d493..d0242dc 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1305,7 +1305,7 @@ EQMP
 },
 {
 .name   = "block-job-complete",
-.args_type  = "device:B",
+.args_type  = "id:s?,device:B?",
 .mhandler.cmd_new = qmp_marshal_block_job_complete,
 },
 {
-- 
2.8.1

[Qemu-block] [PATCH 15/15] blockjob: Add 'id' field to 'BlockJobInfo' and all BLOCK_JOB_* events

2016-06-09 Thread Alberto Garcia 
Now that all jobs have an ID and all QMP commands that create and
operate on block jobs can specify one we can finally expose the ID in
all BLOCK_JOB_* events and the BlockJobInfo structure.

This modifies the output of several iotests, but now we have full
control of the job IDs so this patch updates the affected ones.

Signed-off-by: Alberto Garcia 
---
 blockjob.c |  6 +-
 docs/qmp-events.txt|  4 
 qapi/block-core.json   | 18 --
 tests/qemu-iotests/095 |  2 +-
 tests/qemu-iotests/095.out |  2 +-
 tests/qemu-iotests/124 |  3 ++-
 tests/qemu-iotests/141 |  6 +-
 tests/qemu-iotests/141.out | 14 +++---
 tests/qemu-iotests/144 |  1 +
 tests/qemu-iotests/144.out |  4 ++--
 10 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index dd0eb7f..0b7fe50 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -416,6 +416,7 @@ BlockJobInfo *block_job_query(BlockJob *job)
 {
 BlockJobInfo *info = g_new0(BlockJobInfo, 1);
 info->type  = g_strdup(BlockJobType_lookup[job->driver->job_type]);
+info->id= g_strdup(job->id);
 info->device= g_strdup(job->device);
 info->len   = job->len;
 info->busy  = job->busy;
@@ -438,6 +439,7 @@ static void block_job_iostatus_set_err(BlockJob *job, int 
error)
 void block_job_event_cancelled(BlockJob *job)
 {
 qapi_event_send_block_job_cancelled(job->driver->job_type,
+job->id,
 job->device,
 job->len,
 job->offset,
@@ -448,6 +450,7 @@ void block_job_event_cancelled(BlockJob *job)
 void block_job_event_completed(BlockJob *job, const char *msg)
 {
 qapi_event_send_block_job_completed(job->driver->job_type,
+job->id,
 job->device,
 job->len,
 job->offset,
@@ -462,6 +465,7 @@ void block_job_event_ready(BlockJob *job)
 job->ready = true;
 
 qapi_event_send_block_job_ready(job->driver->job_type,
+job->id,
 job->device,
 job->len,
 job->offset,
@@ -490,7 +494,7 @@ BlockErrorAction block_job_error_action(BlockJob *job, 
BlockdevOnError on_err,
 default:
 abort();
 }
-qapi_event_send_block_job_error(job->device,
+qapi_event_send_block_job_error(job->id, job->device,
 is_read ? IO_OPERATION_TYPE_READ :
 IO_OPERATION_TYPE_WRITE,
 action, _abort);
diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
index fa7574d..2b2af0f 100644
--- a/docs/qmp-events.txt
+++ b/docs/qmp-events.txt
@@ -92,6 +92,7 @@ Data:
 
 - "type": Job type (json-string; "stream" for image streaming
  "commit" for block commit)
+- "id":   Job identifier (json-string)
 - "device":   Device name (json-string)
 - "len":  Maximum progress value (json-int)
 - "offset":   Current progress value (json-int)
@@ -116,6 +117,7 @@ Data:
 
 - "type": Job type (json-string; "stream" for image streaming
  "commit" for block commit)
+- "id":   Job identifier (json-string)
 - "device":   Device name (json-string)
 - "len":  Maximum progress value (json-int)
 - "offset":   Current progress value (json-int)
@@ -143,6 +145,7 @@ Emitted when a block job encounters an error.
 
 Data:
 
+- "id": job identifier (json-string)
 - "device": device name (json-string)
 - "operation": I/O operation (json-string, "read" or "write")
 - "action": action that has been taken, it's one of the following 
(json-string):
@@ -167,6 +170,7 @@ Data:
 
 - "type": Job type (json-string; "stream" for image streaming
  "commit" for block commit)
+- "id":   Job identifier (json-string)
 - "device":   Device name (json-string)
 - "len":  Maximum progress value (json-int)
 - "offset":   Current progress value (json-int)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index f15e62f..b9544d1 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -713,6 +713,8 @@
 #
 # @type: the job type ('stream' for image streaming)
 #
+# @id: the job identifier
+#
 # @device: the block device name
 #
 # @len: the maximum progress value
@@ -734,7 +736,7 @@
 # Since: 1.1
 ##
 { 'struct': 'BlockJobInfo',
-  'data': {'type': 'str', 'device': 'str', 'len': 'int',
+  'data': {'type': 'str', 'id': 'str', 'device': 'str', 'len': 'int',
'offset': 'int', 'busy': 'bool', 'paused': 'bool', 'speed': 'int',
'io-status': 'BlockDeviceIoStatus', 'ready':

[Qemu-block] [PATCH 02/15] blockjob: Decouple the ID from the device name in the BlockJob struct

2016-06-09 Thread Alberto Garcia 
Block jobs are identified by the name of the BlockBackend of the BDS
where the job was started.

We want block jobs to have unique, arbitrary identifiers that are not
tied to a block device, so this patch decouples the ID from the device
name in the BlockJob structure.

The ID is generated automatically for the moment, in later patches
we'll allow the user to set it.

Signed-off-by: Alberto Garcia 
---
 blockjob.c   | 15 +--
 include/block/blockjob.h | 12 
 include/qemu/id.h|  1 +
 util/id.c|  1 +
 4 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index 01b896b..4d42987 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -33,6 +33,7 @@
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qjson.h"
 #include "qemu/coroutine.h"
+#include "qemu/id.h"
 #include "qmp-commands.h"
 #include "qemu/timer.h"
 #include "qapi-event.h"
@@ -82,7 +83,8 @@ void *block_job_create(const BlockJobDriver *driver, 
BlockDriverState *bs,
 bdrv_op_unblock(bs, BLOCK_OP_TYPE_DATAPLANE, job->blocker);
 
 job->driver= driver;
-job->id= g_strdup(bdrv_get_device_name(bs));
+job->device= g_strdup(bdrv_get_device_name(bs));
+job->id= id_generate(ID_JOB);
 job->blk   = blk;
 job->cb= cb;
 job->opaque= opaque;
@@ -119,6 +121,7 @@ void block_job_unref(BlockJob *job)
 bdrv_op_unblock_all(bs, job->blocker);
 blk_unref(job->blk);
 error_free(job->blocker);
+g_free(job->device);
 g_free(job->id);
 QLIST_REMOVE(job, job_list);
 g_free(job);
@@ -389,7 +392,7 @@ BlockJobInfo *block_job_query(BlockJob *job)
 {
 BlockJobInfo *info = g_new0(BlockJobInfo, 1);
 info->type  = g_strdup(BlockJobType_lookup[job->driver->job_type]);
-info->device= g_strdup(job->id);
+info->device= g_strdup(job->device);
 info->len   = job->len;
 info->busy  = job->busy;
 info->paused= job->pause_count > 0;
@@ -411,7 +414,7 @@ static void block_job_iostatus_set_err(BlockJob *job, int 
error)
 void block_job_event_cancelled(BlockJob *job)
 {
 qapi_event_send_block_job_cancelled(job->driver->job_type,
-job->id,
+job->device,
 job->len,
 job->offset,
 job->speed,
@@ -421,7 +424,7 @@ void block_job_event_cancelled(BlockJob *job)
 void block_job_event_completed(BlockJob *job, const char *msg)
 {
 qapi_event_send_block_job_completed(job->driver->job_type,
-job->id,
+job->device,
 job->len,
 job->offset,
 job->speed,
@@ -435,7 +438,7 @@ void block_job_event_ready(BlockJob *job)
 job->ready = true;
 
 qapi_event_send_block_job_ready(job->driver->job_type,
-job->id,
+job->device,
 job->len,
 job->offset,
 job->speed, _abort);
@@ -463,7 +466,7 @@ BlockErrorAction block_job_error_action(BlockJob *job, 
BlockdevOnError on_err,
 default:
 abort();
 }
-qapi_event_send_block_job_error(job->id,
+qapi_event_send_block_job_error(job->device,
 is_read ? IO_OPERATION_TYPE_READ :
 IO_OPERATION_TYPE_WRITE,
 action, _abort);
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 86d2807..1533006 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -85,14 +85,18 @@ struct BlockJob {
 BlockBackend *blk;
 
 /**
- * The ID of the block job. Currently the BlockBackend name of the BDS
- * owning the job at the time when the job is started.
- *
- * TODO Decouple block job IDs from BlockBackend names
+ * The ID of the block job.
  */
 char *id;
 
 /**
+ * BlockBackend name of the BDS owning the job at the time when
+ * the job is started. For compatibility with clients that don't
+ * support the ID field.
+ */
+char *device;
+
+/**
  * The coroutine that executes the job.  If not NULL, it is
  * reentered when busy is false and the job is cancelled.
  */
diff --git a/include/qemu/id.h b/include/qemu/id.h
index 7d90335..c6c73ca 100644
--- a/include/qemu/id.h
+++ b/include/qemu/id.h
@@ -4,6 +4,7 @@
 typedef enum IdSubSystems {
 ID_QDEV,
 ID_BLOCK,
+ID_JOB,
 ID_MAX  /* last element, used as array size */
 } IdSubSystems;
 
diff --git a/util/id.c b/util/id.c

[Qemu-block] [PATCH 05/15] blockjob: Add 'job_id' parameter to block_job_create()

2016-06-09 Thread Alberto Garcia 
Job IDs are generated automatically when a new job is created. This
patch adds a new 'job_id' parameter to let the caller provide one
instead. In this case the ID is verified to be unique and well-formed.

Signed-off-by: Alberto Garcia 
---
 block/backup.c|  3 ++-
 block/commit.c|  2 +-
 block/mirror.c|  2 +-
 block/stream.c|  2 +-
 blockjob.c| 19 +++
 include/block/blockjob.h  |  8 +---
 tests/test-blockjob-txn.c |  2 +-
 7 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index feeb9f8..9245c0c 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -536,7 +536,8 @@ void backup_start(BlockDriverState *bs, BlockDriverState 
*target,
 goto error;
 }
 
-job = block_job_create(_job_driver, bs, speed, cb, opaque, errp);
+job = block_job_create(NULL, _job_driver, bs, speed,
+   cb, opaque, errp);
 if (!job) {
 goto error;
 }
diff --git a/block/commit.c b/block/commit.c
index 444333b..3535ba7 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -236,7 +236,7 @@ void commit_start(BlockDriverState *bs, BlockDriverState 
*base,
 return;
 }
 
-s = block_job_create(_job_driver, bs, speed, cb, opaque, errp);
+s = block_job_create(NULL, _job_driver, bs, speed, cb, opaque, 
errp);
 if (!s) {
 return;
 }
diff --git a/block/mirror.c b/block/mirror.c
index 80fd3c7..28ed447 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -824,7 +824,7 @@ static void mirror_start_job(BlockDriverState *bs, 
BlockDriverState *target,
 buf_size = DEFAULT_MIRROR_BUF_SIZE;
 }
 
-s = block_job_create(driver, bs, speed, cb, opaque, errp);
+s = block_job_create(NULL, driver, bs, speed, cb, opaque, errp);
 if (!s) {
 return;
 }
diff --git a/block/stream.c b/block/stream.c
index c0efbda..e4319d3 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -226,7 +226,7 @@ void stream_start(BlockDriverState *bs, BlockDriverState 
*base,
 {
 StreamBlockJob *s;
 
-s = block_job_create(_job_driver, bs, speed, cb, opaque, errp);
+s = block_job_create(NULL, _job_driver, bs, speed, cb, opaque, 
errp);
 if (!s) {
 return;
 }
diff --git a/blockjob.c b/blockjob.c
index a4a1caf..dd0eb7f 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -74,9 +74,9 @@ BlockJob *block_job_get(const char *id)
 return NULL;
 }
 
-void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
-   int64_t speed, BlockCompletionFunc *cb,
-   void *opaque, Error **errp)
+void *block_job_create(const char *job_id, const BlockJobDriver *driver,
+   BlockDriverState *bs, int64_t speed,
+   BlockCompletionFunc *cb, void *opaque, Error **errp)
 {
 BlockBackend *blk;
 BlockJob *job;
@@ -86,6 +86,17 @@ void *block_job_create(const BlockJobDriver *driver, 
BlockDriverState *bs,
 return NULL;
 }
 
+if (job_id) {
+if (!id_wellformed(job_id)) {
+error_setg(errp, "Invalid job ID '%s'", job_id);
+return NULL;
+}
+if (block_job_get(job_id)) {
+error_setg(errp, "Job ID '%s' already in use", job_id);
+return NULL;
+}
+}
+
 blk = blk_new();
 blk_insert_bs(blk, bs);
 
@@ -97,7 +108,7 @@ void *block_job_create(const BlockJobDriver *driver, 
BlockDriverState *bs,
 
 job->driver= driver;
 job->device= g_strdup(bdrv_get_device_name(bs));
-job->id= id_generate(ID_JOB);
+job->id= job_id ? g_strdup(job_id) : id_generate(ID_JOB);
 job->blk   = blk;
 job->cb= cb;
 job->opaque= opaque;
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 46d2af2..0f2134d 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -202,6 +202,8 @@ BlockJob *block_job_get(const char *id);
 
 /**
  * block_job_create:
+ * @job_id: The id of the newly-created job, or %NULL to have one
+ * generated automatically.
  * @job_type: The class object for the newly-created job.
  * @bs: The block
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
@@ -218,9 +220,9 @@ BlockJob *block_job_get(const char *id);
  * This function is not part of the public job interface; it should be
  * called from a wrapper that is specific to the job type.
  */
-void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
-   int64_t speed, BlockCompletionFunc *cb,
-   void *opaque, Error **errp);
+void *block_job_create(const char *job_id, const BlockJobDriver *driver,
+   BlockDriverState *bs, int64_t speed,
+   BlockCompletionFunc *cb, void *opaque, Error **errp);
 
 /**
  * block_job_sleep_ns:
diff --git

[Qemu-block] [PATCH 06/15] mirror: Add 'job-id' parameter to 'blockdev-mirror' and 'drive-mirror'

2016-06-09 Thread Alberto Garcia 
This patch adds a new optional 'job-id' parameter to 'blockdev-mirror'
and 'drive-mirror', allowing the user to specify the ID of the block
job to be created.

The HMP 'drive_mirror' command remains unchanged.

Signed-off-by: Alberto Garcia 
---
 block/mirror.c| 14 +++---
 blockdev.c| 15 ---
 hmp.c |  2 +-
 include/block/block_int.h |  6 --
 qapi/block-core.json  | 10 +++---
 qmp-commands.hx   |  8 +---
 6 files changed, 32 insertions(+), 23 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 28ed447..ab08da8 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -795,8 +795,8 @@ static const BlockJobDriver commit_active_job_driver = {
 .complete  = mirror_complete,
 };
 
-static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
- const char *replaces,
+static void mirror_start_job(const char *job_id, BlockDriverState *bs,
+ BlockDriverState *target, const char *replaces,
  int64_t speed, uint32_t granularity,
  int64_t buf_size,
  BlockdevOnError on_source_error,
@@ -824,7 +824,7 @@ static void mirror_start_job(BlockDriverState *bs, 
BlockDriverState *target,
 buf_size = DEFAULT_MIRROR_BUF_SIZE;
 }
 
-s = block_job_create(NULL, driver, bs, speed, cb, opaque, errp);
+s = block_job_create(job_id, driver, bs, speed, cb, opaque, errp);
 if (!s) {
 return;
 }
@@ -856,8 +856,8 @@ static void mirror_start_job(BlockDriverState *bs, 
BlockDriverState *target,
 qemu_coroutine_enter(s->common.co, s);
 }
 
-void mirror_start(BlockDriverState *bs, BlockDriverState *target,
-  const char *replaces,
+void mirror_start(const char *job_id, BlockDriverState *bs,
+  BlockDriverState *target, const char *replaces,
   int64_t speed, uint32_t granularity, int64_t buf_size,
   MirrorSyncMode mode, BlockdevOnError on_source_error,
   BlockdevOnError on_target_error,
@@ -874,7 +874,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState 
*target,
 }
 is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
 base = mode == MIRROR_SYNC_MODE_TOP ? backing_bs(bs) : NULL;
-mirror_start_job(bs, target, replaces,
+mirror_start_job(job_id, bs, target, replaces,
  speed, granularity, buf_size,
  on_source_error, on_target_error, unmap, cb, opaque, errp,
  _job_driver, is_none_mode, base);
@@ -922,7 +922,7 @@ void commit_active_start(BlockDriverState *bs, 
BlockDriverState *base,
 }
 }
 
-mirror_start_job(bs, base, NULL, speed, 0, 0,
+mirror_start_job(NULL, bs, base, NULL, speed, 0, 0,
  on_error, on_error, false, cb, opaque, _err,
  _active_job_driver, false, base);
 if (local_err) {
diff --git a/blockdev.c b/blockdev.c
index bd0d5a1..b05dfff 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3412,7 +3412,7 @@ void qmp_blockdev_backup(const char *device, const char 
*target,
 /* Parameter check and block job starting for drive mirroring.
  * Caller should hold @device and @target's aio context (must be the same).
  **/
-static void blockdev_mirror_common(BlockDriverState *bs,
+static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
BlockDriverState *target,
bool has_replaces, const char *replaces,
enum MirrorSyncMode sync,
@@ -3471,15 +3471,15 @@ static void blockdev_mirror_common(BlockDriverState *bs,
 /* pass the node name to replace to mirror start since it's loose coupling
  * and will allow to check whether the node still exist at mirror 
completion
  */
-mirror_start(bs, target,
+mirror_start(job_id, bs, target,
  has_replaces ? replaces : NULL,
  speed, granularity, buf_size, sync,
  on_source_error, on_target_error, unmap,
  block_job_cb, bs, errp);
 }
 
-void qmp_drive_mirror(const char *device, const char *target,
-  bool has_format, const char *format,
+void qmp_drive_mirror(bool has_job_id, const char *job_id, const char *device,
+  const char *target, bool has_format, const char *format,
   bool has_node_name, const char *node_name,
   bool has_replaces, const char *replaces,
   enum MirrorSyncMode sync,
@@ -3616,7 +3616,7 @@ void qmp_drive_mirror(const char *device, const char 
*target,
 
 bdrv_set_aio_context(target_bs, aio_context);
 
-blockdev_mirror_common(bs, target_bs,
+blockdev_mirror_common(has_job_id ? job_id : NULL, bs, target_bs,

[Qemu-block] [PATCH 13/15] blockjob: Add 'id' parameter to 'block-job-resume'

2016-06-09 Thread Alberto Garcia 
This patch allows the 'block-job-resume' command to identify the
job by either its ID or its device name. The latter becomes now
optional since the ID alone is enough to identify the job.

The HMP 'block_job_resume' command remains unchanged.

Signed-off-by: Alberto Garcia 
---
 blockdev.c   | 7 +--
 hmp.c| 2 +-
 qapi/block-core.json | 7 ++-
 qmp-commands.hx  | 2 +-
 4 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 591f163..97e5fef 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3801,10 +3801,13 @@ void qmp_block_job_pause(bool has_id, const char *id,
 aio_context_release(aio_context);
 }
 
-void qmp_block_job_resume(const char *device, Error **errp)
+void qmp_block_job_resume(bool has_id, const char *id,
+  bool has_device, const char *device, Error **errp)
 {
 AioContext *aio_context;
-BlockJob *job = find_block_job(NULL, device, _context, errp);
+BlockJob *job = find_block_job(has_id ? id : NULL,
+   has_device ? device : NULL,
+   _context, errp);
 
 if (!job || !job->user_paused) {
 return;
diff --git a/hmp.c b/hmp.c
index 783c3af..dae1735 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1525,7 +1525,7 @@ void hmp_block_job_resume(Monitor *mon, const QDict 
*qdict)
 Error *error = NULL;
 const char *device = qdict_get_str(qdict, "device");
 
-qmp_block_job_resume(device, );
+qmp_block_job_resume(false, NULL, true, device, );
 
 hmp_handle_error(mon, );
 }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0eaf531..0efa73d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1562,6 +1562,11 @@
 #
 # This command also clears the error status of the job.
 #
+# The job must be identified with the @id or @device parameters, but
+# only one of them must be set.
+#
+# @id: #optional the job identifier. (Since 2.7)
+#
 # @device: the device name
 #
 # Returns: Nothing on success
@@ -1569,7 +1574,7 @@
 #
 # Since: 1.3
 ##
-{ 'command': 'block-job-resume', 'data': { 'device': 'str' } }
+{ 'command': 'block-job-resume', 'data': { '*id': 'str', '*device': 'str' } }
 
 ##
 # @block-job-complete:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 571feb6..716d493 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1300,7 +1300,7 @@ EQMP
 },
 {
 .name   = "block-job-resume",
-.args_type  = "device:B",
+.args_type  = "id:s?,device:B?",
 .mhandler.cmd_new = qmp_marshal_block_job_resume,
 },
 {
-- 
2.8.1

[Qemu-block] [PATCH 07/15] backup: Add 'job-id' parameter to 'blockdev-backup' and 'drive-backup'

2016-06-09 Thread Alberto Garcia 
This patch adds a new optional 'job-id' parameter to 'blockdev-backup'
and 'drive-backup', allowing the user to specify the ID of the block
job to be created.

The HMP 'drive_backup' command remains unchanged.

Signed-off-by: Alberto Garcia 
---
 block/backup.c|  8 
 blockdev.c| 43 ---
 hmp.c |  2 +-
 include/block/block_int.h |  8 +---
 qapi/block-core.json  | 10 +++---
 qmp-commands.hx   |  8 +---
 6 files changed, 46 insertions(+), 33 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 9245c0c..501ae76 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -468,9 +468,9 @@ static void coroutine_fn backup_run(void *opaque)
 block_job_defer_to_main_loop(>common, backup_complete, data);
 }
 
-void backup_start(BlockDriverState *bs, BlockDriverState *target,
-  int64_t speed, MirrorSyncMode sync_mode,
-  BdrvDirtyBitmap *sync_bitmap,
+void backup_start(const char *job_id, BlockDriverState *bs,
+  BlockDriverState *target, int64_t speed,
+  MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap,
   BlockdevOnError on_source_error,
   BlockdevOnError on_target_error,
   BlockCompletionFunc *cb, void *opaque,
@@ -536,7 +536,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState 
*target,
 goto error;
 }
 
-job = block_job_create(NULL, _job_driver, bs, speed,
+job = block_job_create(job_id, _job_driver, bs, speed,
cb, opaque, errp);
 if (!job) {
 goto error;
diff --git a/blockdev.c b/blockdev.c
index b05dfff..4bf1cb3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1834,9 +1834,9 @@ typedef struct DriveBackupState {
 BlockJob *job;
 } DriveBackupState;
 
-static void do_drive_backup(const char *device, const char *target,
-bool has_format, const char *format,
-enum MirrorSyncMode sync,
+static void do_drive_backup(const char *job_id, const char *device,
+const char *target, bool has_format,
+const char *format, enum MirrorSyncMode sync,
 bool has_mode, enum NewImageMode mode,
 bool has_speed, int64_t speed,
 bool has_bitmap, const char *bitmap,
@@ -1874,7 +1874,8 @@ static void drive_backup_prepare(BlkActionState *common, 
Error **errp)
 bdrv_drained_begin(blk_bs(blk));
 state->bs = blk_bs(blk);
 
-do_drive_backup(backup->device, backup->target,
+do_drive_backup(backup->has_job_id ? backup->job_id : NULL,
+backup->device, backup->target,
 backup->has_format, backup->format,
 backup->sync,
 backup->has_mode, backup->mode,
@@ -1919,8 +1920,8 @@ typedef struct BlockdevBackupState {
 AioContext *aio_context;
 } BlockdevBackupState;
 
-static void do_blockdev_backup(const char *device, const char *target,
-   enum MirrorSyncMode sync,
+static void do_blockdev_backup(const char *job_id, const char *device,
+   const char *target, enum MirrorSyncMode sync,
bool has_speed, int64_t speed,
bool has_on_source_error,
BlockdevOnError on_source_error,
@@ -1966,8 +1967,8 @@ static void blockdev_backup_prepare(BlkActionState 
*common, Error **errp)
 state->bs = blk_bs(blk);
 bdrv_drained_begin(state->bs);
 
-do_blockdev_backup(backup->device, backup->target,
-   backup->sync,
+do_blockdev_backup(backup->has_job_id ? backup->job_id : NULL,
+   backup->device, backup->target, backup->sync,
backup->has_speed, backup->speed,
backup->has_on_source_error, backup->on_source_error,
backup->has_on_target_error, backup->on_target_error,
@@ -3175,9 +3176,9 @@ out:
 aio_context_release(aio_context);
 }
 
-static void do_drive_backup(const char *device, const char *target,
-bool has_format, const char *format,
-enum MirrorSyncMode sync,
+static void do_drive_backup(const char *job_id, const char *device,
+const char *target, bool has_format,
+const char *format, enum MirrorSyncMode sync,
 bool has_mode, enum NewImageMode mode,
 bool has_speed, int64_t speed,
 bool has_bitmap, const char *bitmap,
@@ -3296,7 +3297,7 @@ static void do_drive_backup(const char *device, const 
char *target,
 }
 }
 
-backup_start(bs, target_bs, speed, sync,

[Qemu-block] [PATCH 04/15] block: Simplify find_block_job() and make it accept a job ID

2016-06-09 Thread Alberto Garcia 
find_block_job() looks for a block backend with a specified name,
checks whether it has a block job and acquires its AioContext. This
patch uses block_job_next() and iterate directly over the block jobs.

In addition to that we want to identify jobs primarily by their ID, so
this patch updates find_block_job() to allow IDs too. Only one of ID
and device name can be specified when looking for a block job.

Signed-off-by: Alberto Garcia 
---
 blockdev.c | 66 +-
 1 file changed, 35 insertions(+), 31 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 52ec4ae..bd0d5a1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3689,48 +3689,52 @@ void qmp_blockdev_mirror(const char *device, const char 
*target,
 aio_context_release(aio_context);
 }
 
-/* Get the block job for a given device name and acquire its AioContext */
-static BlockJob *find_block_job(const char *device, AioContext **aio_context,
-Error **errp)
+/* Get a block job using its ID or device name and acquire its AioContext */
+static BlockJob *find_block_job(const char *id, const char *device,
+AioContext **aio_context, Error **errp)
 {
-BlockBackend *blk;
-BlockDriverState *bs;
+BlockJob *job = NULL;
 
 *aio_context = NULL;
 
-blk = blk_by_name(device);
-if (!blk) {
-goto notfound;
+if ((id && device) || (!id && !device)) {
+error_setg(errp, "Only one of ID or device name "
+   "must be specified when looking for a block job");
+return NULL;
 }
 
-*aio_context = blk_get_aio_context(blk);
-aio_context_acquire(*aio_context);
-
-if (!blk_is_available(blk)) {
-goto notfound;
+if (id) {
+job = block_job_get(id);
+} else {
+BlockJob *j;
+for (j = block_job_next(NULL); j; j = block_job_next(j)) {
+if (!strcmp(device, j->device)) {
+if (job) {
+/* This scenario is currently not possible, but it
+ * could theoretically happen in the future. */
+error_setg(errp, "More than one job on device '%s', "
+   "use the job ID instead", device);
+return NULL;
+}
+job = j;
+}
+}
 }
-bs = blk_bs(blk);
 
-if (!bs->job) {
-goto notfound;
+if (job) {
+*aio_context = blk_get_aio_context(job->blk);
+aio_context_acquire(*aio_context);
+} else {
+error_setg(errp, "Block job '%s' not found", id ? id : device);
 }
 
-return bs->job;
-
-notfound:
-error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE,
-  "No active block job on device '%s'", device);
-if (*aio_context) {
-aio_context_release(*aio_context);
-*aio_context = NULL;
-}
-return NULL;
+return job;
 }
 
 void qmp_block_job_set_speed(const char *device, int64_t speed, Error **errp)
 {
 AioContext *aio_context;
-BlockJob *job = find_block_job(device, _context, errp);
+BlockJob *job = find_block_job(NULL, device, _context, errp);
 
 if (!job) {
 return;
@@ -3744,7 +3748,7 @@ void qmp_block_job_cancel(const char *device,
   bool has_force, bool force, Error **errp)
 {
 AioContext *aio_context;
-BlockJob *job = find_block_job(device, _context, errp);
+BlockJob *job = find_block_job(NULL, device, _context, errp);
 
 if (!job) {
 return;
@@ -3769,7 +3773,7 @@ out:
 void qmp_block_job_pause(const char *device, Error **errp)
 {
 AioContext *aio_context;
-BlockJob *job = find_block_job(device, _context, errp);
+BlockJob *job = find_block_job(NULL, device, _context, errp);
 
 if (!job || job->user_paused) {
 return;
@@ -3784,7 +3788,7 @@ void qmp_block_job_pause(const char *device, Error **errp)
 void qmp_block_job_resume(const char *device, Error **errp)
 {
 AioContext *aio_context;
-BlockJob *job = find_block_job(device, _context, errp);
+BlockJob *job = find_block_job(NULL, device, _context, errp);
 
 if (!job || !job->user_paused) {
 return;
@@ -3799,7 +3803,7 @@ void qmp_block_job_resume(const char *device, Error 
**errp)
 void qmp_block_job_complete(const char *device, Error **errp)
 {
 AioContext *aio_context;
-BlockJob *job = find_block_job(device, _context, errp);
+BlockJob *job = find_block_job(NULL, device, _context, errp);
 
 if (!job) {
 return;
-- 
2.8.1

[Qemu-block] [PATCH 00/15] Add an 'id' field to block jobs

2016-06-09 Thread Alberto Garcia 
Hello all,

Block jobs are currently identified by the name of the block backend
of the BDS where the job was started.

This series adds a new 'id' field that is specific to the block job
and guaranteed to be unique and always present.

The patches can be summarized as follows:

- 1: Fix the prototype of stream_start(), that is going to be touched
 by a later patch in the series.
- 2-5: Split BlockJob's 'id' field into 'id' and 'device' and adjust
   the internal APIs to allow using the job ID.
- 6-9: Add parameters to allow setting the job ID to all QMP commands
   that create block jobs (block-stream, block-commit, ...)
- 10-14: Add parameters to allow using the job ID to all QMP command
 that manipulate block jobs (cancel, pause, complete, ...).
- 15: Expose the ID field in BlockJobInfo and all BLOCK_JOB_* events.

Things to do:

- Add new tests specific to job IDs.
- Add job ID support to HMP commands.

The series applies on top of Max's block branch (commit 3cc2f35a59)
but it should work on master as well.

Questions, comments, etc, are welcome.

Thanks!

Berto

Alberto Garcia (15):
  stream: Fix prototype of stream_start()
  blockjob: Decouple the ID from the device name in the BlockJob struct
  blockjob: Add block_job_get()
  block: Simplify find_block_job() and make it accept a job ID
  blockjob: Add 'job_id' parameter to block_job_create()
  mirror: Add 'job-id' parameter to 'blockdev-mirror' and 'drive-mirror'
  backup: Add 'job-id' parameter to 'blockdev-backup' and 'drive-backup'
  stream: Add 'job-id' parameter to 'block-stream'
  stream: Add 'job-id' parameter to 'block-commit'
  blockjob: Add 'id' parameter to 'block-job-set-speed'
  blockjob: Add 'id' parameter to 'block-job-cancel'
  blockjob: Add 'id' parameter to 'block-job-pause'
  blockjob: Add 'id' parameter to 'block-job-resume'
  blockjob: Add 'id' parameter to 'block-job-complete'
  blockjob: Add 'id' field to 'BlockJobInfo' and all BLOCK_JOB_* events

 block/backup.c |   9 +--
 block/commit.c |   7 +-
 block/mirror.c |  18 ++---
 block/stream.c |  12 ++--
 blockdev.c | 164 ++---
 blockjob.c |  43 ++--
 docs/qmp-events.txt|   4 ++
 hmp.c  |  16 ++---
 include/block/block_int.h  |  47 -
 include/block/blockjob.h   |  30 +++--
 include/qemu/id.h  |   1 +
 qapi/block-core.json   |  91 +++--
 qemu-img.c |   2 +-
 qmp-commands.hx|  32 +
 tests/qemu-iotests/095 |   2 +-
 tests/qemu-iotests/095.out |   2 +-
 tests/qemu-iotests/124 |   3 +-
 tests/qemu-iotests/141 |   6 +-
 tests/qemu-iotests/141.out |  14 ++--
 tests/qemu-iotests/144 |   1 +
 tests/qemu-iotests/144.out |   4 +-
 tests/test-blockjob-txn.c  |   2 +-
 util/id.c  |   1 +
 23 files changed, 334 insertions(+), 177 deletions(-)

-- 
2.8.1

[Qemu-block] [PATCH 11/15] blockjob: Add 'id' parameter to 'block-job-cancel'

2016-06-09 Thread Alberto Garcia 
This patch allows the 'block-job-cancel' command to identify the
job by either its ID or its device name. The latter becomes now
optional since the ID alone is enough to identify the job.

The HMP 'block_job_cancel' command remains unchanged.

Signed-off-by: Alberto Garcia 
---
 blockdev.c   |  7 +--
 hmp.c|  2 +-
 qapi/block-core.json | 10 --
 qmp-commands.hx  |  2 +-
 4 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 5aaa429..97041b4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3754,11 +3754,14 @@ void qmp_block_job_set_speed(bool has_id, const char 
*id, bool has_device,
 aio_context_release(aio_context);
 }
 
-void qmp_block_job_cancel(const char *device,
+void qmp_block_job_cancel(bool has_id, const char *id,
+  bool has_device, const char *device,
   bool has_force, bool force, Error **errp)
 {
 AioContext *aio_context;
-BlockJob *job = find_block_job(NULL, device, _context, errp);
+BlockJob *job = find_block_job(has_id ? id : NULL,
+   has_device ? device : NULL,
+   _context, errp);
 
 if (!job) {
 return;
diff --git a/hmp.c b/hmp.c
index 0bf5558..adeb4de 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1505,7 +1505,7 @@ void hmp_block_job_cancel(Monitor *mon, const QDict 
*qdict)
 const char *device = qdict_get_str(qdict, "device");
 bool force = qdict_get_try_bool(qdict, "force", false);
 
-qmp_block_job_cancel(device, true, force, );
+qmp_block_job_cancel(false, NULL, true, device, true, force, );
 
 hmp_handle_error(mon, );
 }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 64038cc..38dd3f2 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1505,7 +1505,12 @@
 # operation can be started at a later time to finish copying all data from the
 # backing file.
 #
-# @device: the device name
+# The job must be identified with the @id or @device parameters, but
+# only one of them must be set.
+#
+# @id: #optional the job identifier (Since 2.7)
+#
+# @device: #optional the device name
 #
 # @force: #optional whether to allow cancellation of a paused job (default
 # false).  Since 1.3.
@@ -1515,7 +1520,8 @@
 #
 # Since: 1.1
 ##
-{ 'command': 'block-job-cancel', 'data': { 'device': 'str', '*force': 'bool' } 
}
+{ 'command': 'block-job-cancel',
+  'data': { '*id': 'str', '*device': 'str', '*force': 'bool' } }
 
 ##
 # @block-job-pause:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index bbbab53..6bf8c3d 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1290,7 +1290,7 @@ EQMP
 
 {
 .name   = "block-job-cancel",
-.args_type  = "device:B,force:b?",
+.args_type  = "id:s?,device:B?,force:b?",
 .mhandler.cmd_new = qmp_marshal_block_job_cancel,
 },
 {
-- 
2.8.1

[Qemu-block] [PATCH 10/15] blockjob: Add 'id' parameter to 'block-job-set-speed'

2016-06-09 Thread Alberto Garcia 
This patch allows the 'block-job-set-speed' command to identify the
job by either its ID or its device name. The latter becomes now
optional since the ID alone is enough to identify the job.

The HMP 'block_job_set_speed' command remains unchanged.

Signed-off-by: Alberto Garcia 
---
 blockdev.c   | 7 +--
 hmp.c| 2 +-
 qapi/block-core.json | 9 +++--
 qmp-commands.hx  | 2 +-
 4 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 0f4c433..5aaa429 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3738,10 +3738,13 @@ static BlockJob *find_block_job(const char *id, const 
char *device,
 return job;
 }
 
-void qmp_block_job_set_speed(const char *device, int64_t speed, Error **errp)
+void qmp_block_job_set_speed(bool has_id, const char *id, bool has_device,
+ const char *device, int64_t speed, Error **errp)
 {
 AioContext *aio_context;
-BlockJob *job = find_block_job(NULL, device, _context, errp);
+BlockJob *job = find_block_job(has_id ? id : NULL,
+   has_device ? device : NULL,
+   _context, errp);
 
 if (!job) {
 return;
diff --git a/hmp.c b/hmp.c
index 414a41a..0bf5558 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1494,7 +1494,7 @@ void hmp_block_job_set_speed(Monitor *mon, const QDict 
*qdict)
 const char *device = qdict_get_str(qdict, "device");
 int64_t value = qdict_get_int(qdict, "speed");
 
-qmp_block_job_set_speed(device, value, );
+qmp_block_job_set_speed(false, NULL, true, device, value, );
 
 hmp_handle_error(mon, );
 }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index f754c29..64038cc 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1469,7 +1469,12 @@
 #
 # Throttling can be disabled by setting the speed to 0.
 #
-# @device: the device name
+# The job must be identified with the @id or @device parameters, but
+# only one of them must be set.
+#
+# @id: #optional the job identifier. (Since 2.7)
+#
+# @device: #optional the device name.
 #
 # @speed:  the maximum speed, in bytes per second, or 0 for unlimited.
 #  Defaults to 0.
@@ -1480,7 +1485,7 @@
 # Since: 1.1
 ##
 { 'command': 'block-job-set-speed',
-  'data': { 'device': 'str', 'speed': 'int' } }
+  'data': { '*id': 'str', '*device': 'str', 'speed': 'int' } }
 
 ##
 # @block-job-cancel:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 5f801a9..bbbab53 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1284,7 +1284,7 @@ EQMP
 
 {
 .name   = "block-job-set-speed",
-.args_type  = "device:B,speed:o",
+.args_type  = "id:s?,device:B?,speed:o",
 .mhandler.cmd_new = qmp_marshal_block_job_set_speed,
 },
 
-- 
2.8.1

[Qemu-block] [PATCH 08/15] stream: Add 'job-id' parameter to 'block-stream'

2016-06-09 Thread Alberto Garcia 
This patch adds a new optional 'job-id' parameter to 'block-stream',
allowing the user to specify the ID of the block job to be created.

The HMP 'block_stream' command remains unchanged.

Signed-off-by: Alberto Garcia 
---
 block/stream.c| 12 ++--
 blockdev.c|  6 +++---
 hmp.c |  2 +-
 include/block/block_int.h | 10 ++
 qapi/block-core.json  |  7 +--
 qmp-commands.hx   |  3 ++-
 6 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index e4319d3..54c8cd8 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -218,15 +218,15 @@ static const BlockJobDriver stream_job_driver = {
 .set_speed = stream_set_speed,
 };
 
-void stream_start(BlockDriverState *bs, BlockDriverState *base,
-  const char *backing_file_str, int64_t speed,
-  BlockdevOnError on_error,
-  BlockCompletionFunc *cb,
-  void *opaque, Error **errp)
+void stream_start(const char *job_id, BlockDriverState *bs,
+  BlockDriverState *base, const char *backing_file_str,
+  int64_t speed, BlockdevOnError on_error,
+  BlockCompletionFunc *cb, void *opaque, Error **errp)
 {
 StreamBlockJob *s;
 
-s = block_job_create(NULL, _job_driver, bs, speed, cb, opaque, 
errp);
+s = block_job_create(job_id, _job_driver, bs, speed,
+ cb, opaque, errp);
 if (!s) {
 return;
 }
diff --git a/blockdev.c b/blockdev.c
index 4bf1cb3..f4ff025 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2998,7 +2998,7 @@ static void block_job_cb(void *opaque, int ret)
 }
 }
 
-void qmp_block_stream(const char *device,
+void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
   bool has_base, const char *base,
   bool has_backing_file, const char *backing_file,
   bool has_speed, int64_t speed,
@@ -3057,8 +3057,8 @@ void qmp_block_stream(const char *device,
 /* backing_file string overrides base bs filename */
 base_name = has_backing_file ? backing_file : base_name;
 
-stream_start(bs, base_bs, base_name, has_speed ? speed : 0,
- on_error, block_job_cb, bs, _err);
+stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
+ has_speed ? speed : 0, on_error, block_job_cb, bs, 
_err);
 if (local_err) {
 error_propagate(errp, local_err);
 goto out;
diff --git a/hmp.c b/hmp.c
index 1918750..414a41a 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1481,7 +1481,7 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
 const char *base = qdict_get_try_str(qdict, "base");
 int64_t speed = qdict_get_try_int(qdict, "speed", 0);
 
-qmp_block_stream(device, base != NULL, base, false, NULL,
+qmp_block_stream(false, NULL, device, base != NULL, base, false, NULL,
  qdict_haskey(qdict, "speed"), speed,
  true, BLOCKDEV_ON_ERROR_REPORT, );
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index aa67d29..9693cf4 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -607,6 +607,8 @@ int is_windows_drive(const char *filename);
 
 /**
  * stream_start:
+ * @job_id: The id of the newly-created job, or %NULL to have one
+ * generated automatically.
  * @bs: Block device to operate on.
  * @base: Block device that will become the new base, or %NULL to
  * flatten the whole backing file chain onto @bs.
@@ -625,10 +627,10 @@ int is_windows_drive(const char *filename);
  * @backing_file_str in the written image and to @base in the live
  * BlockDriverState.
  */
-void stream_start(BlockDriverState *bs, BlockDriverState *base,
-  const char *backing_file_str, int64_t speed,
-  BlockdevOnError on_error, BlockCompletionFunc *cb,
-  void *opaque, Error **errp);
+void stream_start(const char *job_id, BlockDriverState *bs,
+  BlockDriverState *base, const char *backing_file_str,
+  int64_t speed, BlockdevOnError on_error,
+  BlockCompletionFunc *cb, void *opaque, Error **errp);
 
 /**
  * commit_start:
diff --git a/qapi/block-core.json b/qapi/block-core.json
index e30e38d..14073f7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1421,6 +1421,8 @@
 # On successful completion the image file is updated to drop the backing file
 # and the BLOCK_JOB_COMPLETED event is emitted.
 #
+# @job-id: #optional identifier for the newly-created block job (Since 2.7)
+#
 # @device: the device name
 #
 # @base:   #optional the common backing file name
@@ -1452,8 +1454,9 @@
 # Since: 1.1
 ##
 { 'command': 'block-stream',
-  'data': { 'device': 'str', '*base': 'str', '*backing-file': 'str',
-'*speed': 'int', '*on-error': 'BlockdevOnError' } }
+  'data': { '*job-id': 'str',

Re: [Qemu-block] [Qemu-devel] coroutines: block: Co-routine re-entered recursively when migrating disk with iothreads

2016-06-09 Thread Stefan Hajnoczi 
On Wed, Jun 8, 2016 at 5:03 PM, Paolo Bonzini  wrote:
> On 08/06/2016 17:30, Stefan Hajnoczi wrote:
>> This needs to be fixed.  I believe Paolo has a patch to continue using
>> dataplane during savevm but that's a workaround rather than a solution
>> to this problem.
>
> That is already in.  Dataplane during migration has been enabled since
> we've gotten rid of vring.

You're right.  This means the problem isn't that dataplane disables
itself during the live migration phase.

The issue occurs because the mirror block job has finished syncing but
is still alive when vmsave happens:

static void virtio_blk_save(QEMUFile *f, void *opaque)
{
VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
VirtIOBlock *s = VIRTIO_BLK(vdev);

if (s->dataplane) {
virtio_blk_data_plane_stop(s->dataplane);
}

virtio_save(vdev, f);
}

We reach the situation I described where BDS AioContext changes but
mirror_run() is still in the IOThread AioContext.

Stefan