Re: [Qemu-devel] [PATCH alt 1/7] block: Add status callback to bdrv_amend_options()

2014-08-01 Thread Max Reitz 

On 31.07.2014 09:51, Benoît Canet wrote:

The Saturday 26 Jul 2014 à 21:22:05 (+0200), Max Reitz wrote :


Depending on the changed options and the image format,
bdrv_amend_options() may take a significant amount of time. In these
cases, a way to be informed about the operation's status is desirable.

Since the operation is rather complex and may fundamentally change the
image, implementing it as AIO or a couroutine does not seem feasible. On
the other hand, implementing it as a block job would be significantly
more difficult than a simple callback and would not add benefits other
than progress report to the amending operation, because it should not
actually be run as a block job at all.

A callback may not be very pretty, but it's very easy to implement and
perfectly fits its purpose here.

Signed-off-by: Max Reitz mre...@redhat.com
---
  block.c   | 5 +++--
  block/qcow2.c | 5 -
  include/block/block.h | 5 -
  include/block/block_int.h | 3 ++-
  qemu-img.c| 2 +-
  5 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 3e252a2..4c8d4d8 100644
--- a/block.c
+++ b/block.c
@@ -5708,12 +5708,13 @@ void bdrv_add_before_write_notifier(BlockDriverState 
*bs,
  notifier_with_return_list_add(bs-before_write_notifiers, notifier);
  }
  
-int bdrv_amend_options(BlockDriverState *bs, QemuOpts *opts)

+int bdrv_amend_options(BlockDriverState *bs, QemuOpts *opts,
+   BlockDriverAmendStatusCB *status_cb)
  {
  if (!bs-drv-bdrv_amend_options) {
  return -ENOTSUP;
  }
-return bs-drv-bdrv_amend_options(bs, opts);
+return bs-drv-bdrv_amend_options(bs, opts, status_cb);
  }
  
  /* This function will be called by the bdrv_recurse_is_first_non_filter method

diff --git a/block/qcow2.c b/block/qcow2.c
index b0faa69..757f890 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2210,7 +2210,8 @@ static int qcow2_downgrade(BlockDriverState *bs, int 
target_version)
  return 0;
  }
  
-static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts)

+static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
+   BlockDriverAmendStatusCB *status_cb)
  {
  BDRVQcowState *s = bs-opaque;
  int old_version = s-qcow_version, new_version = old_version;
@@ -2223,6 +2224,8 @@ static int qcow2_amend_options(BlockDriverState *bs, 
QemuOpts *opts)
  int ret;
  QemuOptDesc *desc = opts-list-desc;
  
+(void)status_cb;

This look like a temporary hack to please the compiler.
Am I right ? Should be comment this ?


It's removed in one of the next patches anyway; but I'll add a comment 
in v2.


Max


+
  while (desc  desc-name) {
  if (!qemu_opt_find(opts, desc-name)) {
  /* only change explicitly defined options */
diff --git a/include/block/block.h b/include/block/block.h
index 32d3676..f2b1799 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -309,7 +309,10 @@ typedef enum {
  
  int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
  
-int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts);

+typedef void BlockDriverAmendStatusCB(BlockDriverState *bs, int64_t offset,
+  int64_t total_work_size);
+int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts,
+   BlockDriverAmendStatusCB *status_cb);
  
  /* external snapshots */

  bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index f6c3bef..5c5798d 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -228,7 +228,8 @@ struct BlockDriver {
  int (*bdrv_check)(BlockDriverState* bs, BdrvCheckResult *result,
  BdrvCheckMode fix);
  
-int (*bdrv_amend_options)(BlockDriverState *bs, QemuOpts *opts);

+int (*bdrv_amend_options)(BlockDriverState *bs, QemuOpts *opts,
+  BlockDriverAmendStatusCB *status_cb);
  
  void (*bdrv_debug_event)(BlockDriverState *bs, BlkDebugEvent event);
  
diff --git a/qemu-img.c b/qemu-img.c

index ef74ae4..90d6b79 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2827,7 +2827,7 @@ static int img_amend(int argc, char **argv)
  goto out;
  }
  
-ret = bdrv_amend_options(bs, opts);

