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

[Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx>]

21.02.2020 19:34, Markus Armbruster wrote:
> 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.
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..

> > > 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.
> [...]

--
Best regards,
Vladimir

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