Re: [PATCH] block/rbd: fix type of task->complete
On 7/7/21 11:04 AM, Peter Lieven wrote:
> task->complete is a bool not an integer.
>
> Signed-off-by: Peter Lieven
> ---
> block/rbd.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/rbd.c b/block/rbd.c
> index 01a7b94d62..dcf82b15b8 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -1066,7 +1066,7 @@ static int qemu_rbd_resize(BlockDriverState *bs,
> uint64_t size)
> static void qemu_rbd_finish_bh(void *opaque)
> {
> RBDTask *task = opaque;
> -task->complete = 1;
> +task->complete = true;
> aio_co_wake(task->co);
> }
>
>
Hi Peter,
What tree/QEMU git sha does this apply to? I am having trouble
finding definitions for RBDTask and qemu_rbd_finish_bh; and the
patch won't apply to my few-minutes-old clone of upstream.
Connor
Re: [PATCH] docs: add table of contents to QAPI references
On 5/11/21 4:25 AM, Daniel P. Berrangé wrote: > The QAPI reference docs for the guest agent, storage daemon and QMP are > all rather long and hard to navigate unless you already know the name of > the command and can do full text search for it. > > A table of contents in each doc will help people locate stuff much more > easily. > > Signed-off-by: Daniel P. Berrangé > --- This looks so much better! Reviewed-by: Connor Kuehl
[PATCH v3] Document qemu-img options data_file and data_file_raw
The contents of this patch were initially developed and posted by Han Han[1], however, it appears the original patch was not applied. Since then, the relevant documentation has been moved and adapted to a new format. I've taken most of the original wording and tweaked it according to some of the feedback from the original patch submission. I've also adapted it to restructured text, which is the format the documentation currently uses. [1] https://lists.nongnu.org/archive/html/qemu-block/2019-10/msg01253.html Fixes: https://bugzilla.redhat.com/1763105 Signed-off-by: Han Han Suggested-by: Max Reitz [ Max: provided description of data_file_raw behavior ] Signed-off-by: Connor Kuehl --- John, my apologies, I failed to CC you on my last revision (v2) where I addressed your comments. Changes since v2: * Pulled in Max's explanation of data_file_raw behaviors with some minor wording tweaks. Changes since v1: * Clarify different behaviors with these options when using qemu-img create vs amend (Max) * Touch on the negative case of how the file becomes inconsistent (John) docs/tools/qemu-img.rst | 31 +++ 1 file changed, 31 insertions(+) diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst index c9efcfaefc..cfe1147879 100644 --- a/docs/tools/qemu-img.rst +++ b/docs/tools/qemu-img.rst @@ -866,6 +866,37 @@ Supported image file formats: issue ``lsattr filename`` to check if the NOCOW flag is set or not (Capital 'C' is NOCOW flag). + ``data_file`` +Filename where all guest data will be stored. If this option is used, +the qcow2 file will only contain the image's metadata. + +Note: Data loss will occur if the given filename already exists when +using this option with ``qemu-img create`` since ``qemu-img`` will create +the data file anew, overwriting the file's original contents. To simply +update the reference to point to the given pre-existing file, use +``qemu-img amend``. + + ``data_file_raw`` +If this option is set to ``on``, QEMU will always keep the external data +file consistent as a standalone read-only raw image. + +It does this by forwarding all write accesses to the qcow2 file through to +the raw data file, including their offsets. Therefore, data that is visible +on the qcow2 node (i.e., to the guest) at some offset is visible at the same +offset in the raw data file. This results in a read-only raw image. Writes +that bypass the qcow2 metadata may corrupt the qcow2 metadata because the +out-of-band writes may result in the metadata falling out of sync with the +raw image. + +If this option is ``off``, QEMU will use the data file to store data in an +arbitrary manner. The file’s content will not make sense without the +accompanying qcow2 metadata. Where data is written will have no relation to +its offset as seen by the guest, and some writes (specifically zero writes) +may not be forwarded to the data file at all, but will only be handled by +modifying qcow2 metadata. + +This option can only be enabled if ``data_file`` is set. + ``Other`` QEMU also supports various other image file formats for -- 2.30.2
Re: [PATCH v2] Document qemu-img options data_file and data_file_raw
On 4/30/21 9:45 AM, Max Reitz wrote: >> + ``data_file_raw`` >> +If this option is set to ``on``, QEMU will always keep the external >> +data file consistent as a standalone read-only raw image. It does >> +this by forwarding updates through to the raw image in addition to >> +updating the image metadata. If set to ``off``, QEMU will only >> +update the image metadata without forwarding the changes through >> +to the raw image. The default value is ``off``. > > Hm, what updates and what changes? I mean, the first part makes sense (the > “It does this by...”), but the second part doesn’t. qemu will still forward > most writes to the data file. (Not all, but most.) > > (Also, nit pick: With data_file_raw=off, the data file is not a raw image. > (You still call it that in the penultimate sentence.)) > When you write data to a qcow2 file with data_file, the data also goes to the > data_file, most of the time. The exception is when it can be handled with a > metadata update, i.e. when it's a zero write or discard. > > In addition, such updates (i.e. zero writes, I presume) not happening to the > data file are usually a minor problem. The real problem is that without > data_file_raw, data clusters can be allocated anywhere in the data file, > whereas with data_file_raw, they are allocated at their respective guest > offset (i.e. the host offset always equals the guest offset). > > I personally would have been fine with the first sentence, but if we want > more of an explanation... Perhaps: > > < > If this option is set to ``on``, QEMU will always keep the external data file > consistent as a standalone read-only raw image. > > It does this by effectively forwarding all write accesses that happen to the > qcow2 file to the raw data file, including their offsets. Therefore, data > that is visible on the qcow2 node (i.e., to the guest) at some offset is > visible at the same offset in the raw data file. > > If this option is ``off``, QEMU will use the data file just to store data in > an effectively arbitrary manner. The file’s content will not make sense > without the accompanying qcow2 metadata. Where data is written will have no > relation to its offset as seen by the guest, and some writes (specifically > zero writes) may not be forwarded to the data file at all, but will only be > handled by modifying qcow2 metadata. > > In short: With data_file_raw, the data file reads as a valid raw VM image > file. Without it, its content can only be interpreted by reading the > accompanying qcow2 metadata. > > Note that this option only makes the data file valid as a read-only raw > image. You should not write to it, as this may effectively corrupt the qcow2 > metadata (for example, dirty bitmaps may become out of sync). > > EOF > > This got longer than I wanted it to be. Hm. Anyway, what do you think? I found it very helpful. I'll incorporate your explanation into the next revision. I'm wondering what the most appropriate trailer would be for the next revision? Suggested-by: Max [..] Co-developed-by: Max [..] Let me know if you have a strong preference, otherwise I'll go with Suggested-by: Thank you, Connor
[PATCH v2] Document qemu-img options data_file and data_file_raw
The contents of this patch were initially developed and posted by Han Han[1], however, it appears the original patch was not applied. Since then, the relevant documentation has been moved and adapted to a new format. I've taken most of the original wording and tweaked it according to some of the feedback from the original patch submission. I've also adapted it to restructured text, which is the format the documentation currently uses. [1] https://lists.nongnu.org/archive/html/qemu-block/2019-10/msg01253.html Fixes: https://bugzilla.redhat.com/1763105 Signed-off-by: Han Han Signed-off-by: Connor Kuehl --- Changes since v1: * Clarify different behaviors with these options when using qemu-img create vs amend (Max) * Touch on the negative case of how the file becomes inconsistent (John) docs/tools/qemu-img.rst | 20 1 file changed, 20 insertions(+) diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst index c9efcfaefc..87b4a65535 100644 --- a/docs/tools/qemu-img.rst +++ b/docs/tools/qemu-img.rst @@ -866,6 +866,26 @@ Supported image file formats: issue ``lsattr filename`` to check if the NOCOW flag is set or not (Capital 'C' is NOCOW flag). + ``data_file`` +Filename where all guest data will be stored. If this option is used, +the qcow2 file will only contain the image's metadata. + +Note: Data loss will occur if the given filename already exists when +using this option with ``qemu-img create`` since ``qemu-img`` will create +the data file anew, overwriting the file's original contents. To simply +update the reference to point to the given pre-existing file, use +``qemu-img amend``. + + ``data_file_raw`` +If this option is set to ``on``, QEMU will always keep the external +data file consistent as a standalone read-only raw image. It does +this by forwarding updates through to the raw image in addition to +updating the image metadata. If set to ``off``, QEMU will only +update the image metadata without forwarding the changes through +to the raw image. The default value is ``off``. + +This option can only be enabled if ``data_file`` is set. + ``Other`` QEMU also supports various other image file formats for -- 2.30.2
[PATCH v4 1/2] iotests/231: Update expected deprecation message
The deprecation message in the expected output has technically been
wrong since the wrong version of a patch was applied to it. Because of
this, the test fails. Correct the expected output so that it passes.
Signed-off-by: Connor Kuehl
Reviewed-by: Max Reitz
Reviewed-by: Stefano Garzarella
---
v3 -> v4:
* Added Stefano's s-o-b to the commit message
tests/qemu-iotests/231.out | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
index 579ba11c16..747dd221bb 100644
--- a/tests/qemu-iotests/231.out
+++ b/tests/qemu-iotests/231.out
@@ -1,9 +1,7 @@
QA output created by 231
-qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated.
Future versions may cease to parse these options in the future.
+qemu-img: warning: RBD options encoded in the filename as keyvalue pairs is
deprecated
unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
error connecting: No such file or directory
unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
qemu-img: Could not open
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
error connecting: No such file or directory
*** done
--
2.30.2
[PATCH v4 0/2] Fix segfault in qemu_rbd_parse_filename
Connor Kuehl (2): iotests/231: Update expected deprecation message block/rbd: Add an escape-aware strchr helper block/rbd.c| 32 +--- tests/qemu-iotests/231 | 4 tests/qemu-iotests/231.out | 7 --- 3 files changed, 29 insertions(+), 14 deletions(-) -- 2.30.2
[PATCH v4 2/2] block/rbd: Add an escape-aware strchr helper
Sometimes the parser needs to further split a token it has collected
from the token input stream. Right now, it does a cursory check to see
if the relevant characters appear in the token to determine if it should
break it down further.
However, qemu_rbd_next_tok() will escape characters as it removes tokens
from the token stream and plain strchr() won't. This can make the
initial strchr() check slightly misleading since it implies
qemu_rbd_next_tok() will find the token and split on it, except the
reality is that qemu_rbd_next_tok() will pass over it if it is escaped.
Use a custom strchr to avoid mixing escaped and unescaped string
operations. Furthermore, this code is identical to how
qemu_rbd_next_tok() seeks its next token, so incorporate this custom
strchr into the body of that function to reduce duplication.
Reported-by: Han Han
Fixes: https://bugzilla.redhat.com/1873913
Signed-off-by: Connor Kuehl
---
v3 -> v4:
* Replace qemu_rbd_next_tok() seek loop with qemu_rbd_strchr() since
they're identical
block/rbd.c| 32 +---
tests/qemu-iotests/231 | 4
tests/qemu-iotests/231.out | 3 +++
3 files changed, 28 insertions(+), 11 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index f098a89c7b..26f64cce7c 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -113,21 +113,31 @@ static int qemu_rbd_connect(rados_t *cluster,
rados_ioctx_t *io_ctx,
const char *keypairs, const char *secretid,
Error **errp);
+static char *qemu_rbd_strchr(char *src, char delim)
+{
+char *p;
+
+for (p = src; *p; ++p) {
+if (*p == delim) {
+return p;
+}
+if (*p == '\\' && p[1] != '\0') {
+++p;
+}
+}
+
+return NULL;
+}
+
+
static char *qemu_rbd_next_tok(char *src, char delim, char **p)
{
char *end;
*p = NULL;
-for (end = src; *end; ++end) {
-if (*end == delim) {
-break;
-}
-if (*end == '\\' && end[1] != '\0') {
-end++;
-}
-}
-if (*end == delim) {
+end = qemu_rbd_strchr(src, delim);
+if (end) {
*p = end + 1;
*end = '\0';
}
@@ -171,7 +181,7 @@ static void qemu_rbd_parse_filename(const char *filename,
QDict *options,
qemu_rbd_unescape(found_str);
qdict_put_str(options, "pool", found_str);
-if (strchr(p, '@')) {
+if (qemu_rbd_strchr(p, '@')) {
image_name = qemu_rbd_next_tok(p, '@', &p);
found_str = qemu_rbd_next_tok(p, ':', &p);
@@ -181,7 +191,7 @@ static void qemu_rbd_parse_filename(const char *filename,
QDict *options,
image_name = qemu_rbd_next_tok(p, ':', &p);
}
/* Check for namespace in the image_name */
-if (strchr(image_name, '/')) {
+if (qemu_rbd_strchr(image_name, '/')) {
found_str = qemu_rbd_next_tok(image_name, '/', &image_name);
qemu_rbd_unescape(found_str);
qdict_put_str(options, "namespace", found_str);
diff --git a/tests/qemu-iotests/231 b/tests/qemu-iotests/231
index 0f66d0ca36..8e6c6447c1 100755
--- a/tests/qemu-iotests/231
+++ b/tests/qemu-iotests/231
@@ -55,6 +55,10 @@ _filter_conf()
$QEMU_IMG info
"json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=${BOGUS_CONF}'}"
2>&1 | _filter_conf
$QEMU_IMG info
"json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'${BOGUS_CONF}'}"
2>&1 | _filter_conf
+# Regression test: the qemu-img invocation is expected to fail, but it should
+# not seg fault the parser.
+$QEMU_IMG create "rbd:rbd/aa\/bb:conf=${BOGUS_CONF}" 1M 2>&1 | _filter_conf
+
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
index 747dd221bb..a785a6e859 100644
--- a/tests/qemu-iotests/231.out
+++ b/tests/qemu-iotests/231.out
@@ -4,4 +4,7 @@ unable to get monitor info from DNS SRV with service name:
ceph-mon
qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
error connecting: No such file or directory
unable to get monitor info from DNS SRV with service name: ceph-mon
qemu-img: Could not open
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
error connecting: No such file or directory
+Formatting 'rbd:rbd/aa\/bb:conf=BOGUS_CONF', fmt=raw size=1048576
+unable to get monitor info from DNS SRV with service name: ceph-mon
+qemu-img: rbd:rbd/aa\/bb:conf=BOGUS_CONF: error connecting: No such file or
directory
*** done
--
2.30.2
Re: [PATCH v3 2/2] block/rbd: Add an escape-aware strchr helper
On 4/21/21 6:04 AM, Stefano Garzarella wrote:
>> +static char *qemu_rbd_strchr(char *src, char delim)
>> +{
>> +char *p;
>> +
>> +for (p = src; *p; ++p) {
>> +if (*p == delim) {
>> +return p;
>> +}
>> +if (*p == '\\' && p[1] != '\0') {
>> +++p;
>> +}
>> +}
>> +
>> +return NULL;
>> +}
>> +
>
> IIUC this is similar to the code used in qemu_rbd_next_tok().
> To avoid code duplication can we use this new function inside it?
Hi Stefano! Good catch. I think you're right. I've incorporated your
suggestion into my next revision. The only thing I changed was the
if-statement from:
if (end && *end == delim) {
to:
if (end) {
Since qemu_rbd_strchr() will only ever return a non-NULL pointer if it
was able to find 'delim'.
Connor
>
> I mean something like this (not tested):
>
> diff --git a/block/rbd.c b/block/rbd.c
> index f098a89c7b..eb6a839362 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -119,15 +119,8 @@ static char *qemu_rbd_next_tok(char *src, char delim,
> char **p)
>
> *p = NULL;
>
> -for (end = src; *end; ++end) {
> -if (*end == delim) {
> -break;
> -}
> -if (*end == '\\' && end[1] != '\0') {
> -end++;
> -}
> -}
> -if (*end == delim) {
> +end = qemu_rbd_strchr(src, delim);
> +if (end && *end == delim) {
> *p = end + 1;
> *end = '\0';
> }
>
>
> The rest LGTM!
>
> Thanks for fixing this issue,
> Stefano
>
Re: [PATCH 0/5] block, migration: improve debugging of migration bdrv_flush failure
On 4/15/21 8:58 AM, Daniel P. Berrangé wrote: I spent a while debugging a tricky migration failure today which was ultimately caused by fdatasync() getting EACCESS. The existing probes were not sufficient to diagnose this, so I had to resort to GDB. This improves probes and block error reporting to make future diagnosis possible without GDB. Daniel P. Berrangé (5): migration: add trace point when vm_stop_force_state fails softmmu: add trace point when bdrv_flush_all fails block: preserve errno from fdatasync failures block: add trace point when fdatasync fails block: remove duplicate trace.h include block/file-posix.c | 10 +- block/trace-events | 1 + migration/migration.c | 1 + migration/trace-events | 1 + softmmu/cpus.c | 7 ++- softmmu/trace-events | 3 +++ 6 files changed, 17 insertions(+), 6 deletions(-) For the series: Reviewed-by: Connor Kuehl
Re: General question about parsing an rbd filename
On 4/9/21 9:27 AM, Markus Armbruster wrote: Connor Kuehl writes: block/rbd.c hints that: * Configuration values containing :, @, or = can be escaped with a * leading "\". Right now, much of the parsing code will allow anyone to escape _anything_ so long as it's preceded by '\'. Is this the intended behavior? Or should the parser be updated to allow escaping only certain sequences. I can't answer this question, but perhaps I can get us a bit closer to an answer. The commend you quoted in part is about "rbd:" pseudo-filenames. By "parsing code", you probably mean qemu_rbd_parse_filename(). It uses qemu_rbd_next_tok() to split off one part after the other, stopping at a special delimiter character, and qemu_rbd_unescape() to unescape most, but not all parts. Both treat '\' followed by a character other than '\0' specially. qemu_rbd_next_tok() doesn't stop at an escaped delimiter character. qemu_rbd_unescape() unescapes escaped characters. I believe the comment you quoted is basically trying to say "to use a character that would normally be a delimiter, escape it with '\'". It doesn't say these are the only characters you may escape. I agree with your interpretation here. Not unescaping some parts feels iffy to me. I wonder if my reading of it placed too much emphasis on the "Configuration values" part of it, which I understand to be the optional key,value pairs that come after the image name. Thank you, Connor
[PATCH v3 2/2] block/rbd: Add an escape-aware strchr helper
Sometimes the parser needs to further split a token it has collected
from the token input stream. Right now, it does a cursory check to see
if the relevant characters appear in the token to determine if it should
break it down further.
However, qemu_rbd_next_tok() will escape characters as it removes tokens
from the token stream and plain strchr() won't. This can make the
initial strchr() check slightly misleading since it implies
qemu_rbd_next_tok() will find the token and split on it, except the
reality is that qemu_rbd_next_tok() will pass over it if it is escaped.
Use a custom strchr to avoid mixing escaped and unescaped string
operations.
Reported-by: Han Han
Fixes: https://bugzilla.redhat.com/1873913
Signed-off-by: Connor Kuehl
---
v2 -> v3:
* Update qemu_rbd_strchr to only skip if there's a delimiter AND the
next character is not the NUL terminator
block/rbd.c| 20 ++--
tests/qemu-iotests/231 | 4
tests/qemu-iotests/231.out | 3 +++
3 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index 9071a00e3f..291e3f09e1 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -134,6 +134,22 @@ static char *qemu_rbd_next_tok(char *src, char delim, char
**p)
return src;
}
+static char *qemu_rbd_strchr(char *src, char delim)
+{
+char *p;
+
+for (p = src; *p; ++p) {
+if (*p == delim) {
+return p;
+}
+if (*p == '\\' && p[1] != '\0') {
+++p;
+}
+}
+
+return NULL;
+}
+
static void qemu_rbd_unescape(char *src)
{
char *p;
@@ -171,7 +187,7 @@ static void qemu_rbd_parse_filename(const char *filename,
QDict *options,
qemu_rbd_unescape(found_str);
qdict_put_str(options, "pool", found_str);
-if (strchr(p, '@')) {
+if (qemu_rbd_strchr(p, '@')) {
image_name = qemu_rbd_next_tok(p, '@', &p);
found_str = qemu_rbd_next_tok(p, ':', &p);
@@ -181,7 +197,7 @@ static void qemu_rbd_parse_filename(const char *filename,
QDict *options,
image_name = qemu_rbd_next_tok(p, ':', &p);
}
/* Check for namespace in the image_name */
-if (strchr(image_name, '/')) {
+if (qemu_rbd_strchr(image_name, '/')) {
found_str = qemu_rbd_next_tok(image_name, '/', &image_name);
qemu_rbd_unescape(found_str);
qdict_put_str(options, "namespace", found_str);
diff --git a/tests/qemu-iotests/231 b/tests/qemu-iotests/231
index 0f66d0ca36..8e6c6447c1 100755
--- a/tests/qemu-iotests/231
+++ b/tests/qemu-iotests/231
@@ -55,6 +55,10 @@ _filter_conf()
$QEMU_IMG info
"json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=${BOGUS_CONF}'}"
2>&1 | _filter_conf
$QEMU_IMG info
"json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'${BOGUS_CONF}'}"
2>&1 | _filter_conf
+# Regression test: the qemu-img invocation is expected to fail, but it should
+# not seg fault the parser.
+$QEMU_IMG create "rbd:rbd/aa\/bb:conf=${BOGUS_CONF}" 1M 2>&1 | _filter_conf
+
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
index 747dd221bb..a785a6e859 100644
--- a/tests/qemu-iotests/231.out
+++ b/tests/qemu-iotests/231.out
@@ -4,4 +4,7 @@ unable to get monitor info from DNS SRV with service name:
ceph-mon
qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
error connecting: No such file or directory
unable to get monitor info from DNS SRV with service name: ceph-mon
qemu-img: Could not open
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
error connecting: No such file or directory
+Formatting 'rbd:rbd/aa\/bb:conf=BOGUS_CONF', fmt=raw size=1048576
+unable to get monitor info from DNS SRV with service name: ceph-mon
+qemu-img: rbd:rbd/aa\/bb:conf=BOGUS_CONF: error connecting: No such file or
directory
*** done
--
2.30.2
[PATCH v3 1/2] iotests/231: Update expected deprecation message
The deprecation message in the expected output has technically been
wrong since the wrong version of a patch was applied to it. Because of
this, the test fails. Correct the expected output so that it passes.
Signed-off-by: Connor Kuehl
Reviewed-by: Max Reitz
---
tests/qemu-iotests/231.out | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
index 579ba11c16..747dd221bb 100644
--- a/tests/qemu-iotests/231.out
+++ b/tests/qemu-iotests/231.out
@@ -1,9 +1,7 @@
QA output created by 231
-qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated.
Future versions may cease to parse these options in the future.
+qemu-img: warning: RBD options encoded in the filename as keyvalue pairs is
deprecated
unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
error connecting: No such file or directory
unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
qemu-img: Could not open
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
error connecting: No such file or directory
*** done
--
2.30.2
[PATCH v3 0/2] Fix segfault in qemu_rbd_parse_filename
Connor Kuehl (2): iotests/231: Update expected deprecation message block/rbd: Add an escape-aware strchr helper block/rbd.c| 20 ++-- tests/qemu-iotests/231 | 4 tests/qemu-iotests/231.out | 7 --- 3 files changed, 26 insertions(+), 5 deletions(-) -- 2.30.2
Re: [PATCH v2 2/2] block/rbd: Add an escape-aware strchr helper
On 4/6/21 9:24 AM, Max Reitz wrote:
On 01.04.21 23:01, Connor Kuehl wrote:
[..]
diff --git a/block/rbd.c b/block/rbd.c
index 9071a00e3f..c0e4d4a952 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -134,6 +134,22 @@ static char *qemu_rbd_next_tok(char *src, char
delim, char **p)
return src;
}
+static char *qemu_rbd_strchr(char *src, char delim)
+{
+ char *p;
+
+ for (p = src; *p; ++p) {
+ if (*p == delim) {
+ return p;
+ }
+ if (*p == '\\') {
+ ++p;
+ }
+ }
+
+ return NULL;
+}
+
So I thought you could make qemu_rbd_do_next_tok() to do this. (I
didn’t say you should, but bear with me.) That would be possible by
giving it a new parameter (e.g. @find), and if that is set, return @end
if *end == delim after the loop, and NULL otherwise.
Now, if you add wrapper functions to make it nice, there’s not much more
difference in lines added compared to just adding a new function, but it
does mean your function should basically be the same as
qemu_rbd_next_tok(), except that no splitting happens, that there is no
*p, and that @end is returned instead of @src.
Do you have a strong preference for this? I agree that
qemu_rbd_next_tok() could grow this functionality, but I think it'd be
simpler to keep it separate in the form of qemu_rbd_strchr().
So there is one difference, and that is that qemu_rbd_next_tok() has
this condition to skip escaped characters:
if (*end == '\\' && end[1] != '\0') {
where qemu_rbd_strchr() has only:
if (*p == '\\') {
And I think qemu_rbd_next_tok() is right; if the string in question has
a trailing backslash, qemu_rbd_strchr() will ignore the final NUL and
continue searching past the end of the string.
Aha, good catch. I'll fix this up.
Thank you,
Connor
General question about parsing an rbd filename
Hi, block/rbd.c hints that: * Configuration values containing :, @, or = can be escaped with a * leading "\". Right now, much of the parsing code will allow anyone to escape _anything_ so long as it's preceded by '\'. Is this the intended behavior? Or should the parser be updated to allow escaping only certain sequences. Just curious. Thanks, Connor
[PATCH v2 1/2] iotests/231: Update expected deprecation message
The deprecation message in the expected output has technically been
wrong since the wrong version of a patch was applied to it. Because of
this, the test fails. Correct the expected output so that it passes.
Signed-off-by: Connor Kuehl
Reviewed-by: Max Reitz
---
Reworded the commit log and added Max's R-b.
tests/qemu-iotests/231.out | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
index 579ba11c16..747dd221bb 100644
--- a/tests/qemu-iotests/231.out
+++ b/tests/qemu-iotests/231.out
@@ -1,9 +1,7 @@
QA output created by 231
-qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated.
Future versions may cease to parse these options in the future.
+qemu-img: warning: RBD options encoded in the filename as keyvalue pairs is
deprecated
unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
error connecting: No such file or directory
unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
qemu-img: Could not open
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
error connecting: No such file or directory
*** done
--
2.30.2
[PATCH v2 2/2] block/rbd: Add an escape-aware strchr helper
Sometimes the parser needs to further split a token it has collected
from the token input stream. Right now, it does a cursory check to see
if the relevant characters appear in the token to determine if it should
break it down further.
However, qemu_rbd_next_tok() will escape characters as it removes tokens
from the token stream and plain strchr() won't. This can make the
initial strchr() check slightly misleading since it implies
qemu_rbd_next_tok() will find the token and split on it, except the
reality is that qemu_rbd_next_tok() will pass over it if it is escaped.
Use a custom strchr to avoid mixing escaped and unescaped string
operations.
Reported-by: Han Han
Fixes: https://bugzilla.redhat.com/1873913
Signed-off-by: Connor Kuehl
---
block/rbd.c| 20 ++--
tests/qemu-iotests/231 | 4
tests/qemu-iotests/231.out | 3 +++
3 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index 9071a00e3f..c0e4d4a952 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -134,6 +134,22 @@ static char *qemu_rbd_next_tok(char *src, char delim, char
**p)
return src;
}
+static char *qemu_rbd_strchr(char *src, char delim)
+{
+char *p;
+
+for (p = src; *p; ++p) {
+if (*p == delim) {
+return p;
+}
+if (*p == '\\') {
+++p;
+}
+}
+
+return NULL;
+}
+
static void qemu_rbd_unescape(char *src)
{
char *p;
@@ -171,7 +187,7 @@ static void qemu_rbd_parse_filename(const char *filename,
QDict *options,
qemu_rbd_unescape(found_str);
qdict_put_str(options, "pool", found_str);
-if (strchr(p, '@')) {
+if (qemu_rbd_strchr(p, '@')) {
image_name = qemu_rbd_next_tok(p, '@', &p);
found_str = qemu_rbd_next_tok(p, ':', &p);
@@ -181,7 +197,7 @@ static void qemu_rbd_parse_filename(const char *filename,
QDict *options,
image_name = qemu_rbd_next_tok(p, ':', &p);
}
/* Check for namespace in the image_name */
-if (strchr(image_name, '/')) {
+if (qemu_rbd_strchr(image_name, '/')) {
found_str = qemu_rbd_next_tok(image_name, '/', &image_name);
qemu_rbd_unescape(found_str);
qdict_put_str(options, "namespace", found_str);
diff --git a/tests/qemu-iotests/231 b/tests/qemu-iotests/231
index 0f66d0ca36..8e6c6447c1 100755
--- a/tests/qemu-iotests/231
+++ b/tests/qemu-iotests/231
@@ -55,6 +55,10 @@ _filter_conf()
$QEMU_IMG info
"json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=${BOGUS_CONF}'}"
2>&1 | _filter_conf
$QEMU_IMG info
"json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'${BOGUS_CONF}'}"
2>&1 | _filter_conf
+# Regression test: the qemu-img invocation is expected to fail, but it should
+# not seg fault the parser.
+$QEMU_IMG create "rbd:rbd/aa\/bb:conf=${BOGUS_CONF}" 1M 2>&1 | _filter_conf
+
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
index 747dd221bb..a785a6e859 100644
--- a/tests/qemu-iotests/231.out
+++ b/tests/qemu-iotests/231.out
@@ -4,4 +4,7 @@ unable to get monitor info from DNS SRV with service name:
ceph-mon
qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
error connecting: No such file or directory
unable to get monitor info from DNS SRV with service name: ceph-mon
qemu-img: Could not open
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
error connecting: No such file or directory
+Formatting 'rbd:rbd/aa\/bb:conf=BOGUS_CONF', fmt=raw size=1048576
+unable to get monitor info from DNS SRV with service name: ceph-mon
+qemu-img: rbd:rbd/aa\/bb:conf=BOGUS_CONF: error connecting: No such file or
directory
*** done
--
2.30.2
[PATCH v2 0/2] Fix segfault in qemu_rbd_parse_filename
Replaced the change to qemu_rbd_next_tok with a standalone escape-aware helper for patch 2. Connor Kuehl (2): iotests/231: Update expected deprecation message block/rbd: Add an escape-aware strchr helper block/rbd.c| 20 ++-- tests/qemu-iotests/231 | 4 tests/qemu-iotests/231.out | 7 --- 3 files changed, 26 insertions(+), 5 deletions(-) -- 2.30.2
Re: [PATCH 2/2] block/rbd: Don't unescape in qemu_rbd_next_tok()
On 4/1/21 12:24 PM, Max Reitz wrote:
On 01.04.21 17:52, Connor Kuehl wrote:
That's qemu_rbd_unescape()'s job! No need to duplicate the labor.
Furthermore, this was causing some confusion in the parsing logic to
where the caller might test for the presence of a character to split on
like so:
if (strchr(image_name, '/')) {
found_str = qemu_rbd_next_tok(image_name, '/', &image_name);
[..]
When qemu_rbd_next_tok() performs unescaping as a side effect, the
parser can get confused thinking that it can split this string, but
really the delimiter '/' gets unescaped and so qemu_rbd_next_tok() never
"finds" the delimiter and consumes the rest of the token input stream.
I don’t fully understand. I understand the strchr() problem and the
thing you explain next. But I would have thought that’s a problem of
using strchr(), i.e. that we’d need a custom function to do strchr() but
consider escaped characters. If it’s just about true/false like in your
example, it could be a new version of qemu_rbd_next_tok() that does not
modify the string.
I went back and forth a lot on the different ways this can be fixed
before sending this, and I agree the most consistent fix here would be
to add an escape-aware strchr. Initially, adding another libc-like
function with more side effects wasn't as appealing to me as removing
the side effects entirely to separate mechanism vs. policy. Your example
below convinced me that this patch would split the token in unexpected
ways. I'll send a v2 :-)
Thanks,
Connor
[..]
Should it? I would have fully expected it to not be split and the
parser complains that the input is invalid. Or, let’s say,
"foo\/bar/baz” should be split into “foo\/bar” and “baz”.
Re: [PATCH 1/2] iotests/231: Update expected deprecation message
On 4/1/21 12:07 PM, Max Reitz wrote: On 01.04.21 18:52, Max Reitz wrote: On 01.04.21 17:52, Connor Kuehl wrote: The deprecation message changed slightly at some point in the past but the expected output wasn't updated along with it; causing it to fail. Fix it, so it passes. Signed-off-by: Connor Kuehl --- tests/qemu-iotests/231.out | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) Uh, well, you know what, I can’t find any version where there was any other output. Even back in 66e6a735e97450ac50fcaf40f78600c688534cae, where this test was introduced, I get this diff. What’s going on there? Okay. So: Jeff’s original patch[1] included the “Future versions may cease to parse...” part. v1 of his subsequent pull request[2] did, too. But v2[3] didn’t. Looks like Markus made a comment on v4 of the patch, and then Jeff fixed up the patch in his branch, but didn’t change the test. In any case it’s clear that the reference output was wrong all along. About the “no monitors specified” part... The only place where I can find “no monitors” is in Jeff’s patches to add this iotest. I have no idea where that orignated from. So: Reviewed-by: Max Reitz Thanks! And excellent sleuthing. This accidental conspiracy went much farther beyond the git log than I thought... Connor [1] https://lists.nongnu.org/archive/html/qemu-block/2018-09/msg00282.html [2] https://lists.nongnu.org/archive/html/qemu-block/2018-09/msg00307.html [3] https://lists.nongnu.org/archive/html/qemu-block/2018-09/msg00592.html
[PATCH 1/2] iotests/231: Update expected deprecation message
The deprecation message changed slightly at some point in the past but
the expected output wasn't updated along with it; causing it to fail.
Fix it, so it passes.
Signed-off-by: Connor Kuehl
---
tests/qemu-iotests/231.out | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
index 579ba11c16..747dd221bb 100644
--- a/tests/qemu-iotests/231.out
+++ b/tests/qemu-iotests/231.out
@@ -1,9 +1,7 @@
QA output created by 231
-qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated.
Future versions may cease to parse these options in the future.
+qemu-img: warning: RBD options encoded in the filename as keyvalue pairs is
deprecated
unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
error connecting: No such file or directory
unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
qemu-img: Could not open
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
error connecting: No such file or directory
*** done
--
2.30.2
[PATCH 0/2] Fix segfault in qemu_rbd_parse_filename
Connor Kuehl (2): iotests/231: Update expected deprecation message block/rbd: Don't unescape in qemu_rbd_next_tok() block/rbd.c| 3 --- tests/qemu-iotests/231 | 4 tests/qemu-iotests/231.out | 7 --- 3 files changed, 8 insertions(+), 6 deletions(-) -- 2.30.2
[PATCH 2/2] block/rbd: Don't unescape in qemu_rbd_next_tok()
That's qemu_rbd_unescape()'s job! No need to duplicate the labor.
Furthermore, this was causing some confusion in the parsing logic to
where the caller might test for the presence of a character to split on
like so:
if (strchr(image_name, '/')) {
found_str = qemu_rbd_next_tok(image_name, '/', &image_name);
[..]
When qemu_rbd_next_tok() performs unescaping as a side effect, the
parser can get confused thinking that it can split this string, but
really the delimiter '/' gets unescaped and so qemu_rbd_next_tok() never
"finds" the delimiter and consumes the rest of the token input stream.
This is problematic because qemu_rbd_next_tok() also steals the input
pointer to the token stream and sets it to NULL. This causes a segfault
where the parser expects to split one string into two.
In this case, the parser is determining if a string is a
namespace/image_name pair like so:
"foo/bar"
And clearly it's looking to split it like so:
namespace: foo
image_name: bar
but if the input is "foo\/bar", it *should* split into
namespace: foo\
image_name: bar
and its subordinate parts can be unescaped after tokenization.
So, instead of tokenizing *and* escaping all at once, do one before the
other to avoid stumbling into a segfault by confusing the parsing logic.
Reported-by: Han Han
Fixes: https://bugzilla.redhat.com/1873913
Signed-off-by: Connor Kuehl
---
block/rbd.c| 3 ---
tests/qemu-iotests/231 | 4
tests/qemu-iotests/231.out | 3 +++
3 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index 9071a00e3f..9bed0863e5 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -123,9 +123,6 @@ static char *qemu_rbd_next_tok(char *src, char delim, char
**p)
if (*end == delim) {
break;
}
-if (*end == '\\' && end[1] != '\0') {
-end++;
-}
}
if (*end == delim) {
*p = end + 1;
diff --git a/tests/qemu-iotests/231 b/tests/qemu-iotests/231
index 0f66d0ca36..8e6c6447c1 100755
--- a/tests/qemu-iotests/231
+++ b/tests/qemu-iotests/231
@@ -55,6 +55,10 @@ _filter_conf()
$QEMU_IMG info
"json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=${BOGUS_CONF}'}"
2>&1 | _filter_conf
$QEMU_IMG info
"json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'${BOGUS_CONF}'}"
2>&1 | _filter_conf
+# Regression test: the qemu-img invocation is expected to fail, but it should
+# not seg fault the parser.
+$QEMU_IMG create "rbd:rbd/aa\/bb:conf=${BOGUS_CONF}" 1M 2>&1 | _filter_conf
+
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
index 747dd221bb..a785a6e859 100644
--- a/tests/qemu-iotests/231.out
+++ b/tests/qemu-iotests/231.out
@@ -4,4 +4,7 @@ unable to get monitor info from DNS SRV with service name:
ceph-mon
qemu-img: Could not open
'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}':
error connecting: No such file or directory
unable to get monitor info from DNS SRV with service name: ceph-mon
qemu-img: Could not open
'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}':
error connecting: No such file or directory
+Formatting 'rbd:rbd/aa\/bb:conf=BOGUS_CONF', fmt=raw size=1048576
+unable to get monitor info from DNS SRV with service name: ceph-mon
+qemu-img: rbd:rbd/aa\/bb:conf=BOGUS_CONF: error connecting: No such file or
directory
*** done
--
2.30.2
[PATCH 1/1] iotests: fix 051.out expected output after error text touchups
A patch was recently applied that touched up some error messages that
pertained to key names like 'node-name'. The trouble is it only updated
tests/qemu-iotests/051.pc.out and not tests/qemu-iotests/051.out as
well.
Do that now.
Fixes: 785ec4b1b9 ("block: Clarify error messages pertaining to
'node-name'")
Signed-off-by: Connor Kuehl
---
tests/qemu-iotests/051.out | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index de4771bcb3..db8c14b903 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -61,13 +61,13 @@ QEMU X.Y.Z monitor - type 'help' for more information
(qemu) quit
Testing: -drive file=TEST_DIR/t.qcow2,node-name=123foo
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=123foo: Invalid node name
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=123foo: Invalid node-name:
'123foo'
Testing: -drive file=TEST_DIR/t.qcow2,node-name=_foo
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=_foo: Invalid node name
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=_foo: Invalid node-name:
'_foo'
Testing: -drive file=TEST_DIR/t.qcow2,node-name=foo#12
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=foo#12: Invalid node name
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=foo#12: Invalid node-name:
'foo#12'
=== Device without drive ===
--
2.30.2
[PATCH 0/1] iotests: fix 051.out expected output after error
Oops, sorry about the churn. I can see why this would have caused a failure but I'm surprised I can't reproduce this when I run the test locally. Christian, would you be willing to test this patch out as a quick sanity check too? Connor Kuehl (1): iotests: fix 051.out expected output after error text touchups tests/qemu-iotests/051.out | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) -- 2.30.2
[PATCH v2 1/2] block: Clarify error messages pertaining to 'node-name'
Some error messages contain ambiguous representations of the 'node-name'
parameter. This can be particularly confusing when exchanging QMP
messages (C = client, S = server):
C: {"execute": "block_resize", "arguments": { "device": "my_file", "size":
26843545600 }}
S: {"error": {"class": "GenericError", "desc": "Cannot find device=my_file nor
node_name="}}
^
This error message suggests one could send a message with a key called
'node_name':
C: {"execute": "block_resize", "arguments": { "node_name": "my_file", "size":
26843545600 }}
^
but using the underscore is actually incorrect, the parameter should be
'node-name':
S: {"error": {"class": "GenericError", "desc": "Parameter 'node_name' is
unexpected"}}
This behavior was uncovered in bz1651437, but I ended up going down a
rabbit hole looking for other areas where this miscommunication might
occur and changing those accordingly as well.
Fixes: https://bugzilla.redhat.com/1651437
Signed-off-by: Connor Kuehl
---
block.c | 8
tests/qemu-iotests/030| 4 ++--
tests/qemu-iotests/040| 4 ++--
tests/qemu-iotests/051.pc.out | 6 +++---
tests/qemu-iotests/081.out| 2 +-
tests/qemu-iotests/085.out| 6 +++---
tests/qemu-iotests/087.out| 2 +-
tests/qemu-iotests/206.out| 2 +-
tests/qemu-iotests/210.out| 2 +-
tests/qemu-iotests/211.out| 2 +-
tests/qemu-iotests/212.out| 2 +-
tests/qemu-iotests/213.out| 2 +-
tests/qemu-iotests/223.out| 4 ++--
tests/qemu-iotests/237.out| 2 +-
tests/qemu-iotests/245| 4 ++--
tests/qemu-iotests/249.out| 2 +-
tests/qemu-iotests/300| 4 ++--
17 files changed, 29 insertions(+), 29 deletions(-)
diff --git a/block.c b/block.c
index a1f3cecd75..2daff6d29a 100644
--- a/block.c
+++ b/block.c
@@ -1440,7 +1440,7 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
* Check for empty string or invalid characters, but not if it is
* generated (generated names use characters not available to the user)
*/
-error_setg(errp, "Invalid node name");
+error_setg(errp, "Invalid node-name: '%s'", node_name);
return;
}
@@ -1453,7 +1453,7 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
/* takes care of avoiding duplicates node names */
if (bdrv_find_node(node_name)) {
-error_setg(errp, "Duplicate node name");
+error_setg(errp, "Duplicate nodes with node-name='%s'", node_name);
goto out;
}
@@ -5432,7 +5432,7 @@ BlockDriverState *bdrv_lookup_bs(const char *device,
}
}
-error_setg(errp, "Cannot find device=%s nor node_name=%s",
+error_setg(errp, "Cannot find device=\'%s\' nor node-name=\'%s\'",
device ? device : "",
node_name ? node_name : "");
return NULL;
@@ -6752,7 +6752,7 @@ BlockDriverState *check_to_replace_node(BlockDriverState
*parent_bs,
AioContext *aio_context;
if (!to_replace_bs) {
-error_setg(errp, "Node name '%s' not found", node_name);
+error_setg(errp, "Failed to find node with node-name='%s'", node_name);
return NULL;
}
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 12aa9ed37e..5fb65b4bef 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -153,7 +153,7 @@ class TestSingleDrive(iotests.QMPTestCase):
def test_device_not_found(self):
result = self.vm.qmp('block-stream', device='nonexistent')
self.assert_qmp(result, 'error/desc',
-'Cannot find device=nonexistent nor node_name=nonexistent')
+'Cannot find device=\'nonexistent\' nor node-name=\'nonexistent\'')
def test_job_id_missing(self):
result = self.vm.qmp('block-stream', device='mid')
@@ -507,7 +507,7 @@ class TestParallelOps(iotests.QMPTestCase):
# Error: the base node does not exist
result = self.vm.qmp('block-stream', device='node4', base_node='none',
job_id='stream')
self.assert_qmp(result, 'error/desc',
-'Cannot find device= nor node_name=none')
+'Cannot find device=\'\' nor node-name=\'none\'')
# Error: the base node is not a backing file of th
[PATCH v2 2/2] blockdev: Clarify error messages pertaining to 'node-name'
Signed-off-by: Connor Kuehl
---
blockdev.c | 13 +++--
tests/qemu-iotests/245 | 6 +++---
2 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index cd438e60e3..7c7ab2b386 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1515,13 +1515,13 @@ static void external_snapshot_prepare(BlkActionState
*common,
s->has_snapshot_node_name ? s->snapshot_node_name : NULL;
if (node_name && !snapshot_node_name) {
-error_setg(errp, "New overlay node name missing");
+error_setg(errp, "New overlay node-name missing");
goto out;
}
if (snapshot_node_name &&
bdrv_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) {
-error_setg(errp, "New overlay node name already in use");
+error_setg(errp, "New overlay node-name already in use");
goto out;
}
@@ -3598,13 +3598,14 @@ void qmp_x_blockdev_reopen(BlockdevOptions *options,
Error **errp)
/* Check for the selected node name */
if (!options->has_node_name) {
-error_setg(errp, "Node name not specified");
+error_setg(errp, "node-name not specified");
goto fail;
}
bs = bdrv_find_node(options->node_name);
if (!bs) {
-error_setg(errp, "Cannot find node named '%s'", options->node_name);
+error_setg(errp, "Failed to find node with node-name='%s'",
+ options->node_name);
goto fail;
}
@@ -3635,7 +3636,7 @@ void qmp_blockdev_del(const char *node_name, Error **errp)
bs = bdrv_find_node(node_name);
if (!bs) {
-error_setg(errp, "Cannot find node %s", node_name);
+error_setg(errp, "Failed to find node with node-name='%s'", node_name);
return;
}
if (bdrv_has_blk(bs)) {
@@ -3758,7 +3759,7 @@ void qmp_x_blockdev_set_iothread(const char *node_name,
StrOrNull *iothread,
bs = bdrv_find_node(node_name);
if (!bs) {
-error_setg(errp, "Cannot find node %s", node_name);
+error_setg(errp, "Failed to find node with node-name='%s'", node_name);
return;
}
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index f8eba7719a..a2a0482469 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -140,8 +140,8 @@ class TestBlockdevReopen(iotests.QMPTestCase):
self.reopen(opts, {'file': 'hd0-file'})
# We cannot change any of these
-self.reopen(opts, {'node-name': 'not-found'}, "Cannot find node named
'not-found'")
-self.reopen(opts, {'node-name': ''}, "Cannot find node named ''")
+self.reopen(opts, {'node-name': 'not-found'}, "Failed to find node
with node-name='not-found'")
+self.reopen(opts, {'node-name': ''}, "Failed to find node with
node-name=''")
self.reopen(opts, {'node-name': None}, "Invalid parameter type for
'node-name', expected: string")
self.reopen(opts, {'driver': 'raw'}, "Cannot change the option
'driver'")
self.reopen(opts, {'driver': ''}, "Invalid parameter ''")
@@ -158,7 +158,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
# node-name is optional in BlockdevOptions, but x-blockdev-reopen
needs it
del opts['node-name']
-self.reopen(opts, {}, "Node name not specified")
+self.reopen(opts, {}, "node-name not specified")
# Check that nothing has changed
self.check_node_graph(original_graph)
--
2.29.2
[PATCH v2 0/2] Clarify error messages pertaining to 'node-name'
v2: - Moved summary into patch #1 - Updated test cases that were missed in v1 from running 'make check'. This time I used 'make check-block SPEED=thorough' and some more grepping to make sure I didn't miss any. - Rebased Connor Kuehl (2): block: Clarify error messages pertaining to 'node-name' blockdev: Clarify error messages pertaining to 'node-name' block.c | 8 blockdev.c| 13 +++-- tests/qemu-iotests/030| 4 ++-- tests/qemu-iotests/040| 4 ++-- tests/qemu-iotests/051.pc.out | 6 +++--- tests/qemu-iotests/081.out| 2 +- tests/qemu-iotests/085.out| 6 +++--- tests/qemu-iotests/087.out| 2 +- tests/qemu-iotests/206.out| 2 +- tests/qemu-iotests/210.out| 2 +- tests/qemu-iotests/211.out| 2 +- tests/qemu-iotests/212.out| 2 +- tests/qemu-iotests/213.out| 2 +- tests/qemu-iotests/223.out| 4 ++-- tests/qemu-iotests/237.out| 2 +- tests/qemu-iotests/245| 10 +- tests/qemu-iotests/249.out| 2 +- tests/qemu-iotests/300| 4 ++-- 18 files changed, 39 insertions(+), 38 deletions(-) -- 2.29.2
Re: [PATCH 0/2] Clarify error messages pertaining to 'node-name'
On 3/3/21 3:53 AM, Kevin Wolf wrote:
Am 02.03.2021 um 00:36 hat Connor Kuehl geschrieben:
Some error messages contain ambiguous representations of the 'node-name'
parameter. This can be particularly confusing when exchanging QMP
messages (C = client, S = server):
C: {"execute": "block_resize", "arguments": { "device": "my_file", "size":
26843545600 }}
S: {"error": {"class": "GenericError", "desc": "Cannot find device=my_file nor
node_name="}}
^
Arguably, this error message isn't great anyway because of the empty
string node name. We didn't even look for a node name, so why mention it
in the error message?
But your patches are certainly a good improvement already. I would have
merged them, but git grep 'nor node_name=' shows that you missed to
update a few tests, so they fail after the series. I suppose you only
caught the ones that are run by default in 'make check' and missed the
ones that require running the qemu-iotests 'check' script manually.
Ah, good catch! Yes, I was only using `make check`, I'll use the `check`
script to uncover the other failures and fix them accordingly.
[..]
This is a good explanation for the change you're making. I think it
deserves being committed to the repository in the commit message for
patch 1.
I'll move this to patch #1 in the next revision of this series.
Thank you!
Connor
[PATCH 2/2] blockdev: Clarify error messages pertaining to 'node-name'
Signed-off-by: Connor Kuehl
---
blockdev.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index cd438e60e3..7c7ab2b386 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1515,13 +1515,13 @@ static void external_snapshot_prepare(BlkActionState
*common,
s->has_snapshot_node_name ? s->snapshot_node_name : NULL;
if (node_name && !snapshot_node_name) {
-error_setg(errp, "New overlay node name missing");
+error_setg(errp, "New overlay node-name missing");
goto out;
}
if (snapshot_node_name &&
bdrv_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) {
-error_setg(errp, "New overlay node name already in use");
+error_setg(errp, "New overlay node-name already in use");
goto out;
}
@@ -3598,13 +3598,14 @@ void qmp_x_blockdev_reopen(BlockdevOptions *options,
Error **errp)
/* Check for the selected node name */
if (!options->has_node_name) {
-error_setg(errp, "Node name not specified");
+error_setg(errp, "node-name not specified");
goto fail;
}
bs = bdrv_find_node(options->node_name);
if (!bs) {
-error_setg(errp, "Cannot find node named '%s'", options->node_name);
+error_setg(errp, "Failed to find node with node-name='%s'",
+ options->node_name);
goto fail;
}
@@ -3635,7 +3636,7 @@ void qmp_blockdev_del(const char *node_name, Error **errp)
bs = bdrv_find_node(node_name);
if (!bs) {
-error_setg(errp, "Cannot find node %s", node_name);
+error_setg(errp, "Failed to find node with node-name='%s'", node_name);
return;
}
if (bdrv_has_blk(bs)) {
@@ -3758,7 +3759,7 @@ void qmp_x_blockdev_set_iothread(const char *node_name,
StrOrNull *iothread,
bs = bdrv_find_node(node_name);
if (!bs) {
-error_setg(errp, "Cannot find node %s", node_name);
+error_setg(errp, "Failed to find node with node-name='%s'", node_name);
return;
}
--
2.29.2
[PATCH 1/2] block: Clarify error messages pertaining to 'node-name'
Reported-by: Tingting Mao
Fixes: https://bugzilla.redhat.com/1651437
Signed-off-by: Connor Kuehl
---
block.c| 8
tests/qemu-iotests/040 | 4 ++--
tests/qemu-iotests/249.out | 2 +-
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/block.c b/block.c
index a1f3cecd75..2daff6d29a 100644
--- a/block.c
+++ b/block.c
@@ -1440,7 +1440,7 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
* Check for empty string or invalid characters, but not if it is
* generated (generated names use characters not available to the user)
*/
-error_setg(errp, "Invalid node name");
+error_setg(errp, "Invalid node-name: '%s'", node_name);
return;
}
@@ -1453,7 +1453,7 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
/* takes care of avoiding duplicates node names */
if (bdrv_find_node(node_name)) {
-error_setg(errp, "Duplicate node name");
+error_setg(errp, "Duplicate nodes with node-name='%s'", node_name);
goto out;
}
@@ -5432,7 +5432,7 @@ BlockDriverState *bdrv_lookup_bs(const char *device,
}
}
-error_setg(errp, "Cannot find device=%s nor node_name=%s",
+error_setg(errp, "Cannot find device=\'%s\' nor node-name=\'%s\'",
device ? device : "",
node_name ? node_name : "");
return NULL;
@@ -6752,7 +6752,7 @@ BlockDriverState *check_to_replace_node(BlockDriverState
*parent_bs,
AioContext *aio_context;
if (!to_replace_bs) {
-error_setg(errp, "Node name '%s' not found", node_name);
+error_setg(errp, "Failed to find node with node-name='%s'", node_name);
return NULL;
}
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 7ebc9ed825..336ff7c4f2 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -175,13 +175,13 @@ class TestSingleDrive(ImageCommitTestCase):
self.assert_no_active_block_jobs()
result = self.vm.qmp('block-commit', device='drive0',
top_node='badfile', base_node='base')
self.assert_qmp(result, 'error/class', 'GenericError')
-self.assert_qmp(result, 'error/desc', "Cannot find device= nor
node_name=badfile")
+self.assert_qmp(result, 'error/desc', "Cannot find device='' nor
node-name='badfile'")
def test_base_node_invalid(self):
self.assert_no_active_block_jobs()
result = self.vm.qmp('block-commit', device='drive0', top_node='mid',
base_node='badfile')
self.assert_qmp(result, 'error/class', 'GenericError')
-self.assert_qmp(result, 'error/desc', "Cannot find device= nor
node_name=badfile")
+self.assert_qmp(result, 'error/desc', "Cannot find device='' nor
node-name='badfile'")
def test_top_path_and_node(self):
self.assert_no_active_block_jobs()
diff --git a/tests/qemu-iotests/249.out b/tests/qemu-iotests/249.out
index 92ec81db03..d2bf9be85e 100644
--- a/tests/qemu-iotests/249.out
+++ b/tests/qemu-iotests/249.out
@@ -18,7 +18,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
backing_file=TEST_DIR/t.
'filter-node-name': '1234'}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"JOB_STATUS_CHANGE", "data": {"status": "null", "id": "job0"}}
-{"error": {"class": "GenericError", "desc": "Invalid node name"}}
+{"error": {"class": "GenericError", "desc": "Invalid node-name: '1234'"}}
=== Send a write command to a drive opened in read-only mode (2)
--
2.29.2
[PATCH 0/2] Clarify error messages pertaining to 'node-name'
Some error messages contain ambiguous representations of the 'node-name'
parameter. This can be particularly confusing when exchanging QMP
messages (C = client, S = server):
C: {"execute": "block_resize", "arguments": { "device": "my_file", "size":
26843545600 }}
S: {"error": {"class": "GenericError", "desc": "Cannot find device=my_file nor
node_name="}}
^
This error message suggests one could send a message with a key called
'node_name':
C: {"execute": "block_resize", "arguments": { "node_name": "my_file", "size":
26843545600 }}
^
but using the underscore is actually incorrect, the parameter should be
'node-name':
S: {"error": {"class": "GenericError", "desc": "Parameter 'node_name' is
unexpected"}}
This behavior was uncovered in bz1651437[1], but I ended up going down a
rabbit hole looking for other areas where this miscommunication might
occur and changing those accordingly as well.
[1] https://bugzilla.redhat.com/1651437
Connor Kuehl (2):
block: Clarify error messages pertaining to 'node-name'
blockdev: Clarify error messages pertaining to 'node-name'
block.c| 8
blockdev.c | 13 +++--
tests/qemu-iotests/040 | 4 ++--
tests/qemu-iotests/249.out | 2 +-
4 files changed, 14 insertions(+), 13 deletions(-)
--
2.29.2
Re: [PATCH v3] block: Raise an error when backing file parameter is an empty string
On 8/13/20 8:47 AM, Connor Kuehl wrote: Providing an empty string for the backing file parameter like so: qemu-img create -f qcow2 -b '' /tmp/foo allows the flow of control to reach and subsequently fail an assert statement because passing an empty string to bdrv_get_full_backing_filename_from_filename() simply results in NULL being returned without an error being raised. To fix this, let's check for an empty string when getting the value from the opts list. Reported-by: Attila Fazekas Fixes: https://bugzilla.redhat.com/1809553 Signed-off-by: Connor Kuehl --- v3: - Moved test case into 049 instead of taking up 298. v2: - Removed 4 spaces to resolve pylint warning - Updated format to be 'iotests.imgfmt' instead of hardcoding 'qcow2' - Use temporary file instead of '/tmp/foo' - Give a size parameter to qemu-img - Run test for qcow2, qcow, qed and *not* raw Ping
[PATCH v3] block: Raise an error when backing file parameter is an empty string
Providing an empty string for the backing file parameter like so:
qemu-img create -f qcow2 -b '' /tmp/foo
allows the flow of control to reach and subsequently fail an assert
statement because passing an empty string to
bdrv_get_full_backing_filename_from_filename()
simply results in NULL being returned without an error being raised.
To fix this, let's check for an empty string when getting the value from
the opts list.
Reported-by: Attila Fazekas
Fixes: https://bugzilla.redhat.com/1809553
Signed-off-by: Connor Kuehl
---
v3:
- Moved test case into 049 instead of taking up
298.
v2:
- Removed 4 spaces to resolve pylint warning
- Updated format to be 'iotests.imgfmt' instead
of hardcoding 'qcow2'
- Use temporary file instead of '/tmp/foo'
- Give a size parameter to qemu-img
- Run test for qcow2, qcow, qed and *not* raw
block.c| 4
tests/qemu-iotests/049 | 4
tests/qemu-iotests/049.out | 5 +
3 files changed, 13 insertions(+)
diff --git a/block.c b/block.c
index d9ac0e07eb..1f72275b87 100644
--- a/block.c
+++ b/block.c
@@ -6117,6 +6117,10 @@ void bdrv_img_create(const char *filename, const char
*fmt,
"same filename as the backing file");
goto out;
}
+if (backing_file[0] == '\0') {
+error_setg(errp, "Expected backing file name, got empty string");
+goto out;
+}
}
backing_fmt = qemu_opt_get(opts, BLOCK_OPT_BACKING_FMT);
diff --git a/tests/qemu-iotests/049 b/tests/qemu-iotests/049
index 051a1c79e0..82b1e6c202 100755
--- a/tests/qemu-iotests/049
+++ b/tests/qemu-iotests/049
@@ -119,6 +119,10 @@ test_qemu_img create -f $IMGFMT -o
compat=1.1,lazy_refcounts=on "$TEST_IMG" 64M
test_qemu_img create -f $IMGFMT -o compat=0.10,lazy_refcounts=off "$TEST_IMG"
64M
test_qemu_img create -f $IMGFMT -o compat=0.10,lazy_refcounts=on "$TEST_IMG"
64M
+echo "== Expect error when backing file name is empty string =="
+echo
+test_qemu_img create -f $IMGFMT -b '' $TEST_IMG 1M
+
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/049.out b/tests/qemu-iotests/049.out
index 4c21dc70a5..61ee90b10e 100644
--- a/tests/qemu-iotests/049.out
+++ b/tests/qemu-iotests/049.out
@@ -209,4 +209,9 @@ qemu-img create -f qcow2 -o compat=0.10,lazy_refcounts=on
TEST_DIR/t.qcow2 64M
Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 cluster_size=65536
compression_type=zlib size=67108864 compat=0.10 lazy_refcounts=on
refcount_bits=16
qemu-img: TEST_DIR/t.qcow2: Lazy refcounts only supported with compatibility
level 1.1 and above (use version=v3 or greater)
+== Expect error when backing file name is empty string ==
+
+qemu-img create -f qcow2 -b TEST_DIR/t.qcow2 1M
+qemu-img: TEST_DIR/t.qcow2: Expected backing file name, got empty string
+
*** done
--
2.25.4
Re: [PATCH v2] block: Raise an error when backing file parameter is an empty string
On 8/12/20 1:58 AM, Kevin Wolf wrote: This looks like a test case that would be better served by not using QMPTestCase, but just printing the qemu-img output and having the message compared against the reference output. In fact, there is already 049 for testing some qemu-img create options and we could just add a line there (or multiple lines to cover other backing file related error cases, too). Putting it there would both simplify the test code and keep 298 free for the other series. None of the above is really a reason to reject the patch. I guess this is more of a "are you sure? (y/n)" before I apply it. :-) Hi Kevin! Thanks for the review :-) I think it'd be best for my own edification to address your comments here instead of applying this now. I'll send a v3. Connor Kevin
[PATCH v2] block: Raise an error when backing file parameter is an empty string
Providing an empty string for the backing file parameter like so:
qemu-img create -f qcow2 -b '' /tmp/foo
allows the flow of control to reach and subsequently fail an assert
statement because passing an empty string to
bdrv_get_full_backing_filename_from_filename()
simply results in NULL being returned without an error being raised.
To fix this, let's check for an empty string when getting the value from
the opts list.
Reported-by: Attila Fazekas
Fixes: https://bugzilla.redhat.com/1809553
Signed-off-by: Connor Kuehl
---
v2:
- Removed 4 spaces to resolve pylint warning
- Updated format to be 'iotests.imgfmt' instead
of hardcoding 'qcow2'
- Use temporary file instead of '/tmp/foo'
- Give a size parameter to qemu-img
- Run test for qcow2, qcow, qed and *not* raw
block.c| 4
tests/qemu-iotests/298 | 49 ++
tests/qemu-iotests/298.out | 5
tests/qemu-iotests/group | 1 +
4 files changed, 59 insertions(+)
create mode 100755 tests/qemu-iotests/298
create mode 100644 tests/qemu-iotests/298.out
diff --git a/block.c b/block.c
index d9ac0e07eb..1f72275b87 100644
--- a/block.c
+++ b/block.c
@@ -6117,6 +6117,10 @@ void bdrv_img_create(const char *filename, const char
*fmt,
"same filename as the backing file");
goto out;
}
+if (backing_file[0] == '\0') {
+error_setg(errp, "Expected backing file name, got empty string");
+goto out;
+}
}
backing_fmt = qemu_opt_get(opts, BLOCK_OPT_BACKING_FMT);
diff --git a/tests/qemu-iotests/298 b/tests/qemu-iotests/298
new file mode 100755
index 00..879dae2d8e
--- /dev/null
+++ b/tests/qemu-iotests/298
@@ -0,0 +1,49 @@
+#!/usr/bin/env python3
+#
+# Copyright (C) 2020 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+
+
+# Regression test for avoiding an assertion when the 'backing file'
+# parameter (-b) is set to an empty string for qemu-img create
+#
+# qemu-img create -f qcow2 -b '' /tmp/foo
+#
+# This ensures the invalid parameter is handled with a user-
+# friendly message instead of a failed assertion.
+
+import iotests
+
+class TestEmptyBackingFilename(iotests.QMPTestCase):
+
+
+def test_empty_backing_file_name(self):
+with iotests.FilePath('test.img') as img_path:
+actual = iotests.qemu_img_pipe(
+'create',
+'-f', iotests.imgfmt,
+'-b', '',
+img_path,
+'1M'
+)
+expected = f'qemu-img: {img_path}: Expected backing file name,' \
+ ' got empty string'
+
+self.assertEqual(actual.strip(), expected.strip())
+
+
+if __name__ == '__main__':
+iotests.main(supported_fmts=['qed', 'qcow', 'qcow2'])
diff --git a/tests/qemu-iotests/298.out b/tests/qemu-iotests/298.out
new file mode 100644
index 00..ae1213e6f8
--- /dev/null
+++ b/tests/qemu-iotests/298.out
@@ -0,0 +1,5 @@
+.
+--
+Ran 1 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 025ed5238d..6f80c884a1 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -306,6 +306,7 @@
295 rw
296 rw
297 meta
+298 img auto quick
299 auto quick
301 backing quick
302 quick
--
2.25.4
Re: [PATCH] block: Raise an error when backing file parameter is an empty string
Ping
On 7/1/20 3:42 PM, Connor Kuehl wrote:
Hi Kevin & Max,
Just pinging this patch for your consideration.
Thank you,
Connor
On 6/17/20 11:27 AM, Connor Kuehl wrote:
Providing an empty string for the backing file parameter like so:
qemu-img create -f qcow2 -b '' /tmp/foo
allows the flow of control to reach and subsequently fail an assert
statement because passing an empty string to
bdrv_get_full_backing_filename_from_filename()
simply results in NULL being returned without an error being raised.
To fix this, let's check for an empty string when getting the value from
the opts list.
Reported-by: Attila Fazekas
Fixes: https://bugzilla.redhat.com/1809553
Signed-off-by: Connor Kuehl
---
block.c | 4
tests/qemu-iotests/298 | 47 ++
tests/qemu-iotests/298.out | 5
tests/qemu-iotests/group | 1 +
4 files changed, 57 insertions(+)
create mode 100755 tests/qemu-iotests/298
create mode 100644 tests/qemu-iotests/298.out
diff --git a/block.c b/block.c
index 6dbcb7e083..b335d6bcb2 100644
--- a/block.c
+++ b/block.c
@@ -6116,6 +6116,10 @@ void bdrv_img_create(const char *filename,
const char *fmt,
"same filename as the backing file");
goto out;
}
+ if (backing_file[0] == '\0') {
+ error_setg(errp, "Expected backing file name, got empty
string");
+ goto out;
+ }
}
backing_fmt = qemu_opt_get(opts, BLOCK_OPT_BACKING_FMT);
diff --git a/tests/qemu-iotests/298 b/tests/qemu-iotests/298
new file mode 100755
index 00..1e30caebec
--- /dev/null
+++ b/tests/qemu-iotests/298
@@ -0,0 +1,47 @@
+#!/usr/bin/env python3
+#
+# Copyright (C) 2020 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+
+
+# Regression test for avoiding an assertion when the 'backing file'
+# parameter (-b) is set to an empty string for qemu-img create
+#
+# qemu-img create -f qcow2 -b '' /tmp/foo
+#
+# This ensures the invalid parameter is handled with a user-
+# friendly message instead of a failed assertion.
+
+import iotests
+
+class TestEmptyBackingFilename(iotests.QMPTestCase):
+
+
+ def test_empty_backing_file_name(self):
+ actual = iotests.qemu_img_pipe(
+ 'create',
+ '-f', 'qcow2',
+ '-b', '',
+ '/tmp/foo'
+ )
+ expected = 'qemu-img: /tmp/foo: Expected backing file name,' \
+ ' got empty string'
+
+ self.assertEqual(actual.strip(), expected.strip())
+
+
+if __name__ == '__main__':
+ iotests.main(supported_fmts=['raw', 'qcow2'])
diff --git a/tests/qemu-iotests/298.out b/tests/qemu-iotests/298.out
new file mode 100644
index 00..ae1213e6f8
--- /dev/null
+++ b/tests/qemu-iotests/298.out
@@ -0,0 +1,5 @@
+.
+--
+Ran 1 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index d886fa0cb3..4bca2d9e05 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -302,3 +302,4 @@
291 rw quick
292 rw auto quick
297 meta
+298 img auto quick
Re: [PATCH] block: Raise an error when backing file parameter is an empty string
Hi Kevin & Max,
Just pinging this patch for your consideration.
Thank you,
Connor
On 6/17/20 11:27 AM, Connor Kuehl wrote:
Providing an empty string for the backing file parameter like so:
qemu-img create -f qcow2 -b '' /tmp/foo
allows the flow of control to reach and subsequently fail an assert
statement because passing an empty string to
bdrv_get_full_backing_filename_from_filename()
simply results in NULL being returned without an error being raised.
To fix this, let's check for an empty string when getting the value from
the opts list.
Reported-by: Attila Fazekas
Fixes: https://bugzilla.redhat.com/1809553
Signed-off-by: Connor Kuehl
---
block.c| 4
tests/qemu-iotests/298 | 47 ++
tests/qemu-iotests/298.out | 5
tests/qemu-iotests/group | 1 +
4 files changed, 57 insertions(+)
create mode 100755 tests/qemu-iotests/298
create mode 100644 tests/qemu-iotests/298.out
diff --git a/block.c b/block.c
index 6dbcb7e083..b335d6bcb2 100644
--- a/block.c
+++ b/block.c
@@ -6116,6 +6116,10 @@ void bdrv_img_create(const char *filename, const char
*fmt,
"same filename as the backing file");
goto out;
}
+if (backing_file[0] == '\0') {
+error_setg(errp, "Expected backing file name, got empty string");
+goto out;
+}
}
backing_fmt = qemu_opt_get(opts, BLOCK_OPT_BACKING_FMT);
diff --git a/tests/qemu-iotests/298 b/tests/qemu-iotests/298
new file mode 100755
index 00..1e30caebec
--- /dev/null
+++ b/tests/qemu-iotests/298
@@ -0,0 +1,47 @@
+#!/usr/bin/env python3
+#
+# Copyright (C) 2020 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+
+
+# Regression test for avoiding an assertion when the 'backing file'
+# parameter (-b) is set to an empty string for qemu-img create
+#
+# qemu-img create -f qcow2 -b '' /tmp/foo
+#
+# This ensures the invalid parameter is handled with a user-
+# friendly message instead of a failed assertion.
+
+import iotests
+
+class TestEmptyBackingFilename(iotests.QMPTestCase):
+
+
+def test_empty_backing_file_name(self):
+actual = iotests.qemu_img_pipe(
+'create',
+'-f', 'qcow2',
+'-b', '',
+'/tmp/foo'
+)
+expected = 'qemu-img: /tmp/foo: Expected backing file name,' \
+ ' got empty string'
+
+self.assertEqual(actual.strip(), expected.strip())
+
+
+if __name__ == '__main__':
+iotests.main(supported_fmts=['raw', 'qcow2'])
diff --git a/tests/qemu-iotests/298.out b/tests/qemu-iotests/298.out
new file mode 100644
index 00..ae1213e6f8
--- /dev/null
+++ b/tests/qemu-iotests/298.out
@@ -0,0 +1,5 @@
+.
+--
+Ran 1 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index d886fa0cb3..4bca2d9e05 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -302,3 +302,4 @@
291 rw quick
292 rw auto quick
297 meta
+298 img auto quick
[PATCH] block: Raise an error when backing file parameter is an empty string
Providing an empty string for the backing file parameter like so:
qemu-img create -f qcow2 -b '' /tmp/foo
allows the flow of control to reach and subsequently fail an assert
statement because passing an empty string to
bdrv_get_full_backing_filename_from_filename()
simply results in NULL being returned without an error being raised.
To fix this, let's check for an empty string when getting the value from
the opts list.
Reported-by: Attila Fazekas
Fixes: https://bugzilla.redhat.com/1809553
Signed-off-by: Connor Kuehl
---
block.c| 4
tests/qemu-iotests/298 | 47 ++
tests/qemu-iotests/298.out | 5
tests/qemu-iotests/group | 1 +
4 files changed, 57 insertions(+)
create mode 100755 tests/qemu-iotests/298
create mode 100644 tests/qemu-iotests/298.out
diff --git a/block.c b/block.c
index 6dbcb7e083..b335d6bcb2 100644
--- a/block.c
+++ b/block.c
@@ -6116,6 +6116,10 @@ void bdrv_img_create(const char *filename, const char
*fmt,
"same filename as the backing file");
goto out;
}
+if (backing_file[0] == '\0') {
+error_setg(errp, "Expected backing file name, got empty string");
+goto out;
+}
}
backing_fmt = qemu_opt_get(opts, BLOCK_OPT_BACKING_FMT);
diff --git a/tests/qemu-iotests/298 b/tests/qemu-iotests/298
new file mode 100755
index 00..1e30caebec
--- /dev/null
+++ b/tests/qemu-iotests/298
@@ -0,0 +1,47 @@
+#!/usr/bin/env python3
+#
+# Copyright (C) 2020 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+
+
+# Regression test for avoiding an assertion when the 'backing file'
+# parameter (-b) is set to an empty string for qemu-img create
+#
+# qemu-img create -f qcow2 -b '' /tmp/foo
+#
+# This ensures the invalid parameter is handled with a user-
+# friendly message instead of a failed assertion.
+
+import iotests
+
+class TestEmptyBackingFilename(iotests.QMPTestCase):
+
+
+def test_empty_backing_file_name(self):
+actual = iotests.qemu_img_pipe(
+'create',
+'-f', 'qcow2',
+'-b', '',
+'/tmp/foo'
+)
+expected = 'qemu-img: /tmp/foo: Expected backing file name,' \
+ ' got empty string'
+
+self.assertEqual(actual.strip(), expected.strip())
+
+
+if __name__ == '__main__':
+iotests.main(supported_fmts=['raw', 'qcow2'])
diff --git a/tests/qemu-iotests/298.out b/tests/qemu-iotests/298.out
new file mode 100644
index 00..ae1213e6f8
--- /dev/null
+++ b/tests/qemu-iotests/298.out
@@ -0,0 +1,5 @@
+.
+--
+Ran 1 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index d886fa0cb3..4bca2d9e05 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -302,3 +302,4 @@
291 rw quick
292 rw auto quick
297 meta
+298 img auto quick
--
2.25.4