+ret = bdrv_amend_options(bs, opts, NULL);
  if (ret  0) {
  error_report(Error while amending options: %s, strerror(-ret));
  goto out;
--
2.0.3

Re: [Qemu-devel] [PATCH alt 1/7] block: Add status callback to bdrv_amend_options()

2014-07-31 Thread Benoît Canet 
The Saturday 26 Jul 2014 à 21:22:05 (+0200), Max Reitz wrote :

 Depending on the changed options and the image format,
 bdrv_amend_options() may take a significant amount of time. In these
 cases, a way to be informed about the operation's status is desirable.
 
 Since the operation is rather complex and may fundamentally change the
 image, implementing it as AIO or a couroutine does not seem feasible. On
 the other hand, implementing it as a block job would be significantly
 more difficult than a simple callback and would not add benefits other
 than progress report to the amending operation, because it should not
 actually be run as a block job at all.
 
 A callback may not be very pretty, but it's very easy to implement and
 perfectly fits its purpose here.
 
 Signed-off-by: Max Reitz mre...@redhat.com
 ---
  block.c   | 5 +++--
  block/qcow2.c | 5 -
  include/block/block.h | 5 -
  include/block/block_int.h | 3 ++-
  qemu-img.c| 2 +-
  5 files changed, 14 insertions(+), 6 deletions(-)
 
 diff --git a/block.c b/block.c
 index 3e252a2..4c8d4d8 100644
 --- a/block.c
 +++ b/block.c
 @@ -5708,12 +5708,13 @@ void bdrv_add_before_write_notifier(BlockDriverState 
 *bs,
  notifier_with_return_list_add(bs-before_write_notifiers, notifier);
  }
  
 -int bdrv_amend_options(BlockDriverState *bs, QemuOpts *opts)
 +int bdrv_amend_options(BlockDriverState *bs, QemuOpts *opts,
 +   BlockDriverAmendStatusCB *status_cb)
  {
  if (!bs-drv-bdrv_amend_options) {
  return -ENOTSUP;
  }
 -return bs-drv-bdrv_amend_options(bs, opts);
 +return bs-drv-bdrv_amend_options(bs, opts, status_cb);
  }
  
  /* This function will be called by the bdrv_recurse_is_first_non_filter 
 method
 diff --git a/block/qcow2.c b/block/qcow2.c
 index b0faa69..757f890 100644
 --- a/block/qcow2.c
 +++ b/block/qcow2.c
 @@ -2210,7 +2210,8 @@ static int qcow2_downgrade(BlockDriverState *bs, int 
 target_version)
  return 0;
  }
  
 -static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts)
 +static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
 +   BlockDriverAmendStatusCB *status_cb)
  {
  BDRVQcowState *s = bs-opaque;
  int old_version = s-qcow_version, new_version = old_version;
 @@ -2223,6 +2224,8 @@ static int qcow2_amend_options(BlockDriverState *bs, 
 QemuOpts *opts)
  int ret;
  QemuOptDesc *desc = opts-list-desc;
  
 +(void)status_cb;

This look like a temporary hack to please the compiler.
Am I right ? Should be comment this ?

 +
  while (desc  desc-name) {
  if (!qemu_opt_find(opts, desc-name)) {
  /* only change explicitly defined options */
 diff --git a/include/block/block.h b/include/block/block.h
 index 32d3676..f2b1799 100644
 --- a/include/block/block.h
 +++ b/include/block/block.h
 @@ -309,7 +309,10 @@ typedef enum {
  
  int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode 
 fix);
  
 -int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts);
 +typedef void BlockDriverAmendStatusCB(BlockDriverState *bs, int64_t offset,
 +  int64_t total_work_size);
 +int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts,
 +   BlockDriverAmendStatusCB *status_cb);
  
  /* external snapshots */
  bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
 diff --git a/include/block/block_int.h b/include/block/block_int.h
 index f6c3bef..5c5798d 100644
 --- a/include/block/block_int.h
 +++ b/include/block/block_int.h
 @@ -228,7 +228,8 @@ struct BlockDriver {
  int (*bdrv_check)(BlockDriverState* bs, BdrvCheckResult *result,
  BdrvCheckMode fix);
  
 -int (*bdrv_amend_options)(BlockDriverState *bs, QemuOpts *opts);
 +int (*bdrv_amend_options)(BlockDriverState *bs, QemuOpts *opts,
 +  BlockDriverAmendStatusCB *status_cb);
  
  void (*bdrv_debug_event)(BlockDriverState *bs, BlkDebugEvent event);
  
 diff --git a/qemu-img.c b/qemu-img.c
 index ef74ae4..90d6b79 100644
 --- a/qemu-img.c
 +++ b/qemu-img.c
 @@ -2827,7 +2827,7 @@ static int img_amend(int argc, char **argv)
  goto out;
  }
  
 -ret = bdrv_amend_options(bs, opts);
 +ret = bdrv_amend_options(bs, opts, NULL);
  if (ret  0) {
  error_report(Error while amending options: %s, strerror(-ret));
  goto out;
 -- 
 2.0.3

Re: [Qemu-devel] [PATCH alt 1/7] block: Add status callback to bdrv_amend_options()

2014-07-31 Thread Benoît Canet 
The Thursday 31 Jul 2014 à 09:51:05 (+0200), Benoît Canet wrote :
 The Saturday 26 Jul 2014 à 21:22:05 (+0200), Max Reitz wrote :
 
  Depending on the changed options and the image format,
  bdrv_amend_options() may take a significant amount of time. In these
  cases, a way to be informed about the operation's status is desirable.
  
  Since the operation is rather complex and may fundamentally change the
  image, implementing it as AIO or a couroutine does not seem feasible. On
  the other hand, implementing it as a block job would be significantly
  more difficult than a simple callback and would not add benefits other
  than progress report to the amending operation, because it should not
  actually be run as a block job at all.
  
  A callback may not be very pretty, but it's very easy to implement and
  perfectly fits its purpose here.
  
  Signed-off-by: Max Reitz mre...@redhat.com
  ---
   block.c   | 5 +++--
   block/qcow2.c | 5 -
   include/block/block.h | 5 -
   include/block/block_int.h | 3 ++-
   qemu-img.c| 2 +-
   5 files changed, 14 insertions(+), 6 deletions(-)
  
  diff --git a/block.c b/block.c
  index 3e252a2..4c8d4d8 100644
  --- a/block.c
  +++ b/block.c
  @@ -5708,12 +5708,13 @@ void 
  bdrv_add_before_write_notifier(BlockDriverState *bs,
   notifier_with_return_list_add(bs-before_write_notifiers, notifier);
   }
   
  -int bdrv_amend_options(BlockDriverState *bs, QemuOpts *opts)
  +int bdrv_amend_options(BlockDriverState *bs, QemuOpts *opts,
  +   BlockDriverAmendStatusCB *status_cb)
   {
   if (!bs-drv-bdrv_amend_options) {
   return -ENOTSUP;
   }
  -return bs-drv-bdrv_amend_options(bs, opts);
  +return bs-drv-bdrv_amend_options(bs, opts, status_cb);
   }
   
   /* This function will be called by the bdrv_recurse_is_first_non_filter 
  method
  diff --git a/block/qcow2.c b/block/qcow2.c
  index b0faa69..757f890 100644
  --- a/block/qcow2.c
  +++ b/block/qcow2.c
  @@ -2210,7 +2210,8 @@ static int qcow2_downgrade(BlockDriverState *bs, int 
  target_version)
   return 0;
   }
   
  -static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts)
  +static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
  +   BlockDriverAmendStatusCB *status_cb)
   {
   BDRVQcowState *s = bs-opaque;
   int old_version = s-qcow_version, new_version = old_version;
  @@ -2223,6 +2224,8 @@ static int qcow2_amend_options(BlockDriverState *bs, 
  QemuOpts *opts)
   int ret;
   QemuOptDesc *desc = opts-list-desc;
   
  +(void)status_cb;
 
 This look like a temporary hack to please the compiler.
 Am I right ? Should be comment this ?

For the further patch I now understand this I am just suprised that you need to 
silence a unused function parameter.

Reviewed-by: Benoit Canet ben...@irqsave.net

 
  +
   while (desc  desc-name) {
   if (!qemu_opt_find(opts, desc-name)) {
   /* only change explicitly defined options */
  diff --git a/include/block/block.h b/include/block/block.h
  index 32d3676..f2b1799 100644
  --- a/include/block/block.h
  +++ b/include/block/block.h
  @@ -309,7 +309,10 @@ typedef enum {
   
   int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode 
  fix);
   
  -int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts);
  +typedef void BlockDriverAmendStatusCB(BlockDriverState *bs, int64_t offset,
  +  int64_t total_work_size);
  +int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts,
  +   BlockDriverAmendStatusCB *status_cb);
   
   /* external snapshots */
   bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
  diff --git a/include/block/block_int.h b/include/block/block_int.h
  index f6c3bef..5c5798d 100644
  --- a/include/block/block_int.h
  +++ b/include/block/block_int.h
  @@ -228,7 +228,8 @@ struct BlockDriver {
   int (*bdrv_check)(BlockDriverState* bs, BdrvCheckResult *result,
   BdrvCheckMode fix);
   
  -int (*bdrv_amend_options)(BlockDriverState *bs, QemuOpts *opts);
  +int (*bdrv_amend_options)(BlockDriverState *bs, QemuOpts *opts,
  +  BlockDriverAmendStatusCB *status_cb);
   
   void (*bdrv_debug_event)(BlockDriverState *bs, BlkDebugEvent event);
   
  diff --git a/qemu-img.c b/qemu-img.c
  index ef74ae4..90d6b79 100644
  --- a/qemu-img.c
  +++ b/qemu-img.c
  @@ -2827,7 +2827,7 @@ static int img_amend(int argc, char **argv)
   goto out;
   }
   
  -ret = bdrv_amend_options(bs, opts);
  +ret = bdrv_amend_options(bs, opts, NULL);
   if (ret  0) {
   error_report(Error while amending options: %s, strerror(-ret));
   goto out;
  -- 
  2.0.3