* [Qemu-devel] [PATCH 1/3] qemu-io: Use purely string blockdev options
2018-05-02 20:20 [Qemu-devel] [PATCH 0/3] qemu-io/img: Fix -U/force-share conflict testing Max Reitz
@ 2018-05-02 20:20 ` Max Reitz
2018-05-02 21:57 ` Eric Blake
2018-05-02 20:20 ` [Qemu-devel] [PATCH 2/3] qemu-img: Use only string options in img_open_opts Max Reitz
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Max Reitz @ 2018-05-02 20:20 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Fam Zheng, qemu-stable
Currently, qemu-io only uses string-valued blockdev options (as all are
converted directly from QemuOpts) -- with one exception: -U adds the
force-share option as a boolean. This in itself is already a bit
questionable, but a real issue is that it also assumes the value already
existing in the options QDict would be a boolean, which is wrong.
That has the following effect:
$ ./qemu-io -r -U --image-opts \
driver=file,filename=/dev/null,force-share=off
[1] 15200 segmentation fault (core dumped) ./qemu-io -r -U
--image-opts driver=file,filename=/dev/null,force-share=off
Since @opts is converted from QemuOpts, the value must be a string, and
we have to compare it as such. Consequently, it makes sense to also set
it as a string instead of a boolean.
Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
qemu-io.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/qemu-io.c b/qemu-io.c
index e692c555e0..0755a30447 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -95,12 +95,12 @@ static int openfile(char *name, int flags, bool writethrough, bool force_share,
opts = qdict_new();
}
if (qdict_haskey(opts, BDRV_OPT_FORCE_SHARE)
- && !qdict_get_bool(opts, BDRV_OPT_FORCE_SHARE)) {
+ && strcmp(qdict_get_str(opts, BDRV_OPT_FORCE_SHARE), "on")) {
error_report("-U conflicts with image options");
QDECREF(opts);
return 1;
}
- qdict_put_bool(opts, BDRV_OPT_FORCE_SHARE, true);
+ qdict_put_str(opts, BDRV_OPT_FORCE_SHARE, "on");
}
qemuio_blk = blk_new_open(name, NULL, opts, flags, &local_err);
if (!qemuio_blk) {
--
2.14.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] qemu-io: Use purely string blockdev options
2018-05-02 20:20 ` [Qemu-devel] [PATCH 1/3] qemu-io: Use purely string blockdev options Max Reitz
@ 2018-05-02 21:57 ` Eric Blake
0 siblings, 0 replies; 9+ messages in thread
From: Eric Blake @ 2018-05-02 21:57 UTC (permalink / raw)
To: Max Reitz, qemu-block
Cc: Kevin Wolf, qemu-stable, Fam Zheng, qemu-devel, Markus Armbruster
On 05/02/2018 03:20 PM, Max Reitz wrote:
> Currently, qemu-io only uses string-valued blockdev options (as all are
> converted directly from QemuOpts) -- with one exception: -U adds the
> force-share option as a boolean. This in itself is already a bit
> questionable, but a real issue is that it also assumes the value already
> existing in the options QDict would be a boolean, which is wrong.
>
> That has the following effect:
>
> $ ./qemu-io -r -U --image-opts \
> driver=file,filename=/dev/null,force-share=off
> [1] 15200 segmentation fault (core dumped) ./qemu-io -r -U
> --image-opts driver=file,filename=/dev/null,force-share=off
>
> Since @opts is converted from QemuOpts, the value must be a string, and
> we have to compare it as such. Consequently, it makes sense to also set
> it as a string instead of a boolean.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> qemu-io.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
The curse of poor typing in QemuOpts keeps on giving ;)
Adding Markus, so he's aware of yet another wart that his command-line
cleanups may need to visit.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 2/3] qemu-img: Use only string options in img_open_opts
2018-05-02 20:20 [Qemu-devel] [PATCH 0/3] qemu-io/img: Fix -U/force-share conflict testing Max Reitz
2018-05-02 20:20 ` [Qemu-devel] [PATCH 1/3] qemu-io: Use purely string blockdev options Max Reitz
@ 2018-05-02 20:20 ` Max Reitz
2018-05-02 22:00 ` Eric Blake
2018-05-02 20:20 ` [Qemu-devel] [PATCH 3/3] iotests: Add test for -U/force-share conflicts Max Reitz
2018-05-09 21:57 ` [Qemu-devel] [PATCH 0/3] qemu-io/img: Fix -U/force-share conflict testing Max Reitz
3 siblings, 1 reply; 9+ messages in thread
From: Max Reitz @ 2018-05-02 20:20 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Fam Zheng, qemu-stable
img_open_opts() takes a QemuOpts and converts them to a QDict, so all
values therein are strings. Then it may try to call qdict_get_bool(),
however, which will fail with a segmentation fault every time:
$ ./qemu-img info -U --image-opts \
driver=file,filename=/dev/null,force-share=off
[1] 27869 segmentation fault (core dumped) ./qemu-img info -U
--image-opts driver=file,filename=/dev/null,force-share=off
Fix this by using qdict_get_str() and comparing the value as a string.
Also, when adding a force-share value to the QDict, add it as a string
so it fits the rest of the dict.
Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
qemu-img.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 855fa52514..42b60917b0 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -277,12 +277,12 @@ static BlockBackend *img_open_opts(const char *optstr,
options = qemu_opts_to_qdict(opts, NULL);
if (force_share) {
if (qdict_haskey(options, BDRV_OPT_FORCE_SHARE)
- && !qdict_get_bool(options, BDRV_OPT_FORCE_SHARE)) {
+ && strcmp(qdict_get_str(options, BDRV_OPT_FORCE_SHARE), "on")) {
error_report("--force-share/-U conflicts with image options");
QDECREF(options);
return NULL;
}
- qdict_put_bool(options, BDRV_OPT_FORCE_SHARE, true);
+ qdict_put_str(options, BDRV_OPT_FORCE_SHARE, "on");
}
blk = blk_new_open(NULL, NULL, options, flags, &local_err);
if (!blk) {
--
2.14.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] qemu-img: Use only string options in img_open_opts
2018-05-02 20:20 ` [Qemu-devel] [PATCH 2/3] qemu-img: Use only string options in img_open_opts Max Reitz
@ 2018-05-02 22:00 ` Eric Blake
2018-05-02 22:02 ` Max Reitz
0 siblings, 1 reply; 9+ messages in thread
From: Eric Blake @ 2018-05-02 22:00 UTC (permalink / raw)
To: Max Reitz, qemu-block
Cc: Kevin Wolf, qemu-stable, Fam Zheng, qemu-devel, Markus Armbruster
On 05/02/2018 03:20 PM, Max Reitz wrote:
> img_open_opts() takes a QemuOpts and converts them to a QDict, so all
> values therein are strings. Then it may try to call qdict_get_bool(),
> however, which will fail with a segmentation fault every time:
I have no idea if it's worth fixing qdict_get_bool() to at least not
segfault when called on a non-bool Dict member (but what should it
return, true or false? or should it abort() for at least a cleaner
failure than a segfault?)
But in the meantime, your fix is correct.
>
> $ ./qemu-img info -U --image-opts \
> driver=file,filename=/dev/null,force-share=off
> [1] 27869 segmentation fault (core dumped) ./qemu-img info -U
> --image-opts driver=file,filename=/dev/null,force-share=off
>
> Fix this by using qdict_get_str() and comparing the value as a string.
> Also, when adding a force-share value to the QDict, add it as a string
> so it fits the rest of the dict.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> qemu-img.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] qemu-img: Use only string options in img_open_opts
2018-05-02 22:00 ` Eric Blake
@ 2018-05-02 22:02 ` Max Reitz
0 siblings, 0 replies; 9+ messages in thread
From: Max Reitz @ 2018-05-02 22:02 UTC (permalink / raw)
To: Eric Blake, qemu-block
Cc: Kevin Wolf, qemu-stable, Fam Zheng, qemu-devel, Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 1440 bytes --]
On 2018-05-03 00:00, Eric Blake wrote:
> On 05/02/2018 03:20 PM, Max Reitz wrote:
>> img_open_opts() takes a QemuOpts and converts them to a QDict, so all
>> values therein are strings. Then it may try to call qdict_get_bool(),
>> however, which will fail with a segmentation fault every time:
>
> I have no idea if it's worth fixing qdict_get_bool() to at least not
> segfault when called on a non-bool Dict member (but what should it
> return, true or false? or should it abort() for at least a cleaner
> failure than a segfault?)
There's qdict_get_try_bool() which returns a default. For testing
whether the member is a bool I suppose you can use
qdict_to(QBool, qdict_get(qdict, member)).
Max
> But in the meantime, your fix is correct.
>
>>
>> $ ./qemu-img info -U --image-opts \
>> driver=file,filename=/dev/null,force-share=off
>> [1] 27869 segmentation fault (core dumped) ./qemu-img info -U
>> --image-opts driver=file,filename=/dev/null,force-share=off
>>
>> Fix this by using qdict_get_str() and comparing the value as a string.
>> Also, when adding a force-share value to the QDict, add it as a string
>> so it fits the rest of the dict.
>>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> qemu-img.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 3/3] iotests: Add test for -U/force-share conflicts
2018-05-02 20:20 [Qemu-devel] [PATCH 0/3] qemu-io/img: Fix -U/force-share conflict testing Max Reitz
2018-05-02 20:20 ` [Qemu-devel] [PATCH 1/3] qemu-io: Use purely string blockdev options Max Reitz
2018-05-02 20:20 ` [Qemu-devel] [PATCH 2/3] qemu-img: Use only string options in img_open_opts Max Reitz
@ 2018-05-02 20:20 ` Max Reitz
2018-05-02 22:02 ` Eric Blake
2018-05-09 21:57 ` [Qemu-devel] [PATCH 0/3] qemu-io/img: Fix -U/force-share conflict testing Max Reitz
3 siblings, 1 reply; 9+ messages in thread
From: Max Reitz @ 2018-05-02 20:20 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Fam Zheng, qemu-stable
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/153 | 17 +++++++++++++++++
tests/qemu-iotests/153.out | 16 ++++++++++++++++
2 files changed, 33 insertions(+)
diff --git a/tests/qemu-iotests/153 b/tests/qemu-iotests/153
index a0fd815483..ec508c758f 100755
--- a/tests/qemu-iotests/153
+++ b/tests/qemu-iotests/153
@@ -242,6 +242,23 @@ _run_cmd $QEMU_IO "${TEST_IMG}" -c 'write 0 512'
_cleanup_qemu
+echo
+echo "== Detecting -U and force-share conflicts =="
+
+echo
+echo 'No conflict:'
+$QEMU_IMG info -U --image-opts driver=null-co,force-share=on
+echo
+echo 'Conflict:'
+$QEMU_IMG info -U --image-opts driver=null-co,force-share=off
+
+echo
+echo 'No conflict:'
+$QEMU_IO -c 'open -r -U -o driver=null-co,force-share=on'
+echo
+echo 'Conflict:'
+$QEMU_IO -c 'open -r -U -o driver=null-co,force-share=off'
+
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/153.out b/tests/qemu-iotests/153.out
index bb721cb747..2510762ba1 100644
--- a/tests/qemu-iotests/153.out
+++ b/tests/qemu-iotests/153.out
@@ -399,4 +399,20 @@ Is another process using the image?
Closing the other
_qemu_io_wrapper TEST_DIR/t.qcow2 -c write 0 512
+
+== Detecting -U and force-share conflicts ==
+
+No conflict:
+image: null-co://
+file format: null-co
+virtual size: 1.0G (1073741824 bytes)
+disk size: unavailable
+
+Conflict:
+qemu-img: --force-share/-U conflicts with image options
+
+No conflict:
+
+Conflict:
+-U conflicts with image options
*** done
--
2.14.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 0/3] qemu-io/img: Fix -U/force-share conflict testing
2018-05-02 20:20 [Qemu-devel] [PATCH 0/3] qemu-io/img: Fix -U/force-share conflict testing Max Reitz
` (2 preceding siblings ...)
2018-05-02 20:20 ` [Qemu-devel] [PATCH 3/3] iotests: Add test for -U/force-share conflicts Max Reitz
@ 2018-05-09 21:57 ` Max Reitz
3 siblings, 0 replies; 9+ messages in thread
From: Max Reitz @ 2018-05-09 21:57 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Kevin Wolf, Fam Zheng, qemu-stable
[-- Attachment #1: Type: text/plain, Size: 781 bytes --]
On 2018-05-02 22:20, Max Reitz wrote:
> qemu-img and qemu-io try to detect when you use both -U and force-share
> manually, but a conflict is not rejected with an error message but with
> a segmentation fault. I guess that works, but it's probably not the way
> it was meant to be.
>
>
> Max Reitz (3):
> qemu-io: Use purely string blockdev options
> qemu-img: Use only string options in img_open_opts
> iotests: Add test for -U/force-share conflicts
>
> qemu-img.c | 4 ++--
> qemu-io.c | 4 ++--
> tests/qemu-iotests/153 | 17 +++++++++++++++++
> tests/qemu-iotests/153.out | 16 ++++++++++++++++
> 4 files changed, 37 insertions(+), 4 deletions(-)
Thanks for the review, applied to my block branch.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread