Re: [PATCH v7 01/11] qapi/error: add (Error **errp) cleaning APIs

[Markus Armbruster <armbru@redhat.com>]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 21.02.2020 19:34, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> 21.02.2020 10:38, Markus Armbruster wrote:
>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>>
>>>>> Add functions to clean Error **errp: call corresponding Error *err
>>>>> cleaning function an set pointer to NULL.
>>>>>
>>>>> New functions:
>>>>>     error_free_errp
>>>>>     error_report_errp
>>>>>     warn_report_errp
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> Reviewed-by: Greg Kurz <groug@kaod.org>
>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>> ---
>>>>>
>>>>> CC: Eric Blake <eblake@redhat.com>
>>>>> CC: Kevin Wolf <kwolf@redhat.com>
>>>>> CC: Max Reitz <mreitz@redhat.com>
>>>>> CC: Greg Kurz <groug@kaod.org>
>>>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>>>> CC: Anthony Perard <anthony.perard@citrix.com>
>>>>> CC: Paul Durrant <paul@xen.org>
>>>>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>>>>> CC: "Philippe Mathieu-Daudé" <philmd@redhat.com>
>>>>> CC: Laszlo Ersek <lersek@redhat.com>
>>>>> CC: Gerd Hoffmann <kraxel@redhat.com>
>>>>> CC: Stefan Berger <stefanb@linux.ibm.com>
>>>>> CC: Markus Armbruster <armbru@redhat.com>
>>>>> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
>>>>> CC: qemu-block@nongnu.org
>>>>> CC: xen-devel@lists.xenproject.org
>>>>>
>>>>>    include/qapi/error.h | 26 ++++++++++++++++++++++++++
>>>>>    1 file changed, 26 insertions(+)
>>>>>
>>>>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>>>>> index ad5b6e896d..d34987148d 100644
>>>>> --- a/include/qapi/error.h
>>>>> +++ b/include/qapi/error.h
>>>>> @@ -309,6 +309,32 @@ void warn_reportf_err(Error *err, const char *fmt, ...)
>>>>>    void error_reportf_err(Error *err, const char *fmt, ...)
>>>>>        GCC_FMT_ATTR(2, 3);
>>>>>    +/*
>>>>> + * Functions to clean Error **errp: call corresponding Error *err cleaning
>>>>> + * function, then set pointer to NULL.
>>>>> + */
>>>>> +static inline void error_free_errp(Error **errp)
>>>>> +{
>>>>> +    assert(errp && *errp);
>>>>> +    error_free(*errp);
>>>>> +    *errp = NULL;
>>>>> +}
>>>>> +
>>>>> +static inline void error_report_errp(Error **errp)
>>>>> +{
>>>>> +    assert(errp && *errp);
>>>>> +    error_report_err(*errp);
>>>>> +    *errp = NULL;
>>>>> +}
>>>>> +
>>>>> +static inline void warn_report_errp(Error **errp)
>>>>> +{
>>>>> +    assert(errp && *errp);
>>>>> +    warn_report_err(*errp);
>>>>> +    *errp = NULL;
>>>>> +}
>>>>> +
>>>>> +
>>>>>    /*
>>>>>     * Just like error_setg(), except you get to specify the error class.
>>>>>     * Note: use of error classes other than ERROR_CLASS_GENERIC_ERROR is
>>>>
>>>> These appear to be unused apart from the Coccinelle script in PATCH 03.
>>>>
>>>> They are used in the full "[RFC v5 000/126] error: auto propagated
>>>> local_err" series.  Options:
>>>>
>>>> 1. Pick a few more patches into this part I series, so these guys come
>>>>      with users.
>>>
>>> It needs some additional effort for this series.. But it's possible. Still,
>>> I think that we at least should not pull out patches which should be in
>>> future series (for example from ppc or block/)..
>>
>> Yes, we want to keep related stuff together.
>>
>>> Grepping through v5:
>>>   for x in {warn_report_errp,error_report_errp,error_free_errp}; do echo == $x ==; git grep -l $x | grep -v coccinelle | grep -v 'error\.h'; echo; done
>>> == warn_report_errp ==
>>> block/file-posix.c
>>> hw/ppc/spapr.c
>>> hw/ppc/spapr_caps.c
>>> hw/ppc/spapr_irq.c
>>> hw/vfio/pci.c
>>> net/tap.c
>>> qom/object.c
>>>
>>> == error_report_errp ==
>>> hw/block/vhost-user-blk.c
>>> util/oslib-posix.c
>>>
>>> == error_free_errp ==
>>> block.c
>>> block/qapi.c
>>> block/sheepdog.c
>>> block/snapshot.c
>>> blockdev.c
>>> chardev/char-socket.c
>>> hw/audio/intel-hda.c
>>> hw/core/qdev-properties.c
>>> hw/pci-bridge/pci_bridge_dev.c
>>> hw/pci-bridge/pcie_pci_bridge.c
>>> hw/scsi/megasas.c
>>> hw/scsi/mptsas.c
>>> hw/usb/hcd-xhci.c
>>> io/net-listener.c
>>> migration/colo.c
>>> qga/commands-posix.c
>>> qga/commands-win32.c
>>> util/qemu-sockets.c
>>>
>>> What do you want to add?
>>
>> PATCH v5 032 uses both error_report_errp() and error_free_errp().
>> Adding warn_report_errp() without a user is okay with me.  What do you
>> think?
>>
>> If there are patches you consider related to 032, feel free to throw
>> them in.
>
> 032 is qga/commands-win32.c and util/oslib-posix.c
>
> Seems that they are wrongly grouped into one patch.
>
> qga/commands-win32.c matches qga/ (Michael Roth)
> and  util/oslib-posix.c matches POSIX (Paolo Bonzini)
>
> So, it should be two separate patches anyway.
>
> For [1.] I only afraid that we'll have to wait for maintainers, who were
> not interested in previous iterations, to review these new patches..

We won't.

We should and we will give every maintainer a chance to review these
patches, even though the changes are mechanical.  Maintainers are free
to decline or ignore this offer.  I will feel free to interpret that as
"go ahead and merge this through your tree".

In fact, I fully expect the bulk of the changes to go through my tree.
Chasing umpteen maintainers for each one to merge a trivial part of this
massive tree-wide change would take ages and accomplish nothing.

[...]