[Qemu-devel] [PATCH v1 0/6] qemu-img: improve convert & dd commands

[Qemu-devel] [PATCH v1 0/6] qemu-img: improve convert & dd commands

[Daniel P. Berrange]

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(-)

-- 
2.9.3

[Qemu-devel] [PATCH v1 1/6] qemu-img: add support for --object with 'dd' command

[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 <berrange@redhat.com>
---
 qemu-img.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index 74e3362..391a141 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -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) {
-- 
2.9.3

[Qemu-devel] [PATCH v1 2/6] qemu-img: fix --image-opts usage with dd command

[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
opts.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 qemu-img.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 391a141..629f9e9 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -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;
-- 
2.9.3

[Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command

[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.

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

[Qemu-devel] [PATCH v1 4/6] qemu-img: add support for -o arg to dd command

[Daniel P. Berrange]

The -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.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 qemu-img-cmds.hx |  4 ++--
 qemu-img.c       | 32 +++++++++++++++++++++++++++++++-
 qemu-img.texi    |  6 ++++--
 3 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 6732713..ef691fa 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -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,
diff --git a/qemu-img.c b/qemu-img.c
index 4d8d041..8f91a95 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -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);
diff --git a/qemu-img.texi b/qemu-img.texi
index b952d6a..9a391d4 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -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
-- 
2.9.3

[Qemu-devel] [PATCH v1 5/6] qemu-img: introduce --target-image-opts for 'convert' command

[Daniel P. Berrange]

The '--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.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 qemu-img-cmds.hx |   8 ++--
 qemu-img.c       | 131 ++++++++++++++++++++++++++++++++++++-------------------
 qemu-img.texi    |  12 ++++-
 3 files changed, 99 insertions(+), 52 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index ef691fa..aba83b3 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -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,
diff --git a/qemu-img.c b/qemu-img.c
index 8f91a95..a751781 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -59,6 +59,7 @@ enum {
     OPTION_PATTERN = 260,
     OPTION_FLUSH_INTERVAL = 261,
     OPTION_NO_DRAIN = 262,
+    OPTION_TARGET_IMAGE_OPTS = 263,
 };
 
 typedef enum OutputFormat {
@@ -1763,7 +1764,7 @@ static int img_convert(int argc, char **argv)
     int progress = 0, flags, src_flags;
     bool writethrough, src_writethrough;
     const char *fmt, *out_fmt, *cache, *src_cache, *out_baseimg, *out_filename;
-    BlockDriver *drv, *proto_drv;
+    BlockDriver *drv = NULL, *proto_drv = NULL;
     BlockBackend **blk = NULL, *out_blk = NULL;
     BlockDriverState **bs = NULL, *out_bs = NULL;
     int64_t total_sectors;
@@ -1781,9 +1782,10 @@ static int img_convert(int argc, char **argv)
     QemuOpts *sn_opts = NULL;
     ImgConvertState state;
     bool image_opts = false;
+    bool tgt_image_opts = false;
 
+    out_fmt = NULL;
     fmt = NULL;
-    out_fmt = "raw";
     cache = "unsafe";
     src_cache = BDRV_DEFAULT_CACHE;
     out_baseimg = NULL;
@@ -1794,6 +1796,7 @@ static int img_convert(int argc, char **argv)
             {"help", no_argument, 0, 'h'},
             {"object", required_argument, 0, OPTION_OBJECT},
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+            {"target-image-opts", no_argument, 0, OPTION_TARGET_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
         c = getopt_long(argc, argv, "hf:O:B:ce6o:s:l:S:pt:T:qn",
@@ -1898,15 +1901,27 @@ static int img_convert(int argc, char **argv)
         case OPTION_IMAGE_OPTS:
             image_opts = true;
             break;
+        case OPTION_TARGET_IMAGE_OPTS:
+            tgt_image_opts = true;
+            break;
         }
     }
 
+    if (!out_fmt && !tgt_image_opts) {
+        out_fmt = "raw";
+    }
+
     if (qemu_opts_foreach(&qemu_object_opts,
                           user_creatable_add_opts_foreach,
                           NULL, NULL)) {
         goto fail_getopt;
     }
 
+    if (tgt_image_opts && !skip_create) {
+        error_report("--target-image-opts requires use of -n flag");
+        goto fail_getopt;
+    }
+
     /* Initialize before goto out */
     if (quiet) {
         progress = 0;
@@ -1916,7 +1931,7 @@ static int img_convert(int argc, char **argv)
     bs_n = argc - optind - 1;
     out_filename = bs_n >= 1 ? argv[argc - 1] : NULL;
 
-    if (options && has_help_option(options)) {
+    if (out_fmt && options && has_help_option(options)) {
         ret = print_block_option_help(out_filename, out_fmt);
         goto out;
     }
@@ -1985,22 +2000,22 @@ static int img_convert(int argc, char **argv)
         goto out;
     }
 
-    /* Find driver and parse its options */
-    drv = bdrv_find_format(out_fmt);
-    if (!drv) {
-        error_report("Unknown file format '%s'", out_fmt);
-        ret = -1;
-        goto out;
-    }
+    if (!skip_create) {
+        /* Find driver and parse its options */
+        drv = bdrv_find_format(out_fmt);
+        if (!drv) {
+            error_report("Unknown file format '%s'", out_fmt);
+            ret = -1;
+            goto out;
+        }
 
-    proto_drv = bdrv_find_protocol(out_filename, true, &local_err);
-    if (!proto_drv) {
-        error_report_err(local_err);
-        ret = -1;
-        goto out;
-    }
+        proto_drv = bdrv_find_protocol(out_filename, true, &local_err);
+        if (!proto_drv) {
+            error_report_err(local_err);
+            ret = -1;
+            goto out;
+        }
 
-    if (!skip_create) {
         if (!drv->create_opts) {
             error_report("Format driver '%s' does not support image creation",
                          drv->format_name);
@@ -2089,12 +2104,18 @@ static int img_convert(int argc, char **argv)
         goto out;
     }
 
-    /* XXX we should allow --image-opts to trigger use of
-     * img_open() here, but then we have trouble with
-     * the bdrv_create() call which takes different params.
-     * Not critical right now, so fix can wait...
-     */
-    out_blk = img_open_file(out_filename, out_fmt, flags, writethrough, quiet);
+    if (skip_create) {
+        out_blk = img_open(tgt_image_opts, out_filename, out_fmt,
+                           flags, writethrough, quiet);
+    } else {
+        /* TODO ultimately we should allow --target-image-opts
+         * to be used even when -n is not given.
+         * 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);
+    }
     if (!out_blk) {
         ret = -1;
         goto out;
@@ -3917,8 +3938,9 @@ static int img_dd(int argc, char **argv)
     QemuOptsList *create_opts = NULL;
     Error *local_err = NULL;
     bool image_opts = false;
+    bool tgt_image_opts = false;
     int c, i, skip_create = 0;
-    const char *out_fmt = "raw";
+    const char *out_fmt = NULL;
     const char *fmt = NULL;
     char *optionstr = NULL;
     int64_t size = 0, out_size;
@@ -3952,6 +3974,7 @@ static int img_dd(int argc, char **argv)
         { "help", no_argument, 0, 'h'},
         { "object", required_argument, 0, OPTION_OBJECT},
         { "image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+        {"target-image-opts", no_argument, 0, OPTION_TARGET_IMAGE_OPTS},
         { 0, 0, 0, 0 }
     };
 
@@ -4001,9 +4024,21 @@ static int img_dd(int argc, char **argv)
         case OPTION_IMAGE_OPTS:
             image_opts = true;
             break;
+        case OPTION_TARGET_IMAGE_OPTS:
+            tgt_image_opts = true;
+            break;
         }
     }
 
+    if (tgt_image_opts && !skip_create) {
+        error_report("--target-image-opts requires use of -n flag");
+        goto out;
+    }
+
+    if (!out_fmt && !tgt_image_opts) {
+        out_fmt = "raw";
+    }
+
     for (i = optind; i < argc; i++) {
         int j;
         arg = g_strdup(argv[i]);
@@ -4043,7 +4078,7 @@ static int img_dd(int argc, char **argv)
         goto out;
     }
 
-    if (optionstr && has_help_option(optionstr)) {
+    if (out_fmt && optionstr && has_help_option(optionstr)) {
         ret = print_block_option_help(out.filename, out_fmt);
         goto out;
     }
@@ -4061,21 +4096,21 @@ static int img_dd(int argc, char **argv)
         goto out;
     }
 
-    drv = bdrv_find_format(out_fmt);
-    if (!drv) {
-        error_report("Unknown file format");
-        ret = -1;
-        goto out;
-    }
-    proto_drv = bdrv_find_protocol(out.filename, true, &local_err);
+    if (!skip_create) {
+        drv = bdrv_find_format(out_fmt);
+        if (!drv) {
+            error_report("Unknown file format");
+            ret = -1;
+            goto out;
+        }
+        proto_drv = bdrv_find_protocol(out.filename, true, &local_err);
 
-    if (!proto_drv) {
-        error_report_err(local_err);
-        ret = -1;
-        goto out;
-    }
+        if (!proto_drv) {
+            error_report_err(local_err);
+            ret = -1;
+            goto out;
+        }
 
-    if (!skip_create) {
         if (!drv->create_opts) {
             error_report("Format driver '%s' does not support image creation",
                          drv->format_name);
@@ -4125,7 +4160,6 @@ static int img_dd(int argc, char **argv)
 
     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,
@@ -4136,13 +4170,18 @@ static int img_dd(int argc, char **argv)
         }
     }
 
-    /* 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 (skip_create) {
+        blk2 = img_open(tgt_image_opts, out.filename, out_fmt,
+                        BDRV_O_RDWR, false, false);
+    } else {
+        /* TODO ultimately we should allow --target-image-opts
+         * to be used even when -n is not given.
+         * 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);
+    }
 
     if (!blk2) {
         ret = -1;
diff --git a/qemu-img.texi b/qemu-img.texi
index 9a391d4..dfdfb65 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -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
-- 
2.9.3

[Qemu-devel] [PATCH v1 6/6] qemu-img: copy *key-secret opts when opening newly created files

[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.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 qemu-img.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 47 insertions(+), 4 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index a751781..130cec7 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -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) {
-- 
2.9.3

Re: [Qemu-devel] [PATCH v1 2/6] qemu-img: fix --image-opts usage with dd command

[Fam Zheng]

On Thu, 01/26 11:04, Daniel P. Berrange wrote:
> 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 :)

> opts.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  qemu-img.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 391a141..629f9e9 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -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;
> -- 
> 2.9.3
> 
> 

Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command

[Fam Zheng]

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
> 
> 

Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command

[Daniel P. Berrange]

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
-- 
|: 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

[Fam Zheng]

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

Re: [Qemu-devel] [PATCH v1 1/6] qemu-img: add support for --object with 'dd' command

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 547 bytes --]

On 01/26/2017 05:04 AM, Daniel P. Berrange wrote:
> 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 <berrange@redhat.com>
> ---
>  qemu-img.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1257 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v1 6/6] qemu-img: copy *key-secret opts when opening newly created files

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 703 bytes --]

On 01/26/2017 05:04 AM, Daniel P. Berrange wrote:
> 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/

> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  qemu-img.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 47 insertions(+), 4 deletions(-)
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1357 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command

[Daniel P. Berrange]

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
-- 
|: 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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2527 bytes --]

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
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command

[Daniel P. Berrange]

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
-- 
|: 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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3347 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command

[Daniel P. Berrange]

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
-- 
|: 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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2627 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command

[Markus Armbruster]

"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.)
        

Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 6087 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command

[Daniel P. Berrange]

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
-- 
|: 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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3299 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command

[Markus Armbruster]

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.

[...]

Re: [Qemu-devel] [PATCH v1 3/6] qemu-img: add support for -n arg to dd command

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 4886 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]