[Qemu-devel] [PATCH] qemu-config: Accept empty option values

[Qemu-devel] [PATCH] qemu-config: Accept empty option values

[Eduardo Habkost]

Currently it is impossible to set an option in a config file to an empty
string, because the parser matches only lines containing non-empty
strings between double-quotes.

As sscanf() "[" conversion specifier only matches non-empty strings, add
a special case for empty strings.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 util/qemu-config.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/util/qemu-config.c b/util/qemu-config.c
index 2d32ce7..9f9577d 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -413,7 +413,8 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
             opts = qemu_opts_create(list, NULL, 0, &error_abort);
             continue;
         }
-        if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2) {
+        if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2 ||
+            (value[0] = '\0', sscanf(line, " %63s = \"\"", arg) == 1)) {
             /* arg = value */
             if (opts == NULL) {
                 error_report("no group defined");
-- 
2.1.0

Re: [Qemu-devel] [PATCH] qemu-config: Accept empty option values

[Eric Blake]

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

On 04/08/2015 12:16 PM, Eduardo Habkost wrote:
> Currently it is impossible to set an option in a config file to an empty
> string, because the parser matches only lines containing non-empty
> strings between double-quotes.
> 
> As sscanf() "[" conversion specifier only matches non-empty strings, add
> a special case for empty strings.

I avoid sscanf() as a rule (as it's behavior on %d is undefined in the
face of malicious input), so I had to read the man page; but you are
right.  Libvirt is trying to completely ban use of *scanf for that
reason; but obviously qemu is not quite so opposed to it.

> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  util/qemu-config.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index 2d32ce7..9f9577d 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -413,7 +413,8 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
>              opts = qemu_opts_create(list, NULL, 0, &error_abort);
>              continue;
>          }
> -        if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2) {
> +        if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2 ||
> +            (value[0] = '\0', sscanf(line, " %63s = \"\"", arg) == 1)) {

This is one of the few times I've seen sscanf used in a well-defined
manner (albeit still arbitrarily limiting, in that we have fixed-size
buffers) - but having to rely on a comma operator to get there makes
this look quite arcane.  I still wonder if hand-rolling a real scanner
would beat the compactness of sscanf by making the code intentions a
little more discernible, and have the benefits of avoiding my sscanf
red-flag checker.  But my wonder is not enough to stop me from accepting
this hack as-is.

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

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


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

Re: [Qemu-devel] [PATCH] qemu-config: Accept empty option values

[Paolo Bonzini]

On 08/04/2015 20:16, Eduardo Habkost wrote:
> Currently it is impossible to set an option in a config file to an empty
> string, because the parser matches only lines containing non-empty
> strings between double-quotes.
> 
> As sscanf() "[" conversion specifier only matches non-empty strings, add
> a special case for empty strings.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  util/qemu-config.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index 2d32ce7..9f9577d 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -413,7 +413,8 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
>              opts = qemu_opts_create(list, NULL, 0, &error_abort);
>              continue;
>          }
> -        if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2) {
> +        if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2 ||
> +            (value[0] = '\0', sscanf(line, " %63s = \"\"", arg) == 1)) {
>              /* arg = value */
>              if (opts == NULL) {
>                  error_report("no group defined");
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH] qemu-config: Accept empty option values

[Eduardo Habkost]

On Thu, Apr 09, 2015 at 02:11:11PM +0200, Paolo Bonzini wrote:
> On 08/04/2015 20:16, Eduardo Habkost wrote:
> > Currently it is impossible to set an option in a config file to an empty
> > string, because the parser matches only lines containing non-empty
> > strings between double-quotes.
> > 
> > As sscanf() "[" conversion specifier only matches non-empty strings, add
> > a special case for empty strings.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  util/qemu-config.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/util/qemu-config.c b/util/qemu-config.c
> > index 2d32ce7..9f9577d 100644
> > --- a/util/qemu-config.c
> > +++ b/util/qemu-config.c
> > @@ -413,7 +413,8 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
> >              opts = qemu_opts_create(list, NULL, 0, &error_abort);
> >              continue;
> >          }
> > -        if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2) {
> > +        if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2 ||
> > +            (value[0] = '\0', sscanf(line, " %63s = \"\"", arg) == 1)) {
> >              /* arg = value */
> >              if (opts == NULL) {
> >                  error_report("no group defined");
> > 
> 
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Thanks! Whose tree this should go through?

-- 
Eduardo

Re: [Qemu-devel] [PATCH] qemu-config: Accept empty option values

[Paolo Bonzini]

On 09/04/2015 20:50, Eduardo Habkost wrote:
> On Thu, Apr 09, 2015 at 02:11:11PM +0200, Paolo Bonzini wrote:
>> On 08/04/2015 20:16, Eduardo Habkost wrote:
>>> Currently it is impossible to set an option in a config file to an empty
>>> string, because the parser matches only lines containing non-empty
>>> strings between double-quotes.
>>>
>>> As sscanf() "[" conversion specifier only matches non-empty strings, add
>>> a special case for empty strings.
>>>
>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>> ---
>>>  util/qemu-config.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/util/qemu-config.c b/util/qemu-config.c
>>> index 2d32ce7..9f9577d 100644
>>> --- a/util/qemu-config.c
>>> +++ b/util/qemu-config.c
>>> @@ -413,7 +413,8 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
>>>              opts = qemu_opts_create(list, NULL, 0, &error_abort);
>>>              continue;
>>>          }
>>> -        if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2) {
>>> +        if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2 ||
>>> +            (value[0] = '\0', sscanf(line, " %63s = \"\"", arg) == 1)) {
>>>              /* arg = value */
>>>              if (opts == NULL) {
>>>                  error_report("no group defined");
>>>
>>
>> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Thanks! Whose tree this should go through?

Acked-by generally means that you're welcome to take it through yours.

My "misc" tree hosts non-trivial patches outside well-defined
maintainership area, but it's less work to carry patches only from
people who don't send pull requests.  Maybe it's not the case of this
patch, but using your own tree also means that it's simpler for you to
sort out the dependencies.

Paolo