Re: [Qemu-devel] [PATCH 5/5] block: extend block-commit to accept a string for the backing file

[Benoît Canet]

The Wednesday 14 May 2014 à 23:20:19 (-0400), Jeff Cody wrote :
> On some image chains, QEMU may not always be able to resolve the
> filenames properly, when updating the backing file of an image
> after a block commit.
> 
> For instance, certain relative pathnames may fail, or drives may
> have been specified originally by file descriptor (e.g. /dev/fd/???),
> or a relative protocol pathname may have been used.
> 
> In these instances, QEMU may lack the information to be able to make
> the correct choice, but the user or management layer most likely does
> have that knowledge.
> 
> With this extension to the block-commit api, the user is able to change
> the backing file of the overlay image as part of the block-commit
> operation.
> 
> This allows the change to be 'safe', in the sense that if the attempt
> to write the overlay image metadata fails, then the block-commit
> operation returns failure, without disrupting the guest.
> 
> If the commit top is the active layer, then specifying the backing
> file string will be treated as an error (there is no overlay image
> to modify in that case).
> 
> If a backing file string is not specified in the command, the backing
> file string to use is determined in the same manner as it was
> previously.
> 
> Signed-off-by: Jeff Cody <address@hidden>
> ---
>  block.c                   |  8 ++++++--
>  block/commit.c            |  9 ++++++---
>  blockdev.c                |  8 +++++++-
>  include/block/block.h     |  3 ++-
>  include/block/block_int.h |  3 ++-
>  qapi-schema.json          | 18 ++++++++++++++++--
>  qmp-commands.hx           | 14 +++++++++++++-
>  7 files changed, 52 insertions(+), 11 deletions(-)
> 
> diff --git a/block.c b/block.c
> index c4f77c2..7e63a1f 100644
> --- a/block.c
> +++ b/block.c
> @@ -2547,12 +2547,15 @@ typedef struct BlkIntermediateStates {
>   *
>   * base <- active
>   *
> + * If backing_file_str is non-NULL, it will be used when modifying top's
> + * overlay image metadata.
> + *
>   * Error conditions:
>   *  if active == top, that is considered an error
>   *
>   */
>  int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
> -                           BlockDriverState *base)
> +                           BlockDriverState *base, const char 
> *backing_file_str)
>  {
>      BlockDriverState *intermediate;
>      BlockDriverState *base_bs = NULL;
> @@ -2604,7 +2607,8 @@ int bdrv_drop_intermediate(BlockDriverState *active, 
> BlockDriverState *top,
>      }
>  
>      /* success - we can delete the intermediate states, and link top->base */
> -    ret = bdrv_change_backing_file(new_top_bs, base_bs->filename,
> +    backing_file_str = backing_file_str ? backing_file_str : 
> base_bs->filename;
> +    ret = bdrv_change_backing_file(new_top_bs, backing_file_str,
>                                     base_bs->drv ? base_bs->drv->format_name 
> : "");
>      if (ret) {
>          goto exit;
> diff --git a/block/commit.c b/block/commit.c
> index 5c09f44..91517d3 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -37,6 +37,7 @@ typedef struct CommitBlockJob {
>      BlockdevOnError on_error;
>      int base_flags;
>      int orig_overlay_flags;
> +    char *backing_file_str;
>  } CommitBlockJob;
>  
>  static int coroutine_fn commit_populate(BlockDriverState *bs,
> @@ -141,7 +142,7 @@ wait:
>  
>      if (!block_job_is_cancelled(&s->common) && sector_num == end) {
>          /* success */
> -        ret = bdrv_drop_intermediate(active, top, base);
> +        ret = bdrv_drop_intermediate(active, top, base, s->backing_file_str);
>      }
>  
>  exit_free_buf:
> @@ -158,7 +159,7 @@ exit_restore_reopen:
>      if (overlay_bs && s->orig_overlay_flags != bdrv_get_flags(overlay_bs)) {
>          bdrv_reopen(overlay_bs, s->orig_overlay_flags, NULL);
>      }
> -
> +    g_free(s->backing_file_str);
>      block_job_completed(&s->common, ret);
>  }
>  
> @@ -182,7 +183,7 @@ static const BlockJobDriver commit_job_driver = {
>  void commit_start(BlockDriverState *bs, BlockDriverState *base,
>                    BlockDriverState *top, int64_t speed,
>                    BlockdevOnError on_error, BlockDriverCompletionFunc *cb,
> -                  void *opaque, Error **errp)
> +                  void *opaque, const char *backing_file_str, Error **errp)
>  {
>      CommitBlockJob *s;
>      BlockReopenQueue *reopen_queue = NULL;
> @@ -244,6 +245,8 @@ void commit_start(BlockDriverState *bs, BlockDriverState 
> *base,
>      s->base_flags          = orig_base_flags;
>      s->orig_overlay_flags  = orig_overlay_flags;
>  
> +    s->backing_file_str = g_strdup(backing_file_str);
> +
>      s->on_error = on_error;
>      s->common.co = qemu_coroutine_create(commit_run);
>  
> diff --git a/blockdev.c b/blockdev.c
> index d8cb396..c0c7867 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1872,6 +1872,7 @@ void qmp_block_commit(const char *device,
>                        bool has_base_node_name, const char *base_node_name,
>                        bool has_top, const char *top,
>                        bool has_top_node_name, const char *top_node_name,
> +                      bool has_backing_file, const char *backing_file,
>                        bool has_speed, int64_t speed,
>                        Error **errp)
>  {
> @@ -1951,11 +1952,16 @@ void qmp_block_commit(const char *device,
>      }
>  
>      if (top_bs == bs) {
> +        if (has_backing_file) {
> +            error_setg(errp, "'backing-file' specified,"
> +                             " but 'top' is the active layer");
> +            return;
> +        }
>          commit_active_start(bs, base_bs, speed, on_error, block_job_cb,
>                              bs, &local_err);
>      } else {
>          commit_start(bs, base_bs, top_bs, speed, on_error, block_job_cb, bs,
> -                    &local_err);
> +                     backing_file, &local_err);

I don't know QAPI well enough to be sure but are we certain that if 
has_backing_file == false then backing_file == NULL and not some
random pointer ?

If am thinking to add has_backing_file ? backing_file : NULL here.


>      }
>      if (local_err != NULL) {
>          error_propagate(errp, local_err);
> diff --git a/include/block/block.h b/include/block/block.h
> index 283a6f3..e7aeca6 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -266,7 +266,8 @@ int bdrv_change_backing_file(BlockDriverState *bs,
>      const char *backing_file, const char *backing_fmt);
>  void bdrv_register(BlockDriver *bdrv);
>  int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
> -                           BlockDriverState *base);
> +                           BlockDriverState *base,
> +                           const char *backing_file_str);
>  BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
>                                      BlockDriverState *bs);
>  BlockDriverState *bdrv_find_base(BlockDriverState *bs);
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 9ffcb69..8e7dce7 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -428,13 +428,14 @@ void stream_start(BlockDriverState *bs, 
> BlockDriverState *base,
>   * @on_error: The action to take upon error.
>   * @cb: Completion function for the job.
>   * @opaque: Opaque pointer value passed to @cb.
> + * @backing_file_str: String to use as the backing file in @top's overlay

Miss a final dot to be like the other lines.

>   * @errp: Error object.
>   *
>   */
>  void commit_start(BlockDriverState *bs, BlockDriverState *base,
>                   BlockDriverState *top, int64_t speed,
>                   BlockdevOnError on_error, BlockDriverCompletionFunc *cb,
> -                 void *opaque, Error **errp);
> +                 void *opaque, const char *backing_file_str, Error **errp);
>  /**
>   * commit_active_start:
>   * @bs: Active block device to be committed.
> diff --git a/qapi-schema.json b/qapi-schema.json
> index eddb2b8..ef775fe 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2118,6 +2118,18 @@
>  #                           topmost data to be committed down.
>  #                           (Since 2.1)
>  #
> +# @backing-file: #optional The backing file string to write into the overlay
> +#                          image of 'top'.  If 'top' is the active layer,
> +#                          specifying a backing file string is an error. This
> +#                          backing file string is only written into the the
> +#                          image file metadata - internal structures inside
> +#                          QEMU are not updated,  and the string is not 
> validated.
> +#                          If not specified, QEMU will automatically 
> determine
> +#                          the backing file string to use.  Care should be 
> taken
> +#                          when specifying the string, to specify a valid 
> filename
> +#                          or protocol.
> +#                          (Since 2.1)
> +#
>  #                    If top == base, that is an error.
>  #                    If top == active, the job will not be completed by 
> itself,
>  #                    user needs to complete the job with the 
> block-job-complete
> @@ -2130,7 +2142,6 @@
>  #                    size of the smaller top, you can safely truncate it
>  #                    yourself once the commit operation successfully 
> completes.
>  #
> -#

Spurious line removal.

>  # @speed:  #optional the maximum speed, in bytes per second
>  #
>  # Returns: Nothing on success
> @@ -2141,13 +2152,16 @@
>  #          If @speed is invalid, InvalidParameter
>  #          If both @base and @base-node-name are specified, GenericError
>  #          If both @top and @top-node-name are specified, GenericError
> +#          If @top or @top-node-name is the active layer, and @backing-file 
> is
> +#             specified, GenericError
>  #
>  # Since: 1.3
>  #
>  ##
>  { 'command': 'block-commit',
>    'data': { 'device': 'str', '*base': 'str', '*base-node-name': 'str',
> -            '*top': 'str', '*top-node-name': 'str', '*speed': 'int' } }
> +            '*top': 'str', '*top-node-name': 'str', '*backing-file': 'str',
> +            '*speed': 'int' } }
>  
>  ##
>  # @drive-backup
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 2b9b1dc..e2c446f 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -985,7 +985,7 @@ EQMP
>  
>      {
>          .name       = "block-commit",
> -        .args_type  = 
> "device:B,base:s?,base-node-name:s?,top:s?,top-node-name:s?,speed:o?",
> +        .args_type  = 
> "device:B,base:s?,base-node-name:s?,top:s?,top-node-name:s?,backing-file:s?,speed:o?",
>          .mhandler.cmd_new = qmp_marshal_input_block_commit,
>      },
>  
> @@ -1021,6 +1021,18 @@ For 'top', either top or top-node-name must be set but 
> not both.
>                     topmost data to be committed down.
>                     (json-string, optional) (Since 2.1)
>  
> +- "backing-file": The backing file string to write into the overlay
> +                  image of 'top'.  If 'top' is the active layer,
> +                  specifying a backing file string is an error. This
> +                  backing file string is only written into the the
> +                  image file metadata - internal structures inside
> +                  QEMU are not updated,  and the string is not validated.
> +                  If not specified, QEMU will automatically determine
> +                  the backing file string to use.  Care should be taken
> +                  when specifying the string, to specify a valid filename
> +                  or protocol.
> +                  (json-string, optional) (Since 2.1)
> +
>            If top == base, that is an error.
>            If top == active, the job will not be completed by itself,
>            user needs to complete the job with the block-job-complete
> -- 
> 1.8.3.1
> 
>