* [PATCH] qemu-img: add seek and -n option to dd command
@ 2021-01-28 14:07 Peter Lieven
2021-02-02 10:20 ` David Edmondson
2021-02-02 15:51 ` Eric Blake
0 siblings, 2 replies; 10+ messages in thread
From: Peter Lieven @ 2021-01-28 14:07 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, Peter Lieven, qemu-devel, mreitz
Signed-off-by: Peter Lieven <pl@kamp.de>
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index b615aa8419..7d4564c2b8 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -209,6 +209,10 @@ Parameters to dd subcommand:
.. program:: qemu-img-dd
+.. option:: -n
+
+ Skip the creation of the output file
+
.. option:: bs=BLOCK_SIZE
Defines the block size
@@ -229,6 +233,10 @@ Parameters to dd subcommand:
Sets the number of input blocks to skip
+.. option:: sseek=BLOCKS
+
+ Sets the number of blocks to seek into the output
+
Parameters to snapshot subcommand:
.. program:: qemu-img-snapshot
diff --git a/qemu-img.c b/qemu-img.c
index 8597d069af..d7f390e382 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -213,12 +213,17 @@ static void QEMU_NORETURN help(void)
" '-s' run in Strict mode - fail on different image size or sector allocation\n"
"\n"
"Parameters to dd subcommand:\n"
+ " '-n' skips the target volume creation (useful if the volume is created\n"
+ " prior to running qemu-img). Note that he behaviour is not identical to\n"
+ " original dd option conv=nocreat. The output is neither truncated nor\n"
+ " is it possible to write past the end of an existing file.\n"
" 'bs=BYTES' read and write up to BYTES bytes at a time "
"(default: 512)\n"
" 'count=N' copy only N input blocks\n"
" 'if=FILE' read from FILE\n"
" 'of=FILE' write to FILE\n"
- " 'skip=N' skip N bs-sized blocks at the start of input\n";
+ " 'skip=N' skip N bs-sized blocks at the start of input\n"
+ " 'seek=N' seek N bs-sized blocks into the output\n";
printf("%s\nSupported formats:", help_msg);
bdrv_iterate_format(format_print, NULL, false);
@@ -4885,6 +4890,7 @@ static int img_bitmap(int argc, char **argv)
#define C_IF 04
#define C_OF 010
#define C_SKIP 020
+#define C_SEEK 040
struct DdInfo {
unsigned int flags;
@@ -4964,6 +4970,19 @@ static int img_dd_skip(const char *arg,
return 0;
}
+static int img_dd_seek(const char *arg,
+ struct DdIo *in, struct DdIo *out,
+ struct DdInfo *dd)
+{
+ out->offset = cvtnum("seek", arg);
+
+ if (in->offset < 0) {
+ return 1;
+ }
+
+ return 0;
+}
+
static int img_dd(int argc, char **argv)
{
int ret = 0;
@@ -4980,7 +4999,7 @@ static int img_dd(int argc, char **argv)
const char *fmt = NULL;
int64_t size = 0;
int64_t block_count = 0, out_pos, in_pos;
- bool force_share = false;
+ bool force_share = false, skip_create = false;
struct DdInfo dd = {
.flags = 0,
.count = 0,
@@ -5004,6 +5023,7 @@ static int img_dd(int argc, char **argv)
{ "if", img_dd_if, C_IF },
{ "of", img_dd_of, C_OF },
{ "skip", img_dd_skip, C_SKIP },
+ { "seek", img_dd_seek, C_SEEK },
{ NULL, NULL, 0 }
};
const struct option long_options[] = {
@@ -5014,7 +5034,7 @@ static int img_dd(int argc, char **argv)
{ 0, 0, 0, 0 }
};
- while ((c = getopt_long(argc, argv, ":hf:O:U", long_options, NULL))) {
+ while ((c = getopt_long(argc, argv, ":hf:O:Un", long_options, NULL))) {
if (c == EOF) {
break;
}
@@ -5037,6 +5057,9 @@ static int img_dd(int argc, char **argv)
case 'U':
force_share = true;
break;
+ case 'n':
+ skip_create = true;
+ break;
case OPTION_OBJECT:
if (!qemu_opts_parse_noisily(&qemu_object_opts, optarg, true)) {
ret = -1;
@@ -5116,22 +5139,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) {
@@ -5145,22 +5171,25 @@ static int img_dd(int argc, char **argv)
size = dd.count * in.bsz;
}
- /* 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);
- } else {
- qemu_opt_set_number(opts, BLOCK_OPT_SIZE,
- size - in.bsz * in.offset, &error_abort);
- }
+ if (!skip_create) {
+ /* 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);
+ } else {
+ qemu_opt_set_number(opts, BLOCK_OPT_SIZE,
+ out.bsz * out.offset + size -
+ in.bsz * in.offset, &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;
+ 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,
@@ -5189,7 +5218,7 @@ static int img_dd(int argc, char **argv)
in.buf = g_new(uint8_t, in.bsz);
- for (out_pos = 0; in_pos < size; block_count++) {
+ for (out_pos = out.offset * out.bsz; in_pos < size; block_count++) {
int in_ret, out_ret;
if (in_pos + in.bsz > size) {
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] qemu-img: add seek and -n option to dd command
2021-01-28 14:07 [PATCH] qemu-img: add seek and -n option to dd command Peter Lieven
@ 2021-02-02 10:20 ` David Edmondson
2021-02-02 15:51 ` Eric Blake
1 sibling, 0 replies; 10+ messages in thread
From: David Edmondson @ 2021-02-02 10:20 UTC (permalink / raw)
To: Peter Lieven, qemu-block; +Cc: kwolf, Peter Lieven, qemu-devel, mreitz
On Thursday, 2021-01-28 at 15:07:04 +01, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
>
> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
> index b615aa8419..7d4564c2b8 100644
> --- a/docs/tools/qemu-img.rst
> +++ b/docs/tools/qemu-img.rst
> @@ -209,6 +209,10 @@ Parameters to dd subcommand:
>
> .. program:: qemu-img-dd
>
> +.. option:: -n
> +
> + Skip the creation of the output file
> +
> .. option:: bs=BLOCK_SIZE
>
> Defines the block size
> @@ -229,6 +233,10 @@ Parameters to dd subcommand:
>
> Sets the number of input blocks to skip
>
> +.. option:: sseek=BLOCKS
> +
> + Sets the number of blocks to seek into the output
> +
> Parameters to snapshot subcommand:
>
> .. program:: qemu-img-snapshot
> diff --git a/qemu-img.c b/qemu-img.c
> index 8597d069af..d7f390e382 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -213,12 +213,17 @@ static void QEMU_NORETURN help(void)
> " '-s' run in Strict mode - fail on different image size or sector allocation\n"
> "\n"
> "Parameters to dd subcommand:\n"
> + " '-n' skips the target volume creation (useful if the volume is created\n"
> + " prior to running qemu-img). Note that he behaviour is not identical to\n"
s/he/the/
> + " original dd option conv=nocreat. The output is neither truncated nor\n"
> + " is it possible to write past the end of an existing file.\n"
> " 'bs=BYTES' read and write up to BYTES bytes at a time "
> "(default: 512)\n"
> " 'count=N' copy only N input blocks\n"
> " 'if=FILE' read from FILE\n"
> " 'of=FILE' write to FILE\n"
> - " 'skip=N' skip N bs-sized blocks at the start of input\n";
> + " 'skip=N' skip N bs-sized blocks at the start of input\n"
> + " 'seek=N' seek N bs-sized blocks into the output\n";
>
> printf("%s\nSupported formats:", help_msg);
> bdrv_iterate_format(format_print, NULL, false);
> @@ -4885,6 +4890,7 @@ static int img_bitmap(int argc, char **argv)
> #define C_IF 04
> #define C_OF 010
> #define C_SKIP 020
> +#define C_SEEK 040
>
> struct DdInfo {
> unsigned int flags;
> @@ -4964,6 +4970,19 @@ static int img_dd_skip(const char *arg,
> return 0;
> }
>
> +static int img_dd_seek(const char *arg,
> + struct DdIo *in, struct DdIo *out,
> + struct DdInfo *dd)
> +{
> + out->offset = cvtnum("seek", arg);
> +
> + if (in->offset < 0) {
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> static int img_dd(int argc, char **argv)
> {
> int ret = 0;
> @@ -4980,7 +4999,7 @@ static int img_dd(int argc, char **argv)
> const char *fmt = NULL;
> int64_t size = 0;
> int64_t block_count = 0, out_pos, in_pos;
> - bool force_share = false;
> + bool force_share = false, skip_create = false;
> struct DdInfo dd = {
> .flags = 0,
> .count = 0,
> @@ -5004,6 +5023,7 @@ static int img_dd(int argc, char **argv)
> { "if", img_dd_if, C_IF },
> { "of", img_dd_of, C_OF },
> { "skip", img_dd_skip, C_SKIP },
> + { "seek", img_dd_seek, C_SEEK },
> { NULL, NULL, 0 }
> };
> const struct option long_options[] = {
> @@ -5014,7 +5034,7 @@ static int img_dd(int argc, char **argv)
> { 0, 0, 0, 0 }
> };
>
> - while ((c = getopt_long(argc, argv, ":hf:O:U", long_options, NULL))) {
> + while ((c = getopt_long(argc, argv, ":hf:O:Un", long_options, NULL))) {
> if (c == EOF) {
> break;
> }
> @@ -5037,6 +5057,9 @@ static int img_dd(int argc, char **argv)
> case 'U':
> force_share = true;
> break;
> + case 'n':
> + skip_create = true;
> + break;
> case OPTION_OBJECT:
> if (!qemu_opts_parse_noisily(&qemu_object_opts, optarg, true)) {
> ret = -1;
> @@ -5116,22 +5139,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) {
> @@ -5145,22 +5171,25 @@ static int img_dd(int argc, char **argv)
> size = dd.count * in.bsz;
> }
>
> - /* 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);
> - } else {
> - qemu_opt_set_number(opts, BLOCK_OPT_SIZE,
> - size - in.bsz * in.offset, &error_abort);
> - }
> + if (!skip_create) {
> + /* 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);
> + } else {
> + qemu_opt_set_number(opts, BLOCK_OPT_SIZE,
> + out.bsz * out.offset + size -
> + in.bsz * in.offset, &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;
> + 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,
> @@ -5189,7 +5218,7 @@ static int img_dd(int argc, char **argv)
>
> in.buf = g_new(uint8_t, in.bsz);
>
> - for (out_pos = 0; in_pos < size; block_count++) {
> + for (out_pos = out.offset * out.bsz; in_pos < size; block_count++) {
> int in_ret, out_ret;
>
> if (in_pos + in.bsz > size) {
> --
> 2.17.1
dme.
--
But they'll laugh at you in Jackson, and I'll be dancin' on a Pony Keg.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] qemu-img: add seek and -n option to dd command
2021-01-28 14:07 [PATCH] qemu-img: add seek and -n option to dd command Peter Lieven
2021-02-02 10:20 ` David Edmondson
@ 2021-02-02 15:51 ` Eric Blake
2021-02-04 20:09 ` Peter Lieven
1 sibling, 1 reply; 10+ messages in thread
From: Eric Blake @ 2021-02-02 15:51 UTC (permalink / raw)
To: Peter Lieven, qemu-block; +Cc: kwolf, qemu-devel, mreitz
On 1/28/21 8:07 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
Your commit message says 'what', but not 'why'. Generally, the one-line
'what' works well as the subject line, but you want the commit body to
give an argument why your patch should be applied, rather than blank.
Here's the last time we tried to improve qemu-img dd:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg02618.html
where I also proposed adding seek=, and fixing skip= with count=. Your
patch does not do the latter. But the bigger complaint back then was
that 'qemu-img copy' should be able to do everything, and that qemu-img
dd should then just be a thin shim around 'qemu-img copy', rather than
having two parallel projects that diverge in their implementations.
Your patch does not have the typical '---' divider and diffstat between
the commit message and the patch proper; this may be a factor of which
git packages you have installed, but having the diffstat present makes
it easier to see at a glance what your patch touches without reading the
entire email. I had to go hunting to learn if you added iotest coverage
of this new feature...
...and the answer was no, you didn't. You'll need to add that in v2
(see the link to my earlier attempt at modifying dd for an example).
>
> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
> index b615aa8419..7d4564c2b8 100644
> --- a/docs/tools/qemu-img.rst
> +++ b/docs/tools/qemu-img.rst
> @@ -209,6 +209,10 @@ Parameters to dd subcommand:
>
> .. program:: qemu-img-dd
>
> +.. option:: -n
> +
> + Skip the creation of the output file
> +
> .. option:: bs=BLOCK_SIZE
>
> Defines the block size
> @@ -229,6 +233,10 @@ Parameters to dd subcommand:
>
> Sets the number of input blocks to skip
>
> +.. option:: sseek=BLOCKS
Typo: seek=
> +
> + Sets the number of blocks to seek into the output
> +
> Parameters to snapshot subcommand:
>
> .. program:: qemu-img-snapshot
> diff --git a/qemu-img.c b/qemu-img.c
> index 8597d069af..d7f390e382 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -213,12 +213,17 @@ static void QEMU_NORETURN help(void)
> " '-s' run in Strict mode - fail on different image size or sector allocation\n"
> "\n"
> "Parameters to dd subcommand:\n"
> + " '-n' skips the target volume creation (useful if the volume is created\n"
> + " prior to running qemu-img). Note that he behaviour is not identical to\n"
the
> + " original dd option conv=nocreat. The output is neither truncated nor\n"
> + " is it possible to write past the end of an existing file.\n"
and hence why you spell it -n rather than the more dd-like conv=nocreat.
We absolutely want a different spelling to emphasize the different
behavior.
Is it worth splitting this patch in two parts, one for -n, the other for
seek=?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] qemu-img: add seek and -n option to dd command
2021-02-02 15:51 ` Eric Blake
@ 2021-02-04 20:09 ` Peter Lieven
2021-02-04 20:44 ` Eric Blake
2021-02-05 8:18 ` Max Reitz
0 siblings, 2 replies; 10+ messages in thread
From: Peter Lieven @ 2021-02-04 20:09 UTC (permalink / raw)
To: Eric Blake, qemu-block; +Cc: kwolf, qemu-devel, mreitz
Am 02.02.21 um 16:51 schrieb Eric Blake:
> On 1/28/21 8:07 AM, Peter Lieven wrote:
>> Signed-off-by: Peter Lieven <pl@kamp.de>
> Your commit message says 'what', but not 'why'. Generally, the one-line
> 'what' works well as the subject line, but you want the commit body to
> give an argument why your patch should be applied, rather than blank.
>
> Here's the last time we tried to improve qemu-img dd:
> https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg02618.html
I was not aware of that story. My use case is that I want to be
able to "patch" an image that Qemu is able to handle by overwriting
certain sectors. And I especially do not want to "mount" that image
via qemu-nbd because I might not trust it. I totally want to avoid that the host
system tries to analyse that image in terms of scanning the bootsector, partprobe,
lvm etc. pp.
>
> where I also proposed adding seek=, and fixing skip= with count=. Your
> patch does not do the latter. But the bigger complaint back then was
> that 'qemu-img copy' should be able to do everything, and that qemu-img
> dd should then just be a thin shim around 'qemu-img copy', rather than
> having two parallel projects that diverge in their implementations.
understood. I was not aware of an issue with skip and count.
The patch works for me and I wanted to share it. But when I read
the thread it seems that it would be a difficult task to get it merged.
>
> Your patch does not have the typical '---' divider and diffstat between
> the commit message and the patch proper; this may be a factor of which
> git packages you have installed, but having the diffstat present makes
> it easier to see at a glance what your patch touches without reading the
> entire email. I had to go hunting to learn if you added iotest coverage
> of this new feature...
>
> ...and the answer was no, you didn't. You'll need to add that in v2
> (see the link to my earlier attempt at modifying dd for an example).
I did not. Maybe I accidently killed the '---' divider. If I will make a V2 I will add
an I/O test.
Thanks for your suggestions,
Peter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] qemu-img: add seek and -n option to dd command
2021-02-04 20:09 ` Peter Lieven
@ 2021-02-04 20:44 ` Eric Blake
2021-02-05 10:43 ` Richard W.M. Jones
2021-02-05 8:18 ` Max Reitz
1 sibling, 1 reply; 10+ messages in thread
From: Eric Blake @ 2021-02-04 20:44 UTC (permalink / raw)
To: Peter Lieven, qemu-block; +Cc: kwolf, Richard W.M. Jones, qemu-devel, mreitz
On 2/4/21 2:09 PM, Peter Lieven wrote:
> Am 02.02.21 um 16:51 schrieb Eric Blake:
>> On 1/28/21 8:07 AM, Peter Lieven wrote:
>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> Your commit message says 'what', but not 'why'. Generally, the one-line
>> 'what' works well as the subject line, but you want the commit body to
>> give an argument why your patch should be applied, rather than blank.
>>
>> Here's the last time we tried to improve qemu-img dd:
>> https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg02618.html
>
>
> I was not aware of that story. My use case is that I want to be
>
> able to "patch" an image that Qemu is able to handle by overwriting
>
> certain sectors. And I especially do not want to "mount" that image
>
> via qemu-nbd because I might not trust it. I totally want to avoid that the host
>
> system tries to analyse that image in terms of scanning the bootsector, partprobe,
>
> lvm etc. pp.
qemu-nbd does not have to mount an image (yes, one use of qemu-nbd is to
use -c /dev/nbdX to get the kernel to mount it; but other uses are to
expose the NBD image in user-space only with no kernel involvement, and
therefore no system mount efforts).
Another thing you might try is libnbd, which now includes a utility
nbdcopy. It should make it easier to overwrite a portion of an NBD
image using only user-space actions. I'm not sure if Rich has got it
doing partial file overwrites yet (.../me goes and compiles the latest
git checkout... nope, still a TODO item to implement subsetting), but it
may be possible to combine nbdkit's --filter=offset with the full NBD
image in order to then easily point nbdcopy to only the subset you care
about. Definitely some ideas worthy of implementation.
>
>>
>> where I also proposed adding seek=, and fixing skip= with count=. Your
>> patch does not do the latter. But the bigger complaint back then was
>> that 'qemu-img copy' should be able to do everything, and that qemu-img
>> dd should then just be a thin shim around 'qemu-img copy', rather than
>> having two parallel projects that diverge in their implementations.
>
>
> understood. I was not aware of an issue with skip and count.
>
> The patch works for me and I wanted to share it. But when I read
>
> the thread it seems that it would be a difficult task to get it merged.
Just because we're reluctant to improve qemu-img dd without also
improving qemu-img copy does not mean that your improvements are
unwanted. And hacking on nbdcopy may be faster than waiting for
qemu-img to catch up to where we want it to go.
>
>
>>
>> Your patch does not have the typical '---' divider and diffstat between
>> the commit message and the patch proper; this may be a factor of which
>> git packages you have installed, but having the diffstat present makes
>> it easier to see at a glance what your patch touches without reading the
>> entire email. I had to go hunting to learn if you added iotest coverage
>> of this new feature...
>>
>> ...and the answer was no, you didn't. You'll need to add that in v2
>> (see the link to my earlier attempt at modifying dd for an example).
>
>
> I did not. Maybe I accidently killed the '---' divider. If I will make a V2 I will add
>
> an I/O test.
>
>
> Thanks for your suggestions,
Good luck!
>
> Peter
>
>
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] qemu-img: add seek and -n option to dd command
2021-02-04 20:09 ` Peter Lieven
2021-02-04 20:44 ` Eric Blake
@ 2021-02-05 8:18 ` Max Reitz
2021-02-05 8:47 ` Peter Lieven
1 sibling, 1 reply; 10+ messages in thread
From: Max Reitz @ 2021-02-05 8:18 UTC (permalink / raw)
To: Peter Lieven, Eric Blake, qemu-block; +Cc: kwolf, qemu-devel
On 04.02.21 21:09, Peter Lieven wrote:
> Am 02.02.21 um 16:51 schrieb Eric Blake:
>> On 1/28/21 8:07 AM, Peter Lieven wrote:
>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> Your commit message says 'what', but not 'why'. Generally, the one-line
>> 'what' works well as the subject line, but you want the commit body to
>> give an argument why your patch should be applied, rather than blank.
>>
>> Here's the last time we tried to improve qemu-img dd:
>> https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg02618.html
>
>
> I was not aware of that story. My use case is that I want to be
>
> able to "patch" an image that Qemu is able to handle by overwriting
>
> certain sectors. And I especially do not want to "mount" that image
>
> via qemu-nbd because I might not trust it. I totally want to avoid that the host
>
> system tries to analyse that image in terms of scanning the bootsector, partprobe,
>
> lvm etc. pp.
qemu will have FUSE exporting as of 6.0 (didn’t quite make it into 5.2),
so you can do something like this:
$ qemu-storage-daemon \
--blockdev node-name=export,driver=qcow2,\
file.driver=file,file.filename=image.qcow2 \
--export fuse,id=fuse,node-name=export,mountpoint=image.qcow2
This exports the image on image.qcow2 (i.e., on itself) and so by
accessing the image file you then get raw access to its contents (so you
can use system tools like dd).
Doesn’t require root rights, and shouldn’t make the kernel scan
anything, because it’s exported as just a regular file.
Max
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] qemu-img: add seek and -n option to dd command
2021-02-05 8:18 ` Max Reitz
@ 2021-02-05 8:47 ` Peter Lieven
2021-02-05 9:16 ` Max Reitz
0 siblings, 1 reply; 10+ messages in thread
From: Peter Lieven @ 2021-02-05 8:47 UTC (permalink / raw)
To: Max Reitz, Eric Blake, qemu-block; +Cc: kwolf, qemu-devel
Am 05.02.21 um 09:18 schrieb Max Reitz:
> On 04.02.21 21:09, Peter Lieven wrote:
>> Am 02.02.21 um 16:51 schrieb Eric Blake:
>>> On 1/28/21 8:07 AM, Peter Lieven wrote:
>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>> Your commit message says 'what', but not 'why'. Generally, the one-line
>>> 'what' works well as the subject line, but you want the commit body to
>>> give an argument why your patch should be applied, rather than blank.
>>>
>>> Here's the last time we tried to improve qemu-img dd:
>>> https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg02618.html
>>
>>
>> I was not aware of that story. My use case is that I want to be
>>
>> able to "patch" an image that Qemu is able to handle by overwriting
>>
>> certain sectors. And I especially do not want to "mount" that image
>>
>> via qemu-nbd because I might not trust it. I totally want to avoid that the host
>>
>> system tries to analyse that image in terms of scanning the bootsector, partprobe,
>>
>> lvm etc. pp.
>
> qemu will have FUSE exporting as of 6.0 (didn’t quite make it into 5.2), so you can do something like this:
>
> $ qemu-storage-daemon \
> --blockdev node-name=export,driver=qcow2,\
> file.driver=file,file.filename=image.qcow2 \
> --export fuse,id=fuse,node-name=export,mountpoint=image.qcow2
>
> This exports the image on image.qcow2 (i.e., on itself) and so by accessing the image file you then get raw access to its contents (so you can use system tools like dd).
>
> Doesn’t require root rights, and shouldn’t make the kernel scan anything, because it’s exported as just a regular file.
Okay, but that is still more housekeeping than just invoking a single command.
Would it be an option to extend qemu-io to write data at a certain offset which it reads from STDIN?
Peter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] qemu-img: add seek and -n option to dd command
2021-02-05 8:47 ` Peter Lieven
@ 2021-02-05 9:16 ` Max Reitz
2021-02-05 10:06 ` Max Reitz
0 siblings, 1 reply; 10+ messages in thread
From: Max Reitz @ 2021-02-05 9:16 UTC (permalink / raw)
To: Peter Lieven, Eric Blake, qemu-block; +Cc: kwolf, qemu-devel
On 05.02.21 09:47, Peter Lieven wrote:
> Am 05.02.21 um 09:18 schrieb Max Reitz:
>> On 04.02.21 21:09, Peter Lieven wrote:
>>> Am 02.02.21 um 16:51 schrieb Eric Blake:
>>>> On 1/28/21 8:07 AM, Peter Lieven wrote:
>>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>> Your commit message says 'what', but not 'why'. Generally, the one-line
>>>> 'what' works well as the subject line, but you want the commit body to
>>>> give an argument why your patch should be applied, rather than blank.
>>>>
>>>> Here's the last time we tried to improve qemu-img dd:
>>>> https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg02618.html
>>>
>>>
>>> I was not aware of that story. My use case is that I want to be
>>>
>>> able to "patch" an image that Qemu is able to handle by overwriting
>>>
>>> certain sectors. And I especially do not want to "mount" that image
>>>
>>> via qemu-nbd because I might not trust it. I totally want to avoid that the host
>>>
>>> system tries to analyse that image in terms of scanning the bootsector, partprobe,
>>>
>>> lvm etc. pp.
>>
>> qemu will have FUSE exporting as of 6.0 (didn’t quite make it into 5.2), so you can do something like this:
>>
>> $ qemu-storage-daemon \
>> --blockdev node-name=export,driver=qcow2,\
>> file.driver=file,file.filename=image.qcow2 \
>> --export fuse,id=fuse,node-name=export,mountpoint=image.qcow2
>>
>> This exports the image on image.qcow2 (i.e., on itself) and so by accessing the image file you then get raw access to its contents (so you can use system tools like dd).
>>
>> Doesn’t require root rights, and shouldn’t make the kernel scan anything, because it’s exported as just a regular file.
>
>
> Okay, but that is still more housekeeping than just invoking a single command.
Yes, but I personally see this as much better than copying all of dd’s
functionality into qemu-img.
My personal complaint is only that it’s a pain in the ass to invoke QSD
this way. It would be nice to have a script that does the same via
$ qemu-blk-fuse-export image.qcow2
Would probably be trivial to write, but well, first we gotta do it, and
have justification to keep it as part of qemu...
And if that’s still too much housekeeping, we could even write a qemu-dd
script that scans all file arguments for non-raw images, launches a QSD
instance to present them as raw, and then invokes dd.
All much simpler and much more feature-complete than to add more dd
functionality to qemu-img.
> Would it be an option to extend qemu-io to write data at a certain offset which it reads from STDIN?
You mean read the data from stdin? Isn’t that what “write -s <file>” does?
Max
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] qemu-img: add seek and -n option to dd command
2021-02-05 9:16 ` Max Reitz
@ 2021-02-05 10:06 ` Max Reitz
0 siblings, 0 replies; 10+ messages in thread
From: Max Reitz @ 2021-02-05 10:06 UTC (permalink / raw)
To: Peter Lieven, Eric Blake, qemu-block; +Cc: kwolf, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2640 bytes --]
On 05.02.21 10:16, Max Reitz wrote:
> On 05.02.21 09:47, Peter Lieven wrote:
>> Am 05.02.21 um 09:18 schrieb Max Reitz:
>>> On 04.02.21 21:09, Peter Lieven wrote:
>>>> Am 02.02.21 um 16:51 schrieb Eric Blake:
>>>>> On 1/28/21 8:07 AM, Peter Lieven wrote:
>>>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>>> Your commit message says 'what', but not 'why'. Generally, the
>>>>> one-line
>>>>> 'what' works well as the subject line, but you want the commit body to
>>>>> give an argument why your patch should be applied, rather than blank.
>>>>>
>>>>> Here's the last time we tried to improve qemu-img dd:
>>>>> https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg02618.html
>>>>
>>>>
>>>> I was not aware of that story. My use case is that I want to be
>>>>
>>>> able to "patch" an image that Qemu is able to handle by overwriting
>>>>
>>>> certain sectors. And I especially do not want to "mount" that image
>>>>
>>>> via qemu-nbd because I might not trust it. I totally want to avoid
>>>> that the host
>>>>
>>>> system tries to analyse that image in terms of scanning the
>>>> bootsector, partprobe,
>>>>
>>>> lvm etc. pp.
>>>
>>> qemu will have FUSE exporting as of 6.0 (didn’t quite make it into
>>> 5.2), so you can do something like this:
>>>
>>> $ qemu-storage-daemon \
>>> --blockdev node-name=export,driver=qcow2,\
>>> file.driver=file,file.filename=image.qcow2 \
>>> --export fuse,id=fuse,node-name=export,mountpoint=image.qcow2
>>>
>>> This exports the image on image.qcow2 (i.e., on itself) and so by
>>> accessing the image file you then get raw access to its contents (so
>>> you can use system tools like dd).
>>>
>>> Doesn’t require root rights, and shouldn’t make the kernel scan
>>> anything, because it’s exported as just a regular file.
>>
>>
>> Okay, but that is still more housekeeping than just invoking a single
>> command.
>
> Yes, but I personally see this as much better than copying all of dd’s
> functionality into qemu-img.
>
> My personal complaint is only that it’s a pain in the ass to invoke QSD
> this way. It would be nice to have a script that does the same via
>
> $ qemu-blk-fuse-export image.qcow2
>
> Would probably be trivial to write, but well, first we gotta do it, and
> have justification to keep it as part of qemu...
>
> And if that’s still too much housekeeping, we could even write a qemu-dd
> script that scans all file arguments for non-raw images, launches a QSD
> instance to present them as raw, and then invokes dd.
Since today’s Day of Learning at Red Hat, I decided to have some fun
writing a qemu-dd.py script.
Max
[-- Attachment #2: qemu-dd.py --]
[-- Type: text/x-python, Size: 2981 bytes --]
#!/usr/bin/python
import json
import os
import subprocess
import sys
from typing import Optional
images = {}
for arg in sys.argv[1:]:
if arg.startswith('if=') or arg.startswith('of='):
filename = arg[3:]
# Ignore non-existing files, the user probably wants to make
# dd create them (as raw images)
if not os.path.exists(filename):
continue
qemu_img = subprocess.Popen(('qemu-img', 'info', filename),
stdout=subprocess.PIPE,
universal_newlines=True)
output: str = qemu_img.communicate()[0]
if qemu_img.returncode != 0:
sys.exit(os.EX_NOINPUT)
fmt_line = next(line
for line in output.split('\n')
if line.startswith('file format: '))
fmt = fmt_line.split(': ', 1)[1]
if fmt != 'raw':
images[filename] = fmt
qsd: Optional[subprocess.Popen] = None
if images:
args = ['qemu-storage-daemon',
'--chardev', 'stdio,id=monitor',
'--monitor', 'monitor']
for i, (image, fmt) in enumerate(images.items()):
args += ['--blockdev',
f'{fmt},node-name=export{i},' +
f'file.driver=file,file.filename={image}',
'--export',
f'fuse,id=fuse{i},node-name=export{i},' +
f'mountpoint={image},writable=on']
qsd = subprocess.Popen(args,
stdin=subprocess.PIPE,
stdout=subprocess.PIPE,
universal_newlines=True)
assert qsd.stdin is not None
assert qsd.stdout is not None
# Do some QMP communication so we know for sure that the exports
# are up
qmp_line = json.loads(qsd.stdout.readline())
if 'QMP' in qmp_line:
qsd.terminate()
qsd.wait()
sys.stderr.write('The QEMU storage daemon did not provide ' +
'a QMP monitor\n')
sys.exit(os.EX_SOFTWARE)
qsd.stdin.write('{ "execute": "qmp_capabilities" }')
qsd.stdin.flush()
# qmp_capabilities response
if json.loads(qsd.stdout.readline()) != {'return': {}}:
qsd.terminate()
qsd.wait()
sys.stderr.write('Communication error with the QEMU storage daemon\n')
sys.exit(os.EX_SOFTWARE)
while True:
qsd.stdin.write('{ "execute": "query-block-exports" }')
qsd.stdin.flush()
exports = json.loads(qsd.stdout.readline())['return']
if exports is None or len(exports) > len(images):
qsd.terminate()
qsd.wait()
sys.stderr.write('Communication error with the ' +
'QEMU storage daemon\n')
sys.exit(os.EX_SOFTWARE)
if len(exports) == len(images):
break
subprocess.Popen(['dd'] + sys.argv[1:]).wait()
if qsd is not None:
qsd.terminate()
qsd.wait()
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] qemu-img: add seek and -n option to dd command
2021-02-04 20:44 ` Eric Blake
@ 2021-02-05 10:43 ` Richard W.M. Jones
0 siblings, 0 replies; 10+ messages in thread
From: Richard W.M. Jones @ 2021-02-05 10:43 UTC (permalink / raw)
To: Eric Blake; +Cc: kwolf, Peter Lieven, qemu-devel, qemu-block, mreitz
On Thu, Feb 04, 2021 at 02:44:03PM -0600, Eric Blake wrote:
> On 2/4/21 2:09 PM, Peter Lieven wrote:
> > Am 02.02.21 um 16:51 schrieb Eric Blake:
> >> On 1/28/21 8:07 AM, Peter Lieven wrote:
> >>> Signed-off-by: Peter Lieven <pl@kamp.de>
> >> Your commit message says 'what', but not 'why'. Generally, the one-line
> >> 'what' works well as the subject line, but you want the commit body to
> >> give an argument why your patch should be applied, rather than blank.
> >>
> >> Here's the last time we tried to improve qemu-img dd:
> >> https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg02618.html
> >
> >
> > I was not aware of that story. My use case is that I want to be
> >
> > able to "patch" an image that Qemu is able to handle by overwriting
> >
> > certain sectors. And I especially do not want to "mount" that image
> >
> > via qemu-nbd because I might not trust it. I totally want to avoid that the host
> >
> > system tries to analyse that image in terms of scanning the bootsector, partprobe,
> >
> > lvm etc. pp.
>
> qemu-nbd does not have to mount an image (yes, one use of qemu-nbd is to
> use -c /dev/nbdX to get the kernel to mount it; but other uses are to
> expose the NBD image in user-space only with no kernel involvement, and
> therefore no system mount efforts).
I agree, there's nothing unsafe about qemu-nbd (provided you don't use
the -c option).
> Another thing you might try is libnbd, which now includes a utility
> nbdcopy. It should make it easier to overwrite a portion of an NBD
> image using only user-space actions. I'm not sure if Rich has got it
> doing partial file overwrites yet (.../me goes and compiles the latest
> git checkout... nope, still a TODO item to implement subsetting), but it
> may be possible to combine nbdkit's --filter=offset with the full NBD
> image in order to then easily point nbdcopy to only the subset you care
> about. Definitely some ideas worthy of implementation.
TBH I would use nbdsh. For example to overwrite the sector at 1M in a
qcow2 image with "1"s:
$ qemu-img create -f qcow2 test.qcow2 10M
$ nbdsh -c 'h.connect_systemd_socket_activation(["qemu-nbd","-t","-f","qcow2","test.qcow2"])' \
-c 'h.pwrite(b"1"*512, 1024*1024)'
and to show it was really overwritten:
$ nbdcopy -- [ qemu-nbd -f qcow2 test.qcow2 ] - | hexdump -C
00000000 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
*
00100000 31 31 31 31 31 31 31 31 31 31 31 31 31 31 31 31 |1111111111111111|
*
00100200 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
*
00a00000
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine. Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-02-05 10:44 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-28 14:07 [PATCH] qemu-img: add seek and -n option to dd command Peter Lieven
2021-02-02 10:20 ` David Edmondson
2021-02-02 15:51 ` Eric Blake
2021-02-04 20:09 ` Peter Lieven
2021-02-04 20:44 ` Eric Blake
2021-02-05 10:43 ` Richard W.M. Jones
2021-02-05 8:18 ` Max Reitz
2021-02-05 8:47 ` Peter Lieven
2021-02-05 9:16 ` Max Reitz
2021-02-05 10:06 ` Max Reitz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.