Re: [Qemu-devel] [RFC PATCH 10/11] qcow2: Store data file name in the image
On 22.02.19 17:06, Kevin Wolf wrote: > Am 22.02.2019 um 16:43 hat Max Reitz geschrieben: >> On 22.02.19 16:35, Kevin Wolf wrote: >>> Am 22.02.2019 um 15:16 hat Max Reitz geschrieben: On 19.02.19 10:04, Kevin Wolf wrote: > Am 19.02.2019 um 01:18 hat Max Reitz geschrieben: >> 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 >>> --- >>> 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. > > I think we want the normal error message for the !s->image_data_file > case. With allow_none=true, we would have to come up with a new one here > (in the else branch below). > >> 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? > > You mean offering only one of the two ways to configure the node? Either just the filename from the image header, or ignore that and take all options from the user (who'd have to give a syntactically complete QAPI BlockdevRef object). > The 'data-file' runtime option is a must so that libvirt can build the > graph node by node (and possibly use file descriptor passing one day). > But having to specify the option every time is very unfriendly for human > users, so I think allowing to store the file name in the header is a > must, too. Sure. But I don't know whether we have to support taking the filename from the image header, and the user overriding some of the node's options (e.g. caching). >>> >>> So essentially this would mean passing NULL instead of options to >>> bdrv_open_child() when we retry with the filename from the header. >>> >>> But it's inconsistent with all other places, which comes with two >> >> "all other places"? Really it's just backing files, as everywhere else >> there is no filename that doesn't come from the command line. >> >> Yes, you can use -drive file=foo.qcow2,file.locking=off, but I consider >> that case a bit different. Although maybe it really isn't. *shrug* > > Why would it be different? At least as a user, I consider them the same. > It's true that bs->file an
Re: [Qemu-devel] [RFC PATCH 10/11] qcow2: Store data file name in the image
Am 22.02.2019 um 16:43 hat Max Reitz geschrieben: > On 22.02.19 16:35, Kevin Wolf wrote: > > Am 22.02.2019 um 15:16 hat Max Reitz geschrieben: > >> On 19.02.19 10:04, Kevin Wolf wrote: > >>> Am 19.02.2019 um 01:18 hat Max Reitz geschrieben: > 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 > > --- > > 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. > >>> > >>> I think we want the normal error message for the !s->image_data_file > >>> case. With allow_none=true, we would have to come up with a new one here > >>> (in the else branch below). > >>> > 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? > >>> > >>> You mean offering only one of the two ways to configure the node? > >> > >> Either just the filename from the image header, or ignore that and take > >> all options from the user (who'd have to give a syntactically complete > >> QAPI BlockdevRef object). > >> > >>> The 'data-file' runtime option is a must so that libvirt can build the > >>> graph node by node (and possibly use file descriptor passing one day). > >>> But having to specify the option every time is very unfriendly for human > >>> users, so I think allowing to store the file name in the header is a > >>> must, too. > >> > >> Sure. But I don't know whether we have to support taking the filename > >> from the image header, and the user overriding some of the node's > >> options (e.g. caching). > > > > So essentially this would mean passing NULL instead of options to > > bdrv_open_child() when we retry with the filename from the header. > > > > But it's inconsistent with all other places, which comes with two > > "all other places"? Really it's just backing files, as everywhere else > there is no filename that doesn't come from the command line. > > Yes, you can use -drive file=foo.qcow2,file.locking=off, but I consider > that case a bit different. Although maybe it really isn't. *shrug* Why would it be different? At least as a user, I consider them the same. It's true that bs->file and bs->backing come with some additional magic, b
Re: [Qemu-devel] [RFC PATCH 10/11] qcow2: Store data file name in the image
On 22.02.19 16:35, Kevin Wolf wrote: > Am 22.02.2019 um 15:16 hat Max Reitz geschrieben: >> On 19.02.19 10:04, Kevin Wolf wrote: >>> Am 19.02.2019 um 01:18 hat Max Reitz geschrieben: 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 > --- > 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. >>> >>> I think we want the normal error message for the !s->image_data_file >>> case. With allow_none=true, we would have to come up with a new one here >>> (in the else branch below). >>> 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? >>> >>> You mean offering only one of the two ways to configure the node? >> >> Either just the filename from the image header, or ignore that and take >> all options from the user (who'd have to give a syntactically complete >> QAPI BlockdevRef object). >> >>> The 'data-file' runtime option is a must so that libvirt can build the >>> graph node by node (and possibly use file descriptor passing one day). >>> But having to specify the option every time is very unfriendly for human >>> users, so I think allowing to store the file name in the header is a >>> must, too. >> >> Sure. But I don't know whether we have to support taking the filename >> from the image header, and the user overriding some of the node's >> options (e.g. caching). > > So essentially this would mean passing NULL instead of options to > bdrv_open_child() when we retry with the filename from the header. > > But it's inconsistent with all other places, which comes with two "all other places"? Really it's just backing files, as everywhere else there is no filename that doesn't come from the command line. Yes, you can use -drive file=foo.qcow2,file.locking=off, but I consider that case a bit different. Although maybe it really isn't. *shrug* > problems. It's confusing for users who are used to overriding just that > one option of a child. And do we actually spare you any headaches or do > we create new ones because we have now two different behaviours of > bdrv_open_child() callers that we must preserve in the future? It means I can act out on my pain by being angry on how .backing behaves. That's better for my health than having to keep it
Re: [Qemu-devel] [RFC PATCH 10/11] qcow2: Store data file name in the image
Am 22.02.2019 um 15:16 hat Max Reitz geschrieben: > On 19.02.19 10:04, Kevin Wolf wrote: > > Am 19.02.2019 um 01:18 hat Max Reitz geschrieben: > >> 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 > >>> --- > >>> 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. > > > > I think we want the normal error message for the !s->image_data_file > > case. With allow_none=true, we would have to come up with a new one here > > (in the else branch below). > > > >> 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? > > > > You mean offering only one of the two ways to configure the node? > > Either just the filename from the image header, or ignore that and take > all options from the user (who'd have to give a syntactically complete > QAPI BlockdevRef object). > > > The 'data-file' runtime option is a must so that libvirt can build the > > graph node by node (and possibly use file descriptor passing one day). > > But having to specify the option every time is very unfriendly for human > > users, so I think allowing to store the file name in the header is a > > must, too. > > Sure. But I don't know whether we have to support taking the filename > from the image header, and the user overriding some of the node's > options (e.g. caching). So essentially this would mean passing NULL instead of options to bdrv_open_child() when we retry with the filename from the header. But it's inconsistent with all other places, which comes with two problems. It's confusing for users who are used to overriding just that one option of a child. And do we actually spare you any headaches or do we create new ones because we have now two different behaviours of bdrv_open_child() callers that we must preserve in the future? Kevin signature.asc Description: PGP signature
Re: [Qemu-devel] [RFC PATCH 10/11] qcow2: Store data file name in the image
On 19.02.19 10:04, Kevin Wolf wrote: > Am 19.02.2019 um 01:18 hat Max Reitz geschrieben: >> 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 >>> --- >>> 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. > > I think we want the normal error message for the !s->image_data_file > case. With allow_none=true, we would have to come up with a new one here > (in the else branch below). > >> 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? > > You mean offering only one of the two ways to configure the node? Either just the filename from the image header, or ignore that and take all options from the user (who'd have to give a syntactically complete QAPI BlockdevRef object). > The 'data-file' runtime option is a must so that libvirt can build the > graph node by node (and possibly use file descriptor passing one day). > But having to specify the option every time is very unfriendly for human > users, so I think allowing to store the file name in the header is a > must, too. Sure. But I don't know whether we have to support taking the filename from the image header, and the user overriding some of the node's options (e.g. caching). >>> +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? > > I think it's saved for the day that 'qemu-img create' is extended (or > replaced with an alternative) to provide the same functionality as QMP > blockdev-create. Ah, yes. :-) Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [RFC PATCH 10/11] qcow2: Store data file name in the image
Am 19.02.2019 um 01:18 hat Max Reitz geschrieben: > 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 > > --- > > 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. I think we want the normal error message for the !s->image_data_file case. With allow_none=true, we would have to come up with a new one here (in the else branch below). > 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? You mean offering only one of the two ways to configure the node? The 'data-file' runtime option is a must so that libvirt can build the graph node by node (and possibly use file descriptor passing one day). But having to specify the option every time is very unfriendly for human users, so I think allowing to store the file name in the header is a must, too. > > +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? I think it's saved for the day that 'qemu-img create' is extended (or replaced with an alternative) to provide the same functionality as QMP blockdev-create. Kevin signature.asc Description: PGP signature
Re: [Qemu-devel] [RFC PATCH 10/11] qcow2: Store data file name in the image
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 > --- > 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
Re: [Qemu-devel] [RFC PATCH 10/11] qcow2: Store data file name in the image
On Thu, Jan 31, 2019 at 8:20 PM 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. > If the data file is the backing file, we don't need this change. > > Signed-off-by: Kevin Wolf > --- > 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.h b/block/qcow2.h > index e2114900b4..a1e2600643 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -341,6 +341,7 @@ typedef struct BDRVQcow2State { > * override) */ > char *image_backing_file; > char *image_backing_format; > +char *image_data_file; > > CoQueue compress_wait_queue; > int nb_compress_threads; > 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"); > +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; > +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; > @@ -1627,6 +1652,7 @@ static int coroutine_fn > qcow2_do_open(BlockDriverState *bs, QDict *options, > return ret; > > fail: > +g_free(s->image_data_file); > if (has_data_file(bs)) { > bdrv_unref_child(bs, s->data_file); > } > @@ -2246,6 +2272,7 @@ static void qcow2_close(BlockDriverState *bs) > g_free(s->unknown_header_fields); > cleanup_unknown_header_ext(bs); > > +g_free(s->image_data_file); > g_free(s->image_backing_file); > g_free(s->image_backing_format); > > @@ -2422,6 +2449,19 @@ int qcow2_update_header(BlockDriverState *bs) > buflen -= ret; > } > > +/* External data file header extension */ > +if (has_data_file(bs) && s->image_data_file) { > +ret = header_ext_add(buf, QCOW2_EXT_MAGIC_DATA_FILE, > + s->image_data_file, > strlen(s->image_data_file), > + buflen); > +if (ret < 0) { > +goto fail; > +} > + > +buf += ret; > +buflen -= ret; > +} > + > /* Full disk encryption header pointer extension */ > if (s->crypto_header.offset != 0) { > s->crypto_header.offset = cpu_to_be64(s->crypto_header.offset); > @@ -3166,6 +3206,7 @@ static int coroutine_fn qcow2_co_create_opts(const > char *filename, QemuOpts *opt > QDict *qdict; > Visitor *v; > BlockDriverState *bs = NULL; > +BlockDriverState *data_bs = NULL; > Error *local_err = NULL; > const char *val; > int ret; > @@ -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); > +if (ret < 0) { > +goto finish; > +} > + > +data_bs = bdrv_open(val, NULL, NULL, > +BDRV_O_RDWR | BDRV_O_RESIZE | BDR
[Qemu-devel] [RFC PATCH 10/11] qcow2: Store data file name in the image
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 --- 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.h b/block/qcow2.h index e2114900b4..a1e2600643 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -341,6 +341,7 @@ typedef struct BDRVQcow2State { * override) */ char *image_backing_file; char *image_backing_format; +char *image_data_file; CoQueue compress_wait_queue; int nb_compress_threads; 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"); +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; +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; @@ -1627,6 +1652,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, return ret; fail: +g_free(s->image_data_file); if (has_data_file(bs)) { bdrv_unref_child(bs, s->data_file); } @@ -2246,6 +2272,7 @@ static void qcow2_close(BlockDriverState *bs) g_free(s->unknown_header_fields); cleanup_unknown_header_ext(bs); +g_free(s->image_data_file); g_free(s->image_backing_file); g_free(s->image_backing_format); @@ -2422,6 +2449,19 @@ int qcow2_update_header(BlockDriverState *bs) buflen -= ret; } +/* External data file header extension */ +if (has_data_file(bs) && s->image_data_file) { +ret = header_ext_add(buf, QCOW2_EXT_MAGIC_DATA_FILE, + s->image_data_file, strlen(s->image_data_file), + buflen); +if (ret < 0) { +goto fail; +} + +buf += ret; +buflen -= ret; +} + /* Full disk encryption header pointer extension */ if (s->crypto_header.offset != 0) { s->crypto_header.offset = cpu_to_be64(s->crypto_header.offset); @@ -3166,6 +3206,7 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt QDict *qdict; Visitor *v; BlockDriverState *bs = NULL; +BlockDriverState *data_bs = NULL; Error *local_err = NULL; const char *val; int ret; @@ -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); +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(qdic