Re: [Qemu-block] [RFC PATCH 10/11] qcow2: Store data file name in the image

[Max Reitz]

On 31.01.19 18:55, Kevin Wolf wrote:
> Rather than requiring that the external data file node is passed
> explicitly when creating the qcow2 node, store the filename in the
> designated header extension during .bdrv_create and read it from there
> as a default during .bdrv_open.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block/qcow2.h              |  1 +
>  block/qcow2.c              | 69 +++++++++++++++++++++++++++++++++++++-
>  tests/qemu-iotests/082.out | 27 +++++++++++++++
>  3 files changed, 96 insertions(+), 1 deletion(-)

[...]

> diff --git a/block/qcow2.c b/block/qcow2.c
> index 6cf862e8b9..4959bf16a4 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -398,6 +398,20 @@ static int qcow2_read_extensions(BlockDriverState *bs, 
> uint64_t start_offset,
>  #endif
>              break;
>  
> +        case QCOW2_EXT_MAGIC_DATA_FILE:
> +        {
> +            s->image_data_file = g_malloc0(ext.len + 1);
> +            ret = bdrv_pread(bs->file, offset, s->image_data_file, ext.len);
> +            if (ret < 0) {
> +                error_setg_errno(errp, -ret, "ERROR: Could not data file 
> name");

I think you accidentally a word.

> +                return ret;
> +            }
> +#ifdef DEBUG_EXT
> +            printf("Qcow2: Got external data file %s\n", s->image_data_file);
> +#endif
> +            break;
> +        }
> +
>          default:
>              /* unknown magic - save it in case we need to rewrite the header 
> */
>              /* If you add a new feature, make sure to also update the fast
> @@ -1444,7 +1458,18 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
> *bs, QDict *options,
>      /* Open external data file */
>      if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {
>          s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
> -                                       &child_file, false, errp);
> +                                       &child_file, false, &local_err);
> +        if (!s->data_file) {
> +            if (s->image_data_file) {
> +                error_free(local_err);
> +                local_err = NULL;

This looked a bit weird to me at first because I was wondering why you
wouldn't just pass allow_none=true and then handle errors (by passing
them on).  But right, we want to retry with a filename set, maybe that
makes more sense of the options.

Hm.  But then again, do we really?  It matches what we do with backing
files, but that does give at least me headaches from time to time.  How
bad would it be to allow either passing all valid options through
@options (which would make qcow2 ignore the string in the header), or
use the filename given in the header alone?

> +                s->data_file = bdrv_open_child(s->image_data_file, options,
> +                                               "data-file", bs, &child_file,
> +                                               false, errp);
> +            } else {
> +                error_propagate(errp, local_err);
> +            }
> +        }
>          if (!s->data_file) {
>              ret = -EINVAL;
>              goto fail;

[...]

> @@ -3229,6 +3270,26 @@ static int coroutine_fn qcow2_co_create_opts(const 
> char *filename, QemuOpts *opt
>          goto finish;
>      }
>  
> +    /* Create and open an external data file (protocol layer) */
> +    val = qdict_get_try_str(qdict, BLOCK_OPT_DATA_FILE);
> +    if (val) {
> +        ret = bdrv_create_file(val, opts, errp);

I suppose taking an existing file is saved for later?

Max

> +        if (ret < 0) {
> +            goto finish;
> +        }
> +
> +        data_bs = bdrv_open(val, NULL, NULL,
> +                            BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
> +                            errp);
> +        if (data_bs == NULL) {
> +            ret = -EIO;
> +            goto finish;
> +        }
> +
> +        qdict_del(qdict, BLOCK_OPT_DATA_FILE);
> +        qdict_put_str(qdict, "data-file", data_bs->node_name);
> +    }
> +
>      /* Set 'driver' and 'node' options */
>      qdict_put_str(qdict, "driver", "qcow2");
>      qdict_put_str(qdict, "file", bs->node_name);

signature.asc
Description: OpenPGP digital signature