Re: [RFC PATCH 07/14] qemu-options: Consolidate qemu_opts_parse* functions

[Fabiano Rosas <farosas@suse.de>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, Sep 18, 2026 at 07:29:54PM -0300, Fabiano Rosas wrote:
>> The qemu_opts_parse, qemu_opts_parse_list and qemu_opts_parse_noisily
>> functions all call opts_parse(), they can reuse code.
>> 
>> Consolidate them at qemu_opts_parse_list(). Use the presence of the
>> Error pointer to determine whether the caller wants to be noisy: if
>> there is an Error pointer, then use it, otherwise print the message.
>> 
>> This makes the code more opaque, so keep the qemu_opts_parse and
>> qemu_opts_parse_noisily functions as wrappers to make sure the intent
>> at the callers is still clear.
>
> It is also somewhat differnt from the normal semantics in QEMU.
>
> The behaviour of passing NULL for an "Error **errp" parameter
> indicates that the errors are to be ignored/discarded.
>
> This patch introduces atypical semantics, that the errors must
> be printed, so I'm not entirely convinced this patch is a win.
>

I understand. This is the main reason why this series is RFC. I could
exchange the implicit "pass NULL to mean noisily" with an explicit
parameter 'bool noisily'.

>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  tests/unit/test-opts-visitor.c |  2 +-
>>  util/qemu-option.c             | 32 ++++++++++++++++++--------------
>>  2 files changed, 19 insertions(+), 15 deletions(-)
>
>> diff --git a/util/qemu-option.c b/util/qemu-option.c
>> index bb38d305c9..c8e552823b 100644
>> --- a/util/qemu-option.c
>> +++ b/util/qemu-option.c
>> @@ -938,7 +938,23 @@ QemuOpts *qemu_opts_parse(const char *group, const char *params,
>>  QemuOpts *qemu_opts_parse_list(QemuOptsList *list, const char *params,
>>                                 bool permit_abbrev, Error **errp)
>>  {
>> -    return opts_parse(list, params, permit_abbrev, false, false, errp);
>> +    bool noisily = !errp;
>> +    QemuOpts *opts = NULL;
>> +
>> +    if (noisily) {
>> +        Error *err = NULL;
>> +        bool help_wanted = !opts_accepts_any(list);
>> +
>> +        opts = opts_parse(list, params, permit_abbrev, true, help_wanted, &err);
>> +        if (!opts && err) {
>> +            error_report_err(err);
>> +        }
>> +
>> +    } else {
>> +        opts = opts_parse(list, params, permit_abbrev, false, false, errp);
>> +    }
>> +
>> +    return opts;
>>  }
>>  
>>  /**
>> @@ -952,19 +968,7 @@ QemuOpts *qemu_opts_parse_list(QemuOptsList *list, const char *params,
>>  QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params,
>>                                    bool permit_abbrev)
>>  {
>> -    Error *err = NULL;
>> -    QemuOpts *opts;
>> -    bool help_wanted = !opts_accepts_any(list);
>> -
>> -    opts = opts_parse(list, params, permit_abbrev, true, help_wanted, &err);
>> -    if (!opts) {
>> -        if (help_wanted) {
>> -            assert(!err);
>> -        } else {
>> -            error_report_err(err);
>> -        }
>> -    }
>> -    return opts;
>> +    return qemu_opts_parse_list(list, params, permit_abbrev, NULL);
>>  }
>>  
>>  static bool qemu_opts_from_qdict_entry(QemuOpts *opts,
>> -- 
>> 2.53.0
>> 
>> 
>
> With regards,
> Daniel