Patchwork [v3,1/6] qemu-option: Allow integer defaults

login
register
mail settings
Submitter Eric Blake
Date Jan. 10, 2019, 7:18 p.m.
Message ID <20190110191901.5082-2-eblake@redhat.com>
Download mbox | patch
Permalink /patch/697215/
State New
Headers show

Comments

Eric Blake - Jan. 10, 2019, 7:18 p.m.
Set the framework up for declaring integer options with an integer
default, instead of our current insane approach of requiring the
default value to be given as a string (which then has to be reparsed
at every use that wants a number).  git grep '[^.]def_value_str' says
that we have done a good job of NOT abusing the internal field
outside of the implementation in qemu-option.c; therefore, it is not
too hard to audit that all code can either handle the new integer
defaults correctly or abort because a caller violated constraints.
Sadly, we DO have a constraint that qemu_opt_get() should not be
called on an option that has an integer type, because we have no
where to stash a cached const char * result; but callers that want
an integer should be using qemu_opt_get_number() and friends
anyways.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/qemu/option.h | 12 ++++++++
 util/qemu-option.c    | 69 +++++++++++++++++++++++++++++++++++++------
 2 files changed, 72 insertions(+), 9 deletions(-)
Leonid Bloch - Jan. 11, 2019, 2:14 p.m.
On 1/10/19 9:18 PM, Eric Blake wrote:
> Set the framework up for declaring integer options with an integer

> default, instead of our current insane approach of requiring the

> default value to be given as a string (which then has to be reparsed

> at every use that wants a number).  git grep '[^.]def_value_str' says

> that we have done a good job of NOT abusing the internal field

> outside of the implementation in qemu-option.c; therefore, it is not

> too hard to audit that all code can either handle the new integer

> defaults correctly or abort because a caller violated constraints.

> Sadly, we DO have a constraint that qemu_opt_get() should not be

> called on an option that has an integer type, because we have no

> where to stash a cached const char * result; but callers that want

> an integer should be using qemu_opt_get_number() and friends

> anyways.

> 

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

> ---

>   include/qemu/option.h | 12 ++++++++

>   util/qemu-option.c    | 69 +++++++++++++++++++++++++++++++++++++------

>   2 files changed, 72 insertions(+), 9 deletions(-)

> 

> diff --git a/include/qemu/option.h b/include/qemu/option.h

> index 844587cab39..46b80d5a6e1 100644

> --- a/include/qemu/option.h

> +++ b/include/qemu/option.h

> @@ -46,6 +46,18 @@ typedef struct QemuOptDesc {

>       const char *name;

>       enum QemuOptType type;

>       const char *help;

> +    /*

> +     * For QEMU_OPT_STRING: Leave def_value_int 0, and set def_value_str

> +     * to a default value or leave NULL for no default.

> +     *

> +     * For other types: Initialize at most non-zero def_value_int or a

> +     * parseable def_value_str for a default (must use a string for an

> +     * explicit default of 0, although an implicit default generally

> +     * works).  If setting def_value_int, calling qemu_opt_get() on

> +     * that option will abort(); instead, call qemu_opt_get_del() or a

> +     * typed getter.

> +     */

> +    uint64_t def_value_int;

>       const char *def_value_str;

>   } QemuOptDesc;

> 

> diff --git a/util/qemu-option.c b/util/qemu-option.c

> index de42e2a406a..06c4e8102a8 100644

> --- a/util/qemu-option.c

> +++ b/util/qemu-option.c

> @@ -321,7 +321,8 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name)

>       opt = qemu_opt_find(opts, name);

>       if (!opt) {

>           const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);

> -        if (desc && desc->def_value_str) {

> +        if (desc) {

> +            assert(!desc->def_value_int);

>               return desc->def_value_str;

>           }

>       }

> @@ -364,8 +365,22 @@ char *qemu_opt_get_del(QemuOpts *opts, const char *name)

>       opt = qemu_opt_find(opts, name);

