qemu-img: improve convert & dd commands
cover qemu-img: improve convert & dd commands
This series is in response to Max pointing out that you cannot use 'convert' for an encrypted target image.
The 'convert' and 'dd' commands need to first create the image and then open it. The bdrv_create() method takes a set of options for creating the image, which let us provide a key-secret for the encryption key. When the commands then open the new image, they don't provide any options, so the image is unable to be opened due to lack of encryption key. It is also not possible to use the --image-opts argument to provide structured options in the target image name - it must be a plain filename to satisfy the bdrv_create() API contract.
This series addresses these problems to some extent
- Adds a new --target-image-opts flag which is used to say that the target filename is using structured options. It is *only* permitted to use this when -n is also set. ie the target image must be pre-created so convert/dd don't need to run bdrv_create().
- When --target-image-opts is not used, add special case code that identifies options passed to bdrv_create() named "*key-secret" and adds them to the options used to open the new image
In future it is desirable to make --target-image-opts work even when -n is *not* given. This requires considerable work to create a new bdrv_create() API impl.
The first four patches improve the 'dd' command to address feature gaps wrt the 'convert' command. The last two patches implement the improvements described above.
Daniel P. Berrange (6):
qemu-img: add support for --object with 'dd' command
qemu-img: fix --image-opts usage with dd command
qemu-img: add support for -n arg to dd command
qemu-img: add support for -o arg to dd command
qemu-img: introduce --target-image-opts for 'convert' command
qemu-img: copy *key-secret opts when opening newly created files
qemu-img-cmds.hx | 8 +- qemu-img.c | 286 ++++++++++++++++++++++++++++++++++++++++++------------- qemu-img.texi | 23 ++++- 3 files changed, 244 insertions(+), 73 deletions(-)
Signature · 1 line
2.9.3
PATCH 1/6 qemu-img: add support for --object with 'dd' command
The qemu-img dd command added --image-opts support, but missed the corresponding --object support. This prevented passing secrets (eg auth passwords) needed by certain disk images.
--- qemu-img.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
qemu-img.c +16 -0
@@ -3949,6 +3949,7 @@ static int img_dd(int argc, char **argv) };const struct option long_options[] = { {"help" , no_argument,0 ,'h' }, + {"object" , required_argument,0 ,OPTION_OBJECT }, {"image-opts" , no_argument,0 ,OPTION_IMAGE_OPTS }, {0 ,0 ,0 ,0 } };
@@ -3971,6 +3972,14 @@ static int img_dd(int argc, char **argv)case 'h' :help ();break ; +case OPTION_OBJECT : { +QemuOpts *opts; + opts =qemu_opts_parse_noisily (&qemu_object_opts, + optarg, true); +if (!opts) { +return 1 ; + } + }break ;case OPTION_IMAGE_OPTS : image_opts = true;break ;
@@ -4015,6 +4024,13 @@ static int img_dd(int argc, char **argv) ret =-1 ; goto out; } + +if (qemu_opts_foreach (&qemu_object_opts, + user_creatable_add_opts_foreach, +NULL ,NULL )) { +return 1 ; + } + blk1 =img_open (image_opts, in.filename, fmt,0 , false, false);if (!blk1) {
Signature · 1 line
2.9.3
Re: [Qemu-devel] [PATCH v1 1/6] qemu-img: add support for --object with 'dd' command
Quoted 9 lines · Daniel P. Berrange
The qemu-img dd command added --image-opts support, but missed the corresponding --object support. This prevented passing secrets (eg auth passwords) needed by certain disk images.
Signed-off-by Daniel P. Berrange <[email protected]>--- qemu-img.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 2/6 qemu-img: fix --image-opts usage with dd command
In reply to Daniel P. BerrangeThe --image-opts flag can only be used to affect the parsing of the source image. The target image has to be specified in the traditional style regardless, since it needs to be passed to the brdv_create() API which does not support the new style opts.
--- qemu-img.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
qemu-img.c +7 -2
@@ -4098,8 +4098,13 @@ static int img_dd(int argc, char **argv) goto out; } - blk2 =img_open (image_opts, out.filename, out_fmt,BDRV_O_RDWR , - false, false); +/* TODO, we can't honour --image-opts for the target, +* since it needs to be given in a format compatible +* with the bdrv_create() call above which does not +* support image-opts style. +*/ + blk2 =img_open_file (out.filename, out_fmt,BDRV_O_RDWR , + false, false);if (!blk2) { ret =-1 ;
Signature · 1 line
2.9.3
Re: [Qemu-devel] [PATCH v1 2/6] qemu-img: fix --image-opts usage with dd command
Quoted 4 lines · Daniel P. Berrange
The --image-opts flag can only be used to affect the parsing of the source image. The target image has to be specified in the traditional style regardless, since it needs to be passed to the
brdv_create()API which does not support the new style
"bdrv_create", if you respin :)
Quoted 31 lines
opts.
Signed-off-by Daniel P. Berrange <[email protected]>--- qemu-img.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)qemu-img.c +7
-2@@ -4098,8 +4098,13 @@ static int img_dd(int argc, char **argv) goto out; } - blk2 =img_open (image_opts, out.filename, out_fmt,BDRV_O_RDWR , - false, false); +/* TODO, we can't honour --image-opts for the target, +* since it needs to be given in a format compatible +* with the bdrv_create() call above which does not +* support image-opts style. +*/ + blk2 =img_open_file (out.filename, out_fmt,BDRV_O_RDWR , + false, false);if (!blk2) { ret =-1 ;Signature · 1 line
2.9.3
PATCH 3/6 qemu-img: add support for -n arg to dd command
In reply to Daniel P. BerrangeThe -n arg to the convert command allows use of a pre-existing image, rather than creating a new image. This adds a -n arg to the dd command to get feature parity.
--- qemu-img-cmds.hx | 4 +-- qemu-img.c | 79 ++++++++++++++++++++++++++++++++++++-------------------- qemu-img.texi | 7 ++++- 3 files changed, 59 insertions(+), 31 deletions(-)
qemu-img-cmds.hx +2 -2
@@ -46,9 +46,9 @@ STEXI ETEXI DEF("dd", img_dd, - "dd [--image-opts] [-f fmt] [-O output_fmt] [bs=block_size] [count=blocks] [skip=blocks] if=input of=output") + "dd [--image-opts] [-n] [-f fmt] [-O output_fmt] [bs=block_size] [count=blocks] [skip=blocks] if=input of=output") STEXI -@item dd [--image-opts] [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] if=@var{input} of=@var{output} +@item dd [--image-opts] [-n] [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] if=@var{input} of=@var{output} ETEXI DEF("info", img_info,
qemu-img.c +51 -28
121 lines not on this page · raw message
qemu-img.texi +6 -1
@@ -326,7 +326,7 @@ skipped. This is useful for formats such as @code{rbd} if the target volume has already been created with site specific options that cannot be supplied through qemu-img. -@item dd [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] if=@var{input} of=@var{output} +@item dd [-n] [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] if=@var{input} of=@var{output} Dd copies from @var{input} file to @var{output} file converting it from @var{fmt} format to @var{output_fmt} format.
@@ -337,6 +337,11 @@ dd will stop reading input after reading @var{blocks} input blocks. The size syntax is similar to dd(1)'s size syntax. +If the @code{-n} option is specified, the target volume creation will be +skipped. This is useful for formats such as @code{rbd} if the target +volume has already been created with site specific options that cannot +be supplied through qemu-img. + @item info [-f @var{fmt}] [--output=@var{ofmt}] [--backing-chain] @var{filename} Give information about the disk image @var{filename}. Use it in
Signature · 1 line
2.9.3
Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command
Quoted 3 lines · Daniel P. Berrange
The -n arg to the convert command allows use of a pre-existing image, rather than creating a new image. This adds a -n arg to the dd command to get feature parity.
I remember there was a discussion about changing qemu-img dd's default to a "conv=nocreat" semantic, if so, "-n" might not be that useful. But that part hasn't made it into the tree, and I'm not sure which direction we should take. (Personally I think default to nocreat is a good idea).
Fam
Quoted 184 lines
Signed-off-by Daniel P. Berrange <[email protected]>--- qemu-img-cmds.hx | 4 +-- qemu-img.c | 79 ++++++++++++++++++++++++++++++++++++-------------------- qemu-img.texi | 7 ++++- 3 files changed, 59 insertions(+), 31 deletions(-)qemu-img-cmds.hx +2
-2@@ -46,9 +46,9 @@ STEXI ETEXI DEF("dd", img_dd, - "dd [--image-opts] [-f fmt] [-O output_fmt] [bs=block_size] [count=blocks] [skip=blocks] if=input of=output") + "dd [--image-opts] [-n] [-f fmt] [-O output_fmt] [bs=block_size] [count=blocks] [skip=blocks] if=input of=output") STEXI -@item dd [--image-opts] [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] if=@var{input} of=@var{output} +@item dd [--image-opts] [-n] [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] if=@var{input} of=@var{output} ETEXI DEF("info", img_info,qemu-img.c +51
-28@@ -3917,10 +3917,10 @@ static int img_dd(int argc, char **argv)QemuOptsList *create_opts =NULL ;Error *local_err =NULL ;bool image_opts = false; -int c, i; +int c, i, skip_create =0 ;const char *out_fmt ="raw" ;const char *fmt =NULL ; -int64_t size =0 ; +int64_t size =0 , out_size;int64_t block_count =0 , out_pos, in_pos;struct DdInfo dd = { .flags =0 ,@@ -3954,7 +3954,7 @@ static int img_dd(int argc, char **argv) { 0, 0, 0, 0 } }; -while ((c =getopt_long (argc, argv,"hf:O:" , long_options,NULL ))) { +while ((c =getopt_long (argc, argv,"hnf:O:" , long_options,NULL ))) {if (c ==EOF ) {break ; }@@ -3965,6 +3965,9 @@ static int img_dd(int argc, char **argv)case 'f' : fmt = optarg;break ; +case 'n' : + skip_create =1 ; +break ;case '?' :error_report ("Try 'qemu-img --help' for more information." ); ret =-1 ;@@ -4051,22 +4054,25 @@ static int img_dd(int argc, char **argv) ret =-1 ; goto out; } -if (!drv->create_opts) { -error_report ("Format driver '%s' does not support image creation" , - drv->format_name); - ret =-1 ; - goto out; - } -if (!proto_drv->create_opts) { -error_report ("Protocol driver '%s' does not support image creation" , - proto_drv->format_name); - ret =-1 ; - goto out; - } - create_opts =qemu_opts_append (create_opts, drv->create_opts); - create_opts =qemu_opts_append (create_opts, proto_drv->create_opts); - opts =qemu_opts_create (create_opts,NULL ,0 , &error_abort); +if (!skip_create) { +if (!drv->create_opts) { +error_report ("Format driver '%s' does not support image creation" , + drv->format_name); + ret =-1 ; + goto out; + } +if (!proto_drv->create_opts) { +error_report ("Protocol driver '%s' does not support image creation" , + proto_drv->format_name); + ret =-1 ; + goto out; + } + create_opts =qemu_opts_append (create_opts, drv->create_opts); + create_opts =qemu_opts_append (create_opts, proto_drv->create_opts); + + opts =qemu_opts_create (create_opts,NULL ,0 , &error_abort); + } size =blk_getlength (blk1);if (size <0 ) {@@ -4083,19 +4089,22 @@ static int img_dd(int argc, char **argv)/* Overflow means the specified offset is beyond input image's size */ if (dd.flags &C_SKIP && (in.offset >INT64_MAX / in.bsz || size < in.bsz * in.offset)) { -qemu_opt_set_number (opts,BLOCK_OPT_SIZE ,0 , &error_abort); + out_size =0 ; }else { -qemu_opt_set_number (opts,BLOCK_OPT_SIZE , - size - in.bsz * in.offset, &error_abort); + out_size = size - in.bsz * in.offset; } - ret =bdrv_create (drv, out.filename, opts, &local_err); -if (ret <0 ) { -error_reportf_err (local_err, -"%s: error while creating output image: " , - out.filename); - ret =-1 ; - goto out; +if (!skip_create) { +qemu_opt_set_number (opts,BLOCK_OPT_SIZE , out_size, &error_abort); + + ret =bdrv_create (drv, out.filename, opts, &local_err); +if (ret <0 ) { +error_reportf_err (local_err, +"%s: error while creating output image: " , + out.filename); + ret =-1 ; + goto out; + } }/* TODO, we can't honour --image-opts for the target, @@ -4111,6 +4120,20 @@ static int img_dd(int argc, char **argv) goto out; } +if (skip_create) { +int64_t existing_size =blk_getlength (blk2); +if (existing_size <0 ) { +error_report ("unable to get output image length: %s" , +strerror (-existing_size)); + ret =-1 ; + goto out; + }else if (existing_size < out_size) { +error_report ("output file is smaller than input data length" ); + ret =-1 ; + goto out; + } + } +if (dd.flags &C_SKIP && (in.offset >INT64_MAX / in.bsz || size < in.offset * in.bsz)) {/* We give a warning if the skip option is bigger than the input qemu-img.texi +6
-1@@ -326,7 +326,7 @@ skipped. This is useful for formats such as @code{rbd} if the target volume has already been created with site specific options that cannot be supplied through qemu-img. -@item dd [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] if=@var{input} of=@var{output} +@item dd [-n] [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] if=@var{input} of=@var{output} Dd copies from @var{input} file to @var{output} file converting it from @var{fmt} format to @var{output_fmt} format.@@ -337,6 +337,11 @@ dd will stop reading input after reading @var{blocks} input blocks. The size syntax is similar to dd(1)'s size syntax. +If the @code{-n} option is specified, the target volume creation will be +skipped. This is useful for formats such as @code{rbd} if the target +volume has already been created with site specific options that cannot +be supplied through qemu-img. + @item info [-f @var{fmt}] [--output=@var{ofmt}] [--backing-chain] @var{filename} Give information about the disk image @var{filename}. Use it inSignature · 1 line
2.9.3
Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command
Quoted 9 lines · Fam Zheng
Quoted 3 lines · Daniel P. Berrange
The -n arg to the convert command allows use of a pre-existing image, rather than creating a new image. This adds a -n arg to the dd command to get feature parity.
I remember there was a discussion about changing qemu-img dd's default to a "conv=nocreat" semantic, if so, "-n" might not be that useful. But that part hasn't made it into the tree, and I'm not sure which direction we should take. (Personally I think default to nocreat is a good idea).
Use nocreat by default would be semantically different from real "dd" binary which feels undesirable if the goal is to make "qemu-img dd" be as consistent with "dd" as possible.
It would be trivial to rewrite this patch to add support for the "conv" option, allowing the user to explicitly give 'qemu-img dd conv=nocreat' instead of my 'qemu-img dd -n' syntax, without changing default semantics.
Regards,
Daniel
Signature · 3 lines
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command
Quoted 3 lines · Daniel P. Berrange
It would be trivial to rewrite this patch to add support for the "conv" option, allowing the user to explicitly give 'qemu-img dd conv=nocreat' instead of my 'qemu-img dd -n' syntax, without changing default semantics.
OK, that would be an undebatably way to do this.
Fam
Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command
In reply to Daniel P. BerrangeQuoted 11 lines · Daniel P. Berrange
Quoted 9 lines · Fam Zheng
Quoted 3 lines · Daniel P. Berrange
The -n arg to the convert command allows use of a pre-existing image, rather than creating a new image. This adds a -n arg to the dd command to get feature parity.
I remember there was a discussion about changing qemu-img dd's default to a "conv=nocreat" semantic, if so, "-n" might not be that useful. But that part hasn't made it into the tree, and I'm not sure which direction we should take. (Personally I think default to nocreat is a good idea).
Quoted 7 lines
Use nocreat by default would be semantically different from real "dd" binary which feels undesirable if the goal is to make "qemu-img dd" be as consistent with "dd" as possible.
It would be trivial to rewrite this patch to add support for the "conv" option, allowing the user to explicitly give 'qemu-img dd conv=nocreat' instead of my 'qemu-img dd -n' syntax, without changing default semantics.
Adding 'conv=nocreat' (and not '-n') feels like the right way to me.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command
Quoted 16 lines · Eric Blake
Quoted 15 lines · Daniel P. Berrange
Quoted 9 lines · Fam Zheng
Quoted 3 lines · Daniel P. Berrange
The -n arg to the convert command allows use of a pre-existing image, rather than creating a new image. This adds a -n arg to the dd command to get feature parity.
I remember there was a discussion about changing qemu-img dd's default to a "conv=nocreat" semantic, if so, "-n" might not be that useful. But that part hasn't made it into the tree, and I'm not sure which direction we should take. (Personally I think default to nocreat is a good idea).
Use nocreat by default would be semantically different from real "dd" binary which feels undesirable if the goal is to make "qemu-img dd" be as consistent with "dd" as possible.
Quoted 5 lines
Quoted 3 lines
It would be trivial to rewrite this patch to add support for the "conv" option, allowing the user to explicitly give 'qemu-img dd conv=nocreat' instead of my 'qemu-img dd -n' syntax, without changing default semantics.
Adding 'conv=nocreat' (and not '-n') feels like the right way to me.
The original idea was to make conv=nocreat a mandatory option, I think. qemu-img was supposed error out if the user did not specify it.
Max
Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command
Quoted 17 lines · Max Reitz
Quoted 16 lines · Eric Blake
Quoted 15 lines · Daniel P. Berrange
Quoted 9 lines · Fam Zheng
Quoted 3 lines · Daniel P. Berrange
The -n arg to the convert command allows use of a pre-existing image, rather than creating a new image. This adds a -n arg to the dd command to get feature parity.
I remember there was a discussion about changing qemu-img dd's default to a "conv=nocreat" semantic, if so, "-n" might not be that useful. But that part hasn't made it into the tree, and I'm not sure which direction we should take. (Personally I think default to nocreat is a good idea).
Use nocreat by default would be semantically different from real "dd" binary which feels undesirable if the goal is to make "qemu-img dd" be as consistent with "dd" as possible.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
It would be trivial to rewrite this patch to add support for the "conv" option, allowing the user to explicitly give 'qemu-img dd conv=nocreat' instead of my 'qemu-img dd -n' syntax, without changing default semantics.
Adding 'conv=nocreat' (and not '-n') feels like the right way to me.
The original idea was to make conv=nocreat a mandatory option, I think. qemu-img was supposed error out if the user did not specify it.
I'm not really seeing a benefit in doing that - it would just break existing usage of qemu-img dd for no obvious benefit.
FYI, I've implemented "conv" with "nocreat" and "notrunc" options which i'll post once i've tested some more.
Regards,
Daniel
Signature · 3 lines
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command
Quoted 22 lines · Daniel P. Berrange
Quoted 21 lines · Max Reitz
Quoted 20 lines · Eric Blake
Quoted 18 lines · Daniel P. Berrange
Quoted 9 lines · Fam Zheng
Quoted 3 lines · Daniel P. Berrange
The -n arg to the convert command allows use of a pre-existing image, rather than creating a new image. This adds a -n arg to the dd command to get feature parity.
I remember there was a discussion about changing qemu-img dd's default to a "conv=nocreat" semantic, if so, "-n" might not be that useful. But that part hasn't made it into the tree, and I'm not sure which direction we should take. (Personally I think default to nocreat is a good idea).
Use nocreat by default would be semantically different from real "dd" binary which feels undesirable if the goal is to make "qemu-img dd" be as consistent with "dd" as possible.
It would be trivial to rewrite this patch to add support for the "conv" option, allowing the user to explicitly give 'qemu-img dd conv=nocreat' instead of my 'qemu-img dd -n' syntax, without changing default semantics.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Adding 'conv=nocreat' (and not '-n') feels like the right way to me.
The original idea was to make conv=nocreat a mandatory option, I think. qemu-img was supposed error out if the user did not specify it.
I'm not really seeing a benefit in doing that - it would just break existing usage of qemu-img dd for no obvious benefit.
Well... Is there existing usage?
The benefit would be that one could (should?) expect qemu-img dd to behave on disk images as if they were block devices; and dd to a block device will not truncate or "recreate" it.
If you don't give nocreat, it's thus a bit unclear whether you want to delete and recreate the target or whether you want to write into it. Some may expect qemu-img dd to behave as if the target is a normal file (delete and recreate it), others may expect it's treated like a block device (just write into it). If you force the user to specify nocreat, it would make the behavior clear.
(And you can always delete+recreate the target with qemu-img create.)
It's all a bit complicated. :-/
Max
Quoted 6 lines
FYI, I've implemented "conv" with "nocreat" and "notrunc" options which i'll post once i've tested some more.
Regards,
Daniel
Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command
Quoted 25 lines · Max Reitz
Quoted 24 lines · Daniel P. Berrange
Quoted 23 lines · Max Reitz
Quoted 21 lines · Eric Blake
Quoted 11 lines · Daniel P. Berrange
Quoted 9 lines · Fam Zheng
Quoted 3 lines · Daniel P. Berrange
The -n arg to the convert command allows use of a pre-existing image, rather than creating a new image. This adds a -n arg to the dd command to get feature parity.
I remember there was a discussion about changing qemu-img dd's default to a "conv=nocreat" semantic, if so, "-n" might not be that useful. But that part hasn't made it into the tree, and I'm not sure which direction we should take. (Personally I think default to nocreat is a good idea).
Quoted 7 lines
Use nocreat by default would be semantically different from real "dd" binary which feels undesirable if the goal is to make "qemu-img dd" be as consistent with "dd" as possible.
It would be trivial to rewrite this patch to add support for the "conv" option, allowing the user to explicitly give 'qemu-img dd conv=nocreat' instead of my 'qemu-img dd -n' syntax, without changing default semantics.
Adding 'conv=nocreat' (and not '-n') feels like the right way to me.
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
The original idea was to make conv=nocreat a mandatory option, I think. qemu-img was supposed error out if the user did not specify it.
I'm not really seeing a benefit in doing that - it would just break existing usage of qemu-img dd for no obvious benefit.
Well... Is there existing usage?
It shipped in 2.8.0 though, so imho that means we have to assume there are users, and thus additions must to be backwards compatible from now on.
Quoted 11 lines
The benefit would be that one could (should?) expect qemu-img dd to behave on disk images as if they were block devices; and dd to a block device will not truncate or "recreate" it.
If you don't give nocreat, it's thus a bit unclear whether you want to delete and recreate the target or whether you want to write into it. Some may expect qemu-img dd to behave as if the target is a normal file (delete and recreate it), others may expect it's treated like a block device (just write into it). If you force the user to specify nocreat, it would make the behavior clear.
Quoted 3 lines
(And you can always delete+recreate the target with qemu-img create.)
It's all a bit complicated. :-/
If the goal is to be compatible with /usr/bin/dd then IIUC, the behaviour needs to be
- If target is a block device, then silently assume nocreat|notrunc is set, even if not specified by user
- If target is a file, then silently create & truncate the file unless nocreat or notrunc are set
Regards,
Daniel
Signature · 3 lines
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command
Quoted 29 lines · Daniel P. Berrange
Quoted 28 lines · Max Reitz
Quoted 27 lines · Daniel P. Berrange
Quoted 25 lines · Max Reitz
Quoted 16 lines · Eric Blake
Quoted 15 lines · Daniel P. Berrange
Quoted 9 lines · Fam Zheng
Quoted 3 lines · Daniel P. Berrange
The -n arg to the convert command allows use of a pre-existing image, rather than creating a new image. This adds a -n arg to the dd command to get feature parity.
I remember there was a discussion about changing qemu-img dd's default to a "conv=nocreat" semantic, if so, "-n" might not be that useful. But that part hasn't made it into the tree, and I'm not sure which direction we should take. (Personally I think default to nocreat is a good idea).
Use nocreat by default would be semantically different from real "dd" binary which feels undesirable if the goal is to make "qemu-img dd" be as consistent with "dd" as possible.
Quoted 5 lines
Quoted 3 lines
It would be trivial to rewrite this patch to add support for the "conv" option, allowing the user to explicitly give 'qemu-img dd conv=nocreat' instead of my 'qemu-img dd -n' syntax, without changing default semantics.
Adding 'conv=nocreat' (and not '-n') feels like the right way to me.
The original idea was to make conv=nocreat a mandatory option, I think. qemu-img was supposed error out if the user did not specify it.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
I'm not really seeing a benefit in doing that - it would just break existing usage of qemu-img dd for no obvious benefit.
Well... Is there existing usage?
It shipped in 2.8.0 though, so imho that means we have to assume there are users, and thus additions must to be backwards compatible from now on.
Depends. I don't think there are too many users, so we could still justify a change if there's a very good reason for it.
I do agree that it's probably not a very good reason, though.
Quoted 15 lines
Quoted 11 lines
The benefit would be that one could (should?) expect qemu-img dd to behave on disk images as if they were block devices; and dd to a block device will not truncate or "recreate" it.
If you don't give nocreat, it's thus a bit unclear whether you want to delete and recreate the target or whether you want to write into it. Some may expect qemu-img dd to behave as if the target is a normal file (delete and recreate it), others may expect it's treated like a block device (just write into it). If you force the user to specify nocreat, it would make the behavior clear.
Quoted 3 lines
(And you can always delete+recreate the target with qemu-img create.)
It's all a bit complicated. :-/
Quoted 8 lines
If the goal is to be compatible with
/usr/bin/ddthen IIUC, the behaviour needs to be- If target is a block device, then silently assume nocreat|notrunc is set, even if not specified by user
- If target is a file, then silently create & truncate the file unless nocreat or notrunc are set
Yes. But you could easily argue that every image file is a "block device".
Max
Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command
Quoted 62 lines · Max Reitz
Quoted 29 lines · Daniel P. Berrange
Quoted 28 lines · Max Reitz
Quoted 27 lines · Daniel P. Berrange
Quoted 25 lines · Max Reitz
Quoted 16 lines · Eric Blake
Quoted 15 lines · Daniel P. Berrange
Quoted 9 lines · Fam Zheng
Quoted 3 lines · Daniel P. Berrange
The -n arg to the convert command allows use of a pre-existing image, rather than creating a new image. This adds a -n arg to the dd command to get feature parity.
I remember there was a discussion about changing qemu-img dd's default to a "conv=nocreat" semantic, if so, "-n" might not be that useful. But that part hasn't made it into the tree, and I'm not sure which direction we should take. (Personally I think default to nocreat is a good idea).
Use nocreat by default would be semantically different from real "dd" binary which feels undesirable if the goal is to make "qemu-img dd" be as consistent with "dd" as possible.
Quoted 5 lines
Quoted 3 lines
It would be trivial to rewrite this patch to add support for the "conv" option, allowing the user to explicitly give 'qemu-img dd conv=nocreat' instead of my 'qemu-img dd -n' syntax, without changing default semantics.
Adding 'conv=nocreat' (and not '-n') feels like the right way to me.
The original idea was to make conv=nocreat a mandatory option, I think. qemu-img was supposed error out if the user did not specify it.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
I'm not really seeing a benefit in doing that - it would just break existing usage of qemu-img dd for no obvious benefit.
Well... Is there existing usage?
It shipped in 2.8.0 though, so imho that means we have to assume there are users, and thus additions must to be backwards compatible from now on.
Depends. I don't think there are too many users, so we could still justify a change if there's a very good reason for it.
I do agree that it's probably not a very good reason, though.
Quoted 18 lines
Quoted 11 lines
The benefit would be that one could (should?) expect qemu-img dd to behave on disk images as if they were block devices; and dd to a block device will not truncate or "recreate" it.
If you don't give nocreat, it's thus a bit unclear whether you want to delete and recreate the target or whether you want to write into it. Some may expect qemu-img dd to behave as if the target is a normal file (delete and recreate it), others may expect it's treated like a block device (just write into it). If you force the user to specify nocreat, it would make the behavior clear.
Quoted 3 lines
(And you can always delete+recreate the target with qemu-img create.)
It's all a bit complicated. :-/
If the goal is to be compatible with
/usr/bin/ddthen IIUC, the behaviour needs to be
Quoted 7 lines
Quoted 5 lines
- If target is a block device, then silently assume nocreat|notrunc is set, even if not specified by user
- If target is a file, then silently create & truncate the file unless nocreat or notrunc are set
Yes. But you could easily argue that every image file is a "block device".
IMHO that would be a bad idea as it would mean different behaviour from dd vs qemu-img dd, when run on raw files.
If we assume nocreat|notrunc behaviour by default, then we would likely need to invent new "creat|trunc" flags to let people turn the previous behaviour back on, which would diverge from 'dd' command.
Regards,
Daniel
Signature · 3 lines
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command
Quoted 4 lines · Daniel P. Berrange
Quoted 3 lines · Max Reitz
Quoted 2 lines · Daniel P. Berrange
Quoted 1 line · Max Reitz
On 01.02.2017 13:16, Daniel P. Berrange wrote:
[...]
Quoted 21 lines
Quoted 21 lines
Quoted 21 lines
Quoted 11 lines
The benefit would be that one could (should?) expect qemu-img dd to behave on disk images as if they were block devices; and dd to a block device will not truncate or "recreate" it.
If you don't give nocreat, it's thus a bit unclear whether you want to delete and recreate the target or whether you want to write into it. Some may expect qemu-img dd to behave as if the target is a normal file (delete and recreate it), others may expect it's treated like a block device (just write into it). If you force the user to specify nocreat, it would make the behavior clear.
Quoted 3 lines
(And you can always delete+recreate the target with qemu-img create.)
It's all a bit complicated. :-/
If the goal is to be compatible with
/usr/bin/ddthen IIUC, the behaviour needs to be- If target is a block device, then silently assume nocreat|notrunc is set, even if not specified by user
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
- If target is a file, then silently create & truncate the file unless nocreat or notrunc are set
Yes. But you could easily argue that every image file is a "block device".
IMHO that would be a bad idea as it would mean different behaviour from dd vs qemu-img dd, when run on raw files.
From the perspective of qemu-img, however, a raw file is not a raw file but a disk image in raw format.
Quoted 3 lines
If we assume nocreat|notrunc behaviour by default, then we would likely need to invent new "creat|trunc" flags to let people turn the previous behaviour back on, which would diverge from 'dd' command.
Not really, because you could just use qemu-img create.
I understand your standpoint. I'm just saying there is another standpoint that also makes a lot of sense and that would imply different default behavior.
In the end, retaining backwards compatibility will probably win the discussion automatically, though. That is, always overwrite the target image by default.
By the way, I plan to integrate all of qemu-img dd's functionality into qemu-img convert eventually and make qemu-img dd just another interface for qemu-img convert. Therefore, I'm all in favor of not touch qemu-img dd until that is done so it doesn't get any more weird behavior that needs to be supported later.
Max
Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command
In reply to Daniel P. BerrangeQuoted 69 lines · Daniel P. Berrange
Quoted 68 lines · Max Reitz
Quoted 29 lines · Daniel P. Berrange
Quoted 28 lines · Max Reitz
Quoted 27 lines · Daniel P. Berrange
Quoted 25 lines · Max Reitz
Quoted 16 lines · Eric Blake
Quoted 15 lines · Daniel P. Berrange
Quoted 9 lines · Fam Zheng
Quoted 3 lines · Daniel P. Berrange
The -n arg to the convert command allows use of a pre-existing image, rather than creating a new image. This adds a -n arg to the dd command to get feature parity.
I remember there was a discussion about changing qemu-img dd's default to a "conv=nocreat" semantic, if so, "-n" might not be that useful. But that part hasn't made it into the tree, and I'm not sure which direction we should take. (Personally I think default to nocreat is a good idea).
Use nocreat by default would be semantically different from real "dd" binary which feels undesirable if the goal is to make "qemu-img dd" be as consistent with "dd" as possible.
Quoted 5 lines
Quoted 3 lines
It would be trivial to rewrite this patch to add support for the "conv" option, allowing the user to explicitly give 'qemu-img dd conv=nocreat' instead of my 'qemu-img dd -n' syntax, without changing default semantics.
Adding 'conv=nocreat' (and not '-n') feels like the right way to me.
The original idea was to make conv=nocreat a mandatory option, I think. qemu-img was supposed error out if the user did not specify it.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
I'm not really seeing a benefit in doing that - it would just break existing usage of qemu-img dd for no obvious benefit.
Well... Is there existing usage?
It shipped in 2.8.0 though, so imho that means we have to assume there are users, and thus additions must to be backwards compatible from now on.
Depends. I don't think there are too many users, so we could still justify a change if there's a very good reason for it.
I do agree that it's probably not a very good reason, though.
Quoted 23 lines
Quoted 11 lines
The benefit would be that one could (should?) expect qemu-img dd to behave on disk images as if they were block devices; and dd to a block device will not truncate or "recreate" it.
If you don't give nocreat, it's thus a bit unclear whether you want to delete and recreate the target or whether you want to write into it. Some may expect qemu-img dd to behave as if the target is a normal file (delete and recreate it), others may expect it's treated like a block device (just write into it). If you force the user to specify nocreat, it would make the behavior clear.
Quoted 3 lines
(And you can always delete+recreate the target with qemu-img create.)
It's all a bit complicated. :-/
If the goal is to be compatible with
/usr/bin/ddthen IIUC, the behaviour needs to be- If target is a block device, then silently assume nocreat|notrunc is set, even if not specified by user
- If target is a file, then silently create & truncate the file unless nocreat or notrunc are set
Quoted 8 lines
Quoted 1 line
Yes. But you could easily argue that every image file is a "block device".
IMHO that would be a bad idea as it would mean different behaviour from dd vs qemu-img dd, when run on raw files.
If we assume nocreat|notrunc behaviour by default, then we would likely need to invent new "creat|trunc" flags to let people turn the previous behaviour back on, which would diverge from 'dd' command.
/bin/dd provides conv=excl, but it's not POSIX.
Quote http://pubs.opengroup.org/onlinepubs/9699919799/
of=file
Specify the output pathname; the default is standard output. If the seek= expr conversion is not also specified, the output file shall be truncated before the copy begins if an explicit of= file operand is specified, unless conv= notrunc is specified. If seek= expr is specified, but conv= notrunc is not, the effect of the copy shall be to preserve the blocks in the output file over which dd seeks, but no other portion of the output file shall be preserved. (If the size of the seek plus the size of the input file is less than the previous size of the output file, the output file shall be shortened by the copy. If the input file is empty and either the size of the seek is greater than the previous size of the output file or the output file did not previously exist, the size of the output file shall be set to the file offset after the seek.)
[...]
conv=value[,value ...]
[...]
notrunc
Do not truncate the output file. Preserve blocks in the output file not explicitly written by this invocation of the dd utility. (See also the preceding of= file operand.)
Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command
In reply to Max ReitzQuoted 62 lines · Max Reitz
Quoted 29 lines · Daniel P. Berrange
Quoted 28 lines · Max Reitz
Quoted 27 lines · Daniel P. Berrange
Quoted 25 lines · Max Reitz
Quoted 16 lines · Eric Blake
Quoted 15 lines · Daniel P. Berrange
Quoted 9 lines · Fam Zheng
Quoted 3 lines · Daniel P. Berrange
The -n arg to the convert command allows use of a pre-existing image, rather than creating a new image. This adds a -n arg to the dd command to get feature parity.
I remember there was a discussion about changing qemu-img dd's default to a "conv=nocreat" semantic, if so, "-n" might not be that useful. But that part hasn't made it into the tree, and I'm not sure which direction we should take. (Personally I think default to nocreat is a good idea).
Use nocreat by default would be semantically different from real "dd" binary which feels undesirable if the goal is to make "qemu-img dd" be as consistent with "dd" as possible.
Quoted 5 lines
Quoted 3 lines
It would be trivial to rewrite this patch to add support for the "conv" option, allowing the user to explicitly give 'qemu-img dd conv=nocreat' instead of my 'qemu-img dd -n' syntax, without changing default semantics.
Adding 'conv=nocreat' (and not '-n') feels like the right way to me.
The original idea was to make conv=nocreat a mandatory option, I think. qemu-img was supposed error out if the user did not specify it.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
I'm not really seeing a benefit in doing that - it would just break existing usage of qemu-img dd for no obvious benefit.
Well... Is there existing usage?
It shipped in 2.8.0 though, so imho that means we have to assume there are users, and thus additions must to be backwards compatible from now on.
Depends. I don't think there are too many users, so we could still justify a change if there's a very good reason for it.
I do agree that it's probably not a very good reason, though.
Quoted 18 lines
Quoted 11 lines
The benefit would be that one could (should?) expect qemu-img dd to behave on disk images as if they were block devices; and dd to a block device will not truncate or "recreate" it.
If you don't give nocreat, it's thus a bit unclear whether you want to delete and recreate the target or whether you want to write into it. Some may expect qemu-img dd to behave as if the target is a normal file (delete and recreate it), others may expect it's treated like a block device (just write into it). If you force the user to specify nocreat, it would make the behavior clear.
Quoted 3 lines
(And you can always delete+recreate the target with qemu-img create.)
It's all a bit complicated. :-/
If the goal is to be compatible with
/usr/bin/ddthen IIUC, the behaviour needs to be
Quoted 7 lines
Quoted 5 lines
- If target is a block device, then silently assume nocreat|notrunc is set, even if not specified by user
- If target is a file, then silently create & truncate the file unless nocreat or notrunc are set
Yes. But you could easily argue that every image file is a "block device".
No. /bin/dd treats exactly block special files as block special files, so the qemu-img command that tries to behave like it should do, too.
In case you say that's inconvenient: pretty much everything about dd's archaic user interface is inconvenient. If you want convenient, roll your own. If you want familiar, stick to the original.
Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command
Quoted 67 lines · Markus Armbruster
Quoted 65 lines · Max Reitz
Quoted 29 lines · Daniel P. Berrange
Quoted 28 lines · Max Reitz
Quoted 27 lines · Daniel P. Berrange
Quoted 25 lines · Max Reitz
Quoted 16 lines · Eric Blake
Quoted 15 lines · Daniel P. Berrange
Quoted 9 lines · Fam Zheng
Quoted 3 lines · Daniel P. Berrange
The -n arg to the convert command allows use of a pre-existing image, rather than creating a new image. This adds a -n arg to the dd command to get feature parity.
I remember there was a discussion about changing qemu-img dd's default to a "conv=nocreat" semantic, if so, "-n" might not be that useful. But that part hasn't made it into the tree, and I'm not sure which direction we should take. (Personally I think default to nocreat is a good idea).
Use nocreat by default would be semantically different from real "dd" binary which feels undesirable if the goal is to make "qemu-img dd" be as consistent with "dd" as possible.
Quoted 5 lines
Quoted 3 lines
It would be trivial to rewrite this patch to add support for the "conv" option, allowing the user to explicitly give 'qemu-img dd conv=nocreat' instead of my 'qemu-img dd -n' syntax, without changing default semantics.
Adding 'conv=nocreat' (and not '-n') feels like the right way to me.
The original idea was to make conv=nocreat a mandatory option, I think. qemu-img was supposed error out if the user did not specify it.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
I'm not really seeing a benefit in doing that - it would just break existing usage of qemu-img dd for no obvious benefit.
Well... Is there existing usage?
It shipped in 2.8.0 though, so imho that means we have to assume there are users, and thus additions must to be backwards compatible from now on.
Depends. I don't think there are too many users, so we could still justify a change if there's a very good reason for it.
I do agree that it's probably not a very good reason, though.
Quoted 21 lines
Quoted 11 lines
The benefit would be that one could (should?) expect qemu-img dd to behave on disk images as if they were block devices; and dd to a block device will not truncate or "recreate" it.
If you don't give nocreat, it's thus a bit unclear whether you want to delete and recreate the target or whether you want to write into it. Some may expect qemu-img dd to behave as if the target is a normal file (delete and recreate it), others may expect it's treated like a block device (just write into it). If you force the user to specify nocreat, it would make the behavior clear.
Quoted 3 lines
(And you can always delete+recreate the target with qemu-img create.)
It's all a bit complicated. :-/
If the goal is to be compatible with
/usr/bin/ddthen IIUC, the behaviour needs to be- If target is a block device, then silently assume nocreat|notrunc is set, even if not specified by user
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
- If target is a file, then silently create & truncate the file unless nocreat or notrunc are set
Yes. But you could easily argue that every image file is a "block device".
No.
/bin/ddtreats exactly block special files as block special files, so the qemu-img command that tries to behave like it should do, too.
*/usr*/bin/dd (O:-)) also treats qcow2 files like raw files.
Quoted 3 lines
In case you say that's inconvenient: pretty much everything about dd's archaic user interface is inconvenient. If you want convenient, roll your own. If you want familiar, stick to the original.
I agree. But qemu-img dd already is not dd. It interprets disk image files as virtual disks instead of as plain files. The question is whether virtual disks are to be treated as block devices or as files.
I don't have a strong opinion on the matter. Either way will surprise some people. The original issue was whether to make nocreat/notrunc a mandatory option, so if we didn't have any backwards compatibility issues, it would be the following two surprises:
(1) Don't make nocreat/notrunc mandatory (as it is now). Then people who expect qemu-img dd to treat image files as block devices will be surprised that all their data is gone. Bad.
(2) Make it mandatory. Then people who don't expect this (i.e. everyone) will be surprised about the error message "nocreat is mandatory, please use it." Then they will fix their command line and can proceed. Much less bad.
Of course, "much less bad" is offset by the fact that everyone is affected. I think it's still less bad, though.
But we do have backwards compatibility to care about (at least a bit), so I don't think it's worth changing the behavior now.
You may argue that actually nobody will be affected by (1). But I don't think so. I personally usually use dd to copy something off or onto a device file, often block devices. Disk images literally are block devices in the guest. It is entirely conceivable that someone would (a) associate dd with "copy things from/to block devices" and (b) assume that *qemu*-img dd behaves as from the guest's perspective (where the image is a block device).
That is because qemu-img dd does *not* view the image entirely from the host's perspective. It does interpret the image file contents, i.e., it does look at it from a guest's perspective in that regard.
I agree that qemu-img dd probably was a bad idea. I agree that we need to make sure people know that it is basically a different interface of qemu-img convert.
Yes, yes, I'll get to write the conversion and man page updates when I find the time to do so.
Max
Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command
Quoted 8 lines · Max Reitz
Quoted 3 lines
In case you say that's inconvenient: pretty much everything about dd's archaic user interface is inconvenient. If you want convenient, roll your own. If you want familiar, stick to the original.
I agree. But qemu-img dd already is not dd. It interprets disk image files as virtual disks instead of as plain files. The question is whether virtual disks are to be treated as block devices or as files.
Quoted 8 lines
I don't have a strong opinion on the matter. Either way will surprise some people. The original issue was whether to make nocreat/notrunc a mandatory option, so if we didn't have any backwards compatibility issues, it would be the following two surprises:
(1) Don't make nocreat/notrunc mandatory (as it is now). Then people who expect qemu-img dd to treat image files as block devices will be surprised that all their data is gone. Bad.
I don't think people really expect qemu-img to treat image file as if they were block devices when operating on the host.
It is like saying people expect /usr/bin/dd to treat a plain file as a block device, because they might use it with losetup later.
Regards,
Daniel
Signature · 3 lines
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command
First, because this is perhaps the most important thing: I think I remembered what the original proposal to solve all this mess, or at least move it to a later point:
We wanted to just disallow overwriting existing files without conv=notrunc. I think.
The thing is that it's pretty much impossible with the qemu block layer to determine whether a file exists or not. Maybe you cannot open it but it would be possible to overwrite it. This is the reason the patches for this did not make it into 2.8.
Quoted 18 lines · Daniel P. Berrange
Quoted 8 lines · Max Reitz
Quoted 3 lines
In case you say that's inconvenient: pretty much everything about dd's archaic user interface is inconvenient. If you want convenient, roll your own. If you want familiar, stick to the original.
I agree. But qemu-img dd already is not dd. It interprets disk image files as virtual disks instead of as plain files. The question is whether virtual disks are to be treated as block devices or as files.
Quoted 8 lines
I don't have a strong opinion on the matter. Either way will surprise some people. The original issue was whether to make nocreat/notrunc a mandatory option, so if we didn't have any backwards compatibility issues, it would be the following two surprises:
(1) Don't make nocreat/notrunc mandatory (as it is now). Then people who expect qemu-img dd to treat image files as block devices will be surprised that all their data is gone. Bad.
Quoted 5 lines
I don't think people really expect qemu-img to treat image file as if they were block devices when operating on the host.
It is like saying people expect
/usr/bin/ddto treat a plain file as a block device, because they might use it with losetup later.
That's not a good comparison. Disk images are meant to be used with qemu (or some other VMM, or, yes, with losetup if it's a raw image). Plain files can be anything. No, dd does not look into the file to determine whether it may be a raw disk image or not, but it does execute fstat() to find out whether it's a plain file or a block device.
Furthermore, you are omitting the sentence where I said that qemu-img dd does not operate on just the host level. It interprets the disk image contents, thus it is, in a sense, operating from the guest's perspective. It's somewhere in between, which is why I argued that some people may assume it's a guest-level tool. At least it's on the same level as block jobs are, whatever that may mean.
(Well, it does mean that block jobs with mode=existing do not truncate the output, but the default is mode=absolute-paths, so they would overwrite it anyway. So there's that.)
I agree that I would probably not expect qemu-img dd to treat images like block devices. At least I would test it first. But I stand by the point that it is conceivable to think so, and thus we have to assume there are such people.
They may be few, but I reasoned that reality would hit them much harder than all the rest would be affected if we implemented the opposite behavior.
It's a simple trade-off that we did not consider; or rather that we considered too late.
I also agree with you that it's too late to change now.
Max
Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command
Quoted 11 lines · Max Reitz
First, because this is perhaps the most important thing: I think I remembered what the original proposal to solve all this mess, or at least move it to a later point:
We wanted to just disallow overwriting existing files without conv=notrunc. I think.
The thing is that it's pretty much impossible with the qemu block layer to determine whether a file exists or not. Maybe you cannot open it but it would be possible to overwrite it. This is the reason the patches for this did not make it into 2.8.
The only sane way to do "create unless it already exists" is O_CREAT|O_EXCL. Either you can do that, or you can't.
Quoted 22 lines · Daniel P. Berrange
Quoted 21 lines · Max Reitz
Quoted 8 lines
Quoted 3 lines
In case you say that's inconvenient: pretty much everything about dd's archaic user interface is inconvenient. If you want convenient, roll your own. If you want familiar, stick to the original.
I agree. But qemu-img dd already is not dd. It interprets disk image files as virtual disks instead of as plain files. The question is whether virtual disks are to be treated as block devices or as files.
Quoted 8 lines
I don't have a strong opinion on the matter. Either way will surprise some people. The original issue was whether to make nocreat/notrunc a mandatory option, so if we didn't have any backwards compatibility issues, it would be the following two surprises:
(1) Don't make nocreat/notrunc mandatory (as it is now). Then people who expect qemu-img dd to treat image files as block devices will be surprised that all their data is gone. Bad.
I don't think people really expect qemu-img to treat image file as if they were block devices when operating on the host.
Quoted 8 lines
Quoted 2 lines
It is like saying people expect
/usr/bin/ddto treat a plain file as a block device, because they might use it with losetup later.That's not a good comparison. Disk images are meant to be used with qemu (or some other VMM, or, yes, with losetup if it's a raw image). Plain files can be anything. No, dd does not look into the file to determine whether it may be a raw disk image or not, but it does execute
fstat()to find out whether it's a plain file or a block device.
Actually, it doesn't. coreutils-8.26/src/dd.c:
mode_t perms = MODE_RW_UGO;
int opts
= (output_flags
| (conversions_mask & C_NOCREAT ? 0 : O_CREAT)
| (conversions_mask & C_EXCL ? O_EXCL : 0)
| (seek_records || (conversions_mask & C_NOTRUNC) ? 0 : O_TRUNC));
/* Open the output file with *read* access only if we might
need to read to satisfy a 'seek=' request. If we can't read
the file, go ahead with write-only access; it might work. */
if ((! seek_records
|| ifd_reopen (STDOUT_FILENO, output_file, O_RDWR | opts, perms) < 0)
&& (ifd_reopen (STDOUT_FILENO, output_file, O_WRONLY | opts, perms)
< 0))
die (EXIT_FAILURE, errno, _("failed to open %s"),
quoteaf (output_file));ifd_reopen() is a wrapper around open() that forces the file descriptor to a desired value (here: STDOUT_FILENO) and protects against EINTR.
If this doesn't truncate block special for you, it's simply because your OS interprets it that way, under license from POSIX:
O_TRUNC
If the file exists and is a regular file, and the file is successfully opened O_RDWR or O_WRONLY, its length is truncated to 0 and the mode and owner are unchanged. It will have no effect on FIFO special files or terminal device files. Its effect on other file types is implementation-dependent. The result of using O_TRUNC with O_RDONLY is undefined.
http://pubs.opengroup.org/onlinepubs/7990989775/xsh/open.html
Ignoring O_TRUNC is the traditional behavior. But the OS is free to surprise its applications and users with non-traditional behavior.
[...]
Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command
Quoted 9 lines · Markus Armbruster
Quoted 7 lines · Max Reitz
First, because this is perhaps the most important thing: I think I remembered what the original proposal to solve all this mess, or at least move it to a later point:
We wanted to just disallow overwriting existing files without conv=notrunc. I think.
Quoted 7 lines
Quoted 4 lines
The thing is that it's pretty much impossible with the qemu block layer to determine whether a file exists or not. Maybe you cannot open it but it would be possible to overwrite it. This is the reason the patches for this did not make it into 2.8.
The only sane way to do "create unless it already exists" is O_CREAT|O_EXCL. Either you can do that, or you can't.
All the more reason why we couldn't implement it in time.
(Also, implementing that just for qemu-img dd... Well, no.)
Quoted 64 lines
Quoted 22 lines · Daniel P. Berrange
Quoted 21 lines · Max Reitz
Quoted 8 lines
Quoted 3 lines
In case you say that's inconvenient: pretty much everything about dd's archaic user interface is inconvenient. If you want convenient, roll your own. If you want familiar, stick to the original.
I agree. But qemu-img dd already is not dd. It interprets disk image files as virtual disks instead of as plain files. The question is whether virtual disks are to be treated as block devices or as files.
Quoted 8 lines
I don't have a strong opinion on the matter. Either way will surprise some people. The original issue was whether to make nocreat/notrunc a mandatory option, so if we didn't have any backwards compatibility issues, it would be the following two surprises:
(1) Don't make nocreat/notrunc mandatory (as it is now). Then people who expect qemu-img dd to treat image files as block devices will be surprised that all their data is gone. Bad.
I don't think people really expect qemu-img to treat image file as if they were block devices when operating on the host.
Quoted 8 lines
Quoted 2 lines
It is like saying people expect
/usr/bin/ddto treat a plain file as a block device, because they might use it with losetup later.That's not a good comparison. Disk images are meant to be used with qemu (or some other VMM, or, yes, with losetup if it's a raw image). Plain files can be anything. No, dd does not look into the file to determine whether it may be a raw disk image or not, but it does execute
fstat()to find out whether it's a plain file or a block device.Actually, it doesn't.
coreutils-8.26/src/dd.c:mode_t perms = MODE_RW_UGO; int opts = (output_flags | (conversions_mask & C_NOCREAT ? 0 : O_CREAT) | (conversions_mask & C_EXCL ? O_EXCL : 0) | (seek_records || (conversions_mask & C_NOTRUNC) ? 0 : O_TRUNC)); /* Open the output file with *read* access only if we might need to read to satisfy a 'seek=' request. If we can't read the file, go ahead with write-only access; it might work. */ if ((! seek_records || ifd_reopen (STDOUT_FILENO, output_file, O_RDWR | opts, perms) < 0) && (ifd_reopen (STDOUT_FILENO, output_file, O_WRONLY | opts, perms) < 0)) die (EXIT_FAILURE, errno, _("failed to open %s"), quoteaf (output_file));
ifd_reopen()is a wrapper aroundopen()that forces the file descriptor to a desired value (here:STDOUT_FILENO) and protects against EINTR.If this doesn't truncate block special for you, it's simply because your OS interprets it that way, under license from POSIX:
O_TRUNC
If the file exists and is a regular file, and the file is successfully openedO_RDWRorO_WRONLY, its length is truncated to 0 and the mode and owner are unchanged. It will have no effect on FIFO special files or terminal device files. Its effect on other file types is implementation-dependent. The result of usingO_TRUNCwithO_RDONLYis undefined.
Quoted 4 lines
http://pubs.opengroup.org/onlinepubs/7990989775/xsh/open.html
Ignoring
O_TRUNCis the traditional behavior. But the OS is free to surprise its applications and users with non-traditional behavior.
Interesting. Thanks for the insight.
Well, then I have less issues about auto-truncate. "What? Your OS does not truncate your hard disk?"
Not that this changes my course of action (not going to change the default at this point), but maybe this will help me sleep better.
(Although arguing with what tools on POSIX-compatible OS are technically allowed to do compared to what they are usually expected to do is a bit of an issue still...)
Max
PATCH 4/6 qemu-img: add support for -o arg to dd command
In reply to Daniel P. BerrangeThe -o arg to the convert command allows specification of format/protocol options for the newly created image. This adds a -o arg to the dd command to get feature parity.
--- qemu-img-cmds.hx | 4 ++-- qemu-img.c | 32 +++++++++++++++++++++++++++++++- qemu-img.texi | 6 ++++-- 3 files changed, 37 insertions(+), 5 deletions(-)
qemu-img-cmds.hx +2 -2
@@ -46,9 +46,9 @@ STEXI ETEXI DEF("dd", img_dd, - "dd [--image-opts] [-n] [-f fmt] [-O output_fmt] [bs=block_size] [count=blocks] [skip=blocks] if=input of=output") + "dd [--image-opts] [-n] [-f fmt] [-O output_fmt] [-o options] [bs=block_size] [count=blocks] [skip=blocks] if=input of=output") STEXI -@item dd [--image-opts] [-n] [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] if=@var{input} of=@var{output} +@item dd [--image-opts] [-n] [-f @var{fmt}] [-O @var{output_fmt}] [-o @var{options}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] if=@var{input} of=@var{output} ETEXI DEF("info", img_info,
qemu-img.c +31 -1
@@ -3920,6 +3920,7 @@ static int img_dd(int argc, char **argv)int c, i, skip_create =0 ;const char *out_fmt ="raw" ;const char *fmt =NULL ; +char *optionstr =NULL ;int64_t size =0 , out_size;int64_t block_count =0 , out_pos, in_pos;struct DdInfo dd = {
@@ -3954,7 +3955,7 @@ static int img_dd(int argc, char **argv) { 0, 0, 0, 0 } }; -while ((c =getopt_long (argc, argv,"hnf:O:" , long_options,NULL ))) { +while ((c =getopt_long (argc, argv,"hno:f:O:" , long_options,NULL ))) {if (c ==EOF ) {break ; }
@@ -3968,6 +3969,20 @@ static int img_dd(int argc, char **argv)case 'n' : skip_create =1 ;break ; +case 'o' : +if (!is_valid_option_list (optarg)) { +error_report ("Invalid option list: %s" , optarg); + ret =-1 ; + goto out; + } +if (!optionstr) { + optionstr =g_strdup (optarg); + }else { +char *old_options = optionstr; + optionstr =g_strdup_printf ("%s,%s" , optionstr, optarg); +g_free (old_options); + } +break ;case '?' :error_report ("Try 'qemu-img --help' for more information." ); ret =-1 ;
@@ -4028,6 +4043,11 @@ static int img_dd(int argc, char **argv) goto out; } +if (optionstr &&has_help_option (optionstr)) { + ret =print_block_option_help (out.filename, out_fmt); + goto out; + } +if (qemu_opts_foreach (&qemu_object_opts, user_creatable_add_opts_foreach,NULL ,NULL )) {
@@ -4072,6 +4092,15 @@ static int img_dd(int argc, char **argv) create_opts =qemu_opts_append (create_opts, proto_drv->create_opts); opts =qemu_opts_create (create_opts,NULL ,0 , &error_abort); + +if (optionstr) { +qemu_opts_do_parse (opts, optionstr,NULL , &local_err); +if (local_err) { +error_report_err (local_err); + ret =-1 ; + goto out; + } + } } size =blk_getlength (blk1);
@@ -4176,6 +4205,7 @@ static int img_dd(int argc, char **argv) out:g_free (arg); +g_free (optionstr);qemu_opts_del (opts);qemu_opts_free (create_opts);blk_unref (blk1);
qemu-img.texi +4 -2
@@ -326,10 +326,12 @@ skipped. This is useful for formats such as @code{rbd} if the target volume has already been created with site specific options that cannot be supplied through qemu-img. -@item dd [-n] [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] if=@var{input} of=@var{output} +@item dd [-n] [-f @var{fmt}] [-O @var{output_fmt}] [-o @var{options}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] if=@var{input} of=@var{output} Dd copies from @var{input} file to @var{output} file converting it from -@var{fmt} format to @var{output_fmt} format. +@var{fmt} format to @var{output_fmt} format. Depending on the output file +format, you can add one or more @var{options} that enable additional +features of this format. The data is by default read and written using blocks of 512 bytes but can be modified by specifying @var{block_size}. If count=@var{blocks} is specified
Signature · 1 line
2.9.3
PATCH 5/6 qemu-img: introduce --target-image-opts for 'convert' command
In reply to Daniel P. BerrangeThe '--image-opts' flags indicates whether the source filename includes options. The target filename has to remain in the plain filename format though, since it needs to be passed to bdrv_create(). When using --skip-create though, it would be possible to use image-opts syntax. This adds --target-image-opts to indicate that the target filename includes options. Currently this mandates use of the --skip-create flag too.
--- qemu-img-cmds.hx | 8 ++-- qemu-img.c | 131 ++++++++++++++++++++++++++++++++++++------------------- qemu-img.texi | 12 ++++- 3 files changed, 99 insertions(+), 52 deletions(-)
qemu-img-cmds.hx +4 -4
@@ -40,15 +40,15 @@ STEXI ETEXI DEF("convert", img_convert, - "convert [--object objectdef] [--image-opts] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] [-l snapshot_param] [-S sparse_size] filename [filename2 [...]] output_filename") + "convert [--object objectdef] [--image-opts] [--target-image-opts] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] [-l snapshot_param] [-S sparse_size] filename [filename2 [...]] output_filename") STEXI -@item convert [--object @var{objectdef}] [--image-opts] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename} +@item convert [--object @var{objectdef}] [--image-opts] [--target-image-opts] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename} ETEXI DEF("dd", img_dd, - "dd [--image-opts] [-n] [-f fmt] [-O output_fmt] [-o options] [bs=block_size] [count=blocks] [skip=blocks] if=input of=output") + "dd [--image-opts] [--target-image-opts] [-n] [-f fmt] [-O output_fmt] [-o options] [bs=block_size] [count=blocks] [skip=blocks] if=input of=output") STEXI -@item dd [--image-opts] [-n] [-f @var{fmt}] [-O @var{output_fmt}] [-o @var{options}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] if=@var{input} of=@var{output} +@item dd [--image-opts] [--target-image-opts] [-n] [-f @var{fmt}] [-O @var{output_fmt}] [-o @var{options}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] if=@var{input} of=@var{output} ETEXI DEF("info", img_info,
qemu-img.c +85 -46
240 lines not on this page · raw message
qemu-img.texi +10 -2
@@ -45,9 +45,17 @@ keys. @item --image-opts -Indicates that the @var{filename} parameter is to be interpreted as a +Indicates that the source @var{filename} parameter is to be interpreted as a full option string, not a plain filename. This parameter is mutually -exclusive with the @var{-f} and @var{-F} parameters. +exclusive with the @var{-f} parameter. + +@item --target-image-opts + +Indicates that the target @var{filename} parameter(s) are to be interpreted a +a full option string, not a plain filename. This parameter is mutually +exclusive with the @var{-F} or @var{-O} parameters. It is currently required +to also use the @var{-n} parameter to skip image creation. This restriction +will be relaxed in a future release. @item fmt is the disk image format. It is guessed automatically in most cases. See below
Signature · 1 line
2.9.3
PATCH 6/6 qemu-img: copy *key-secret opts when opening newly created files
In reply to Daniel P. BerrangeThe qemu-img dd/convert commands will create a image file and then try to open it. Historically it has been possible to open new files without passing any options. With encrypted files though, the *key-secret options are mandatory, so we need to provide those options when opening the newlky created file.
--- qemu-img.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 47 insertions(+), 4 deletions(-)
qemu-img.c +47 -4
@@ -317,6 +317,49 @@ static BlockBackend *img_open_file(const char *filename, } +static int img_add_key_secrets (void *opaque, +const char *name,const char *value, +Error **errp) +{ +QDict **options = opaque; + +if (g_str_has_suffix (name,"key-secret" )) { +if (!*options) { + *options =qdict_new (); + } +qdict_put (*options, name,qstring_from_str (value)); + } + +return 0 ; +} + +static BlockBackend *img_open_new_file (const char *filename, +QemuOpts *create_opts, +const char *fmt,int flags, +bool writethrough,bool quiet) +{ +BlockBackend *blk; +Error *local_err =NULL ; +QDict *options =NULL ; + +if (fmt) { + options =qdict_new (); +qdict_put (options,"driver" ,qstring_from_str (fmt)); + } + +qemu_opt_foreach (create_opts, img_add_key_secrets, &options,NULL ); + + blk =blk_new_open (filename,NULL , options, flags, &local_err); +if (!blk) { +error_reportf_err (local_err,"Could not open '%s': " , filename); +return NULL ; + } +blk_set_enable_write_cache (blk, !writethrough); + +return blk; +} + + static BlockBackend *img_open(bool image_opts, const char *filename, const char *fmt, int flags, bool writethrough,
@@ -2113,8 +2156,8 @@ static int img_convert(int argc, char **argv)* That has to wait for bdrv_create to be improved * to allow filenames in option syntax */ - out_blk =img_open_file (out_filename, out_fmt, - flags, writethrough, quiet); + out_blk =img_open_new_file (out_filename, opts, out_fmt, + flags, writethrough, quiet); }if (!out_blk) { ret =-1 ;
@@ -4179,8 +4222,8 @@ static int img_dd(int argc, char **argv)* That has to wait for bdrv_create to be improved * to allow filenames in option syntax */ - blk2 =img_open_file (out.filename, out_fmt, -BDRV_O_RDWR , false, false); + blk2 =img_open_new_file (out.filename, opts, out_fmt, +BDRV_O_RDWR , false, false); }if (!blk2) {
Signature · 1 line
2.9.3
Re: [Qemu-devel] [PATCH v1 6/6] qemu-img: copy *key-secret opts when opening newly created files
Quoted 5 lines · Daniel P. Berrange
The qemu-img dd/convert commands will create a image file and then try to open it. Historically it has been possible to open new files without passing any options. With encrypted files though, the *key-secret options are mandatory, so we need to provide those options when opening the newlky created file.
s/newlky/newly/
Quoted 6 lines
Signed-off-by Daniel P. Berrange <[email protected]>--- qemu-img.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 47 insertions(+), 4 deletions(-)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org