| Message ID | 20200131130118.1716-2-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v7,01/11] qapi/error: add (Error **errp) cleaning APIs | expand |
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. 2. Punt this patch to the first part that has users, along with the part of the Coccinelle script that deals with them. 3. Do nothing: accept the functions without users. I habitually dislike 3., but reviewing the rest of this series might make me override that dislike.
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/).. 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? > > 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.].. > > 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. > ---------------- same grep with maintainers: 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' | while read f; do echo $f; ./scripts/get_maintainer.pl -f --no-rolestats $f | grep -v 'qemu-block\|qemu-devel' | sed -e 's/^/ /'; done; echo; done == warn_report_errp == block/file-posix.c Kevin Wolf <kwolf@redhat.com> Max Reitz <mreitz@redhat.com> hw/ppc/spapr.c David Gibson <david@gibson.dropbear.id.au> qemu-ppc@nongnu.org hw/ppc/spapr_caps.c David Gibson <david@gibson.dropbear.id.au> qemu-ppc@nongnu.org hw/ppc/spapr_irq.c David Gibson <david@gibson.dropbear.id.au> qemu-ppc@nongnu.org hw/vfio/pci.c Alex Williamson <alex.williamson@redhat.com> net/tap.c Jason Wang <jasowang@redhat.com> qom/object.c Paolo Bonzini <pbonzini@redhat.com> "Daniel P. Berrangé" <berrange@redhat.com> Eduardo Habkost <ehabkost@redhat.com> == error_report_errp == hw/block/vhost-user-blk.c "Michael S. Tsirkin" <mst@redhat.com> Kevin Wolf <kwolf@redhat.com> Max Reitz <mreitz@redhat.com> util/oslib-posix.c Paolo Bonzini <pbonzini@redhat.com> == error_free_errp == block.c Kevin Wolf <kwolf@redhat.com> Max Reitz <mreitz@redhat.com> block/qapi.c Markus Armbruster <armbru@redhat.com> Kevin Wolf <kwolf@redhat.com> Max Reitz <mreitz@redhat.com> block/sheepdog.c Liu Yuan <namei.unix@gmail.com> Kevin Wolf <kwolf@redhat.com> Max Reitz <mreitz@redhat.com> sheepdog@lists.wpkg.org block/snapshot.c Kevin Wolf <kwolf@redhat.com> Max Reitz <mreitz@redhat.com> blockdev.c Markus Armbruster <armbru@redhat.com> Kevin Wolf <kwolf@redhat.com> Max Reitz <mreitz@redhat.com> chardev/char-socket.c "Marc-André Lureau" <marcandre.lureau@redhat.com> Paolo Bonzini <pbonzini@redhat.com> hw/audio/intel-hda.c Gerd Hoffmann <kraxel@redhat.com> hw/core/qdev-properties.c Paolo Bonzini <pbonzini@redhat.com> "Daniel P. Berrangé" <berrange@redhat.com> Eduardo Habkost <ehabkost@redhat.com> hw/pci-bridge/pci_bridge_dev.c "Michael S. Tsirkin" <mst@redhat.com> Marcel Apfelbaum <marcel.apfelbaum@gmail.com> hw/pci-bridge/pcie_pci_bridge.c "Michael S. Tsirkin" <mst@redhat.com> Marcel Apfelbaum <marcel.apfelbaum@gmail.com> hw/scsi/megasas.c Hannes Reinecke <hare@suse.com> Paolo Bonzini <pbonzini@redhat.com> Fam Zheng <fam@euphon.net> hw/scsi/mptsas.c Paolo Bonzini <pbonzini@redhat.com> Fam Zheng <fam@euphon.net> hw/usb/hcd-xhci.c Gerd Hoffmann <kraxel@redhat.com> io/net-listener.c "Daniel P. Berrangé" <berrange@redhat.com> migration/colo.c Hailiang Zhang <zhang.zhanghailiang@huawei.com> Juan Quintela <quintela@redhat.com> "Dr. David Alan Gilbert" <dgilbert@redhat.com> qga/commands-posix.c Michael Roth <mdroth@linux.vnet.ibm.com> qga/commands-win32.c Michael Roth <mdroth@linux.vnet.ibm.com> util/qemu-sockets.c "Daniel P. Berrangé" <berrange@redhat.com> Gerd Hoffmann <kraxel@redhat.com>
On 2/21/20 3:20 AM, Vladimir Sementsov-Ogievskiy wrote: >>> +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/).. > >> 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. Splitting the coccinelle script across multiple patches is actually quite reviewable, and still easy to backport. Consider this series by Philippe: https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg05554.html which makes multiple additions to scripts/coccinelle/exec_rw_const.cocci over the course of the series.
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. >> >> 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. [...]
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.. > >>> >>> 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. > [...] >
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. [...]
22.02.2020 11:23, Markus Armbruster wrote: > 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. > > [...] > 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..
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