| Message ID | 20170126110435.2777-4-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 01/26 11:04, Daniel P. Berrange wrote: > 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 > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > qemu-img-cmds.hx | 4 +-- > qemu-img.c | 79 ++++++++++++++++++++++++++++++++++++-------------------- > qemu-img.texi | 7 ++++- > 3 files changed, 59 insertions(+), 31 deletions(-) > > diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx > index f054599..6732713 100644 > --- a/qemu-img-cmds.hx > +++ b/qemu-img-cmds.hx > @@ -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, > diff --git a/qemu-img.c b/qemu-img.c > index 629f9e9..4d8d041 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -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 > diff --git a/qemu-img.texi b/qemu-img.texi > index 174aae3..b952d6a 100644 > --- a/qemu-img.texi > +++ b/qemu-img.texi > @@ -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 > -- > 2.9.3 > >
On Thu, Jan 26, 2017 at 08:35:30PM +0800, Fam Zheng wrote: > On Thu, 01/26 11:04, Daniel P. Berrange wrote: > > 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
On Thu, 01/26 13:27, Daniel P. Berrange wrote: > 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
On 01/26/2017 07:27 AM, Daniel P. Berrange wrote: > On Thu, Jan 26, 2017 at 08:35:30PM +0800, Fam Zheng wrote: >> On Thu, 01/26 11:04, Daniel P. Berrange wrote: >>> 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. Adding 'conv=nocreat' (and not '-n') feels like the right way to me.
On 30.01.2017 19:37, Eric Blake wrote: > On 01/26/2017 07:27 AM, Daniel P. Berrange wrote: >> On Thu, Jan 26, 2017 at 08:35:30PM +0800, Fam Zheng wrote: >>> On Thu, 01/26 11:04, Daniel P. Berrange wrote: >>>> 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. > > 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
On Wed, Feb 01, 2017 at 01:13:39PM +0100, Max Reitz wrote: > On 30.01.2017 19:37, Eric Blake wrote: > > On 01/26/2017 07:27 AM, Daniel P. Berrange wrote: > >> On Thu, Jan 26, 2017 at 08:35:30PM +0800, Fam Zheng wrote: > >>> On Thu, 01/26 11:04, Daniel P. Berrange wrote: > >>>> 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. > > > > 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
On 01.02.2017 13:16, Daniel P. Berrange wrote: > On Wed, Feb 01, 2017 at 01:13:39PM +0100, Max Reitz wrote: >> On 30.01.2017 19:37, Eric Blake wrote: >>> On 01/26/2017 07:27 AM, Daniel P. Berrange wrote: >>>> On Thu, Jan 26, 2017 at 08:35:30PM +0800, Fam Zheng wrote: >>>>> On Thu, 01/26 11:04, Daniel P. Berrange wrote: >>>>>> 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. >>> >>> 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 > FYI, I've implemented "conv" with "nocreat" and "notrunc" options > which i'll post once i've tested some more. > > Regards, > Daniel >
On Wed, Feb 01, 2017 at 01:23:54PM +0100, Max Reitz wrote: > On 01.02.2017 13:16, Daniel P. Berrange wrote: > > On Wed, Feb 01, 2017 at 01:13:39PM +0100, Max Reitz wrote: > >> On 30.01.2017 19:37, Eric Blake wrote: > >>> On 01/26/2017 07:27 AM, Daniel P. Berrange wrote: > >>>> On Thu, Jan 26, 2017 at 08:35:30PM +0800, Fam Zheng wrote: > >>>>> On Thu, 01/26 11:04, Daniel P. Berrange wrote: > >>>>>> 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. > >>> > >>> 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? 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. > 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. :-/ 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
On 01.02.2017 13:28, Daniel P. Berrange wrote: > On Wed, Feb 01, 2017 at 01:23:54PM +0100, Max Reitz wrote: >> On 01.02.2017 13:16, Daniel P. Berrange wrote: >>> On Wed, Feb 01, 2017 at 01:13:39PM +0100, Max Reitz wrote: >>>> On 30.01.2017 19:37, Eric Blake wrote: >>>>> On 01/26/2017 07:27 AM, Daniel P. Berrange wrote: >>>>>> On Thu, Jan 26, 2017 at 08:35:30PM +0800, Fam Zheng wrote: >>>>>>> On Thu, 01/26 11:04, Daniel P. Berrange wrote: >>>>>>>> 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. >>>>> >>>>> 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? > > 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. >> 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. :-/ > > 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 Yes. But you could easily argue that every image file is a "block device". Max
On Wed, Feb 01, 2017 at 01:31:01PM +0100, Max Reitz wrote: > On 01.02.2017 13:28, Daniel P. Berrange wrote: > > On Wed, Feb 01, 2017 at 01:23:54PM +0100, Max Reitz wrote: > >> On 01.02.2017 13:16, Daniel P. Berrange wrote: > >>> On Wed, Feb 01, 2017 at 01:13:39PM +0100, Max Reitz wrote: > >>>> On 30.01.2017 19:37, Eric Blake wrote: > >>>>> On 01/26/2017 07:27 AM, Daniel P. Berrange wrote: > >>>>>> On Thu, Jan 26, 2017 at 08:35:30PM +0800, Fam Zheng wrote: > >>>>>>> On Thu, 01/26 11:04, Daniel P. Berrange wrote: > >>>>>>>> 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. > >>>>> > >>>>> 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? > > > > 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. > > >> 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. :-/ > > > > 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 > > 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
On 01.02.2017 13:40, Daniel P. Berrange wrote: > On Wed, Feb 01, 2017 at 01:31:01PM +0100, Max Reitz wrote: >> On 01.02.2017 13:28, Daniel P. Berrange wrote: >>> On Wed, Feb 01, 2017 at 01:23:54PM +0100, Max Reitz wrote: >>>> On 01.02.2017 13:16, Daniel P. Berrange wrote: [...] >>>> 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. :-/ >>> >>> 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 >> >> 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. > 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
Max Reitz <mreitz@redhat.com> writes: > On 01.02.2017 13:28, Daniel P. Berrange wrote: >> On Wed, Feb 01, 2017 at 01:23:54PM +0100, Max Reitz wrote: >>> On 01.02.2017 13:16, Daniel P. Berrange wrote: >>>> On Wed, Feb 01, 2017 at 01:13:39PM +0100, Max Reitz wrote: >>>>> On 30.01.2017 19:37, Eric Blake wrote: >>>>>> On 01/26/2017 07:27 AM, Daniel P. Berrange wrote: >>>>>>> On Thu, Jan 26, 2017 at 08:35:30PM +0800, Fam Zheng wrote: >>>>>>>> On Thu, 01/26 11:04, Daniel P. Berrange wrote: >>>>>>>>> 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. >>>>>> >>>>>> 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? >> >> 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. > >>> 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. :-/ >> >> 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 > > 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.
"Daniel P. Berrange" <berrange@redhat.com> writes: > On Wed, Feb 01, 2017 at 01:31:01PM +0100, Max Reitz wrote: >> On 01.02.2017 13:28, Daniel P. Berrange wrote: >> > On Wed, Feb 01, 2017 at 01:23:54PM +0100, Max Reitz wrote: >> >> On 01.02.2017 13:16, Daniel P. Berrange wrote: >> >>> On Wed, Feb 01, 2017 at 01:13:39PM +0100, Max Reitz wrote: >> >>>> On 30.01.2017 19:37, Eric Blake wrote: >> >>>>> On 01/26/2017 07:27 AM, Daniel P. Berrange wrote: >> >>>>>> On Thu, Jan 26, 2017 at 08:35:30PM +0800, Fam Zheng wrote: >> >>>>>>> On Thu, 01/26 11:04, Daniel P. Berrange wrote: >> >>>>>>>> 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. >> >>>>> >> >>>>> 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? >> > >> > 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. >> >> >> 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. :-/ >> > >> > 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 >> >> 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.)
On 02.02.2017 08:32, Markus Armbruster wrote: > Max Reitz <mreitz@redhat.com> writes: > >> On 01.02.2017 13:28, Daniel P. Berrange wrote: >>> On Wed, Feb 01, 2017 at 01:23:54PM +0100, Max Reitz wrote: >>>> On 01.02.2017 13:16, Daniel P. Berrange wrote: >>>>> On Wed, Feb 01, 2017 at 01:13:39PM +0100, Max Reitz wrote: >>>>>> On 30.01.2017 19:37, Eric Blake wrote: >>>>>>> On 01/26/2017 07:27 AM, Daniel P. Berrange wrote: >>>>>>>> On Thu, Jan 26, 2017 at 08:35:30PM +0800, Fam Zheng wrote: >>>>>>>>> On Thu, 01/26 11:04, Daniel P. Berrange wrote: >>>>>>>>>> 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. >>>>>>> >>>>>>> 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? >>> >>> 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. >> >>>> 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. :-/ >>> >>> 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 >> >> 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. */usr*/bin/dd (O:-)) also treats qcow2 files like raw files. > 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
On Fri, Feb 03, 2017 at 07:56:11PM +0100, Max Reitz wrote: > > 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. 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
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. On 06.02.2017 11:31, Daniel P. Berrange wrote: > On Fri, Feb 03, 2017 at 07:56:11PM +0100, Max Reitz wrote: >>> 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. > > 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. 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
Max Reitz <mreitz@redhat.com> writes: > 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. > On 06.02.2017 11:31, Daniel P. Berrange wrote: >> On Fri, Feb 03, 2017 at 07:56:11PM +0100, Max Reitz wrote: >>>> 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. >> >> 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. > > 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. [...]
On 08.02.2017 10:19, Markus Armbruster wrote: > Max Reitz <mreitz@redhat.com> writes: > >> 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. All the more reason why we couldn't implement it in time. (Also, implementing that just for qemu-img dd... Well, no.) >> On 06.02.2017 11:31, Daniel P. Berrange wrote: >>> On Fri, Feb 03, 2017 at 07:56:11PM +0100, Max Reitz wrote: >>>>> 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. >>> >>> 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. >> >> 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. 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
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx index f054599..6732713 100644 --- a/qemu-img-cmds.hx +++ b/qemu-img-cmds.hx @@ -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, diff --git a/qemu-img.c b/qemu-img.c index 629f9e9..4d8d041 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -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 diff --git a/qemu-img.texi b/qemu-img.texi index 174aae3..b952d6a 100644 --- a/qemu-img.texi +++ b/qemu-img.texi @@ -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
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. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- qemu-img-cmds.hx | 4 +-- qemu-img.c | 79 ++++++++++++++++++++++++++++++++++++-------------------- qemu-img.texi | 7 ++++- 3 files changed, 59 insertions(+), 31 deletions(-)