>       if (!opt) {

>           desc = find_desc_by_name(opts->list->desc, name);

> -        if (desc && desc->def_value_str) {

> -            str = g_strdup(desc->def_value_str);

> +        if (desc) {

> +            if (desc->def_value_str) {

> +                str = g_strdup(desc->def_value_str);

> +            } else if (desc->def_value_int) {

> +                switch (desc->type) {

> +                case QEMU_OPT_BOOL:

> +                    str = g_strdup("on");

> +                    break;

> +                case QEMU_OPT_NUMBER:

> +                case QEMU_OPT_SIZE:

> +                    str = g_strdup_printf("%" PRId64, desc->def_value_int);

> +                    break;

> +                default:

> +                    abort();

> +                }

> +            }

>           }

>           return str;

>       }

> @@ -400,8 +415,15 @@ static bool qemu_opt_get_bool_helper(QemuOpts *opts, const char *name,

>       opt = qemu_opt_find(opts, name);

>       if (opt == NULL) {

>           const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);

> -        if (desc && desc->def_value_str) {

> -            parse_option_bool(name, desc->def_value_str, &ret, &error_abort);

> +        if (desc) {

> +            if (desc->def_value_int) {

> +                assert(desc->type != QEMU_OPT_STRING);

> +                return true;

> +            }

> +            if (desc->def_value_str) {

> +                parse_option_bool(name, desc->def_value_str, &ret,

> +                                  &error_abort);

> +            }

>           }

>           return ret;

>       }

> @@ -436,8 +458,15 @@ static uint64_t qemu_opt_get_number_helper(QemuOpts *opts, const char *name,

>       opt = qemu_opt_find(opts, name);

>       if (opt == NULL) {

>           const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);

> -        if (desc && desc->def_value_str) {

> -            parse_option_number(name, desc->def_value_str, &ret, &error_abort);

> +        if (desc) {

> +            if (desc->def_value_int) {

> +                assert(desc->type != QEMU_OPT_STRING);

> +                return desc->def_value_int;

> +            }

> +            if (desc->def_value_str) {

> +                parse_option_number(name, desc->def_value_str, &ret,

> +                                    &error_abort);

> +            }

>           }

>           return ret;

>       }

> @@ -473,8 +502,15 @@ static uint64_t qemu_opt_get_size_helper(QemuOpts *opts, const char *name,

>       opt = qemu_opt_find(opts, name);

>       if (opt == NULL) {

>           const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);

> -        if (desc && desc->def_value_str) {

> -            parse_option_size(name, desc->def_value_str, &ret, &error_abort);

> +        if (desc) {

> +            if (desc->def_value_int) {

> +                assert(desc->type != QEMU_OPT_STRING);

> +                return desc->def_value_int;

> +            }

> +            if (desc->def_value_str) {

> +                parse_option_size(name, desc->def_value_str, &ret,

> +                                  &error_abort);

> +            }

>           }

>           return ret;

>       }

> @@ -787,9 +823,23 @@ void qemu_opts_print(QemuOpts *opts, const char *separator)

>       }

>       for (; desc && desc->name; desc++) {

>           const char *value;

> +        char *tmp = NULL;

>           opt = qemu_opt_find(opts, desc->name);

> 

>           value = opt ? opt->str : desc->def_value_str;

> +        if (!value && desc->def_value_int) {

> +            switch (desc->type) {

> +            case QEMU_OPT_BOOL:

> +                value = tmp = g_strdup("on");

> +                break;

> +            case QEMU_OPT_NUMBER:

> +            case QEMU_OPT_SIZE:

> +                value = tmp = g_strdup_printf("%" PRId64, desc->def_value_int);

> +                break;

> +            default:

> +                abort();

> +            }

> +        }

>           if (!value) {

>               continue;

>           }

> @@ -803,6 +853,7 @@ void qemu_opts_print(QemuOpts *opts, const char *separator)

>               printf("%s%s=%s", sep, desc->name, value);

>           }

>           sep = separator;

> +        g_free(tmp);

>       }

>   }

> 


If I understand correctly, the number will still be compiled into the 
binary as an expression string, but will be printed correctly during 
runtime?
If so, it still doesn't feel right to put expressions in binaries, but I 
agree that it's more elegant than my solution with the lookup table. I'd 
say the table would be more elegant if there would be more cases in 
which it's needed, but that's not the case.

The solution which Markus described as "attractively stupid" also could 
work, as there are exactly 2 places (which I know of) where it's really 
needed. Also, absolutely no changes is an option, as was mentioned before.

Leonid.
Eric Blake - Jan. 11, 2019, 4:23 p.m.
On 1/11/19 8:14 AM, Leonid Bloch wrote:
> On 1/10/19 9:18 PM, Eric Blake wrote:
>> Set the framework up for declaring integer options with an integer
>> default, instead of our current insane approach of requiring the
>> default value to be given as a string (which then has to be reparsed
>> at every use that wants a number).  git grep '[^.]def_value_str' says
>> that we have done a good job of NOT abusing the internal field
>> outside of the implementation in qemu-option.c; therefore, it is not
>> too hard to audit that all code can either handle the new integer
>> defaults correctly or abort because a caller violated constraints.
>> Sadly, we DO have a constraint that qemu_opt_get() should not be
>> called on an option that has an integer type, because we have no
>> where to stash a cached const char * result; but callers that want
>> an integer should be using qemu_opt_get_number() and friends
>> anyways.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>   include/qemu/option.h | 12 ++++++++
>>   util/qemu-option.c    | 69 +++++++++++++++++++++++++++++++++++++------
>>   2 files changed, 72 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/qemu/option.h b/include/qemu/option.h
>> index 844587cab39..46b80d5a6e1 100644
>> --- a/include/qemu/option.h
>> +++ b/include/qemu/option.h
>> @@ -46,6 +46,18 @@ typedef struct QemuOptDesc {
>>       const char *name;
>>       enum QemuOptType type;
>>       const char *help;
>> +    /*
>> +     * For QEMU_OPT_STRING: Leave def_value_int 0, and set def_value_str
>> +     * to a default value or leave NULL for no default.
>> +     *
>> +     * For other types: Initialize at most non-zero def_value_int or a
>> +     * parseable def_value_str for a default (must use a string for an
>> +     * explicit default of 0, although an implicit default generally
>> +     * works).  If setting def_value_int, calling qemu_opt_get() on
>> +     * that option will abort(); instead, call qemu_opt_get_del() or a
>> +     * typed getter.
>> +     */
>> +    uint64_t def_value_int;
>>       const char *def_value_str;
>>   } QemuOptDesc;
>>

> 
> If I understand correctly, the number will still be compiled into the 
> binary as an expression string, but will be printed correctly during 
> runtime?

Anyone that uses .def_value_int will compile into the binary as an
8-byte integer (regardless of how that number was spelled in C, either
as a single constant, or as a constant expression), and NOT as a decimal
string.  String conversions for code that asks for a string will happen
by a runtime printf() (ideally, such code is limited to the case of
printing out information during help output).  Code that is smart enough
to request a number now gets the default value directly, rather than the
old way of having to strtoll() decode a decimal string back into a
number.  No one should ever be using .def_value_str = stringify(macro),
when they can instead just use .def_value_int = macro (which is what the
next patch fixes).

I split the addition of def_value_int from the use in order to ease review.

> If so, it still doesn't feel right to put expressions in binaries, but I 
> agree that it's more elegant than my solution with the lookup table. I'd 
> say the table would be more elegant if there would be more cases in 
> which it's needed, but that's not the case.
> 
> The solution which Markus described as "attractively stupid" also could 
> work, as there are exactly 2 places (which I know of) where it's really 
> needed. Also, absolutely no changes is an option, as was mentioned before.


Markus' suggestion boils down to forgoing patch 1 and 2, rewriting patch
3 and 4 to use hard-coded strings in the two spots that need them (more
of a maintenance burden - if we ever change the macro's value, we have
to also remember to change the string), but keeping patch 5 and 6 as-is.
 It's a smaller diffstat, but less type-safe, and thus a slightly larger
maintenance burden (more technical debt).  My approach is to get rid of
the technical debt (use more type safety, so that we can catch bugs
sooner, and avoid the risk of duplicated values getting out of sync ifa
future patch changes a macro but not the corresponding string). I also
replied to my thread about how we can make further enhancements by even
more patches to FORCE that all integer-typed defaults use only
def_value_int, all string-typed defaults use only def_value_str, at
which point we could switch to using a union for def_value_str/int once
we are sure our code base properly follows the safe typing rules.  But
of course, doing it right costs more time up front (we're avoiding
technical debt, but the patch is bigger and has to be reviewed).  So now
it boils down to Markus' opinion on whether adding type-safe defaults to
QemuOpts is worth it for less technical debt, or if it is adding too
much burden to the fact that we already know that QemuOpts is a hack and
we want to get rid of it in favor of QAPI.
Leonid Bloch - Jan. 11, 2019, 6:54 p.m.
On 1/11/19 6:23 PM, Eric Blake wrote:
> On 1/11/19 8:14 AM, Leonid Bloch wrote:

>> On 1/10/19 9:18 PM, Eric Blake wrote:

>>> Set the framework up for declaring integer options with an integer

>>> default, instead of our current insane approach of requiring the

>>> default value to be given as a string (which then has to be reparsed

>>> at every use that wants a number).  git grep '[^.]def_value_str' says

>>> that we have done a good job of NOT abusing the internal field

>>> outside of the implementation in qemu-option.c; therefore, it is not

>>> too hard to audit that all code can either handle the new integer

>>> defaults correctly or abort because a caller violated constraints.

>>> Sadly, we DO have a constraint that qemu_opt_get() should not be

>>> called on an option that has an integer type, because we have no

>>> where to stash a cached const char * result; but callers that want

>>> an integer should be using qemu_opt_get_number() and friends

>>> anyways.

>>>

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

>>> ---

>>>    include/qemu/option.h | 12 ++++++++

>>>    util/qemu-option.c    | 69 +++++++++++++++++++++++++++++++++++++------

>>>    2 files changed, 72 insertions(+), 9 deletions(-)

>>>

>>> diff --git a/include/qemu/option.h b/include/qemu/option.h

>>> index 844587cab39..46b80d5a6e1 100644

>>> --- a/include/qemu/option.h

>>> +++ b/include/qemu/option.h

>>> @@ -46,6 +46,18 @@ typedef struct QemuOptDesc {

>>>        const char *name;

>>>        enum QemuOptType type;

>>>        const char *help;

>>> +    /*

>>> +     * For QEMU_OPT_STRING: Leave def_value_int 0, and set def_value_str

>>> +     * to a default value or leave NULL for no default.

>>> +     *

>>> +     * For other types: Initialize at most non-zero def_value_int or a

>>> +     * parseable def_value_str for a default (must use a string for an

>>> +     * explicit default of 0, although an implicit default generally

>>> +     * works).  If setting def_value_int, calling qemu_opt_get() on

>>> +     * that option will abort(); instead, call qemu_opt_get_del() or a

>>> +     * typed getter.

>>> +     */

>>> +    uint64_t def_value_int;

>>>        const char *def_value_str;

>>>    } QemuOptDesc;

>>>

> 

>>

>> If I understand correctly, the number will still be compiled into the

>> binary as an expression string, but will be printed correctly during

>> runtime?

> 

> Anyone that uses .def_value_int will compile into the binary as an

> 8-byte integer (regardless of how that number was spelled in C, either

> as a single constant, or as a constant expression), and NOT as a decimal

> string.  String conversions for code that asks for a string will happen

> by a runtime printf() (ideally, such code is limited to the case of

> printing out information during help output).  Code that is smart enough

> to request a number now gets the default value directly, rather than the

> old way of having to strtoll() decode a decimal string back into a

> number.  No one should ever be using .def_value_str = stringify(macro),

> when they can instead just use .def_value_int = macro (which is what the

> next patch fixes).


Yes, you're right. Thanks for the explanation!

Patch

diff --git a/include/qemu/option.h b/include/qemu/option.h
index 844587cab39..46b80d5a6e1 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -46,6 +46,18 @@  typedef struct QemuOptDesc {
     const char *name;
     enum QemuOptType type;
     const char *help;
+    /*
+     * For QEMU_OPT_STRING: Leave def_value_int 0, and set def_value_str
+     * to a default value or leave NULL for no default.
+     *
+     * For other types: Initialize at most non-zero def_value_int or a
+     * parseable def_value_str for a default (must use a string for an
+     * explicit default of 0, although an implicit default generally
+     * works).  If setting def_value_int, calling qemu_opt_get() on
+     * that option will abort(); instead, call qemu_opt_get_del() or a
+     * typed getter.
+     */
+    uint64_t def_value_int;
     const char *def_value_str;
 } QemuOptDesc;

diff --git a/util/qemu-option.c b/util/qemu-option.c
index de42e2a406a..06c4e8102a8 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -321,7 +321,8 @@  const char *qemu_opt_get(QemuOpts *opts, const char *name)
     opt = qemu_opt_find(opts, name);
     if (!opt) {
         const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
-        if (desc && desc->def_value_str) {
+        if (desc) {
+            assert(!desc->def_value_int);
             return desc->def_value_str;
         }
     }
@@ -364,8 +365,22 @@  char *qemu_opt_get_del(QemuOpts *opts, const char *name)
     opt = qemu_opt_find(opts, name);
     if (!opt) {
         desc = find_desc_by_name(opts->list->desc, name);
-        if (desc && desc->def_value_str) {
-            str = g_strdup(desc->def_value_str);
+        if (desc) {
+            if (desc->def_value_str) {
+                str = g_strdup(desc->def_value_str);
+            } else if (desc->def_value_int) {
+                switch (desc->type) {
+                case QEMU_OPT_BOOL:
+                    str = g_strdup("on");
+                    break;
+                case QEMU_OPT_NUMBER:
+                case QEMU_OPT_SIZE:
+                    str = g_strdup_printf("%" PRId64, desc->def_value_int);
+                    break;
+                default:
+                    abort();
+                }
+            }
         }
         return str;
     }
@@ -400,8 +415,15 @@  static bool qemu_opt_get_bool_helper(QemuOpts *opts, const char *name,
     opt = qemu_opt_find(opts, name);
     if (opt == NULL) {
         const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
-        if (desc && desc->def_value_str) {
-            parse_option_bool(name, desc->def_value_str, &ret, &error_abort);
+        if (desc) {
+            if (desc->def_value_int) {
+                assert(desc->type != QEMU_OPT_STRING);
+                return true;
+            }
+            if (desc->def_value_str) {
+                parse_option_bool(name, desc->def_value_str, &ret,
+                                  &error_abort);
+            }
         }
         return ret;
     }
@@ -436,8 +458,15 @@  static uint64_t qemu_opt_get_number_helper(QemuOpts *opts, const char *name,
     opt = qemu_opt_find(opts, name);
     if (opt == NULL) {
         const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
-        if (desc && desc->def_value_str) {
-            parse_option_number(name, desc->def_value_str, &ret, &error_abort);
+        if (desc) {
+            if (desc->def_value_int) {
+                assert(desc->type != QEMU_OPT_STRING);
+                return desc->def_value_int;
+            }
+            if (desc->def_value_str) {
+                parse_option_number(name, desc->def_value_str, &ret,
+                                    &error_abort);
+            }
         }
         return ret;
     }
@@ -473,8 +502,15 @@  static uint64_t qemu_opt_get_size_helper(QemuOpts *opts, const char *name,
     opt = qemu_opt_find(opts, name);
     if (opt == NULL) {
         const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
-        if (desc && desc->def_value_str) {
-            parse_option_size(name, desc->def_value_str, &ret, &error_abort);
+        if (desc) {
+            if (desc->def_value_int) {
+                assert(desc->type != QEMU_OPT_STRING);
+                return desc->def_value_int;
+            }
+            if (desc->def_value_str) {
+                parse_option_size(name, desc->def_value_str, &ret,
+                                  &error_abort);
+            }
         }
         return ret;
     }
@@ -787,9 +823,23 @@  void qemu_opts_print(QemuOpts *opts, const char *separator)
     }
     for (; desc && desc->name; desc++) {
         const char *value;
+        char *tmp = NULL;
         opt = qemu_opt_find(opts, desc->name);

         value = opt ? opt->str : desc->def_value_str;
+        if (!value && desc->def_value_int) {
+            switch (desc->type) {
+            case QEMU_OPT_BOOL:
+                value = tmp = g_strdup("on");
+                break;
+            case QEMU_OPT_NUMBER:
+            case QEMU_OPT_SIZE:
+                value = tmp = g_strdup_printf("%" PRId64, desc->def_value_int);
+                break;
+            default:
+                abort();
+            }
+        }
         if (!value) {
             continue;
         }
@@ -803,6 +853,7 @@  void qemu_opts_print(QemuOpts *opts, const char *separator)
             printf("%s%s=%s", sep, desc->name, value);
         }
         sep = separator;
+        g_free(tmp);
     }
 }