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

[Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx>]

22.02.2020 11:23, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx> writes:
>
> > 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..
> 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.
>
> [...]
Hmm, then OK. I'll add these two patches.. Still, you pointed missed in cocci 
script
cases about error_reportf_err() and warn_reportf_err()... I'm afraid we just 
don't have
corresponding files, and therefore don't want to add unused errp wrappers for 
them..

--
Best regards,
Vladimir

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