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

[Markus Armbruster <armbru@xxxxxxxxxx>]

Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> writes:

> 21.02.2020 10:38, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>>> Reviewed-by: Greg Kurz <groug@xxxxxxxx>
>>> Reviewed-by: Eric Blake <eblake@xxxxxxxxxx>
>>> ---
>>>
>>> CC: Eric Blake <eblake@xxxxxxxxxx>
>>> CC: Kevin Wolf <kwolf@xxxxxxxxxx>
>>> CC: Max Reitz <mreitz@xxxxxxxxxx>
>>> CC: Greg Kurz <groug@xxxxxxxx>
>>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>> CC: Anthony Perard <anthony.perard@xxxxxxxxxx>
>>> CC: Paul Durrant <paul@xxxxxxx>
>>> CC: Stefan Hajnoczi <stefanha@xxxxxxxxxx>
>>> CC: "Philippe Mathieu-Daudé" <philmd@xxxxxxxxxx>
>>> CC: Laszlo Ersek <lersek@xxxxxxxxxx>
>>> CC: Gerd Hoffmann <kraxel@xxxxxxxxxx>
>>> CC: Stefan Berger <stefanb@xxxxxxxxxxxxx>
>>> CC: Markus Armbruster <armbru@xxxxxxxxxx>
>>> CC: Michael Roth <mdroth@xxxxxxxxxxxxxxxxxx>
>>> CC: qemu-block@xxxxxxxxxx
>>> CC: xen-devel@xxxxxxxxxxxxxxxxxxxx
>>>
>>>   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.

>>
>> 2. Punt this patch to the first part that has users, along with the
>>     part of the Coccinelle script that deals with them.
>
> But coccinelle script would be wrong, if we drop this part from it. I think,
> that after commit which adds coccinelle script, it should work with any file,
> not only subset of these series.
>
> So, it's probably OK for now to drop these functions, forcing their addition 
> if
> coccinelle script will be applied where these functions are needed. We can, 
> for
> example comment these three functions.
>
> Splitting coccinelle script into two parts, which will be in different series 
> will
> not help any patch-porting processes.
>
> Moreover, this will create dependencies between future series updating other 
> files.
>
> So, I don't like [2.]..

And it's likely more work than 1.

>> 3. Do nothing: accept the functions without users.
>
> OK for me)
>
>>
>> I habitually dislike 3., but reviewing the rest of this series might
>> make me override that dislike.
[...]


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel