Re: [Qemu-devel] [PATCH 06/13] QemuOpts: Drop qemu_opt_set(), rename qemu_opt_set_err(), fix use

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 02/16/2015 07:44 AM, Markus Armbruster wrote:
>> qemu_opt_set() is a wrapper around qemu_opt_set() that reports the
>> error with qerror_report_err().
>> 
>> Most of its users assume the function can't fail.  Make them use
>> qemu_opt_set_err() with &error_abort, so that should the assumption
>> ever break, it'll break noisily.
>> 
>> Just two users remain, in util/qemu-config.c.  Switch them to
>> qemu_opt_set_err() as well, then rename qemu_opt_set_err() to
>> qemu_opt_set().
>
> Might be better to split this into fixing the two users in one patch,
> then doing the rename in another (since the rename touches more than two
> callers).  On the other hand, it would represent more churn with
> changing existing qemu_opt_set() to qemu_opt_set_err(,&error_abort) just
> to change back to qemu_opt_set later.

Yeah, that's why I chose to do it in one go.

>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block.c                  |  5 ++--
>>  block/qcow2.c            |  3 +-
>>  block/vvfat.c            |  2 +-
>>  blockdev.c               | 17 +++++------
>>  hw/pci/pci-hotplug-old.c |  2 +-
>>  hw/usb/dev-network.c     |  4 +--
>>  hw/usb/dev-storage.c     |  6 ++--
>>  hw/watchdog/watchdog.c   |  2 +-
>>  include/qemu/option.h    |  5 ++--
>>  net/net.c                |  2 +-
>>  qdev-monitor.c           |  6 ++--
>>  qemu-char.c              | 58 +++++++++++++++++++-------------------
>>  qemu-img.c               |  6 ++--
>>  tests/test-qemu-opts.c   |  6 ++--
>>  util/qemu-config.c       |  9 ++++--
>>  util/qemu-option.c       | 22 +++------------
>>  util/qemu-sockets.c      | 34 +++++++++++-----------
>>  vl.c | 73 ++++++++++++++++++++++++++----------------------
>>  18 files changed, 131 insertions(+), 131 deletions(-)
>
> Also, the patch is fairly obvious that it is mechanical, so even if you
> don't split, I could live with:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
>> +++ b/util/qemu-config.c
>> @@ -219,6 +219,7 @@ void qemu_add_opts(QemuOptsList *list)
>>  
>>  int qemu_set_option(const char *str)
>>  {
>> +    Error *local_err = NULL;
>>      char group[64], id[64], arg[64];
>>      QemuOptsList *list;
>>      QemuOpts *opts;
>> @@ -242,7 +243,9 @@ int qemu_set_option(const char *str)
>>          return -1;
>>      }
>>  
>> -    if (qemu_opt_set(opts, arg, str+offset+1) == -1) {
>> +    qemu_opt_set(opts, arg, str+offset+1, &local_err);
>
> Worth adding whitespace around the 2 '+' operators while touching this?

Yes, if I need to respin or send a pull request.

Thanks!