[Xen-devel] [PATCH v7 00/11] error: auto propagated local_err part I

[Xen-devel] [PATCH v7 00/11] error: auto propagated local_err part I

[Vladimir Sementsov-Ogievskiy]

Hi all!

v7 is available at
 https://src.openvz.org/scm/~vsementsov/qemu.git #tag up-auto-local-err-partI-v7

Changes v6->v7:

01: - improve commit message
    - fix typo in comment [Eric]
    - add Eric's and Greg's r-b
02: - grammar/wording [Eric]
    - add Eric's and Greg's r-b
03: - improve commit message
    - grammar [Eric]
    - improve script to rename unusual (Error **) parameters
      to errp, and after it switch errp back to be "symbol"
      instead of "identifier"
04: - add Eric's r-b
08: - add Greg's a-b
11: - add Paul's a-b


v6 is available at
 https://src.openvz.org/scm/~vsementsov/qemu.git #tag up-auto-local-err-partI-v6 

Changes v5->v6:
01: use errp name for the parameter, add assertion
02: add a lot of text information, drop Eric's r-b.
    no semantic changes.
03: add more comments
    skip functions with pattern error_append_.*_hint in name
    make errp identifier, to match any name of Error ** paramter
    some other improvements
04: only commit message changed,
    keep Philippe's r-b
05: new, manual update for hw/sd/ssi-sd
06: only commit message changed,
    keep Philippe's r-b
07: only commit message changed,
    keep Philippe's r-b
08: local_parse_opts() changed, so patch changed in this
    function, drop a-b mark
    also, indentation fixed, by improvement in coccinelle script
09: only commit message changed,
    keep Stefan's r-b
10: commit message and a bit of context changed, still seems
    valid to keep Eric's r-b
11: add new hunk: hw/pci-host/xen_igd_pt.c, so, drop r-b
    also, indentation fixed, by improvement in coccinelle script

In these series, there is no commit-per-subsystem script, each generated
commit is generated in separate.

Still, generating commands are very similar, and looks like

    sed -n '/^<Subsystem name>$/,/^$/{s/^F: //p}' MAINTAINERS | \
    xargs git ls-files | grep '\.[hc]$' | \
    xargs spatch \
        --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
        --macro-file scripts/cocci-macro-file.h \
        --in-place --no-show-diff --max-width 80

Note, that in each generated commit, generation command is the only
text, indented by 8 spaces in 'git log -1' output, so, to regenerate all
commits (for example, after rebase, or change in coccinelle script), you
may use the following command:

git rebase -x "sh -c \"git show --pretty= --name-only | xargs git checkout HEAD^ -- ; git reset; git log -1 | grep '^        ' | sh\"" HEAD~7

Which will start automated interactive rebase for generated patches,
which will stop if generated patch changed
(you may do git commit --amend to apply updated generated changes).

Note:
  git show --pretty= --name-only   - lists files, changed in HEAD
  git log -1 | grep '^        ' | sh   - rerun generation command of HEAD


Check for compilation of changed .c files
git rebase -x "sh -c \"git show --pretty= --name-only | sed -n 's/\.c$/.o/p' | xargs make -j9\"" HEAD~7
  

Vladimir Sementsov-Ogievskiy (11):
  qapi/error: add (Error **errp) cleaning APIs
  error: auto propagated local_err
  scripts: add coccinelle script to use auto propagated errp
  hw/sd/ssi-sd: fix error handling in ssi_sd_realize
  SD (Secure Card): introduce ERRP_AUTO_PROPAGATE
  pflash: introduce ERRP_AUTO_PROPAGATE
  fw_cfg: introduce ERRP_AUTO_PROPAGATE
  virtio-9p: introduce ERRP_AUTO_PROPAGATE
  TPM: introduce ERRP_AUTO_PROPAGATE
  nbd: introduce ERRP_AUTO_PROPAGATE
  xen: introduce ERRP_AUTO_PROPAGATE

 include/block/nbd.h                           |   1 +
 include/qapi/error.h                          | 112 ++++++++++++-
 block/nbd.c                                   |  49 +++---
 hw/9pfs/9p-local.c                            |  12 +-
 hw/9pfs/9p.c                                  |   1 +
 hw/block/dataplane/xen-block.c                |  17 +-
 hw/block/pflash_cfi01.c                       |   7 +-
 hw/block/pflash_cfi02.c                       |   7 +-
 hw/block/xen-block.c                          | 125 ++++++--------
 hw/nvram/fw_cfg.c                             |  14 +-
 hw/pci-host/xen_igd_pt.c                      |   7 +-
 hw/sd/sdhci-pci.c                             |   7 +-
 hw/sd/sdhci.c                                 |  21 +--
 hw/sd/ssi-sd.c                                |  26 ++-
 hw/tpm/tpm_util.c                             |   7 +-
 hw/xen/xen-backend.c                          |   7 +-
 hw/xen/xen-bus.c                              | 100 +++++------
 hw/xen/xen-host-pci-device.c                  |  27 ++-
 hw/xen/xen_pt.c                               |  25 ++-
 hw/xen/xen_pt_config_init.c                   |  20 +--
 nbd/client.c                                  |   5 +
 nbd/server.c                                  |   5 +
 tpm.c                                         |   7 +-
 scripts/coccinelle/auto-propagated-errp.cocci | 158 ++++++++++++++++++
 24 files changed, 500 insertions(+), 267 deletions(-)
 create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci

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

-- 
2.21.0


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

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

[Vladimir Sementsov-Ogievskiy]

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
-- 
2.21.0


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

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

[Markus Armbruster]

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.


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

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

[Vladimir Sementsov-Ogievskiy]

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>



-- 
Best regards,
Vladimir

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

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

[Eric Blake]

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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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

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

[Markus Armbruster]

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


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

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

[Vladimir Sementsov-Ogievskiy]

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


-- 
Best regards,
Vladimir

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

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

[Markus Armbruster]

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.

[...]


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

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

[Vladimir Sementsov-Ogievskiy]

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

-- 
Best regards,
Vladimir

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

[Xen-devel] [PATCH v7 02/11] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
functions with an errp OUT parameter.

It has three goals:

1. Fix issue with error_fatal and error_prepend/error_append_hint: user
can't see this additional information, because exit() happens in
error_setg earlier than information is added. [Reported by Greg Kurz]

2. Fix issue with error_abort and error_propagate: when we wrap
error_abort by local_err+error_propagate, the resulting coredump will
refer to error_propagate and not to the place where error happened.
(the macro itself doesn't fix the issue, but it allows us to [3.] drop
the local_err+error_propagate pattern, which will definitely fix the
issue) [Reported by Kevin Wolf]

3. Drop local_err+error_propagate pattern, which is used to workaround
void functions with errp parameter, when caller wants to know resulting
status. (Note: actually these functions could be merely updated to
return int error code).

To achieve these goals, later patches will add invocations
of this macro at the start of functions with either use
error_prepend/error_append_hint (solving 1) or which use
local_err+error_propagate to check errors, switching those
functions to use *errp instead (solving 2 and 3).

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 | 83 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 82 insertions(+), 1 deletion(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index d34987148d..b9452d4806 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -78,7 +78,7 @@
  * Call a function treating errors as fatal:
  *     foo(arg, &error_fatal);
  *
- * Receive an error and pass it on to the caller:
+ * Receive an error and pass it on to the caller (DEPRECATED*):
  *     Error *err = NULL;
  *     foo(arg, &err);
  *     if (err) {
@@ -98,6 +98,50 @@
  *     foo(arg, errp);
  * for readability.
  *
+ * DEPRECATED* This pattern is deprecated now, the use ERRP_AUTO_PROPAGATE macro
+ * instead (defined below).
+ * It's deprecated because of two things:
+ *
+ * 1. Issue with error_abort and error_propagate: when we wrap error_abort by
+ * local_err+error_propagate, the resulting coredump will refer to
+ * error_propagate and not to the place where error happened.
+ *
+ * 2. A lot of extra code of the same pattern
+ *
+ * How to update old code to use ERRP_AUTO_PROPAGATE?
+ *
+ * All you need is to add ERRP_AUTO_PROPAGATE() invocation at function start,
+ * than you may safely dereference errp to check errors and do not need any
+ * additional local Error variables or calls to error_propagate().
+ *
+ * Example:
+ *
+ * old code
+ *
+ *     void fn(..., Error **errp) {
+ *         Error *err = NULL;
+ *         foo(arg, &err);
+ *         if (err) {
+ *             handle the error...
+ *             error_propagate(errp, err);
+ *             return;
+ *         }
+ *         ...
+ *     }
+ *
+ * updated code
+ *
+ *     void fn(..., Error **errp) {
+ *         ERRP_AUTO_PROPAGATE();
+ *         foo(arg, errp);
+ *         if (*errp) {
+ *             handle the error...
+ *             return;
+ *         }
+ *         ...
+ *     }
+ *
+ *
  * Receive and accumulate multiple errors (first one wins):
  *     Error *err = NULL, *local_err = NULL;
  *     foo(arg, &err);
@@ -348,6 +392,43 @@ void error_set_internal(Error **errp,
                         ErrorClass err_class, const char *fmt, ...)
     GCC_FMT_ATTR(6, 7);
 
+typedef struct ErrorPropagator {
+    Error *local_err;
+    Error **errp;
+} ErrorPropagator;
+
+static inline void error_propagator_cleanup(ErrorPropagator *prop)
+{
+    error_propagate(prop->errp, prop->local_err);
+}
+
+G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
+
+/*
+ * ERRP_AUTO_PROPAGATE
+ *
+ * This macro is created to be the first line of a function which use
+ * Error **errp parameter to report error. It's needed only in cases where we
+ * want to use error_prepend, error_append_hint or dereference *errp. It's
+ * still safe (but useless) in other cases.
+ *
+ * If errp is NULL or points to error_fatal, it is rewritten to point to a
+ * local Error object, which will be automatically propagated to the original
+ * errp on function exit (see error_propagator_cleanup).
+ *
+ * After invocation of this macro it is always safe to dereference errp
+ * (as it's not NULL anymore) and to add information by error_prepend or
+ * error_append_hint (as, if it was error_fatal, we swapped it with a
+ * local_error to be propagated on cleanup).
+ *
+ * Note: we don't wrap the error_abort case, as we want resulting coredump
+ * to point to the place where the error happened, not to error_propagate.
+ */
+#define ERRP_AUTO_PROPAGATE()                                  \
+    g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp};  \
+    errp = ((errp == NULL || *errp == error_fatal)             \
+            ? &_auto_errp_prop.local_err : errp)
+
 /*
  * Special error destination to abort on error.
  * See error_setg() and error_propagate() for details.
-- 
2.21.0


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

Re: [Xen-devel] [PATCH v7 02/11] error: auto propagated local_err

[Markus Armbruster]

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

> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
> functions with an errp OUT parameter.
>
> It has three goals:
>
> 1. Fix issue with error_fatal and error_prepend/error_append_hint: user
> can't see this additional information, because exit() happens in
> error_setg earlier than information is added. [Reported by Greg Kurz]
>
> 2. Fix issue with error_abort and error_propagate: when we wrap
> error_abort by local_err+error_propagate, the resulting coredump will
> refer to error_propagate and not to the place where error happened.
> (the macro itself doesn't fix the issue, but it allows us to [3.] drop
> the local_err+error_propagate pattern, which will definitely fix the
> issue) [Reported by Kevin Wolf]
>
> 3. Drop local_err+error_propagate pattern, which is used to workaround
> void functions with errp parameter, when caller wants to know resulting
> status. (Note: actually these functions could be merely updated to
> return int error code).
>
> To achieve these goals, later patches will add invocations
> of this macro at the start of functions with either use
> error_prepend/error_append_hint (solving 1) or which use
> local_err+error_propagate to check errors, switching those
> functions to use *errp instead (solving 2 and 3).
>
> 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 | 83 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 82 insertions(+), 1 deletion(-)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index d34987148d..b9452d4806 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -78,7 +78,7 @@
>   * Call a function treating errors as fatal:
>   *     foo(arg, &error_fatal);
>   *
> - * Receive an error and pass it on to the caller:
> + * Receive an error and pass it on to the caller (DEPRECATED*):
>   *     Error *err = NULL;
>   *     foo(arg, &err);
>   *     if (err) {
> @@ -98,6 +98,50 @@
>   *     foo(arg, errp);
>   * for readability.
>   *
> + * DEPRECATED* This pattern is deprecated now, the use ERRP_AUTO_PROPAGATE macro
> + * instead (defined below).
> + * It's deprecated because of two things:
> + *
> + * 1. Issue with error_abort and error_propagate: when we wrap error_abort by
> + * local_err+error_propagate, the resulting coredump will refer to
> + * error_propagate and not to the place where error happened.
> + *
> + * 2. A lot of extra code of the same pattern
> + *
> + * How to update old code to use ERRP_AUTO_PROPAGATE?
> + *
> + * All you need is to add ERRP_AUTO_PROPAGATE() invocation at function start,
> + * than you may safely dereference errp to check errors and do not need any
> + * additional local Error variables or calls to error_propagate().
> + *
> + * Example:
> + *
> + * old code
> + *
> + *     void fn(..., Error **errp) {
> + *         Error *err = NULL;
> + *         foo(arg, &err);
> + *         if (err) {
> + *             handle the error...
> + *             error_propagate(errp, err);
> + *             return;
> + *         }
> + *         ...
> + *     }
> + *
> + * updated code
> + *
> + *     void fn(..., Error **errp) {
> + *         ERRP_AUTO_PROPAGATE();
> + *         foo(arg, errp);
> + *         if (*errp) {
> + *             handle the error...
> + *             return;
> + *         }
> + *         ...
> + *     }
> + *
> + *
>   * Receive and accumulate multiple errors (first one wins):
>   *     Error *err = NULL, *local_err = NULL;
>   *     foo(arg, &err);

Let's explain what should be done *first*, and only then talk about the
deprecated pattern and how to convert it to current usage.

> @@ -348,6 +392,43 @@ void error_set_internal(Error **errp,
>                          ErrorClass err_class, const char *fmt, ...)
>      GCC_FMT_ATTR(6, 7);
>  
> +typedef struct ErrorPropagator {
> +    Error *local_err;
> +    Error **errp;
> +} ErrorPropagator;
> +
> +static inline void error_propagator_cleanup(ErrorPropagator *prop)
> +{
> +    error_propagate(prop->errp, prop->local_err);
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
> +
> +/*
> + * ERRP_AUTO_PROPAGATE
> + *
> + * This macro is created to be the first line of a function which use
> + * Error **errp parameter to report error. It's needed only in cases where we
> + * want to use error_prepend, error_append_hint or dereference *errp. It's
> + * still safe (but useless) in other cases.
> + *
> + * If errp is NULL or points to error_fatal, it is rewritten to point to a
> + * local Error object, which will be automatically propagated to the original
> + * errp on function exit (see error_propagator_cleanup).
> + *
> + * After invocation of this macro it is always safe to dereference errp
> + * (as it's not NULL anymore) and to add information by error_prepend or
> + * error_append_hint (as, if it was error_fatal, we swapped it with a
> + * local_error to be propagated on cleanup).
> + *
> + * Note: we don't wrap the error_abort case, as we want resulting coredump
> + * to point to the place where the error happened, not to error_propagate.

Tradeoff: we gain more useful backtraces, we lose message improvements
from error_prepend(), error_append_hint() and such, if any.  Makes
sense.

> + */

The comment's contents looks okay to me.  I'll want to tweak formatting
to better blend in with the rest of this file, but let's not worry about
that now.

> +#define ERRP_AUTO_PROPAGATE()                                  \
> +    g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp};  \
> +    errp = ((errp == NULL || *errp == error_fatal)             \
> +            ? &_auto_errp_prop.local_err : errp)
> +
>  /*
>   * Special error destination to abort on error.
>   * See error_setg() and error_propagate() for details.

*errp == error_fatal tests *errp == NULL, which is not what you want.
You need to test errp == &error_fatal, just like error_handle_fatal().

Superfluous parenthesis around the first operand of ?:.

Wouldn't

   #define ERRP_AUTO_PROPAGATE()                                  \
       g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp};  \
       if (!errp || errp == &error_fatal) {                       \
           errp = &_auto_errp_prop.local_err;                     \
       }

be clearer?


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

Re: [Xen-devel] [PATCH v7 02/11] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

21.02.2020 12:19, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
>> functions with an errp OUT parameter.
>>
>> It has three goals:
>>
>> 1. Fix issue with error_fatal and error_prepend/error_append_hint: user
>> can't see this additional information, because exit() happens in
>> error_setg earlier than information is added. [Reported by Greg Kurz]
>>
>> 2. Fix issue with error_abort and error_propagate: when we wrap
>> error_abort by local_err+error_propagate, the resulting coredump will
>> refer to error_propagate and not to the place where error happened.
>> (the macro itself doesn't fix the issue, but it allows us to [3.] drop
>> the local_err+error_propagate pattern, which will definitely fix the
>> issue) [Reported by Kevin Wolf]
>>
>> 3. Drop local_err+error_propagate pattern, which is used to workaround
>> void functions with errp parameter, when caller wants to know resulting
>> status. (Note: actually these functions could be merely updated to
>> return int error code).
>>
>> To achieve these goals, later patches will add invocations
>> of this macro at the start of functions with either use
>> error_prepend/error_append_hint (solving 1) or which use
>> local_err+error_propagate to check errors, switching those
>> functions to use *errp instead (solving 2 and 3).
>>
>> 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 | 83 +++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 82 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index d34987148d..b9452d4806 100644
>> --- a/include/qapi/error.h
>> +++ b/include/qapi/error.h
>> @@ -78,7 +78,7 @@
>>    * Call a function treating errors as fatal:
>>    *     foo(arg, &error_fatal);
>>    *
>> - * Receive an error and pass it on to the caller:
>> + * Receive an error and pass it on to the caller (DEPRECATED*):
>>    *     Error *err = NULL;
>>    *     foo(arg, &err);
>>    *     if (err) {
>> @@ -98,6 +98,50 @@
>>    *     foo(arg, errp);
>>    * for readability.
>>    *
>> + * DEPRECATED* This pattern is deprecated now, the use ERRP_AUTO_PROPAGATE macro
>> + * instead (defined below).
>> + * It's deprecated because of two things:
>> + *
>> + * 1. Issue with error_abort and error_propagate: when we wrap error_abort by
>> + * local_err+error_propagate, the resulting coredump will refer to
>> + * error_propagate and not to the place where error happened.
>> + *
>> + * 2. A lot of extra code of the same pattern
>> + *
>> + * How to update old code to use ERRP_AUTO_PROPAGATE?
>> + *
>> + * All you need is to add ERRP_AUTO_PROPAGATE() invocation at function start,
>> + * than you may safely dereference errp to check errors and do not need any
>> + * additional local Error variables or calls to error_propagate().
>> + *
>> + * Example:
>> + *
>> + * old code
>> + *
>> + *     void fn(..., Error **errp) {
>> + *         Error *err = NULL;
>> + *         foo(arg, &err);
>> + *         if (err) {
>> + *             handle the error...
>> + *             error_propagate(errp, err);
>> + *             return;
>> + *         }
>> + *         ...
>> + *     }
>> + *
>> + * updated code
>> + *
>> + *     void fn(..., Error **errp) {
>> + *         ERRP_AUTO_PROPAGATE();
>> + *         foo(arg, errp);
>> + *         if (*errp) {
>> + *             handle the error...
>> + *             return;
>> + *         }
>> + *         ...
>> + *     }
>> + *
>> + *
>>    * Receive and accumulate multiple errors (first one wins):
>>    *     Error *err = NULL, *local_err = NULL;
>>    *     foo(arg, &err);
> 
> Let's explain what should be done *first*, and only then talk about the
> deprecated pattern and how to convert it to current usage.
> 
>> @@ -348,6 +392,43 @@ void error_set_internal(Error **errp,
>>                           ErrorClass err_class, const char *fmt, ...)
>>       GCC_FMT_ATTR(6, 7);
>>   
>> +typedef struct ErrorPropagator {
>> +    Error *local_err;
>> +    Error **errp;
>> +} ErrorPropagator;
>> +
>> +static inline void error_propagator_cleanup(ErrorPropagator *prop)
>> +{
>> +    error_propagate(prop->errp, prop->local_err);
>> +}
>> +
>> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
>> +
>> +/*
>> + * ERRP_AUTO_PROPAGATE
>> + *
>> + * This macro is created to be the first line of a function which use
>> + * Error **errp parameter to report error. It's needed only in cases where we
>> + * want to use error_prepend, error_append_hint or dereference *errp. It's
>> + * still safe (but useless) in other cases.
>> + *
>> + * If errp is NULL or points to error_fatal, it is rewritten to point to a
>> + * local Error object, which will be automatically propagated to the original
>> + * errp on function exit (see error_propagator_cleanup).
>> + *
>> + * After invocation of this macro it is always safe to dereference errp
>> + * (as it's not NULL anymore) and to add information by error_prepend or
>> + * error_append_hint (as, if it was error_fatal, we swapped it with a
>> + * local_error to be propagated on cleanup).
>> + *
>> + * Note: we don't wrap the error_abort case, as we want resulting coredump
>> + * to point to the place where the error happened, not to error_propagate.
> 
> Tradeoff: we gain more useful backtraces, we lose message improvements
> from error_prepend(), error_append_hint() and such, if any.  Makes
> sense.
> 
>> + */
> 
> The comment's contents looks okay to me.  I'll want to tweak formatting
> to better blend in with the rest of this file, but let's not worry about
> that now.
> 
>> +#define ERRP_AUTO_PROPAGATE()                                  \
>> +    g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp};  \
>> +    errp = ((errp == NULL || *errp == error_fatal)             \
>> +            ? &_auto_errp_prop.local_err : errp)
>> +
>>   /*
>>    * Special error destination to abort on error.
>>    * See error_setg() and error_propagate() for details.
> 
> *errp == error_fatal tests *errp == NULL, which is not what you want.
> You need to test errp == &error_fatal, just like error_handle_fatal().

Oops, great bug) And nobody noticed before) Of course, you are right.

> 
> Superfluous parenthesis around the first operand of ?:.
> 
> Wouldn't
> 
>     #define ERRP_AUTO_PROPAGATE()                                  \
>         g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp};  \
>         if (!errp || errp == &error_fatal) {                       \
>             errp = &_auto_errp_prop.local_err;                     \
>         }
> 
> be clearer?
> 

Hmm, notation with "if" will allow omitting ';' after macro invocation, which seems not good..
And if I'm not wrong we've already discussed it somewhere in previous versions.

Still, no objections for s/errp == NULL/!errp/ and we need s/*errp == error_fatal/errp == &error_fatal/ for sure.

-- 
Best regards,
Vladimir

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

Re: [Xen-devel] [PATCH v7 02/11] error: auto propagated local_err

[Eric Blake]

On 2/21/20 3:42 AM, Vladimir Sementsov-Ogievskiy wrote:

>>> +#define ERRP_AUTO_PROPAGATE()                                  \
>>> +    g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp};  \
>>> +    errp = ((errp == NULL || *errp == error_fatal)             \
>>> +            ? &_auto_errp_prop.local_err : errp)
>>> +
>>>   /*
>>>    * Special error destination to abort on error.
>>>    * See error_setg() and error_propagate() for details.
>>
>> *errp == error_fatal tests *errp == NULL, which is not what you want.
>> You need to test errp == &error_fatal, just like error_handle_fatal().
> 
> Oops, great bug) And nobody noticed before) Of course, you are right.

Sorry I missed it in my earlier looks.

> 
>>
>> Superfluous parenthesis around the first operand of ?:.
>>
>> Wouldn't
>>
>>     #define ERRP_AUTO_PROPAGATE()                                  \
>>         g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp};  \
>>         if (!errp || errp == &error_fatal) {                       \
>>             errp = &_auto_errp_prop.local_err;                     \
>>         }
>>
>> be clearer?
>>
> 
> Hmm, notation with "if" will allow omitting ';' after macro invocation, 
> which seems not good..

Then wrap it:

g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp}; \
do { \
   if (!errp || errp == &error_fata) {
     errp = &_auto_errp_prop.local_err; \
   } \
while (0)


> And if I'm not wrong we've already discussed it somewhere in previous 
> versions.

The original use of ?: stems from my suggestion on an earlier revision 
when we were still trying to pack everything into two consecutive 
declaration lines, rather than a declaration and a statement (as ?: is 
necessary for conditionals in declarations).  But since then, we decided 
to go with a statement (we require a C99 compiler, so declaration after 
statement is supported by our compiler, even if our coding style 
currently avoids it where possible), so as long as we support 
statements, we might as well go with a legible statement instead of 
insisting on the compact ?: form.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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

Re: [Xen-devel] [PATCH v7 02/11] error: auto propagated local_err

[Markus Armbruster]

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

> 21.02.2020 12:19, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
>>> functions with an errp OUT parameter.
>>>
>>> It has three goals:
>>>
>>> 1. Fix issue with error_fatal and error_prepend/error_append_hint: user
>>> can't see this additional information, because exit() happens in
>>> error_setg earlier than information is added. [Reported by Greg Kurz]
>>>
>>> 2. Fix issue with error_abort and error_propagate: when we wrap
>>> error_abort by local_err+error_propagate, the resulting coredump will
>>> refer to error_propagate and not to the place where error happened.
>>> (the macro itself doesn't fix the issue, but it allows us to [3.] drop
>>> the local_err+error_propagate pattern, which will definitely fix the
>>> issue) [Reported by Kevin Wolf]
>>>
>>> 3. Drop local_err+error_propagate pattern, which is used to workaround
>>> void functions with errp parameter, when caller wants to know resulting
>>> status. (Note: actually these functions could be merely updated to
>>> return int error code).
>>>
>>> To achieve these goals, later patches will add invocations
>>> of this macro at the start of functions with either use
>>> error_prepend/error_append_hint (solving 1) or which use
>>> local_err+error_propagate to check errors, switching those
>>> functions to use *errp instead (solving 2 and 3).
>>>
>>> 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 | 83 +++++++++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 82 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>>> index d34987148d..b9452d4806 100644
>>> --- a/include/qapi/error.h
>>> +++ b/include/qapi/error.h
>>> @@ -78,7 +78,7 @@
>>>    * Call a function treating errors as fatal:
>>>    *     foo(arg, &error_fatal);
>>>    *
>>> - * Receive an error and pass it on to the caller:
>>> + * Receive an error and pass it on to the caller (DEPRECATED*):
>>>    *     Error *err = NULL;
>>>    *     foo(arg, &err);
>>>    *     if (err) {
>>> @@ -98,6 +98,50 @@
>>>    *     foo(arg, errp);
>>>    * for readability.
>>>    *
>>> + * DEPRECATED* This pattern is deprecated now, the use ERRP_AUTO_PROPAGATE macro
>>> + * instead (defined below).
>>> + * It's deprecated because of two things:
>>> + *
>>> + * 1. Issue with error_abort and error_propagate: when we wrap error_abort by
>>> + * local_err+error_propagate, the resulting coredump will refer to
>>> + * error_propagate and not to the place where error happened.
>>> + *
>>> + * 2. A lot of extra code of the same pattern
>>> + *
>>> + * How to update old code to use ERRP_AUTO_PROPAGATE?
>>> + *
>>> + * All you need is to add ERRP_AUTO_PROPAGATE() invocation at function start,
>>> + * than you may safely dereference errp to check errors and do not need any
>>> + * additional local Error variables or calls to error_propagate().
>>> + *
>>> + * Example:
>>> + *
>>> + * old code
>>> + *
>>> + *     void fn(..., Error **errp) {
>>> + *         Error *err = NULL;
>>> + *         foo(arg, &err);
>>> + *         if (err) {
>>> + *             handle the error...
>>> + *             error_propagate(errp, err);
>>> + *             return;
>>> + *         }
>>> + *         ...
>>> + *     }
>>> + *
>>> + * updated code
>>> + *
>>> + *     void fn(..., Error **errp) {
>>> + *         ERRP_AUTO_PROPAGATE();
>>> + *         foo(arg, errp);
>>> + *         if (*errp) {
>>> + *             handle the error...
>>> + *             return;
>>> + *         }
>>> + *         ...
>>> + *     }
>>> + *
>>> + *
>>>    * Receive and accumulate multiple errors (first one wins):
>>>    *     Error *err = NULL, *local_err = NULL;
>>>    *     foo(arg, &err);
>>
>> Let's explain what should be done *first*, and only then talk about the
>> deprecated pattern and how to convert it to current usage.
>>
>>> @@ -348,6 +392,43 @@ void error_set_internal(Error **errp,
>>>                           ErrorClass err_class, const char *fmt, ...)
>>>       GCC_FMT_ATTR(6, 7);
>>>   +typedef struct ErrorPropagator {
>>> +    Error *local_err;
>>> +    Error **errp;
>>> +} ErrorPropagator;
>>> +
>>> +static inline void error_propagator_cleanup(ErrorPropagator *prop)
>>> +{
>>> +    error_propagate(prop->errp, prop->local_err);
>>> +}
>>> +
>>> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
>>> +
>>> +/*
>>> + * ERRP_AUTO_PROPAGATE
>>> + *
>>> + * This macro is created to be the first line of a function which use
>>> + * Error **errp parameter to report error. It's needed only in cases where we
>>> + * want to use error_prepend, error_append_hint or dereference *errp. It's
>>> + * still safe (but useless) in other cases.
>>> + *
>>> + * If errp is NULL or points to error_fatal, it is rewritten to point to a
>>> + * local Error object, which will be automatically propagated to the original
>>> + * errp on function exit (see error_propagator_cleanup).
>>> + *
>>> + * After invocation of this macro it is always safe to dereference errp
>>> + * (as it's not NULL anymore) and to add information by error_prepend or
>>> + * error_append_hint (as, if it was error_fatal, we swapped it with a
>>> + * local_error to be propagated on cleanup).
>>> + *
>>> + * Note: we don't wrap the error_abort case, as we want resulting coredump
>>> + * to point to the place where the error happened, not to error_propagate.
>>
>> Tradeoff: we gain more useful backtraces, we lose message improvements
>> from error_prepend(), error_append_hint() and such, if any.  Makes
>> sense.
>>
>>> + */
>>
>> The comment's contents looks okay to me.  I'll want to tweak formatting
>> to better blend in with the rest of this file, but let's not worry about
>> that now.
>>
>>> +#define ERRP_AUTO_PROPAGATE()                                  \
>>> +    g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp};  \
>>> +    errp = ((errp == NULL || *errp == error_fatal)             \
>>> +            ? &_auto_errp_prop.local_err : errp)
>>> +
>>>   /*
>>>    * Special error destination to abort on error.
>>>    * See error_setg() and error_propagate() for details.
>>
>> *errp == error_fatal tests *errp == NULL, which is not what you want.
>> You need to test errp == &error_fatal, just like error_handle_fatal().
>
> Oops, great bug) And nobody noticed before) Of course, you are right.

That's why we review patches :)

>> Superfluous parenthesis around the first operand of ?:.
>>
>> Wouldn't
>>
>>     #define ERRP_AUTO_PROPAGATE()                                  \
>>         g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp};  \
>>         if (!errp || errp == &error_fatal) {                       \
>>             errp = &_auto_errp_prop.local_err;                     \
>>         }
>>
>> be clearer?
>>
>
> Hmm, notation with "if" will allow omitting ';' after macro invocation, which seems not good..
> And if I'm not wrong we've already discussed it somewhere in previous versions.

Nevermind.  We'd have to add do ... while semicolon armor, and then it's
hardly clearer anymore.

> Still, no objections for s/errp == NULL/!errp/ and we need s/*errp == error_fatal/errp == &error_fatal/ for sure.

Okay!


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

[Xen-devel] [PATCH v7 03/11] scripts: add coccinelle script to use auto propagated errp

[Vladimir Sementsov-Ogievskiy]

Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
does corresponding changes in code (look for details in
include/qapi/error.h)

Usage example:
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
 --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
 blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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                          |   3 +
 scripts/coccinelle/auto-propagated-errp.cocci | 158 ++++++++++++++++++
 2 files changed, 161 insertions(+)
 create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci

diff --git a/include/qapi/error.h b/include/qapi/error.h
index b9452d4806..79f8e95214 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -141,6 +141,9 @@
  *         ...
  *     }
  *
+ * For mass conversion use script
+ *   scripts/coccinelle/auto-propagated-errp.cocci
+ *
  *
  * Receive and accumulate multiple errors (first one wins):
  *     Error *err = NULL, *local_err = NULL;
diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
new file mode 100644
index 0000000000..fb03c871cb
--- /dev/null
+++ b/scripts/coccinelle/auto-propagated-errp.cocci
@@ -0,0 +1,158 @@
+// Use ERRP_AUTO_PROPAGATE (see include/qapi/error.h)
+//
+// Copyright (c) 2020 Virtuozzo International GmbH.
+//
+// This program is free software; you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation; either version 2 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+//
+// Usage example:
+// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
+//  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
+//  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
+
+@rule0@
+// Add invocation to errp-functions where necessary
+// We should skip functions with "Error *const *errp"
+// parameter, but how to do it with coccinelle?
+// I don't know, so, I skip them by function name regex.
+// It's safe: if we did not skip some functions with
+// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
+// will fail to compile, because of const violation.
+identifier fn !~ "error_append_.*_hint";
+identifier local_err, ERRP;
+@@
+
+ fn(..., Error **ERRP, ...)
+ {
++   ERRP_AUTO_PROPAGATE();
+    <+...
+        when != ERRP_AUTO_PROPAGATE();
+(
+    error_append_hint(ERRP, ...);
+|
+    error_prepend(ERRP, ...);
+|
+    Error *local_err = NULL;
+)
+    ...+>
+ }
+
+@@
+// Switch unusual (Error **) parameter names to errp
+// (this is necessary to use ERRP_AUTO_PROPAGATE).
+identifier rule0.fn;
+identifier rule0.ERRP != errp;
+@@
+
+ fn(...,
+-   Error **ERRP
++   Error **errp
+    ,...)
+ {
+     <...
+-    ERRP
++    errp
+     ...>
+ }
+
+@rule1@
+// We want to patch error propagation in functions regardless of
+// whether the function already uses ERRP_AUTO_PROPAGATE prior to
+// applying rule0, hence this one does not inherit from it.
+identifier fn !~ "error_append_.*_hint";
+identifier local_err;
+symbol errp;
+@@
+
+ fn(..., Error **errp, ...)
+ {
+     <...
+-    Error *local_err = NULL;
+     ...>
+ }
+
+@@
+// Handle pattern with goto, otherwise we'll finish up
+// with labels at function end which will not compile.
+identifier rule1.fn, rule1.local_err;
+identifier OUT;
+@@
+
+ fn(...)
+ {
+     <...
+-    goto OUT;
++    return;
+     ...>
+- OUT:
+-    error_propagate(errp, local_err);
+ }
+
+@@
+identifier rule1.fn, rule1.local_err;
+expression list args; // to reindent error_propagate_prepend
+@@
+
+ fn(...)
+ {
+     <...
+(
+-    error_free(local_err);
+-    local_err = NULL;
++    error_free_errp(errp);
+|
+-    error_free(local_err);
++    error_free_errp(errp);
+|
+-    error_report_err(local_err);
++    error_report_errp(errp);
+|
+-    warn_report_err(local_err);
++    warn_report_errp(errp);
+|
+-    error_propagate_prepend(errp, local_err, args);
++    error_prepend(errp, args);
+|
+-    error_propagate(errp, local_err);
+)
+     ...>
+ }
+
+@@
+identifier rule1.fn, rule1.local_err;
+@@
+
+ fn(...)
+ {
+     <...
+(
+-    &local_err
++    errp
+|
+-    local_err
++    *errp
+)
+     ...>
+ }
+
+@@
+identifier rule1.fn;
+@@
+
+ fn(...)
+ {
+     <...
+- *errp != NULL
++ *errp
+     ...>
+ }
-- 
2.21.0


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

Re: [Xen-devel] [PATCH v7 03/11] scripts: add coccinelle script to use auto propagated errp

[Markus Armbruster]

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

> Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
> does corresponding changes in code (look for details in
> include/qapi/error.h)
>
> Usage example:
> spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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                          |   3 +
>  scripts/coccinelle/auto-propagated-errp.cocci | 158 ++++++++++++++++++
>  2 files changed, 161 insertions(+)
>  create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index b9452d4806..79f8e95214 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -141,6 +141,9 @@
>   *         ...
>   *     }
>   *
> + * For mass conversion use script
> + *   scripts/coccinelle/auto-propagated-errp.cocci
> + *
>   *
>   * Receive and accumulate multiple errors (first one wins):
>   *     Error *err = NULL, *local_err = NULL;

Extra blank line.

> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
> new file mode 100644
> index 0000000000..fb03c871cb
> --- /dev/null
> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
> @@ -0,0 +1,158 @@
> +// Use ERRP_AUTO_PROPAGATE (see include/qapi/error.h)
> +//
> +// Copyright (c) 2020 Virtuozzo International GmbH.
> +//
> +// This program is free software; you can redistribute it and/or modify
> +// it under the terms of the GNU General Public License as published by
> +// the Free Software Foundation; either version 2 of the License, or
> +// (at your option) any later version.
> +//
> +// This program is distributed in the hope that it will be useful,
> +// but WITHOUT ANY WARRANTY; without even the implied warranty of
> +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +// GNU General Public License for more details.
> +//
> +// You should have received a copy of the GNU General Public License
> +// along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +//
> +// Usage example:
> +// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
> +//  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
> +//  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
> +
> +@rule0@
> +// Add invocation to errp-functions where necessary
> +// We should skip functions with "Error *const *errp"
> +// parameter, but how to do it with coccinelle?
> +// I don't know, so, I skip them by function name regex.
> +// It's safe: if we did not skip some functions with
> +// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
> +// will fail to compile, because of const violation.

Not skipping a function we should skip fails to compile.

What about skipping a function we should not skip?

> +identifier fn !~ "error_append_.*_hint";
> +identifier local_err, ERRP;

A few of our coccinelle scripts use ALL_CAPS for meta-variables.  Most
don't.  Either is fine with me.  Mixing the two styles feels a bit
confusing, though.

> +@@
> +
> + fn(..., Error **ERRP, ...)
> + {
> ++   ERRP_AUTO_PROPAGATE();
> +    <+...
> +        when != ERRP_AUTO_PROPAGATE();
> +(
> +    error_append_hint(ERRP, ...);
> +|
> +    error_prepend(ERRP, ...);
> +|
> +    Error *local_err = NULL;
> +)
> +    ...+>
> + }

Misses error_vprepend().  Currently harmless, but as long as we commit
the script, we better make it as robust as we reasonably can.

The previous patch explains this Coccinelle script's intent:

  To achieve these goals, later patches will add invocations
  of this macro at the start of functions with either use
  error_prepend/error_append_hint (solving 1) or which use
  local_err+error_propagate to check errors, switching those
  functions to use *errp instead (solving 2 and 3).

This rule matches "use error_prepend/error_append_hint" directly.  It
appears to use presence of a local Error * variable as proxy for "use
local_err+error_propagate to check errors".  Hmm.

We obviously have such a variable when we use "local_err+error_propagate
to check errors".  But we could also have such variables without use of
error_propagate().  In fact, error.h documents such use:

 * Call a function and receive an error from it:
 *     Error *err = NULL;
 *     foo(arg, &err);
 *     if (err) {
 *         handle the error...
 *     }

where "handle the error" frees it.

I figure such uses typically occur in functions without an Error **errp
parameter.  This rule doesn't apply then.  But they could occur even in
functions with such a parameter.  Consider:

    void foo(Error **errp)
    {
        Error *err = NULL;

        bar(&err);
        if (err) {
            error_free(err);
            error_setg(errp, "completely different error");
        }
    }

Reasonable enough when bar() gives us an error that's misleading in this
context, isn't it?

The script transforms it like this:

    void foo(Error **errp)
    {
   -    Error *err = NULL;
   +    ERRP_AUTO_PROPAGATE();

   -    bar(&err);
   -    if (err) {
   -        error_free(err);
   +    bar(errp);
   +    if (*errp) {
   +        error_free_errp(errp);
            error_setg(errp, "completely different error");
        }
    }

Unwanted.

Now, if this script applied in just a few dozen places, we could rely on
eyeballing its output to catch unwanted transformations.  Since it
applies in so many more, I don't feel comfortable relying on reviewer
eyeballs.

Can we make rule0 directly match error_propagate(errp, local_err)
somehow?

Another observation: the rule does not match error_reportf_err() and
warn_reportf_err().  These combine error_prepend(),
error_report()/warn_report() and error_free(), for convenience.  Don't
their users need ERRP_AUTO_PROPAGATE() just like error_prepend()'s
users?

> +
> +@@
> +// Switch unusual (Error **) parameter names to errp
> +// (this is necessary to use ERRP_AUTO_PROPAGATE).

Please put your rule comments right before the rule, i.e. before the
@-line introducing metavariable declarations, not after.  Same
elsewhere.

> +identifier rule0.fn;
> +identifier rule0.ERRP != errp;
> +@@
> +
> + fn(...,
> +-   Error **ERRP
> ++   Error **errp
> +    ,...)
> + {
> +     <...
> +-    ERRP
> ++    errp
> +     ...>
> + }

This normalizes errp parameter naming.  It matches exactly when rule0
matches (and inserts ERRP_AUTO_PROPAGATE()) and the Error ** parameter
is unusual.  Good.

> +
> +@rule1@
> +// We want to patch error propagation in functions regardless of
> +// whether the function already uses ERRP_AUTO_PROPAGATE prior to
> +// applying rule0, hence this one does not inherit from it.

I'm not sure I get this comment.  Let's see what the rule does.

> +identifier fn !~ "error_append_.*_hint";
> +identifier local_err;
> +symbol errp;
> +@@
> +
> + fn(..., Error **errp, ...)
> + {
> +     <...
> +-    Error *local_err = NULL;
> +     ...>
> + }

rule1 matches like rule0, except the Error ** parameter match is
tightened from any C identifier to the C identifier errp, and the
function body match tightened from "either use
error_prepend/error_append_hint or which use local_err+error_propagate
to check errors" to just the latter.

I figure tightening the Error ** parameter match has no effect, because
we already normalized the parameter name.

So rule1 deletes variable local_err where rule0 applied.  Correct?

> +
> +@@
> +// Handle pattern with goto, otherwise we'll finish up
> +// with labels at function end which will not compile.
> +identifier rule1.fn, rule1.local_err;
> +identifier OUT;
> +@@
> +
> + fn(...)
> + {
> +     <...
> +-    goto OUT;
> ++    return;
> +     ...>
> +- OUT:
> +-    error_propagate(errp, local_err);
> + }

This is one special case of error_propagate() deletion.  It additionally
gets rid of a goto we no longer want.  For the general case, see below.

The rule applies only where rule1 just deleted the variable.  Thus, the
two rules work in tandem.  Makes sense.

> +
> +@@
> +identifier rule1.fn, rule1.local_err;

This rule also works in tandem with rule1.

> +expression list args; // to reindent error_propagate_prepend

What is the comment trying to tell me?

> +@@
> +
> + fn(...)
> + {
> +     <...
> +(
> +-    error_free(local_err);
> +-    local_err = NULL;
> ++    error_free_errp(errp);

Reminder:

    static inline void error_free_errp(Error **errp)
    {
        assert(errp && *errp);
        error_free(*errp);
        *errp = NULL;
    }

Now let's examine the actual change.

The assertion's first half trivially holds, ERRP_AUTO_PROPAGATE()
ensures it.

The second half is new.  We now crash when we haven't set an error.  Why
is this safe?  Note that error_free(local_err) does nothing when
!local_err.

The zapping of the variable pointing to the Error just freed is
unchanged.

> +|
> +-    error_free(local_err);
> ++    error_free_errp(errp);

Here, the zapping is new.  Zapping dangling pointers is obviously safe.
Needed, or else the automatic error_propagate() due to
ERRP_AUTO_PROPAGATE() would propagate the dangling pointer.

> +|
> +-    error_report_err(local_err);
> ++    error_report_errp(errp);

The only difference to the previous case is that we also report the
error.

The previous case has a buddy that additionally matches *errp = NULL.
Why not this one?

> +|
> +-    warn_report_err(local_err);
> ++    warn_report_errp(errp);

Likewise.

What about error_reportf_err(), warn_reportf_err()?

Up to here, this rule transforms the various forms of error_free().
Next: error_propagate().

> +|
> +-    error_propagate_prepend(errp, local_err, args);
> ++    error_prepend(errp, args);
> +|
> +-    error_propagate(errp, local_err);

rule0's adding of ERRP_AUTO_PROPAGATE() made error_propagate()
redundant.

This is the general case of error_propagate() deletion.

I'd put the plain error_propagate() first, variations second, like you
do with error_free().

If neither of these two patterns match on a path from
ERRP_AUTO_PROPAGATE() to return, we effectively insert error_propagate()
where it wasn't before.  Does nothing when the local error is null
there.  Bug fix when it isn't: it's at least a memory leak, and quite
possibly worse.

Identifying these bug fixes would be nice, but I don't have practical
ideas on how to do that.

Can we explain this in the commit message?

> +)
> +     ...>
> + }
> +
> +@@
> +identifier rule1.fn, rule1.local_err;
> +@@
> +
> + fn(...)
> + {
> +     <...
> +(
> +-    &local_err
> ++    errp
> +|
> +-    local_err
> ++    *errp
> +)
> +     ...>
> + }

Also in tandem with rule1, fixes up uses of local_err.  Good.

> +
> +@@
> +identifier rule1.fn;
> +@@
> +
> + fn(...)
> + {
> +     <...
> +- *errp != NULL
> ++ *errp
> +     ...>
> + }

Still in tandem with rule1, normalizes style.  Good.


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

Re: [Xen-devel] [PATCH v7 03/11] scripts: add coccinelle script to use auto propagated errp

[Vladimir Sementsov-Ogievskiy]

23.02.2020 11:55, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
>> does corresponding changes in code (look for details in
>> include/qapi/error.h)
>>
>> Usage example:
>> spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>   --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>   blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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                          |   3 +
>>   scripts/coccinelle/auto-propagated-errp.cocci | 158 ++++++++++++++++++
>>   2 files changed, 161 insertions(+)
>>   create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
>>
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index b9452d4806..79f8e95214 100644
>> --- a/include/qapi/error.h
>> +++ b/include/qapi/error.h
>> @@ -141,6 +141,9 @@
>>    *         ...
>>    *     }
>>    *
>> + * For mass conversion use script
>> + *   scripts/coccinelle/auto-propagated-errp.cocci
>> + *
>>    *
>>    * Receive and accumulate multiple errors (first one wins):
>>    *     Error *err = NULL, *local_err = NULL;
> 
> Extra blank line.
> 
>> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
>> new file mode 100644
>> index 0000000000..fb03c871cb
>> --- /dev/null
>> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
>> @@ -0,0 +1,158 @@
>> +// Use ERRP_AUTO_PROPAGATE (see include/qapi/error.h)
>> +//
>> +// Copyright (c) 2020 Virtuozzo International GmbH.
>> +//
>> +// This program is free software; you can redistribute it and/or modify
>> +// it under the terms of the GNU General Public License as published by
>> +// the Free Software Foundation; either version 2 of the License, or
>> +// (at your option) any later version.
>> +//
>> +// This program is distributed in the hope that it will be useful,
>> +// but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +// GNU General Public License for more details.
>> +//
>> +// You should have received a copy of the GNU General Public License
>> +// along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +//
>> +// Usage example:
>> +// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>> +//  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>> +//  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>> +
>> +@rule0@
>> +// Add invocation to errp-functions where necessary
>> +// We should skip functions with "Error *const *errp"
>> +// parameter, but how to do it with coccinelle?
>> +// I don't know, so, I skip them by function name regex.
>> +// It's safe: if we did not skip some functions with
>> +// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
>> +// will fail to compile, because of const violation.
> 
> Not skipping a function we should skip fails to compile.
> 
> What about skipping a function we should not skip?

Then it will not be updated.. Not good but I don't have better solution.
Still, I hope, function called *error_append_*_hint will not return error
through errp pointer.

> 
>> +identifier fn !~ "error_append_.*_hint";
>> +identifier local_err, ERRP;
> 
> A few of our coccinelle scripts use ALL_CAPS for meta-variables.  Most
> don't.  Either is fine with me.  Mixing the two styles feels a bit
> confusing, though.
> 
>> +@@
>> +
>> + fn(..., Error **ERRP, ...)
>> + {
>> ++   ERRP_AUTO_PROPAGATE();
>> +    <+...
>> +        when != ERRP_AUTO_PROPAGATE();
>> +(
>> +    error_append_hint(ERRP, ...);
>> +|
>> +    error_prepend(ERRP, ...);
>> +|
>> +    Error *local_err = NULL;
>> +)
>> +    ...+>
>> + }
> 
> Misses error_vprepend().  Currently harmless, but as long as we commit
> the script, we better make it as robust as we reasonably can.
> 
> The previous patch explains this Coccinelle script's intent:
> 
>    To achieve these goals, later patches will add invocations
>    of this macro at the start of functions with either use
>    error_prepend/error_append_hint (solving 1) or which use
>    local_err+error_propagate to check errors, switching those
>    functions to use *errp instead (solving 2 and 3).
> 
> This rule matches "use error_prepend/error_append_hint" directly.  It
> appears to use presence of a local Error * variable as proxy for "use
> local_err+error_propagate to check errors".  Hmm.
> 
> We obviously have such a variable when we use "local_err+error_propagate
> to check errors".  But we could also have such variables without use of
> error_propagate().  In fact, error.h documents such use:
> 
>   * Call a function and receive an error from it:
>   *     Error *err = NULL;
>   *     foo(arg, &err);
>   *     if (err) {
>   *         handle the error...
>   *     }
> 
> where "handle the error" frees it.
> 
> I figure such uses typically occur in functions without an Error **errp
> parameter.  This rule doesn't apply then.  But they could occur even in
> functions with such a parameter.  Consider:
> 
>      void foo(Error **errp)
>      {
>          Error *err = NULL;
> 
>          bar(&err);
>          if (err) {
>              error_free(err);
>              error_setg(errp, "completely different error");
>          }
>      }
> 
> Reasonable enough when bar() gives us an error that's misleading in this
> context, isn't it?
> 
> The script transforms it like this:
> 
>      void foo(Error **errp)
>      {
>     -    Error *err = NULL;
>     +    ERRP_AUTO_PROPAGATE();
> 
>     -    bar(&err);
>     -    if (err) {
>     -        error_free(err);
>     +    bar(errp);
>     +    if (*errp) {
>     +        error_free_errp(errp);
>              error_setg(errp, "completely different error");
>          }
>      }
> 
> Unwanted.

What is the problem with it? Updated code just use "new usual notation"
for handling error of sub-calls in function which reports errors through
errp pointer.

> 
> Now, if this script applied in just a few dozen places, we could rely on
> eyeballing its output to catch unwanted transformations.  Since it
> applies in so many more, I don't feel comfortable relying on reviewer
> eyeballs.
> 
> Can we make rule0 directly match error_propagate(errp, local_err)
> somehow?

I think it is possible, still I'm not sure we need it.

> 
> Another observation: the rule does not match error_reportf_err() and
> warn_reportf_err().  These combine error_prepend(),
> error_report()/warn_report() and error_free(), for convenience.  Don't
> their users need ERRP_AUTO_PROPAGATE() just like error_prepend()'s
> users?

Right. These functions want to add information, which will not work
for error_fatal without wrapping.

> 
>> +
>> +@@
>> +// Switch unusual (Error **) parameter names to errp
>> +// (this is necessary to use ERRP_AUTO_PROPAGATE).
> 
> Please put your rule comments right before the rule, i.e. before the
> @-line introducing metavariable declarations, not after.  Same
> elsewhere.
> 
>> +identifier rule0.fn;
>> +identifier rule0.ERRP != errp;
>> +@@
>> +
>> + fn(...,
>> +-   Error **ERRP
>> ++   Error **errp
>> +    ,...)
>> + {
>> +     <...
>> +-    ERRP
>> ++    errp
>> +     ...>
>> + }
> 
> This normalizes errp parameter naming.  It matches exactly when rule0
> matches (and inserts ERRP_AUTO_PROPAGATE()) and the Error ** parameter
> is unusual.  Good.
> 
>> +
>> +@rule1@
>> +// We want to patch error propagation in functions regardless of
>> +// whether the function already uses ERRP_AUTO_PROPAGATE prior to
>> +// applying rule0, hence this one does not inherit from it.
> 
> I'm not sure I get this comment.  Let's see what the rule does.
> 
>> +identifier fn !~ "error_append_.*_hint";
>> +identifier local_err;
>> +symbol errp;
>> +@@
>> +
>> + fn(..., Error **errp, ...)
>> + {
>> +     <...
>> +-    Error *local_err = NULL;
>> +     ...>
>> + }
> 
> rule1 matches like rule0, except the Error ** parameter match is
> tightened from any C identifier to the C identifier errp, and the
> function body match tightened from "either use
> error_prepend/error_append_hint or which use local_err+error_propagate
> to check errors" to just the latter.
> 
> I figure tightening the Error ** parameter match has no effect, because
> we already normalized the parameter name.
> 
> So rule1 deletes variable local_err where rule0 applied.  Correct?

The difference with rule0 is that rule0 contains
  "when != ERRP_AUTO_PROPAGATE()", so rule0 is not applied where
we already have macro invocation.

This is why we can't inherit from rule0.

No we believe that we have ERRP_AUTO_PROPAGATE invocation in all
corresponding places (added by rule0 or before script run) and want to
update all usage of local_err objects.

> 
>> +
>> +@@
>> +// Handle pattern with goto, otherwise we'll finish up
>> +// with labels at function end which will not compile.
>> +identifier rule1.fn, rule1.local_err;
>> +identifier OUT;
>> +@@
>> +
>> + fn(...)
>> + {
>> +     <...
>> +-    goto OUT;
>> ++    return;
>> +     ...>
>> +- OUT:
>> +-    error_propagate(errp, local_err);
>> + }
> 
> This is one special case of error_propagate() deletion.  It additionally
> gets rid of a goto we no longer want.  For the general case, see below.
> 
> The rule applies only where rule1 just deleted the variable.  Thus, the
> two rules work in tandem.  Makes sense.
> 
>> +
>> +@@
>> +identifier rule1.fn, rule1.local_err;
> 
> This rule also works in tandem with rule1.
> 
>> +expression list args; // to reindent error_propagate_prepend
> 
> What is the comment trying to tell me?

Hmm, we can safely drop it. It's about the following:

instead of

  -    error_propagate_prepend(errp, local_err, args);
  +    error_prepend(errp, args);

we can use "...", like

  - error_propagate_prepend(errp, local_err
  + error_prepend(errp
    , ...);

but with metavar in use, coccinelle will correctly reindent the
whole call, which looks a lot better.

> 
>> +@@
>> +
>> + fn(...)
>> + {
>> +     <...
>> +(
>> +-    error_free(local_err);
>> +-    local_err = NULL;
>> ++    error_free_errp(errp);
> 
> Reminder:
> 
>      static inline void error_free_errp(Error **errp)
>      {
>          assert(errp && *errp);
>          error_free(*errp);
>          *errp = NULL;
>      }
> 
> Now let's examine the actual change.
> 
> The assertion's first half trivially holds, ERRP_AUTO_PROPAGATE()
> ensures it.
> 
> The second half is new.  We now crash when we haven't set an error.  Why
> is this safe?  Note that error_free(local_err) does nothing when
> !local_err.

Hmm. Looks like we should tighten this restriction, and follow error_free
interface, which allows freeing unset errp.

> 
> The zapping of the variable pointing to the Error just freed is
> unchanged.
> 
>> +|
>> +-    error_free(local_err);
>> ++    error_free_errp(errp);
> 
> Here, the zapping is new.  Zapping dangling pointers is obviously safe.
> Needed, or else the automatic error_propagate() due to
> ERRP_AUTO_PROPAGATE() would propagate the dangling pointer.
> 
>> +|
>> +-    error_report_err(local_err);
>> ++    error_report_errp(errp);
> 
> The only difference to the previous case is that we also report the
> error.
> 
> The previous case has a buddy that additionally matches *errp = NULL.
> Why not this one?

Probably because no matches in code. But should be added here for
better case coverage.

> 
>> +|
>> +-    warn_report_err(local_err);
>> ++    warn_report_errp(errp);
> 
> Likewise.
> 
> What about error_reportf_err(), warn_reportf_err()?
> 
> Up to here, this rule transforms the various forms of error_free().
> Next: error_propagate().
> 
>> +|
>> +-    error_propagate_prepend(errp, local_err, args);
>> ++    error_prepend(errp, args);
>> +|
>> +-    error_propagate(errp, local_err);
> 
> rule0's adding of ERRP_AUTO_PROPAGATE() made error_propagate()
> redundant.
> 
> This is the general case of error_propagate() deletion.
> 
> I'd put the plain error_propagate() first, variations second, like you
> do with error_free().
> 
> If neither of these two patterns match on a path from
> ERRP_AUTO_PROPAGATE() to return, we effectively insert error_propagate()
> where it wasn't before.  Does nothing when the local error is null
> there.  Bug fix when it isn't: it's at least a memory leak, and quite
> possibly worse.

Hmm. How can it be memory leak after any of error_free variants?

> 
> Identifying these bug fixes would be nice, but I don't have practical
> ideas on how to do that.
> 
> Can we explain this in the commit message?
> 
>> +)
>> +     ...>
>> + }
>> +
>> +@@
>> +identifier rule1.fn, rule1.local_err;
>> +@@
>> +
>> + fn(...)
>> + {
>> +     <...
>> +(
>> +-    &local_err
>> ++    errp
>> +|
>> +-    local_err
>> ++    *errp
>> +)
>> +     ...>
>> + }
> 
> Also in tandem with rule1, fixes up uses of local_err.  Good.
> 
>> +
>> +@@
>> +identifier rule1.fn;
>> +@@
>> +
>> + fn(...)
>> + {
>> +     <...
>> +- *errp != NULL
>> ++ *errp
>> +     ...>
>> + }
> 
> Still in tandem with rule1, normalizes style.  Good.
> 


-- 
Best regards,
Vladimir

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

Re: [Xen-devel] [PATCH v7 03/11] scripts: add coccinelle script to use auto propagated errp

[Markus Armbruster]

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

> 23.02.2020 11:55, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
>>> does corresponding changes in code (look for details in
>>> include/qapi/error.h)
>>>
>>> Usage example:
>>> spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>>   --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>>   blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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                          |   3 +
>>>   scripts/coccinelle/auto-propagated-errp.cocci | 158 ++++++++++++++++++
>>>   2 files changed, 161 insertions(+)
>>>   create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
>>>
>>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>>> index b9452d4806..79f8e95214 100644
>>> --- a/include/qapi/error.h
>>> +++ b/include/qapi/error.h
>>> @@ -141,6 +141,9 @@
>>>    *         ...
>>>    *     }
>>>    *
>>> + * For mass conversion use script
>>> + *   scripts/coccinelle/auto-propagated-errp.cocci
>>> + *
>>>    *
>>>    * Receive and accumulate multiple errors (first one wins):
>>>    *     Error *err = NULL, *local_err = NULL;
>>
>> Extra blank line.
>>
>>> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
>>> new file mode 100644
>>> index 0000000000..fb03c871cb
>>> --- /dev/null
>>> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
>>> @@ -0,0 +1,158 @@
>>> +// Use ERRP_AUTO_PROPAGATE (see include/qapi/error.h)
>>> +//
>>> +// Copyright (c) 2020 Virtuozzo International GmbH.
>>> +//
>>> +// This program is free software; you can redistribute it and/or modify
>>> +// it under the terms of the GNU General Public License as published by
>>> +// the Free Software Foundation; either version 2 of the License, or
>>> +// (at your option) any later version.
>>> +//
>>> +// This program is distributed in the hope that it will be useful,
>>> +// but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +// GNU General Public License for more details.
>>> +//
>>> +// You should have received a copy of the GNU General Public License
>>> +// along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>> +//
>>> +// Usage example:
>>> +// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>> +//  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>> +//  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>> +
>>> +@rule0@
>>> +// Add invocation to errp-functions where necessary
>>> +// We should skip functions with "Error *const *errp"
>>> +// parameter, but how to do it with coccinelle?
>>> +// I don't know, so, I skip them by function name regex.
>>> +// It's safe: if we did not skip some functions with
>>> +// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
>>> +// will fail to compile, because of const violation.
>>
>> Not skipping a function we should skip fails to compile.
>>
>> What about skipping a function we should not skip?
>
> Then it will not be updated.. Not good but I don't have better solution.
> Still, I hope, function called *error_append_*_hint will not return error
> through errp pointer.

Seems likely.  I just dislike inferring behavior from name patterns.

Ideally, we recognize the true exceptional pattern instead, i.e. the
presence of const.  But figuring out how to make Coccinelle do that for
us may be more trouble than it's worth.

Hmm...  Coccinelle matches the parameter even with const due to what it
calls "isomorphism".  Can I disable it?  *Tinker* *tinker*

diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
index fb03c871cb..0c4414bff3 100644
--- a/scripts/coccinelle/auto-propagated-errp.cocci
+++ b/scripts/coccinelle/auto-propagated-errp.cocci
@@ -20,15 +20,11 @@
 //  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
 //  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
 
-@rule0@
+@rule0 disable optional_qualifier@
 // Add invocation to errp-functions where necessary
-// We should skip functions with "Error *const *errp"
-// parameter, but how to do it with coccinelle?
-// I don't know, so, I skip them by function name regex.
-// It's safe: if we did not skip some functions with
-// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
-// will fail to compile, because of const violation.
-identifier fn !~ "error_append_.*_hint";
+// Disable optional_qualifier to skip functions with "Error *const *errp"
+// parameter, 
+identifier fn;
 identifier local_err, ERRP;
 @@

Could you verify this results in the same tree-wide change as your
version?

>>> +identifier fn !~ "error_append_.*_hint";
>>> +identifier local_err, ERRP;
>>
>> A few of our coccinelle scripts use ALL_CAPS for meta-variables.  Most
>> don't.  Either is fine with me.  Mixing the two styles feels a bit
>> confusing, though.
>>
>>> +@@
>>> +
>>> + fn(..., Error **ERRP, ...)
>>> + {
>>> ++   ERRP_AUTO_PROPAGATE();
>>> +    <+...
>>> +        when != ERRP_AUTO_PROPAGATE();
>>> +(
>>> +    error_append_hint(ERRP, ...);
>>> +|
>>> +    error_prepend(ERRP, ...);
>>> +|
>>> +    Error *local_err = NULL;
>>> +)
>>> +    ...+>
>>> + }
>>
>> Misses error_vprepend().  Currently harmless, but as long as we commit
>> the script, we better make it as robust as we reasonably can.
>>
>> The previous patch explains this Coccinelle script's intent:
>>
>>    To achieve these goals, later patches will add invocations
>>    of this macro at the start of functions with either use
>>    error_prepend/error_append_hint (solving 1) or which use
>>    local_err+error_propagate to check errors, switching those
>>    functions to use *errp instead (solving 2 and 3).
>>
>> This rule matches "use error_prepend/error_append_hint" directly.  It
>> appears to use presence of a local Error * variable as proxy for "use
>> local_err+error_propagate to check errors".  Hmm.
>>
>> We obviously have such a variable when we use "local_err+error_propagate
>> to check errors".  But we could also have such variables without use of
>> error_propagate().  In fact, error.h documents such use:
>>
>>   * Call a function and receive an error from it:
>>   *     Error *err = NULL;
>>   *     foo(arg, &err);
>>   *     if (err) {
>>   *         handle the error...
>>   *     }
>>
>> where "handle the error" frees it.
>>
>> I figure such uses typically occur in functions without an Error **errp
>> parameter.  This rule doesn't apply then.  But they could occur even in
>> functions with such a parameter.  Consider:
>>
>>      void foo(Error **errp)
>>      {
>>          Error *err = NULL;
>>
>>          bar(&err);
>>          if (err) {
>>              error_free(err);
>>              error_setg(errp, "completely different error");
>>          }
>>      }
>>
>> Reasonable enough when bar() gives us an error that's misleading in this
>> context, isn't it?
>>
>> The script transforms it like this:
>>
>>      void foo(Error **errp)
>>      {
>>     -    Error *err = NULL;
>>     +    ERRP_AUTO_PROPAGATE();
>>
>>     -    bar(&err);
>>     -    if (err) {
>>     -        error_free(err);
>>     +    bar(errp);
>>     +    if (*errp) {
>>     +        error_free_errp(errp);
>>              error_setg(errp, "completely different error");
>>          }
>>      }
>>
>> Unwanted.
>
> What is the problem with it? Updated code just use "new usual notation"
> for handling error of sub-calls in function which reports errors through
> errp pointer.

error.h's big comment asks for use of ERRP_AUTO_PROPAGATE() to "Receive
an error and pass it on to the caller".  We're not doing that here.  We
"Call a function and receive an error from it", then "Handle an error
without reporting it".

The updated code works anyway, but it's needlessly complicated.

>> Now, if this script applied in just a few dozen places, we could rely on
>> eyeballing its output to catch unwanted transformations.  Since it
>> applies in so many more, I don't feel comfortable relying on reviewer
>> eyeballs.
>>
>> Can we make rule0 directly match error_propagate(errp, local_err)
>> somehow?
>
> I think it is possible, still I'm not sure we need it.

We don't need it in the sense of "must have to avoid a buggy
transformation".  It's more like "I'd like to have it to stay close to
the documented usage of ERRP_AUTO_PROPAGATE(), and to avoid complicating
cases like the one above".

>> Another observation: the rule does not match error_reportf_err() and
>> warn_reportf_err().  These combine error_prepend(),
>> error_report()/warn_report() and error_free(), for convenience.  Don't
>> their users need ERRP_AUTO_PROPAGATE() just like error_prepend()'s
>> users?
>
> Right. These functions want to add information, which will not work
> for error_fatal without wrapping.

A simple improvement, I hope.

>>> +
>>> +@@
>>> +// Switch unusual (Error **) parameter names to errp
>>> +// (this is necessary to use ERRP_AUTO_PROPAGATE).
>>
>> Please put your rule comments right before the rule, i.e. before the
>> @-line introducing metavariable declarations, not after.  Same
>> elsewhere.
>>
>>> +identifier rule0.fn;
>>> +identifier rule0.ERRP != errp;
>>> +@@
>>> +
>>> + fn(...,
>>> +-   Error **ERRP
>>> ++   Error **errp
>>> +    ,...)
>>> + {
>>> +     <...
>>> +-    ERRP
>>> ++    errp
>>> +     ...>
>>> + }
>>
>> This normalizes errp parameter naming.  It matches exactly when rule0
>> matches (and inserts ERRP_AUTO_PROPAGATE()) and the Error ** parameter
>> is unusual.  Good.
>>
>>> +
>>> +@rule1@
>>> +// We want to patch error propagation in functions regardless of
>>> +// whether the function already uses ERRP_AUTO_PROPAGATE prior to
>>> +// applying rule0, hence this one does not inherit from it.
>>
>> I'm not sure I get this comment.  Let's see what the rule does.
>>
>>> +identifier fn !~ "error_append_.*_hint";
>>> +identifier local_err;
>>> +symbol errp;
>>> +@@
>>> +
>>> + fn(..., Error **errp, ...)
>>> + {
>>> +     <...
>>> +-    Error *local_err = NULL;
>>> +     ...>
>>> + }
>>
>> rule1 matches like rule0, except the Error ** parameter match is
>> tightened from any C identifier to the C identifier errp, and the
>> function body match tightened from "either use
>> error_prepend/error_append_hint or which use local_err+error_propagate
>> to check errors" to just the latter.
>>
>> I figure tightening the Error ** parameter match has no effect, because
>> we already normalized the parameter name.
>>
>> So rule1 deletes variable local_err where rule0 applied.  Correct?
>
> The difference with rule0 is that rule0 contains
>  "when != ERRP_AUTO_PROPAGATE()", so rule0 is not applied where
> we already have macro invocation.

Ah, I missed the when clause.

> This is why we can't inherit from rule0.
>
> No we believe that we have ERRP_AUTO_PROPAGATE invocation in all
> corresponding places (added by rule0 or before script run) and want to
> update all usage of local_err objects.

Let's see whether I got it:

* The first rule (rule0) adds ERRP_AUTO_PROPAGATE() to all functions
  that take an Error ** parameter, and either pass it error_prepend() or
  error_append_hint(), or use local_err, and don't have
  ERRP_AUTO_PROPAGATE() already, except it skips the ones named
  error_append_FOO_hint().  Uff.

  The "use local_err" part is an approximation of "use local_err +
  error_propagate()".

  The "except for the ones named error_append_FOO_hint()" part is an
  approximation of "except for the ones taking an Error *const *
  parameter".

  ERRP_AUTO_PROPAGATE() requires the Error ** parameter to be named
  @errp, which need not be the case.  The next rule fixes it up:

* The second rule ensures the parameter is named @errp wherever the
  first rule applied, renaming if necessary.

  Correct?

  Incorrect transformation followed by fixup is not ideal, because it
  can trip up reviewers.  But ideal is too expensive; this is good
  enough.

* The third rule (rule1) ensures functions that take an Error **errp
  parameter don't declare local_err, except it skips the ones named
  error_append_FOO_hint().

  In isolation, this rule makes no sense.  To make sense of it, we need
  context:

  * Subsequent rules remove all uses of @errp from any function where
    rule1 matches.

  * Preceding rules ensure any function where rule1 matches has
    ERRP_AUTO_PROPAGATE().

  Correct?

  The need for this much context is hard on reviewers.  Good enough for
  transforming the tree now, but I'd hate having to make sense of this
  again in six months.

>>> +
>>> +@@
>>> +// Handle pattern with goto, otherwise we'll finish up
>>> +// with labels at function end which will not compile.
>>> +identifier rule1.fn, rule1.local_err;
>>> +identifier OUT;
>>> +@@
>>> +
>>> + fn(...)
>>> + {
>>> +     <...
>>> +-    goto OUT;
>>> ++    return;
>>> +     ...>
>>> +- OUT:
>>> +-    error_propagate(errp, local_err);
>>> + }
>>
>> This is one special case of error_propagate() deletion.  It additionally
>> gets rid of a goto we no longer want.  For the general case, see below.
>>
>> The rule applies only where rule1 just deleted the variable.  Thus, the
>> two rules work in tandem.  Makes sense.
>>
>>> +
>>> +@@
>>> +identifier rule1.fn, rule1.local_err;
>>
>> This rule also works in tandem with rule1.
>>
>>> +expression list args; // to reindent error_propagate_prepend
>>
>> What is the comment trying to tell me?
>
> Hmm, we can safely drop it. It's about the following:
>
> instead of
>
>  -    error_propagate_prepend(errp, local_err, args);
>  +    error_prepend(errp, args);
>
> we can use "...", like
>
>  - error_propagate_prepend(errp, local_err
>  + error_prepend(errp
>    , ...);
>
> but with metavar in use, coccinelle will correctly reindent the
> whole call, which looks a lot better.

Let's drop the comment.

>>> +@@
>>> +
>>> + fn(...)
>>> + {
>>> +     <...
>>> +(
>>> +-    error_free(local_err);
>>> +-    local_err = NULL;
>>> ++    error_free_errp(errp);
>>
>> Reminder:
>>
>>      static inline void error_free_errp(Error **errp)
>>      {
>>          assert(errp && *errp);
>>          error_free(*errp);
>>          *errp = NULL;
>>      }
>>
>> Now let's examine the actual change.
>>
>> The assertion's first half trivially holds, ERRP_AUTO_PROPAGATE()
>> ensures it.
>>
>> The second half is new.  We now crash when we haven't set an error.  Why
>> is this safe?  Note that error_free(local_err) does nothing when
>> !local_err.
>
> Hmm. Looks like we should tighten this restriction, and follow error_free
> interface, which allows freeing unset errp.
>
>>
>> The zapping of the variable pointing to the Error just freed is
>> unchanged.
>>
>>> +|
>>> +-    error_free(local_err);
>>> ++    error_free_errp(errp);
>>
>> Here, the zapping is new.  Zapping dangling pointers is obviously safe.
>> Needed, or else the automatic error_propagate() due to
>> ERRP_AUTO_PROPAGATE() would propagate the dangling pointer.
>>
>>> +|
>>> +-    error_report_err(local_err);
>>> ++    error_report_errp(errp);
>>
>> The only difference to the previous case is that we also report the
>> error.
>>
>> The previous case has a buddy that additionally matches *errp = NULL.
>> Why not this one?
>
> Probably because no matches in code. But should be added here for
> better case coverage.

Either that or a comment pointing out what's missing, and why, namely
because the pattern doesn't exist in the tree.

>>
>>> +|
>>> +-    warn_report_err(local_err);
>>> ++    warn_report_errp(errp);
>>
>> Likewise.
>>
>> What about error_reportf_err(), warn_reportf_err()?
>>
>> Up to here, this rule transforms the various forms of error_free().
>> Next: error_propagate().
>>
>>> +|
>>> +-    error_propagate_prepend(errp, local_err, args);
>>> ++    error_prepend(errp, args);
>>> +|
>>> +-    error_propagate(errp, local_err);
>>
>> rule0's adding of ERRP_AUTO_PROPAGATE() made error_propagate()
>> redundant.
>>
>> This is the general case of error_propagate() deletion.
>>
>> I'd put the plain error_propagate() first, variations second, like you
>> do with error_free().
>>
>> If neither of these two patterns match on a path from
>> ERRP_AUTO_PROPAGATE() to return, we effectively insert error_propagate()
>> where it wasn't before.  Does nothing when the local error is null
>> there.  Bug fix when it isn't: it's at least a memory leak, and quite
>> possibly worse.
>
> Hmm. How can it be memory leak after any of error_free variants?

Consider nfs_options_qdict_to_qapi() right before commit 54b7af4369a
fixed it:

    static BlockdevOptionsNfs *nfs_options_qdict_to_qapi(QDict *options,
                                                         Error **errp)
    {
        BlockdevOptionsNfs *opts = NULL;
        QObject *crumpled = NULL;
        Visitor *v;
        Error *local_err = NULL;

        crumpled = qdict_crumple(options, errp);
        if (crumpled == NULL) {
            return NULL;
        }

        v = qobject_input_visitor_new_keyval(crumpled);
        visit_type_BlockdevOptionsNfs(v, NULL, &opts, &local_err);
        visit_free(v);
        qobject_unref(crumpled);

        if (local_err) {
            return NULL;
        }

        return opts;
    }

When visit_type_BlockdevOptionsNfs() fails, we return null without
setting an error.  We also leak the error we got from
visit_type_BlockdevOptionsNfs().

Commit 54b7af4369a fixed this:

    --- a/block/nfs.c
    +++ b/block/nfs.c
    @@ -570,6 +570,7 @@ static BlockdevOptionsNfs *nfs_options_qdict_to_qapi(QDict *
    options,
         qobject_unref(crumpled);

         if (local_err) {
    +        error_propagate(errp, local_err);
             return NULL;
         }

If it was still broken, then your transformation would *also* fix it,
wouldn't it?

My point is: your transformation might fix actual bugs!

>> Identifying these bug fixes would be nice, but I don't have practical
>> ideas on how to do that.
>>
>> Can we explain this in the commit message?
>>
>>> +)
>>> +     ...>
>>> + }
>>> +
>>> +@@
>>> +identifier rule1.fn, rule1.local_err;
>>> +@@
>>> +
>>> + fn(...)
>>> + {
>>> +     <...
>>> +(
>>> +-    &local_err
>>> ++    errp
>>> +|
>>> +-    local_err
>>> ++    *errp
>>> +)
>>> +     ...>
>>> + }
>>
>> Also in tandem with rule1, fixes up uses of local_err.  Good.
>>
>>> +
>>> +@@
>>> +identifier rule1.fn;
>>> +@@
>>> +
>>> + fn(...)
>>> + {
>>> +     <...
>>> +- *errp != NULL
>>> ++ *errp
>>> +     ...>
>>> + }
>>
>> Still in tandem with rule1, normalizes style.  Good.
>>


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

Re: [Xen-devel] [PATCH v7 03/11] scripts: add coccinelle script to use auto propagated errp

[Vladimir Sementsov-Ogievskiy]

25.02.2020 15:52, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> 23.02.2020 11:55, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>
>>>> Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
>>>> does corresponding changes in code (look for details in
>>>> include/qapi/error.h)
>>>>
>>>> Usage example:
>>>> spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>>>    --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>>>    blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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                          |   3 +
>>>>    scripts/coccinelle/auto-propagated-errp.cocci | 158 ++++++++++++++++++
>>>>    2 files changed, 161 insertions(+)
>>>>    create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
>>>>
>>>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>>>> index b9452d4806..79f8e95214 100644
>>>> --- a/include/qapi/error.h
>>>> +++ b/include/qapi/error.h
>>>> @@ -141,6 +141,9 @@
>>>>     *         ...
>>>>     *     }
>>>>     *
>>>> + * For mass conversion use script
>>>> + *   scripts/coccinelle/auto-propagated-errp.cocci
>>>> + *
>>>>     *
>>>>     * Receive and accumulate multiple errors (first one wins):
>>>>     *     Error *err = NULL, *local_err = NULL;
>>>
>>> Extra blank line.
>>>
>>>> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
>>>> new file mode 100644
>>>> index 0000000000..fb03c871cb
>>>> --- /dev/null
>>>> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
>>>> @@ -0,0 +1,158 @@
>>>> +// Use ERRP_AUTO_PROPAGATE (see include/qapi/error.h)
>>>> +//
>>>> +// Copyright (c) 2020 Virtuozzo International GmbH.
>>>> +//
>>>> +// This program is free software; you can redistribute it and/or modify
>>>> +// it under the terms of the GNU General Public License as published by
>>>> +// the Free Software Foundation; either version 2 of the License, or
>>>> +// (at your option) any later version.
>>>> +//
>>>> +// This program is distributed in the hope that it will be useful,
>>>> +// but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>> +// GNU General Public License for more details.
>>>> +//
>>>> +// You should have received a copy of the GNU General Public License
>>>> +// along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>>> +//
>>>> +// Usage example:
>>>> +// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>>> +//  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>>> +//  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>>> +
>>>> +@rule0@
>>>> +// Add invocation to errp-functions where necessary
>>>> +// We should skip functions with "Error *const *errp"
>>>> +// parameter, but how to do it with coccinelle?
>>>> +// I don't know, so, I skip them by function name regex.
>>>> +// It's safe: if we did not skip some functions with
>>>> +// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
>>>> +// will fail to compile, because of const violation.
>>>
>>> Not skipping a function we should skip fails to compile.
>>>
>>> What about skipping a function we should not skip?
>>
>> Then it will not be updated.. Not good but I don't have better solution.
>> Still, I hope, function called *error_append_*_hint will not return error
>> through errp pointer.
> 
> Seems likely.  I just dislike inferring behavior from name patterns.
> 
> Ideally, we recognize the true exceptional pattern instead, i.e. the
> presence of const.  But figuring out how to make Coccinelle do that for
> us may be more trouble than it's worth.
> 
> Hmm...  Coccinelle matches the parameter even with const due to what it
> calls "isomorphism".  Can I disable it?  *Tinker* *tinker*
> 
> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
> index fb03c871cb..0c4414bff3 100644
> --- a/scripts/coccinelle/auto-propagated-errp.cocci
> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
> @@ -20,15 +20,11 @@
>   //  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>   //  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>   
> -@rule0@
> +@rule0 disable optional_qualifier@
>   // Add invocation to errp-functions where necessary
> -// We should skip functions with "Error *const *errp"
> -// parameter, but how to do it with coccinelle?
> -// I don't know, so, I skip them by function name regex.
> -// It's safe: if we did not skip some functions with
> -// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
> -// will fail to compile, because of const violation.
> -identifier fn !~ "error_append_.*_hint";
> +// Disable optional_qualifier to skip functions with "Error *const *errp"
> +// parameter,
> +identifier fn;
>   identifier local_err, ERRP;
>   @@
> 
> Could you verify this results in the same tree-wide change as your
> version?

Yes, no difference. Thanks!

> 
>>>> +identifier fn !~ "error_append_.*_hint";
>>>> +identifier local_err, ERRP;
>>>
>>> A few of our coccinelle scripts use ALL_CAPS for meta-variables.  Most
>>> don't.  Either is fine with me.  Mixing the two styles feels a bit
>>> confusing, though.
>>>
>>>> +@@
>>>> +
>>>> + fn(..., Error **ERRP, ...)
>>>> + {
>>>> ++   ERRP_AUTO_PROPAGATE();
>>>> +    <+...
>>>> +        when != ERRP_AUTO_PROPAGATE();
>>>> +(
>>>> +    error_append_hint(ERRP, ...);
>>>> +|
>>>> +    error_prepend(ERRP, ...);
>>>> +|
>>>> +    Error *local_err = NULL;
>>>> +)
>>>> +    ...+>
>>>> + }
>>>
>>> Misses error_vprepend().  Currently harmless, but as long as we commit
>>> the script, we better make it as robust as we reasonably can.
>>>
>>> The previous patch explains this Coccinelle script's intent:
>>>
>>>     To achieve these goals, later patches will add invocations
>>>     of this macro at the start of functions with either use
>>>     error_prepend/error_append_hint (solving 1) or which use
>>>     local_err+error_propagate to check errors, switching those
>>>     functions to use *errp instead (solving 2 and 3).
>>>
>>> This rule matches "use error_prepend/error_append_hint" directly.  It
>>> appears to use presence of a local Error * variable as proxy for "use
>>> local_err+error_propagate to check errors".  Hmm.
>>>
>>> We obviously have such a variable when we use "local_err+error_propagate
>>> to check errors".  But we could also have such variables without use of
>>> error_propagate().  In fact, error.h documents such use:
>>>
>>>    * Call a function and receive an error from it:
>>>    *     Error *err = NULL;
>>>    *     foo(arg, &err);
>>>    *     if (err) {
>>>    *         handle the error...
>>>    *     }
>>>
>>> where "handle the error" frees it.
>>>
>>> I figure such uses typically occur in functions without an Error **errp
>>> parameter.  This rule doesn't apply then.  But they could occur even in
>>> functions with such a parameter.  Consider:
>>>
>>>       void foo(Error **errp)
>>>       {
>>>           Error *err = NULL;
>>>
>>>           bar(&err);
>>>           if (err) {
>>>               error_free(err);
>>>               error_setg(errp, "completely different error");
>>>           }
>>>       }
>>>
>>> Reasonable enough when bar() gives us an error that's misleading in this
>>> context, isn't it?
>>>
>>> The script transforms it like this:
>>>
>>>       void foo(Error **errp)
>>>       {
>>>      -    Error *err = NULL;
>>>      +    ERRP_AUTO_PROPAGATE();
>>>
>>>      -    bar(&err);
>>>      -    if (err) {
>>>      -        error_free(err);
>>>      +    bar(errp);
>>>      +    if (*errp) {
>>>      +        error_free_errp(errp);
>>>               error_setg(errp, "completely different error");
>>>           }
>>>       }
>>>
>>> Unwanted.
>>
>> What is the problem with it? Updated code just use "new usual notation"
>> for handling error of sub-calls in function which reports errors through
>> errp pointer.
> 
> error.h's big comment asks for use of ERRP_AUTO_PROPAGATE() to "Receive
> an error and pass it on to the caller".  We're not doing that here.  We
> "Call a function and receive an error from it", then "Handle an error
> without reporting it".
> 
> The updated code works anyway, but it's needlessly complicated.

OK, will try.

> 
>>> Now, if this script applied in just a few dozen places, we could rely on
>>> eyeballing its output to catch unwanted transformations.  Since it
>>> applies in so many more, I don't feel comfortable relying on reviewer
>>> eyeballs.
>>>
>>> Can we make rule0 directly match error_propagate(errp, local_err)
>>> somehow?
>>
>> I think it is possible, still I'm not sure we need it.
> 
> We don't need it in the sense of "must have to avoid a buggy
> transformation".  It's more like "I'd like to have it to stay close to
> the documented usage of ERRP_AUTO_PROPAGATE(), and to avoid complicating
> cases like the one above".
> 
>>> Another observation: the rule does not match error_reportf_err() and
>>> warn_reportf_err().  These combine error_prepend(),
>>> error_report()/warn_report() and error_free(), for convenience.  Don't
>>> their users need ERRP_AUTO_PROPAGATE() just like error_prepend()'s
>>> users?
>>
>> Right. These functions want to add information, which will not work
>> for error_fatal without wrapping.
> 
> A simple improvement, I hope.
> 
>>>> +
>>>> +@@
>>>> +// Switch unusual (Error **) parameter names to errp
>>>> +// (this is necessary to use ERRP_AUTO_PROPAGATE).
>>>
>>> Please put your rule comments right before the rule, i.e. before the
>>> @-line introducing metavariable declarations, not after.  Same
>>> elsewhere.
>>>
>>>> +identifier rule0.fn;
>>>> +identifier rule0.ERRP != errp;
>>>> +@@
>>>> +
>>>> + fn(...,
>>>> +-   Error **ERRP
>>>> ++   Error **errp
>>>> +    ,...)
>>>> + {
>>>> +     <...
>>>> +-    ERRP
>>>> ++    errp
>>>> +     ...>
>>>> + }
>>>
>>> This normalizes errp parameter naming.  It matches exactly when rule0
>>> matches (and inserts ERRP_AUTO_PROPAGATE()) and the Error ** parameter
>>> is unusual.  Good.
>>>
>>>> +
>>>> +@rule1@
>>>> +// We want to patch error propagation in functions regardless of
>>>> +// whether the function already uses ERRP_AUTO_PROPAGATE prior to
>>>> +// applying rule0, hence this one does not inherit from it.
>>>
>>> I'm not sure I get this comment.  Let's see what the rule does.
>>>
>>>> +identifier fn !~ "error_append_.*_hint";
>>>> +identifier local_err;
>>>> +symbol errp;
>>>> +@@
>>>> +
>>>> + fn(..., Error **errp, ...)
>>>> + {
>>>> +     <...
>>>> +-    Error *local_err = NULL;
>>>> +     ...>
>>>> + }
>>>
>>> rule1 matches like rule0, except the Error ** parameter match is
>>> tightened from any C identifier to the C identifier errp, and the
>>> function body match tightened from "either use
>>> error_prepend/error_append_hint or which use local_err+error_propagate
>>> to check errors" to just the latter.
>>>
>>> I figure tightening the Error ** parameter match has no effect, because
>>> we already normalized the parameter name.
>>>
>>> So rule1 deletes variable local_err where rule0 applied.  Correct?
>>
>> The difference with rule0 is that rule0 contains
>>   "when != ERRP_AUTO_PROPAGATE()", so rule0 is not applied where
>> we already have macro invocation.
> 
> Ah, I missed the when clause.
> 
>> This is why we can't inherit from rule0.
>>
>> No we believe that we have ERRP_AUTO_PROPAGATE invocation in all
>> corresponding places (added by rule0 or before script run) and want to
>> update all usage of local_err objects.
> 
> Let's see whether I got it:
> 
> * The first rule (rule0) adds ERRP_AUTO_PROPAGATE() to all functions
>    that take an Error ** parameter, and either pass it error_prepend() or
>    error_append_hint(), or use local_err, and don't have
>    ERRP_AUTO_PROPAGATE() already, except it skips the ones named
>    error_append_FOO_hint().  Uff.
> 
>    The "use local_err" part is an approximation of "use local_err +
>    error_propagate()".
> 
>    The "except for the ones named error_append_FOO_hint()" part is an
>    approximation of "except for the ones taking an Error *const *
>    parameter".
> 
>    ERRP_AUTO_PROPAGATE() requires the Error ** parameter to be named
>    @errp, which need not be the case.  The next rule fixes it up:
> 
> * The second rule ensures the parameter is named @errp wherever the
>    first rule applied, renaming if necessary.
> 
>    Correct?
> 
>    Incorrect transformation followed by fixup is not ideal, because it
>    can trip up reviewers.  But ideal is too expensive; this is good
>    enough.
> 
> * The third rule (rule1) ensures functions that take an Error **errp
>    parameter don't declare local_err, except it skips the ones named
>    error_append_FOO_hint().
> 
>    In isolation, this rule makes no sense.  To make sense of it, we need
>    context:
> 
>    * Subsequent rules remove all uses of @errp from any function where

of local_err

>      rule1 matches.
> 
>    * Preceding rules ensure any function where rule1 matches has
>      ERRP_AUTO_PROPAGATE().
> 
>    Correct?

Oh, yes, everything is correct.

> 
>    The need for this much context is hard on reviewers.  Good enough for
>    transforming the tree now, but I'd hate having to make sense of this
>    again in six months.

Ohh, yes. Far from good design. I can try to reorder chunks a bit.

> 
>>>> +
>>>> +@@
>>>> +// Handle pattern with goto, otherwise we'll finish up
>>>> +// with labels at function end which will not compile.
>>>> +identifier rule1.fn, rule1.local_err;
>>>> +identifier OUT;
>>>> +@@
>>>> +
>>>> + fn(...)
>>>> + {
>>>> +     <...
>>>> +-    goto OUT;
>>>> ++    return;
>>>> +     ...>
>>>> +- OUT:
>>>> +-    error_propagate(errp, local_err);
>>>> + }
>>>
>>> This is one special case of error_propagate() deletion.  It additionally
>>> gets rid of a goto we no longer want.  For the general case, see below.
>>>
>>> The rule applies only where rule1 just deleted the variable.  Thus, the
>>> two rules work in tandem.  Makes sense.
>>>
>>>> +
>>>> +@@
>>>> +identifier rule1.fn, rule1.local_err;
>>>
>>> This rule also works in tandem with rule1.
>>>
>>>> +expression list args; // to reindent error_propagate_prepend
>>>
>>> What is the comment trying to tell me?
>>
>> Hmm, we can safely drop it. It's about the following:
>>
>> instead of
>>
>>   -    error_propagate_prepend(errp, local_err, args);
>>   +    error_prepend(errp, args);
>>
>> we can use "...", like
>>
>>   - error_propagate_prepend(errp, local_err
>>   + error_prepend(errp
>>     , ...);
>>
>> but with metavar in use, coccinelle will correctly reindent the
>> whole call, which looks a lot better.
> 
> Let's drop the comment.
> 
>>>> +@@
>>>> +
>>>> + fn(...)
>>>> + {
>>>> +     <...
>>>> +(
>>>> +-    error_free(local_err);
>>>> +-    local_err = NULL;
>>>> ++    error_free_errp(errp);
>>>
>>> Reminder:
>>>
>>>       static inline void error_free_errp(Error **errp)
>>>       {
>>>           assert(errp && *errp);
>>>           error_free(*errp);
>>>           *errp = NULL;
>>>       }
>>>
>>> Now let's examine the actual change.
>>>
>>> The assertion's first half trivially holds, ERRP_AUTO_PROPAGATE()
>>> ensures it.
>>>
>>> The second half is new.  We now crash when we haven't set an error.  Why
>>> is this safe?  Note that error_free(local_err) does nothing when
>>> !local_err.
>>
>> Hmm. Looks like we should tighten this restriction, and follow error_free
>> interface, which allows freeing unset errp.
>>
>>>
>>> The zapping of the variable pointing to the Error just freed is
>>> unchanged.
>>>
>>>> +|
>>>> +-    error_free(local_err);
>>>> ++    error_free_errp(errp);
>>>
>>> Here, the zapping is new.  Zapping dangling pointers is obviously safe.
>>> Needed, or else the automatic error_propagate() due to
>>> ERRP_AUTO_PROPAGATE() would propagate the dangling pointer.
>>>
>>>> +|
>>>> +-    error_report_err(local_err);
>>>> ++    error_report_errp(errp);
>>>
>>> The only difference to the previous case is that we also report the
>>> error.
>>>
>>> The previous case has a buddy that additionally matches *errp = NULL.
>>> Why not this one?
>>
>> Probably because no matches in code. But should be added here for
>> better case coverage.
> 
> Either that or a comment pointing out what's missing, and why, namely
> because the pattern doesn't exist in the tree.
> 
>>>
>>>> +|
>>>> +-    warn_report_err(local_err);
>>>> ++    warn_report_errp(errp);
>>>
>>> Likewise.
>>>
>>> What about error_reportf_err(), warn_reportf_err()?
>>>
>>> Up to here, this rule transforms the various forms of error_free().
>>> Next: error_propagate().
>>>
>>>> +|
>>>> +-    error_propagate_prepend(errp, local_err, args);
>>>> ++    error_prepend(errp, args);
>>>> +|
>>>> +-    error_propagate(errp, local_err);
>>>
>>> rule0's adding of ERRP_AUTO_PROPAGATE() made error_propagate()
>>> redundant.
>>>
>>> This is the general case of error_propagate() deletion.
>>>
>>> I'd put the plain error_propagate() first, variations second, like you
>>> do with error_free().
>>>
>>> If neither of these two patterns match on a path from
>>> ERRP_AUTO_PROPAGATE() to return, we effectively insert error_propagate()
>>> where it wasn't before.  Does nothing when the local error is null
>>> there.  Bug fix when it isn't: it's at least a memory leak, and quite
>>> possibly worse.
>>
>> Hmm. How can it be memory leak after any of error_free variants?
> 
> Consider nfs_options_qdict_to_qapi() right before commit 54b7af4369a
> fixed it:
> 
>      static BlockdevOptionsNfs *nfs_options_qdict_to_qapi(QDict *options,
>                                                           Error **errp)
>      {
>          BlockdevOptionsNfs *opts = NULL;
>          QObject *crumpled = NULL;
>          Visitor *v;
>          Error *local_err = NULL;
> 
>          crumpled = qdict_crumple(options, errp);
>          if (crumpled == NULL) {
>              return NULL;
>          }
> 
>          v = qobject_input_visitor_new_keyval(crumpled);
>          visit_type_BlockdevOptionsNfs(v, NULL, &opts, &local_err);
>          visit_free(v);
>          qobject_unref(crumpled);
> 
>          if (local_err) {
>              return NULL;
>          }
> 
>          return opts;
>      }
> 
> When visit_type_BlockdevOptionsNfs() fails, we return null without
> setting an error.  We also leak the error we got from
> visit_type_BlockdevOptionsNfs().
> 
> Commit 54b7af4369a fixed this:
> 
>      --- a/block/nfs.c
>      +++ b/block/nfs.c
>      @@ -570,6 +570,7 @@ static BlockdevOptionsNfs *nfs_options_qdict_to_qapi(QDict *
>      options,
>           qobject_unref(crumpled);
> 
>           if (local_err) {
>      +        error_propagate(errp, local_err);
>               return NULL;
>           }
> 
> If it was still broken, then your transformation would *also* fix it,
> wouldn't it?

Ah, yes. You mean addition of the macro invocation, I thought about transforming
error_free variants to error_free_errp variants.

> 
> My point is: your transformation might fix actual bugs!
> 
>>> Identifying these bug fixes would be nice, but I don't have practical
>>> ideas on how to do that.
>>>
>>> Can we explain this in the commit message?
>>>
>>>> +)
>>>> +     ...>
>>>> + }
>>>> +
>>>> +@@
>>>> +identifier rule1.fn, rule1.local_err;
>>>> +@@
>>>> +
>>>> + fn(...)
>>>> + {
>>>> +     <...
>>>> +(
>>>> +-    &local_err
>>>> ++    errp
>>>> +|
>>>> +-    local_err
>>>> ++    *errp
>>>> +)
>>>> +     ...>
>>>> + }
>>>
>>> Also in tandem with rule1, fixes up uses of local_err.  Good.
>>>
>>>> +
>>>> +@@
>>>> +identifier rule1.fn;
>>>> +@@
>>>> +
>>>> + fn(...)
>>>> + {
>>>> +     <...
>>>> +- *errp != NULL
>>>> ++ *errp
>>>> +     ...>
>>>> + }
>>>
>>> Still in tandem with rule1, normalizes style.  Good.
>>>
> 


-- 
Best regards,
Vladimir

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

Re: [Xen-devel] [PATCH v7 03/11] scripts: add coccinelle script to use auto propagated errp

[Markus Armbruster]

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

> 25.02.2020 15:52, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> 23.02.2020 11:55, Markus Armbruster wrote:
>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>>
>>>>> Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
>>>>> does corresponding changes in code (look for details in
>>>>> include/qapi/error.h)
>>>>>
>>>>> Usage example:
>>>>> spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>>>>    --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>>>>    blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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                          |   3 +
>>>>>    scripts/coccinelle/auto-propagated-errp.cocci | 158 ++++++++++++++++++
>>>>>    2 files changed, 161 insertions(+)
>>>>>    create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
>>>>>
>>>>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>>>>> index b9452d4806..79f8e95214 100644
>>>>> --- a/include/qapi/error.h
>>>>> +++ b/include/qapi/error.h
>>>>> @@ -141,6 +141,9 @@
>>>>>     *         ...
>>>>>     *     }
>>>>>     *
>>>>> + * For mass conversion use script
>>>>> + *   scripts/coccinelle/auto-propagated-errp.cocci
>>>>> + *
>>>>>     *
>>>>>     * Receive and accumulate multiple errors (first one wins):
>>>>>     *     Error *err = NULL, *local_err = NULL;
>>>>
>>>> Extra blank line.
>>>>
>>>>> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
>>>>> new file mode 100644
>>>>> index 0000000000..fb03c871cb
>>>>> --- /dev/null
>>>>> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
>>>>> @@ -0,0 +1,158 @@
>>>>> +// Use ERRP_AUTO_PROPAGATE (see include/qapi/error.h)
>>>>> +//
>>>>> +// Copyright (c) 2020 Virtuozzo International GmbH.
>>>>> +//
>>>>> +// This program is free software; you can redistribute it and/or modify
>>>>> +// it under the terms of the GNU General Public License as published by
>>>>> +// the Free Software Foundation; either version 2 of the License, or
>>>>> +// (at your option) any later version.
>>>>> +//
>>>>> +// This program is distributed in the hope that it will be useful,
>>>>> +// but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>> +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>>> +// GNU General Public License for more details.
>>>>> +//
>>>>> +// You should have received a copy of the GNU General Public License
>>>>> +// along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>>>> +//
>>>>> +// Usage example:
>>>>> +// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>>>> +//  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>>>> +//  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>>>> +
>>>>> +@rule0@
>>>>> +// Add invocation to errp-functions where necessary
>>>>> +// We should skip functions with "Error *const *errp"
>>>>> +// parameter, but how to do it with coccinelle?
>>>>> +// I don't know, so, I skip them by function name regex.
>>>>> +// It's safe: if we did not skip some functions with
>>>>> +// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
>>>>> +// will fail to compile, because of const violation.
>>>>
>>>> Not skipping a function we should skip fails to compile.
>>>>
>>>> What about skipping a function we should not skip?
>>>
>>> Then it will not be updated.. Not good but I don't have better solution.
>>> Still, I hope, function called *error_append_*_hint will not return error
>>> through errp pointer.
>>
>> Seems likely.  I just dislike inferring behavior from name patterns.
>>
>> Ideally, we recognize the true exceptional pattern instead, i.e. the
>> presence of const.  But figuring out how to make Coccinelle do that for
>> us may be more trouble than it's worth.
>>
>> Hmm...  Coccinelle matches the parameter even with const due to what it
>> calls "isomorphism".  Can I disable it?  *Tinker* *tinker*
>>
>> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
>> index fb03c871cb..0c4414bff3 100644
>> --- a/scripts/coccinelle/auto-propagated-errp.cocci
>> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
>> @@ -20,15 +20,11 @@
>>   //  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>   //  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>   -@rule0@
>> +@rule0 disable optional_qualifier@
>>   // Add invocation to errp-functions where necessary
>> -// We should skip functions with "Error *const *errp"
>> -// parameter, but how to do it with coccinelle?
>> -// I don't know, so, I skip them by function name regex.
>> -// It's safe: if we did not skip some functions with
>> -// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
>> -// will fail to compile, because of const violation.
>> -identifier fn !~ "error_append_.*_hint";
>> +// Disable optional_qualifier to skip functions with "Error *const *errp"
>> +// parameter,
>> +identifier fn;
>>   identifier local_err, ERRP;
>>   @@
>>
>> Could you verify this results in the same tree-wide change as your
>> version?
>
> Yes, no difference. Thanks!

Excellent!

[...]
>> Let's see whether I got it:
>>
>> * The first rule (rule0) adds ERRP_AUTO_PROPAGATE() to all functions
>>    that take an Error ** parameter, and either pass it error_prepend() or
>>    error_append_hint(), or use local_err, and don't have
>>    ERRP_AUTO_PROPAGATE() already, except it skips the ones named
>>    error_append_FOO_hint().  Uff.
>>
>>    The "use local_err" part is an approximation of "use local_err +
>>    error_propagate()".
>>
>>    The "except for the ones named error_append_FOO_hint()" part is an
>>    approximation of "except for the ones taking an Error *const *
>>    parameter".
>>
>>    ERRP_AUTO_PROPAGATE() requires the Error ** parameter to be named
>>    @errp, which need not be the case.  The next rule fixes it up:
>>
>> * The second rule ensures the parameter is named @errp wherever the
>>    first rule applied, renaming if necessary.
>>
>>    Correct?
>>
>>    Incorrect transformation followed by fixup is not ideal, because it
>>    can trip up reviewers.  But ideal is too expensive; this is good
>>    enough.
>>
>> * The third rule (rule1) ensures functions that take an Error **errp
>>    parameter don't declare local_err, except it skips the ones named
>>    error_append_FOO_hint().
>>
>>    In isolation, this rule makes no sense.  To make sense of it, we need
>>    context:
>>
>>    * Subsequent rules remove all uses of @errp from any function where
>
> of local_err
>
>>      rule1 matches.
>>
>>    * Preceding rules ensure any function where rule1 matches has
>>      ERRP_AUTO_PROPAGATE().
>>
>>    Correct?
>
> Oh, yes, everything is correct.

Thank you!

>>
>>    The need for this much context is hard on reviewers.  Good enough for
>>    transforming the tree now, but I'd hate having to make sense of this
>>    again in six months.
>
> Ohh, yes. Far from good design. I can try to reorder chunks a bit.

Please don't spend too much effort on it.  The script is primarily for
helping us convert the whole tree within a short time span.  We may also
use it later to convert instances of the old pattern that have crept
back.  We hopefully won't have to change the script then.  Readability
is not as important as it is for code we expect to be read again and
again over a long time.

[...]


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

Re: [Xen-devel] [PATCH v7 03/11] scripts: add coccinelle script to use auto propagated errp

[Vladimir Sementsov-Ogievskiy]

23.02.2020 11:55, Markus Armbruster wrote:
>> +|
>> +-    warn_report_err(local_err);
>> ++    warn_report_errp(errp);
> Likewise.
> 
> What about error_reportf_err(), warn_reportf_err()?

Hmm I'm afraid, we don't have corresponding cases to update..

We can still handle them here, but, then, should we use nonexistent error_reportf_errp, so if it matched, we'll have compilation error?
Or may be coccinelle has some kind of "abort()" on the match, to error-out that "please define error_reportf_errp and update coccinelle script first"...

-- 
Best regards,
Vladimir

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

Re: [Xen-devel] [PATCH v7 03/11] scripts: add coccinelle script to use auto propagated errp

[Vladimir Sementsov-Ogievskiy]

23.02.2020 11:55, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
>> does corresponding changes in code (look for details in
>> include/qapi/error.h)
>>
>> Usage example:
>> spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>   --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>   blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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                          |   3 +
>>   scripts/coccinelle/auto-propagated-errp.cocci | 158 ++++++++++++++++++
>>   2 files changed, 161 insertions(+)
>>   create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
>>
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index b9452d4806..79f8e95214 100644
>> --- a/include/qapi/error.h
>> +++ b/include/qapi/error.h
>> @@ -141,6 +141,9 @@
>>    *         ...
>>    *     }
>>    *
>> + * For mass conversion use script
>> + *   scripts/coccinelle/auto-propagated-errp.cocci
>> + *
>>    *
>>    * Receive and accumulate multiple errors (first one wins):
>>    *     Error *err = NULL, *local_err = NULL;
> 
> Extra blank line.
> 
>> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
>> new file mode 100644
>> index 0000000000..fb03c871cb
>> --- /dev/null
>> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
>> @@ -0,0 +1,158 @@
>> +// Use ERRP_AUTO_PROPAGATE (see include/qapi/error.h)
>> +//
>> +// Copyright (c) 2020 Virtuozzo International GmbH.
>> +//
>> +// This program is free software; you can redistribute it and/or modify
>> +// it under the terms of the GNU General Public License as published by
>> +// the Free Software Foundation; either version 2 of the License, or
>> +// (at your option) any later version.
>> +//
>> +// This program is distributed in the hope that it will be useful,
>> +// but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +// GNU General Public License for more details.
>> +//
>> +// You should have received a copy of the GNU General Public License
>> +// along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +//
>> +// Usage example:
>> +// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>> +//  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>> +//  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>> +
>> +@rule0@
>> +// Add invocation to errp-functions where necessary
>> +// We should skip functions with "Error *const *errp"
>> +// parameter, but how to do it with coccinelle?
>> +// I don't know, so, I skip them by function name regex.
>> +// It's safe: if we did not skip some functions with
>> +// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
>> +// will fail to compile, because of const violation.
> 
> Not skipping a function we should skip fails to compile.
> 
> What about skipping a function we should not skip?
> 
>> +identifier fn !~ "error_append_.*_hint";
>> +identifier local_err, ERRP;
> 
> A few of our coccinelle scripts use ALL_CAPS for meta-variables.  Most
> don't.  Either is fine with me.  Mixing the two styles feels a bit
> confusing, though.
> 
>> +@@
>> +
>> + fn(..., Error **ERRP, ...)
>> + {
>> ++   ERRP_AUTO_PROPAGATE();
>> +    <+...
>> +        when != ERRP_AUTO_PROPAGATE();
>> +(
>> +    error_append_hint(ERRP, ...);
>> +|
>> +    error_prepend(ERRP, ...);
>> +|
>> +    Error *local_err = NULL;
>> +)
>> +    ...+>
>> + }
> 
> Misses error_vprepend().  Currently harmless, but as long as we commit
> the script, we better make it as robust as we reasonably can.
> 
> The previous patch explains this Coccinelle script's intent:
> 
>    To achieve these goals, later patches will add invocations
>    of this macro at the start of functions with either use
>    error_prepend/error_append_hint (solving 1) or which use
>    local_err+error_propagate to check errors, switching those
>    functions to use *errp instead (solving 2 and 3).
> 
> This rule matches "use error_prepend/error_append_hint" directly.  It
> appears to use presence of a local Error * variable as proxy for "use
> local_err+error_propagate to check errors".  Hmm.
> 
> We obviously have such a variable when we use "local_err+error_propagate
> to check errors".  But we could also have such variables without use of
> error_propagate().  In fact, error.h documents such use:
> 
>   * Call a function and receive an error from it:
>   *     Error *err = NULL;
>   *     foo(arg, &err);
>   *     if (err) {
>   *         handle the error...
>   *     }
> 
> where "handle the error" frees it.
> 
> I figure such uses typically occur in functions without an Error **errp
> parameter.  This rule doesn't apply then.  But they could occur even in
> functions with such a parameter.  Consider:
> 
>      void foo(Error **errp)
>      {
>          Error *err = NULL;
> 
>          bar(&err);
>          if (err) {
>              error_free(err);
>              error_setg(errp, "completely different error");
>          }
>      }
> 
> Reasonable enough when bar() gives us an error that's misleading in this
> context, isn't it?
> 
> The script transforms it like this:
> 
>      void foo(Error **errp)
>      {
>     -    Error *err = NULL;
>     +    ERRP_AUTO_PROPAGATE();
> 
>     -    bar(&err);
>     -    if (err) {
>     -        error_free(err);
>     +    bar(errp);
>     +    if (*errp) {
>     +        error_free_errp(errp);
>              error_setg(errp, "completely different error");
>          }
>      }
> 
> Unwanted.
> 
> Now, if this script applied in just a few dozen places, we could rely on
> eyeballing its output to catch unwanted transformations.  Since it
> applies in so many more, I don't feel comfortable relying on reviewer
> eyeballs.
> 
> Can we make rule0 directly match error_propagate(errp, local_err)
> somehow?
> 
> Another observation: the rule does not match error_reportf_err() and
> warn_reportf_err().

They are unrelated, as they take Error* argument, not Error**

> These combine error_prepend(),
> error_report()/warn_report() and error_free(), for convenience.  Don't
> their users need ERRP_AUTO_PROPAGATE() just like error_prepend()'s
> users?
> 
>> +
>> +@@
>> +// Switch unusual (Error **) parameter names to errp
>> +// (this is necessary to use ERRP_AUTO_PROPAGATE).
> 
> Please put your rule comments right before the rule, i.e. before the
> @-line introducing metavariable declarations, not after.  Same
> elsewhere.
> 
>> +identifier rule0.fn;
>> +identifier rule0.ERRP != errp;
>> +@@
>> +
>> + fn(...,
>> +-   Error **ERRP
>> ++   Error **errp
>> +    ,...)
>> + {
>> +     <...
>> +-    ERRP
>> ++    errp
>> +     ...>
>> + }
> 
> This normalizes errp parameter naming.  It matches exactly when rule0
> matches (and inserts ERRP_AUTO_PROPAGATE()) and the Error ** parameter
> is unusual.  Good.
> 
>> +
>> +@rule1@
>> +// We want to patch error propagation in functions regardless of
>> +// whether the function already uses ERRP_AUTO_PROPAGATE prior to
>> +// applying rule0, hence this one does not inherit from it.
> 
> I'm not sure I get this comment.  Let's see what the rule does.
> 
>> +identifier fn !~ "error_append_.*_hint";
>> +identifier local_err;
>> +symbol errp;
>> +@@
>> +
>> + fn(..., Error **errp, ...)
>> + {
>> +     <...
>> +-    Error *local_err = NULL;
>> +     ...>
>> + }
> 
> rule1 matches like rule0, except the Error ** parameter match is
> tightened from any C identifier to the C identifier errp, and the
> function body match tightened from "either use
> error_prepend/error_append_hint or which use local_err+error_propagate
> to check errors" to just the latter.
> 
> I figure tightening the Error ** parameter match has no effect, because
> we already normalized the parameter name.
> 
> So rule1 deletes variable local_err where rule0 applied.  Correct?
> 
>> +
>> +@@
>> +// Handle pattern with goto, otherwise we'll finish up
>> +// with labels at function end which will not compile.
>> +identifier rule1.fn, rule1.local_err;
>> +identifier OUT;
>> +@@
>> +
>> + fn(...)
>> + {
>> +     <...
>> +-    goto OUT;
>> ++    return;
>> +     ...>
>> +- OUT:
>> +-    error_propagate(errp, local_err);
>> + }
> 
> This is one special case of error_propagate() deletion.  It additionally
> gets rid of a goto we no longer want.  For the general case, see below.
> 
> The rule applies only where rule1 just deleted the variable.  Thus, the
> two rules work in tandem.  Makes sense.
> 
>> +
>> +@@
>> +identifier rule1.fn, rule1.local_err;
> 
> This rule also works in tandem with rule1.
> 
>> +expression list args; // to reindent error_propagate_prepend
> 
> What is the comment trying to tell me?
> 
>> +@@
>> +
>> + fn(...)
>> + {
>> +     <...
>> +(
>> +-    error_free(local_err);
>> +-    local_err = NULL;
>> ++    error_free_errp(errp);
> 
> Reminder:
> 
>      static inline void error_free_errp(Error **errp)
>      {
>          assert(errp && *errp);
>          error_free(*errp);
>          *errp = NULL;
>      }
> 
> Now let's examine the actual change.
> 
> The assertion's first half trivially holds, ERRP_AUTO_PROPAGATE()
> ensures it.
> 
> The second half is new.  We now crash when we haven't set an error.  Why
> is this safe?  Note that error_free(local_err) does nothing when
> !local_err.
> 
> The zapping of the variable pointing to the Error just freed is
> unchanged.
> 
>> +|
>> +-    error_free(local_err);
>> ++    error_free_errp(errp);
> 
> Here, the zapping is new.  Zapping dangling pointers is obviously safe.
> Needed, or else the automatic error_propagate() due to
> ERRP_AUTO_PROPAGATE() would propagate the dangling pointer.
> 
>> +|
>> +-    error_report_err(local_err);
>> ++    error_report_errp(errp);
> 
> The only difference to the previous case is that we also report the
> error.
> 
> The previous case has a buddy that additionally matches *errp = NULL.
> Why not this one?
> 
>> +|
>> +-    warn_report_err(local_err);
>> ++    warn_report_errp(errp);
> 
> Likewise.
> 
> What about error_reportf_err(), warn_reportf_err()?
> 
> Up to here, this rule transforms the various forms of error_free().
> Next: error_propagate().
> 
>> +|
>> +-    error_propagate_prepend(errp, local_err, args);
>> ++    error_prepend(errp, args);
>> +|
>> +-    error_propagate(errp, local_err);
> 
> rule0's adding of ERRP_AUTO_PROPAGATE() made error_propagate()
> redundant.
> 
> This is the general case of error_propagate() deletion.
> 
> I'd put the plain error_propagate() first, variations second, like you
> do with error_free().
> 
> If neither of these two patterns match on a path from
> ERRP_AUTO_PROPAGATE() to return, we effectively insert error_propagate()
> where it wasn't before.  Does nothing when the local error is null
> there.  Bug fix when it isn't: it's at least a memory leak, and quite
> possibly worse.
> 
> Identifying these bug fixes would be nice, but I don't have practical
> ideas on how to do that.
> 
> Can we explain this in the commit message?
> 
>> +)
>> +     ...>
>> + }
>> +
>> +@@
>> +identifier rule1.fn, rule1.local_err;
>> +@@
>> +
>> + fn(...)
>> + {
>> +     <...
>> +(
>> +-    &local_err
>> ++    errp
>> +|
>> +-    local_err
>> ++    *errp
>> +)
>> +     ...>
>> + }
> 
> Also in tandem with rule1, fixes up uses of local_err.  Good.
> 
>> +
>> +@@
>> +identifier rule1.fn;
>> +@@
>> +
>> + fn(...)
>> + {
>> +     <...
>> +- *errp != NULL
>> ++ *errp
>> +     ...>
>> + }
> 
> Still in tandem with rule1, normalizes style.  Good.
> 


-- 
Best regards,
Vladimir

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

Re: [Xen-devel] [PATCH v7 03/11] scripts: add coccinelle script to use auto propagated errp

[Markus Armbruster]

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

> 23.02.2020 11:55, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
>>> does corresponding changes in code (look for details in
>>> include/qapi/error.h)
>>>
>>> Usage example:
>>> spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>>   --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>>   blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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                          |   3 +
>>>   scripts/coccinelle/auto-propagated-errp.cocci | 158 ++++++++++++++++++
>>>   2 files changed, 161 insertions(+)
>>>   create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
>>>
>>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>>> index b9452d4806..79f8e95214 100644
>>> --- a/include/qapi/error.h
>>> +++ b/include/qapi/error.h
>>> @@ -141,6 +141,9 @@
>>>    *         ...
>>>    *     }
>>>    *
>>> + * For mass conversion use script
>>> + *   scripts/coccinelle/auto-propagated-errp.cocci
>>> + *
>>>    *
>>>    * Receive and accumulate multiple errors (first one wins):
>>>    *     Error *err = NULL, *local_err = NULL;
>>
>> Extra blank line.
>>
>>> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci b/scripts/coccinelle/auto-propagated-errp.cocci
>>> new file mode 100644
>>> index 0000000000..fb03c871cb
>>> --- /dev/null
>>> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
>>> @@ -0,0 +1,158 @@
>>> +// Use ERRP_AUTO_PROPAGATE (see include/qapi/error.h)
>>> +//
>>> +// Copyright (c) 2020 Virtuozzo International GmbH.
>>> +//
>>> +// This program is free software; you can redistribute it and/or modify
>>> +// it under the terms of the GNU General Public License as published by
>>> +// the Free Software Foundation; either version 2 of the License, or
>>> +// (at your option) any later version.
>>> +//
>>> +// This program is distributed in the hope that it will be useful,
>>> +// but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +// GNU General Public License for more details.
>>> +//
>>> +// You should have received a copy of the GNU General Public License
>>> +// along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>> +//
>>> +// Usage example:
>>> +// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>> +//  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>> +//  blockdev-nbd.c qemu-nbd.c {block/nbd*,nbd/*,include/block/nbd*}.[hc]
>>> +
>>> +@rule0@
>>> +// Add invocation to errp-functions where necessary
>>> +// We should skip functions with "Error *const *errp"
>>> +// parameter, but how to do it with coccinelle?
>>> +// I don't know, so, I skip them by function name regex.
>>> +// It's safe: if we did not skip some functions with
>>> +// "Error *const *errp", ERRP_AUTO_PROPAGATE invocation
>>> +// will fail to compile, because of const violation.
>>
>> Not skipping a function we should skip fails to compile.
>>
>> What about skipping a function we should not skip?
>>
>>> +identifier fn !~ "error_append_.*_hint";
>>> +identifier local_err, ERRP;
>>
>> A few of our coccinelle scripts use ALL_CAPS for meta-variables.  Most
>> don't.  Either is fine with me.  Mixing the two styles feels a bit
>> confusing, though.
>>
>>> +@@
>>> +
>>> + fn(..., Error **ERRP, ...)
>>> + {
>>> ++   ERRP_AUTO_PROPAGATE();
>>> +    <+...
>>> +        when != ERRP_AUTO_PROPAGATE();
>>> +(
>>> +    error_append_hint(ERRP, ...);
>>> +|
>>> +    error_prepend(ERRP, ...);
>>> +|
>>> +    Error *local_err = NULL;
>>> +)
>>> +    ...+>
>>> + }
>>
>> Misses error_vprepend().  Currently harmless, but as long as we commit
>> the script, we better make it as robust as we reasonably can.
>>
>> The previous patch explains this Coccinelle script's intent:
>>
>>    To achieve these goals, later patches will add invocations
>>    of this macro at the start of functions with either use
>>    error_prepend/error_append_hint (solving 1) or which use
>>    local_err+error_propagate to check errors, switching those
>>    functions to use *errp instead (solving 2 and 3).
>>
>> This rule matches "use error_prepend/error_append_hint" directly.  It
>> appears to use presence of a local Error * variable as proxy for "use
>> local_err+error_propagate to check errors".  Hmm.
>>
>> We obviously have such a variable when we use "local_err+error_propagate
>> to check errors".  But we could also have such variables without use of
>> error_propagate().  In fact, error.h documents such use:
>>
>>   * Call a function and receive an error from it:
>>   *     Error *err = NULL;
>>   *     foo(arg, &err);
>>   *     if (err) {
>>   *         handle the error...
>>   *     }
>>
>> where "handle the error" frees it.
>>
>> I figure such uses typically occur in functions without an Error **errp
>> parameter.  This rule doesn't apply then.  But they could occur even in
>> functions with such a parameter.  Consider:
>>
>>      void foo(Error **errp)
>>      {
>>          Error *err = NULL;
>>
>>          bar(&err);
>>          if (err) {
>>              error_free(err);
>>              error_setg(errp, "completely different error");
>>          }
>>      }
>>
>> Reasonable enough when bar() gives us an error that's misleading in this
>> context, isn't it?
>>
>> The script transforms it like this:
>>
>>      void foo(Error **errp)
>>      {
>>     -    Error *err = NULL;
>>     +    ERRP_AUTO_PROPAGATE();
>>
>>     -    bar(&err);
>>     -    if (err) {
>>     -        error_free(err);
>>     +    bar(errp);
>>     +    if (*errp) {
>>     +        error_free_errp(errp);
>>              error_setg(errp, "completely different error");
>>          }
>>      }
>>
>> Unwanted.
>>
>> Now, if this script applied in just a few dozen places, we could rely on
>> eyeballing its output to catch unwanted transformations.  Since it
>> applies in so many more, I don't feel comfortable relying on reviewer
>> eyeballs.
>>
>> Can we make rule0 directly match error_propagate(errp, local_err)
>> somehow?
>>
>> Another observation: the rule does not match error_reportf_err() and
>> warn_reportf_err().
>
> They are unrelated, as they take Error* argument, not Error**
>
>> These combine error_prepend(),
>> error_report()/warn_report() and error_free(), for convenience.  Don't
>> their users need ERRP_AUTO_PROPAGATE() just like error_prepend()'s
>> users?

Right, we never pass *errp to error_reportf_err() and
warn_reportf_err(), so there's no need to wrap it.

But see below.

>>
>>> +
>>> +@@
>>> +// Switch unusual (Error **) parameter names to errp
>>> +// (this is necessary to use ERRP_AUTO_PROPAGATE).
>>
>> Please put your rule comments right before the rule, i.e. before the
>> @-line introducing metavariable declarations, not after.  Same
>> elsewhere.
>>
>>> +identifier rule0.fn;
>>> +identifier rule0.ERRP != errp;
>>> +@@
>>> +
>>> + fn(...,
>>> +-   Error **ERRP
>>> ++   Error **errp
>>> +    ,...)
>>> + {
>>> +     <...
>>> +-    ERRP
>>> ++    errp
>>> +     ...>
>>> + }
>>
>> This normalizes errp parameter naming.  It matches exactly when rule0
>> matches (and inserts ERRP_AUTO_PROPAGATE()) and the Error ** parameter
>> is unusual.  Good.
>>
>>> +
>>> +@rule1@
>>> +// We want to patch error propagation in functions regardless of
>>> +// whether the function already uses ERRP_AUTO_PROPAGATE prior to
>>> +// applying rule0, hence this one does not inherit from it.
>>
>> I'm not sure I get this comment.  Let's see what the rule does.
>>
>>> +identifier fn !~ "error_append_.*_hint";
>>> +identifier local_err;
>>> +symbol errp;
>>> +@@
>>> +
>>> + fn(..., Error **errp, ...)
>>> + {
>>> +     <...
>>> +-    Error *local_err = NULL;
>>> +     ...>
>>> + }
>>
>> rule1 matches like rule0, except the Error ** parameter match is
>> tightened from any C identifier to the C identifier errp, and the
>> function body match tightened from "either use
>> error_prepend/error_append_hint or which use local_err+error_propagate
>> to check errors" to just the latter.
>>
>> I figure tightening the Error ** parameter match has no effect, because
>> we already normalized the parameter name.
>>
>> So rule1 deletes variable local_err where rule0 applied.  Correct?
>>
>>> +
>>> +@@
>>> +// Handle pattern with goto, otherwise we'll finish up
>>> +// with labels at function end which will not compile.
>>> +identifier rule1.fn, rule1.local_err;
>>> +identifier OUT;
>>> +@@
>>> +
>>> + fn(...)
>>> + {
>>> +     <...
>>> +-    goto OUT;
>>> ++    return;
>>> +     ...>
>>> +- OUT:
>>> +-    error_propagate(errp, local_err);
>>> + }
>>
>> This is one special case of error_propagate() deletion.  It additionally
>> gets rid of a goto we no longer want.  For the general case, see below.
>>
>> The rule applies only where rule1 just deleted the variable.  Thus, the
>> two rules work in tandem.  Makes sense.
>>
>>> +
>>> +@@
>>> +identifier rule1.fn, rule1.local_err;
>>
>> This rule also works in tandem with rule1.
>>
>>> +expression list args; // to reindent error_propagate_prepend
>>
>> What is the comment trying to tell me?
>>
>>> +@@
>>> +
>>> + fn(...)
>>> + {
>>> +     <...
>>> +(
>>> +-    error_free(local_err);
>>> +-    local_err = NULL;
>>> ++    error_free_errp(errp);
>>
>> Reminder:
>>
>>      static inline void error_free_errp(Error **errp)
>>      {
>>          assert(errp && *errp);
>>          error_free(*errp);
>>          *errp = NULL;
>>      }
>>
>> Now let's examine the actual change.
>>
>> The assertion's first half trivially holds, ERRP_AUTO_PROPAGATE()
>> ensures it.
>>
>> The second half is new.  We now crash when we haven't set an error.  Why
>> is this safe?  Note that error_free(local_err) does nothing when
>> !local_err.
>>
>> The zapping of the variable pointing to the Error just freed is
>> unchanged.
>>
>>> +|
>>> +-    error_free(local_err);
>>> ++    error_free_errp(errp);
>>
>> Here, the zapping is new.  Zapping dangling pointers is obviously safe.
>> Needed, or else the automatic error_propagate() due to
>> ERRP_AUTO_PROPAGATE() would propagate the dangling pointer.
>>
>>> +|
>>> +-    error_report_err(local_err);
>>> ++    error_report_errp(errp);

error_reportf_err() is just like error_report_err(), except it
additionally modifies the error message.  Does it need a similar
transformation?

>> The only difference to the previous case is that we also report the
>> error.
>>
>> The previous case has a buddy that additionally matches *errp = NULL.
>> Why not this one?
>>
>>> +|
>>> +-    warn_report_err(local_err);
>>> ++    warn_report_errp(errp);

Likewise.

>> Likewise.
>>
>> What about error_reportf_err(), warn_reportf_err()?
>>
>> Up to here, this rule transforms the various forms of error_free().
>> Next: error_propagate().
>>
>>> +|
>>> +-    error_propagate_prepend(errp, local_err, args);
>>> ++    error_prepend(errp, args);
>>> +|
>>> +-    error_propagate(errp, local_err);
>>
>> rule0's adding of ERRP_AUTO_PROPAGATE() made error_propagate()
>> redundant.
>>
>> This is the general case of error_propagate() deletion.
>>
>> I'd put the plain error_propagate() first, variations second, like you
>> do with error_free().
>>
>> If neither of these two patterns match on a path from
>> ERRP_AUTO_PROPAGATE() to return, we effectively insert error_propagate()
>> where it wasn't before.  Does nothing when the local error is null
>> there.  Bug fix when it isn't: it's at least a memory leak, and quite
>> possibly worse.
>>
>> Identifying these bug fixes would be nice, but I don't have practical
>> ideas on how to do that.
>>
>> Can we explain this in the commit message?
>>
>>> +)
>>> +     ...>
>>> + }
>>> +
>>> +@@
>>> +identifier rule1.fn, rule1.local_err;
>>> +@@
>>> +
>>> + fn(...)
>>> + {
>>> +     <...
>>> +(
>>> +-    &local_err
>>> ++    errp
>>> +|
>>> +-    local_err
>>> ++    *errp
>>> +)
>>> +     ...>
>>> + }
>>
>> Also in tandem with rule1, fixes up uses of local_err.  Good.
>>
>>> +
>>> +@@
>>> +identifier rule1.fn;
>>> +@@
>>> +
>>> + fn(...)
>>> + {
>>> +     <...
>>> +- *errp != NULL
>>> ++ *errp
>>> +     ...>
>>> + }
>>
>> Still in tandem with rule1, normalizes style.  Good.
>>


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

[Xen-devel] [PATCH v7 11/11] xen: introduce ERRP_AUTO_PROPAGATE

[Vladimir Sementsov-Ogievskiy]

If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &error_fatal
(the program will exit prior to the error_append_hint() or
error_prepend() call).  Fix such cases.

If we want to check error after errp-function call, we need to
introduce local_err and then propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
   &error_fatal, this means that we don't break error_abort
   (we'll abort on error_set, not on error_propagate)

This commit is generated by command

    sed -n '/^X86 Xen CPUs$/,/^$/{s/^F: //p}' MAINTAINERS | \
    xargs git ls-files | grep '\.[hc]$' | \
    xargs spatch \
        --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
        --macro-file scripts/cocci-macro-file.h \
        --in-place --no-show-diff --max-width 80

Reported-by: Kevin Wolf <kwolf@redhat.com>
Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Acked-by: Paul Durrant <paul@xen.org>
---
 hw/block/dataplane/xen-block.c |  17 ++---
 hw/block/xen-block.c           | 125 ++++++++++++++-------------------
 hw/pci-host/xen_igd_pt.c       |   7 +-
 hw/xen/xen-backend.c           |   7 +-
 hw/xen/xen-bus.c               | 100 ++++++++++++--------------
 hw/xen/xen-host-pci-device.c   |  27 ++++---
 hw/xen/xen_pt.c                |  25 +++----
 hw/xen/xen_pt_config_init.c    |  20 +++---
 8 files changed, 142 insertions(+), 186 deletions(-)

diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
index 3b9caeb2fa..c38e3c3d85 100644
--- a/hw/block/dataplane/xen-block.c
+++ b/hw/block/dataplane/xen-block.c
@@ -727,8 +727,8 @@ void xen_block_dataplane_start(XenBlockDataPlane *dataplane,
                                unsigned int protocol,
                                Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     XenDevice *xendev = dataplane->xendev;
-    Error *local_err = NULL;
     unsigned int ring_size;
     unsigned int i;
 
@@ -764,9 +764,8 @@ void xen_block_dataplane_start(XenBlockDataPlane *dataplane,
     }
 
     xen_device_set_max_grant_refs(xendev, dataplane->nr_ring_ref,
-                                  &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+                                  errp);
+    if (*errp) {
         goto stop;
     }
 
@@ -774,9 +773,8 @@ void xen_block_dataplane_start(XenBlockDataPlane *dataplane,
                                               dataplane->ring_ref,
                                               dataplane->nr_ring_ref,
                                               PROT_READ | PROT_WRITE,
-                                              &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+                                              errp);
+    if (*errp) {
         goto stop;
     }
 
@@ -809,9 +807,8 @@ void xen_block_dataplane_start(XenBlockDataPlane *dataplane,
     dataplane->event_channel =
         xen_device_bind_event_channel(xendev, dataplane->ctx, event_channel,
                                       xen_block_dataplane_event, dataplane,
-                                      &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+                                      errp);
+    if (*errp) {
         goto stop;
     }
 
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 686bbc3f0d..717a80d5b5 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -194,6 +194,7 @@ static const BlockDevOps xen_block_dev_ops = {
 
 static void xen_block_realize(XenDevice *xendev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     XenBlockDevice *blockdev = XEN_BLOCK_DEVICE(xendev);
     XenBlockDeviceClass *blockdev_class =
         XEN_BLOCK_DEVICE_GET_CLASS(xendev);
@@ -201,7 +202,6 @@ static void xen_block_realize(XenDevice *xendev, Error **errp)
     XenBlockVdev *vdev = &blockdev->props.vdev;
     BlockConf *conf = &blockdev->props.conf;
     BlockBackend *blk = conf->blk;
-    Error *local_err = NULL;
 
     if (vdev->type == XEN_BLOCK_VDEV_TYPE_INVALID) {
         error_setg(errp, "vdev property not set");
@@ -211,9 +211,8 @@ static void xen_block_realize(XenDevice *xendev, Error **errp)
     trace_xen_block_realize(type, vdev->disk, vdev->partition);
 
     if (blockdev_class->realize) {
-        blockdev_class->realize(blockdev, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
+        blockdev_class->realize(blockdev, errp);
+        if (*errp) {
             return;
         }
     }
@@ -283,8 +282,8 @@ static void xen_block_frontend_changed(XenDevice *xendev,
                                        enum xenbus_state frontend_state,
                                        Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     enum xenbus_state backend_state = xen_device_backend_get_state(xendev);
-    Error *local_err = NULL;
 
     switch (frontend_state) {
     case XenbusStateInitialised:
@@ -293,15 +292,13 @@ static void xen_block_frontend_changed(XenDevice *xendev,
             break;
         }
 
-        xen_block_disconnect(xendev, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
+        xen_block_disconnect(xendev, errp);
+        if (*errp) {
             break;
         }
 
-        xen_block_connect(xendev, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
+        xen_block_connect(xendev, errp);
+        if (*errp) {
             break;
         }
 
@@ -314,9 +311,8 @@ static void xen_block_frontend_changed(XenDevice *xendev,
 
     case XenbusStateClosed:
     case XenbusStateUnknown:
-        xen_block_disconnect(xendev, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
+        xen_block_disconnect(xendev, errp);
+        if (*errp) {
             break;
         }
 
@@ -403,10 +399,10 @@ static int vbd_name_to_disk(const char *name, const char **endp,
 static void xen_block_set_vdev(Object *obj, Visitor *v, const char *name,
                                void *opaque, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     DeviceState *dev = DEVICE(obj);
     Property *prop = opaque;
     XenBlockVdev *vdev = qdev_get_prop_ptr(dev, prop);
-    Error *local_err = NULL;
     char *str, *p;
     const char *end;
 
@@ -415,9 +411,8 @@ static void xen_block_set_vdev(Object *obj, Visitor *v, const char *name,
         return;
     }
 
-    visit_type_str(v, name, &str, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    visit_type_str(v, name, &str, errp);
+    if (*errp) {
         return;
     }
 
@@ -671,9 +666,9 @@ static void xen_block_blockdev_del(const char *node_name, Error **errp)
 static char *xen_block_blockdev_add(const char *id, QDict *qdict,
                                     Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     const char *driver = qdict_get_try_str(qdict, "driver");
     BlockdevOptions *options = NULL;
-    Error *local_err = NULL;
     char *node_name;
     Visitor *v;
 
@@ -688,18 +683,16 @@ static char *xen_block_blockdev_add(const char *id, QDict *qdict,
     trace_xen_block_blockdev_add(node_name);
 
     v = qobject_input_visitor_new(QOBJECT(qdict));
-    visit_type_BlockdevOptions(v, NULL, &options, &local_err);
+    visit_type_BlockdevOptions(v, NULL, &options, errp);
     visit_free(v);
 
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (*errp) {
         goto fail;
     }
 
-    qmp_blockdev_add(options, &local_err);
+    qmp_blockdev_add(options, errp);
 
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (*errp) {
         goto fail;
     }
 
@@ -718,14 +711,12 @@ fail:
 
 static void xen_block_drive_destroy(XenBlockDrive *drive, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     char *node_name = drive->node_name;
 
     if (node_name) {
-        Error *local_err = NULL;
-
-        xen_block_blockdev_del(node_name, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
+        xen_block_blockdev_del(node_name, errp);
+        if (*errp) {
             return;
         }
         g_free(node_name);
@@ -739,6 +730,7 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
                                              const char *device_type,
                                              QDict *opts, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     const char *params = qdict_get_try_str(opts, "params");
     const char *mode = qdict_get_try_str(opts, "mode");
     const char *direct_io_safe = qdict_get_try_str(opts, "direct-io-safe");
@@ -746,7 +738,6 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
     char *driver = NULL;
     char *filename = NULL;
     XenBlockDrive *drive = NULL;
-    Error *local_err = NULL;
     QDict *file_layer;
     QDict *driver_layer;
 
@@ -825,13 +816,12 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
 
     g_assert(!drive->node_name);
     drive->node_name = xen_block_blockdev_add(drive->id, driver_layer,
-                                              &local_err);
+                                              errp);
 
     qobject_unref(driver_layer);
 
 done:
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (*errp) {
         xen_block_drive_destroy(drive, NULL);
         return NULL;
     }
@@ -856,15 +846,13 @@ static void xen_block_iothread_destroy(XenBlockIOThread *iothread,
 static XenBlockIOThread *xen_block_iothread_create(const char *id,
                                                    Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     XenBlockIOThread *iothread = g_new(XenBlockIOThread, 1);
-    Error *local_err = NULL;
 
     iothread->id = g_strdup(id);
 
-    qmp_object_add(TYPE_IOTHREAD, id, false, NULL, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-
+    qmp_object_add(TYPE_IOTHREAD, id, false, NULL, errp);
+    if (*errp) {
         g_free(iothread->id);
         g_free(iothread);
         return NULL;
@@ -876,6 +864,7 @@ static XenBlockIOThread *xen_block_iothread_create(const char *id,
 static void xen_block_device_create(XenBackendInstance *backend,
                                     QDict *opts, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     XenBus *xenbus = xen_backend_get_bus(backend);
     const char *name = xen_backend_get_name(backend);
     unsigned long number;
@@ -883,7 +872,6 @@ static void xen_block_device_create(XenBackendInstance *backend,
     XenBlockDrive *drive = NULL;
     XenBlockIOThread *iothread = NULL;
     XenDevice *xendev = NULL;
-    Error *local_err = NULL;
     const char *type;
     XenBlockDevice *blockdev;
 
@@ -915,52 +903,48 @@ static void xen_block_device_create(XenBackendInstance *backend,
         goto fail;
     }
 
-    drive = xen_block_drive_create(vdev, device_type, opts, &local_err);
+    drive = xen_block_drive_create(vdev, device_type, opts, errp);
     if (!drive) {
-        error_propagate_prepend(errp, local_err, "failed to create drive: ");
+        error_prepend(errp, "failed to create drive: ");
         goto fail;
     }
 
-    iothread = xen_block_iothread_create(vdev, &local_err);
-    if (local_err) {
-        error_propagate_prepend(errp, local_err,
-                                "failed to create iothread: ");
+    iothread = xen_block_iothread_create(vdev, errp);
+    if (*errp) {
+        error_prepend(errp, "failed to create iothread: ");
         goto fail;
     }
 
     xendev = XEN_DEVICE(qdev_create(BUS(xenbus), type));
     blockdev = XEN_BLOCK_DEVICE(xendev);
 
-    object_property_set_str(OBJECT(xendev), vdev, "vdev", &local_err);
-    if (local_err) {
-        error_propagate_prepend(errp, local_err, "failed to set 'vdev': ");
+    object_property_set_str(OBJECT(xendev), vdev, "vdev", errp);
+    if (*errp) {
+        error_prepend(errp, "failed to set 'vdev': ");
         goto fail;
     }
 
     object_property_set_str(OBJECT(xendev),
                             xen_block_drive_get_node_name(drive), "drive",
-                            &local_err);
-    if (local_err) {
-        error_propagate_prepend(errp, local_err, "failed to set 'drive': ");
+                            errp);
+    if (*errp) {
+        error_prepend(errp, "failed to set 'drive': ");
         goto fail;
     }
 
     object_property_set_str(OBJECT(xendev), iothread->id, "iothread",
-                            &local_err);
-    if (local_err) {
-        error_propagate_prepend(errp, local_err,
-                                "failed to set 'iothread': ");
+                            errp);
+    if (*errp) {
+        error_prepend(errp, "failed to set 'iothread': ");
         goto fail;
     }
 
     blockdev->iothread = iothread;
     blockdev->drive = drive;
 
-    object_property_set_bool(OBJECT(xendev), true, "realized", &local_err);
-    if (local_err) {
-        error_propagate_prepend(errp, local_err,
-                                "realization of device %s failed: ",
-                                type);
+    object_property_set_bool(OBJECT(xendev), true, "realized", errp);
+    if (*errp) {
+        error_prepend(errp, "realization of device %s failed: ", type);
         goto fail;
     }
 
@@ -984,6 +968,7 @@ fail:
 static void xen_block_device_destroy(XenBackendInstance *backend,
                                      Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     XenDevice *xendev = xen_backend_get_device(backend);
     XenBlockDevice *blockdev = XEN_BLOCK_DEVICE(xendev);
     XenBlockVdev *vdev = &blockdev->props.vdev;
@@ -995,23 +980,17 @@ static void xen_block_device_destroy(XenBackendInstance *backend,
     object_unparent(OBJECT(xendev));
 
     if (iothread) {
-        Error *local_err = NULL;
-
-        xen_block_iothread_destroy(iothread, &local_err);
-        if (local_err) {
-            error_propagate_prepend(errp, local_err,
-                                "failed to destroy iothread: ");
+        xen_block_iothread_destroy(iothread, errp);
+        if (*errp) {
+            error_prepend(errp, "failed to destroy iothread: ");
             return;
         }
     }
 
     if (drive) {
-        Error *local_err = NULL;
-
-        xen_block_drive_destroy(drive, &local_err);
-        if (local_err) {
-            error_propagate_prepend(errp, local_err,
-                                "failed to destroy drive: ");
+        xen_block_drive_destroy(drive, errp);
+        if (*errp) {
+            error_prepend(errp, "failed to destroy drive: ");
         }
     }
 }
diff --git a/hw/pci-host/xen_igd_pt.c b/hw/pci-host/xen_igd_pt.c
index efcc9347ff..29ade9ca25 100644
--- a/hw/pci-host/xen_igd_pt.c
+++ b/hw/pci-host/xen_igd_pt.c
@@ -79,17 +79,16 @@ static void host_pci_config_read(int pos, int len, uint32_t *val, Error **errp)
 
 static void igd_pt_i440fx_realize(PCIDevice *pci_dev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     uint32_t val = 0;
     size_t i;
     int pos, len;
-    Error *local_err = NULL;
 
     for (i = 0; i < ARRAY_SIZE(igd_host_bridge_infos); i++) {
         pos = igd_host_bridge_infos[i].offset;
         len = igd_host_bridge_infos[i].len;
-        host_pci_config_read(pos, len, &val, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
+        host_pci_config_read(pos, len, &val, errp);
+        if (*errp) {
             return;
         }
         pci_default_write_config(pci_dev, pos, val, len);
diff --git a/hw/xen/xen-backend.c b/hw/xen/xen-backend.c
index da065f81b7..1cc0694053 100644
--- a/hw/xen/xen-backend.c
+++ b/hw/xen/xen-backend.c
@@ -98,9 +98,9 @@ static void xen_backend_list_remove(XenBackendInstance *backend)
 void xen_backend_device_create(XenBus *xenbus, const char *type,
                                const char *name, QDict *opts, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     const XenBackendImpl *impl = xen_backend_table_lookup(type);
     XenBackendInstance *backend;
-    Error *local_error = NULL;
 
     if (!impl) {
         return;
@@ -110,9 +110,8 @@ void xen_backend_device_create(XenBus *xenbus, const char *type,
     backend->xenbus = xenbus;
     backend->name = g_strdup(name);
 
-    impl->create(backend, opts, &local_error);
-    if (local_error) {
-        error_propagate(errp, local_error);
+    impl->create(backend, opts, errp);
+    if (*errp) {
         g_free(backend->name);
         g_free(backend);
         return;
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index 919e66162a..7f0e91f6c0 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -53,9 +53,9 @@ static char *xen_device_get_frontend_path(XenDevice *xendev)
 
 static void xen_device_unplug(XenDevice *xendev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
     const char *type = object_get_typename(OBJECT(xendev));
-    Error *local_err = NULL;
     xs_transaction_t tid;
 
     trace_xen_device_unplug(type, xendev->name);
@@ -69,14 +69,14 @@ again:
     }
 
     xs_node_printf(xenbus->xsh, tid, xendev->backend_path, "online",
-                   &local_err, "%u", 0);
-    if (local_err) {
+                   errp, "%u", 0);
+    if (*errp) {
         goto abort;
     }
 
     xs_node_printf(xenbus->xsh, tid, xendev->backend_path, "state",
-                   &local_err, "%u", XenbusStateClosing);
-    if (local_err) {
+                   errp, "%u", XenbusStateClosing);
+    if (*errp) {
         goto abort;
     }
 
@@ -96,7 +96,6 @@ abort:
      * from ending the transaction.
      */
     xs_transaction_end(xenbus->xsh, tid, true);
-    error_propagate(errp, local_err);
 }
 
 static void xen_bus_print_dev(Monitor *mon, DeviceState *dev, int indent)
@@ -205,15 +204,13 @@ static XenWatch *watch_list_add(XenWatchList *watch_list, const char *node,
                                 const char *key, XenWatchHandler handler,
                                 void *opaque, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     XenWatch *watch = new_watch(node, key, handler, opaque);
-    Error *local_err = NULL;
 
     notifier_list_add(&watch_list->notifiers, &watch->notifier);
 
-    xs_node_watch(watch_list->xsh, node, key, watch->token, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-
+    xs_node_watch(watch_list->xsh, node, key, watch->token, errp);
+    if (*errp) {
         notifier_remove(&watch->notifier);
         free_watch(watch);
 
@@ -255,11 +252,11 @@ static void xen_bus_backend_create(XenBus *xenbus, const char *type,
                                    const char *name, char *path,
                                    Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     xs_transaction_t tid;
     char **key;
     QDict *opts;
     unsigned int i, n;
-    Error *local_err = NULL;
 
     trace_xen_bus_backend_create(type, path);
 
@@ -314,13 +311,11 @@ again:
         return;
     }
 
-    xen_backend_device_create(xenbus, type, name, opts, &local_err);
+    xen_backend_device_create(xenbus, type, name, opts, errp);
     qobject_unref(opts);
 
-    if (local_err) {
-        error_propagate_prepend(errp, local_err,
-                                "failed to create '%s' device '%s': ",
-                                type, name);
+    if (*errp) {
+        error_prepend(errp, "failed to create '%s' device '%s': ", type, name);
     }
 }
 
@@ -451,9 +446,9 @@ static void xen_bus_unrealize(BusState *bus, Error **errp)
 
 static void xen_bus_realize(BusState *bus, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     XenBus *xenbus = XEN_BUS(bus);
     unsigned int domid;
-    Error *local_err = NULL;
 
     trace_xen_bus_realize();
 
@@ -476,10 +471,10 @@ static void xen_bus_realize(BusState *bus, Error **errp)
 
     xenbus->backend_watch =
         xen_bus_add_watch(xenbus, "", /* domain root node */
-                          "backend", xen_bus_backend_changed, &local_err);
-    if (local_err) {
+                          "backend", xen_bus_backend_changed, errp);
+    if (*errp) {
         /* This need not be treated as a hard error so don't propagate */
-        error_reportf_err(local_err,
+        error_reportf_err(*errp,
                           "failed to set up enumeration watch: ");
     }
 
@@ -692,9 +687,9 @@ static void xen_device_remove_watch(XenDevice *xendev, XenWatch *watch,
 
 static void xen_device_backend_create(XenDevice *xendev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
     struct xs_permissions perms[2];
-    Error *local_err = NULL;
 
     xendev->backend_path = xen_device_get_backend_path(xendev);
 
@@ -706,30 +701,27 @@ static void xen_device_backend_create(XenDevice *xendev, Error **errp)
     g_assert(xenbus->xsh);
 
     xs_node_create(xenbus->xsh, XBT_NULL, xendev->backend_path, perms,
-                   ARRAY_SIZE(perms), &local_err);
-    if (local_err) {
-        error_propagate_prepend(errp, local_err,
-                                "failed to create backend: ");
+                   ARRAY_SIZE(perms), errp);
+    if (*errp) {
+        error_prepend(errp, "failed to create backend: ");
         return;
     }
 
     xendev->backend_state_watch =
         xen_device_add_watch(xendev, xendev->backend_path,
                              "state", xen_device_backend_changed,
-                             &local_err);
-    if (local_err) {
-        error_propagate_prepend(errp, local_err,
-                                "failed to watch backend state: ");
+                             errp);
+    if (*errp) {
+        error_prepend(errp, "failed to watch backend state: ");
         return;
     }
 
     xendev->backend_online_watch =
         xen_device_add_watch(xendev, xendev->backend_path,
                              "online", xen_device_backend_changed,
-                             &local_err);
-    if (local_err) {
-        error_propagate_prepend(errp, local_err,
-                                "failed to watch backend online: ");
+                             errp);
+    if (*errp) {
+        error_prepend(errp, "failed to watch backend online: ");
         return;
     }
 }
@@ -866,9 +858,9 @@ static bool xen_device_frontend_exists(XenDevice *xendev)
 
 static void xen_device_frontend_create(XenDevice *xendev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
     struct xs_permissions perms[2];
-    Error *local_err = NULL;
 
     xendev->frontend_path = xen_device_get_frontend_path(xendev);
 
@@ -885,20 +877,18 @@ static void xen_device_frontend_create(XenDevice *xendev, Error **errp)
         g_assert(xenbus->xsh);
 
         xs_node_create(xenbus->xsh, XBT_NULL, xendev->frontend_path, perms,
-                       ARRAY_SIZE(perms), &local_err);
-        if (local_err) {
-            error_propagate_prepend(errp, local_err,
-                                    "failed to create frontend: ");
+                       ARRAY_SIZE(perms), errp);
+        if (*errp) {
+            error_prepend(errp, "failed to create frontend: ");
             return;
         }
     }
 
     xendev->frontend_state_watch =
         xen_device_add_watch(xendev, xendev->frontend_path, "state",
-                             xen_device_frontend_changed, &local_err);
-    if (local_err) {
-        error_propagate_prepend(errp, local_err,
-                                "failed to watch frontend state: ");
+                             xen_device_frontend_changed, errp);
+    if (*errp) {
+        error_prepend(errp, "failed to watch frontend state: ");
     }
 }
 
@@ -1228,11 +1218,11 @@ static void xen_device_exit(Notifier *n, void *data)
 
 static void xen_device_realize(DeviceState *dev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     XenDevice *xendev = XEN_DEVICE(dev);
     XenDeviceClass *xendev_class = XEN_DEVICE_GET_CLASS(xendev);
     XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
     const char *type = object_get_typename(OBJECT(xendev));
-    Error *local_err = NULL;
 
     if (xendev->frontend_id == DOMID_INVALID) {
         xendev->frontend_id = xen_domid;
@@ -1248,10 +1238,9 @@ static void xen_device_realize(DeviceState *dev, Error **errp)
         goto unrealize;
     }
 
-    xendev->name = xendev_class->get_name(xendev, &local_err);
-    if (local_err) {
-        error_propagate_prepend(errp, local_err,
-                                "failed to get device name: ");
+    xendev->name = xendev_class->get_name(xendev, errp);
+    if (*errp) {
+        error_prepend(errp, "failed to get device name: ");
         goto unrealize;
     }
 
@@ -1274,22 +1263,19 @@ static void xen_device_realize(DeviceState *dev, Error **errp)
     xendev->feature_grant_copy =
         (xengnttab_grant_copy(xendev->xgth, 0, NULL) == 0);
 
-    xen_device_backend_create(xendev, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    xen_device_backend_create(xendev, errp);
+    if (*errp) {
         goto unrealize;
     }
 
-    xen_device_frontend_create(xendev, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    xen_device_frontend_create(xendev, errp);
+    if (*errp) {
         goto unrealize;
     }
 
     if (xendev_class->realize) {
-        xendev_class->realize(xendev, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
+        xendev_class->realize(xendev, errp);
+        if (*errp) {
             goto unrealize;
         }
     }
diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
index 1b44dcafaf..02379c341c 100644
--- a/hw/xen/xen-host-pci-device.c
+++ b/hw/xen/xen-host-pci-device.c
@@ -333,8 +333,8 @@ void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
                              uint8_t bus, uint8_t dev, uint8_t func,
                              Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     unsigned int v;
-    Error *err = NULL;
 
     d->config_fd = -1;
     d->domain = domain;
@@ -342,36 +342,36 @@ void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
     d->dev = dev;
     d->func = func;
 
-    xen_host_pci_config_open(d, &err);
-    if (err) {
+    xen_host_pci_config_open(d, errp);
+    if (*errp) {
         goto error;
     }
 
-    xen_host_pci_get_resource(d, &err);
-    if (err) {
+    xen_host_pci_get_resource(d, errp);
+    if (*errp) {
         goto error;
     }
 
-    xen_host_pci_get_hex_value(d, "vendor", &v, &err);
-    if (err) {
+    xen_host_pci_get_hex_value(d, "vendor", &v, errp);
+    if (*errp) {
         goto error;
     }
     d->vendor_id = v;
 
-    xen_host_pci_get_hex_value(d, "device", &v, &err);
-    if (err) {
+    xen_host_pci_get_hex_value(d, "device", &v, errp);
+    if (*errp) {
         goto error;
     }
     d->device_id = v;
 
-    xen_host_pci_get_dec_value(d, "irq", &v, &err);
-    if (err) {
+    xen_host_pci_get_dec_value(d, "irq", &v, errp);
+    if (*errp) {
         goto error;
     }
     d->irq = v;
 
-    xen_host_pci_get_hex_value(d, "class", &v, &err);
-    if (err) {
+    xen_host_pci_get_hex_value(d, "class", &v, errp);
+    if (*errp) {
         goto error;
     }
     d->class_code = v;
@@ -381,7 +381,6 @@ void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
     return;
 
 error:
-    error_propagate(errp, err);
 
     if (d->config_fd >= 0) {
         close(d->config_fd);
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index b91082cb8b..f57b81588e 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -767,12 +767,12 @@ static void xen_pt_destroy(PCIDevice *d) {
 
 static void xen_pt_realize(PCIDevice *d, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     XenPCIPassthroughState *s = XEN_PT_DEVICE(d);
     int i, rc = 0;
     uint8_t machine_irq = 0, scratch;
     uint16_t cmd = 0;
     int pirq = XEN_PT_UNASSIGNED_PIRQ;
-    Error *err = NULL;
 
     /* register real device */
     XEN_PT_LOG(d, "Assigning real physical device %02x:%02x.%d"
@@ -783,10 +783,9 @@ static void xen_pt_realize(PCIDevice *d, Error **errp)
     xen_host_pci_device_get(&s->real_device,
                             s->hostaddr.domain, s->hostaddr.bus,
                             s->hostaddr.slot, s->hostaddr.function,
-                            &err);
-    if (err) {
-        error_append_hint(&err, "Failed to \"open\" the real pci device");
-        error_propagate(errp, err);
+                            errp);
+    if (*errp) {
+        error_append_hint(errp, "Failed to \"open\" the real pci device");
         return;
     }
 
@@ -813,11 +812,10 @@ static void xen_pt_realize(PCIDevice *d, Error **errp)
             return;
         }
 
-        xen_pt_setup_vga(s, &s->real_device, &err);
-        if (err) {
-            error_append_hint(&err, "Setup VGA BIOS of passthrough"
-                    " GFX failed");
-            error_propagate(errp, err);
+        xen_pt_setup_vga(s, &s->real_device, errp);
+        if (*errp) {
+            error_append_hint(errp, "Setup VGA BIOS of passthrough"
+                              " GFX failed");
             xen_host_pci_device_put(&s->real_device);
             return;
         }
@@ -830,10 +828,9 @@ static void xen_pt_realize(PCIDevice *d, Error **errp)
     xen_pt_register_regions(s, &cmd);
 
     /* reinitialize each config register to be emulated */
-    xen_pt_config_init(s, &err);
-    if (err) {
-        error_append_hint(&err, "PCI Config space initialisation failed");
-        error_propagate(errp, err);
+    xen_pt_config_init(s, errp);
+    if (*errp) {
+        error_append_hint(errp, "PCI Config space initialisation failed");
         rc = -1;
         goto err_out;
     }
diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 31ec5add1d..af3fbd1bfb 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -2008,8 +2008,8 @@ static void xen_pt_config_reg_init(XenPCIPassthroughState *s,
 
 void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     int i, rc;
-    Error *err = NULL;
 
     QLIST_INIT(&s->reg_grps);
 
@@ -2052,10 +2052,9 @@ void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp)
                                                   reg_grp_offset,
                                                   &reg_grp_entry->size);
             if (rc < 0) {
-                error_setg(&err, "Failed to initialize %d/%zu, type = 0x%x,"
+                error_setg(errp, "Failed to initialize %d/%zu, type = 0x%x,"
                            " rc: %d", i, ARRAY_SIZE(xen_pt_emu_reg_grps),
                            xen_pt_emu_reg_grps[i].grp_type, rc);
-                error_propagate(errp, err);
                 xen_pt_config_delete(s);
                 return;
             }
@@ -2068,13 +2067,14 @@ void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp)
 
                 /* initialize capability register */
                 for (j = 0; regs->size != 0; j++, regs++) {
-                    xen_pt_config_reg_init(s, reg_grp_entry, regs, &err);
-                    if (err) {
-                        error_append_hint(&err, "Failed to init register %d"
-                                " offsets 0x%x in grp_type = 0x%x (%d/%zu)", j,
-                                regs->offset, xen_pt_emu_reg_grps[i].grp_type,
-                                i, ARRAY_SIZE(xen_pt_emu_reg_grps));
-                        error_propagate(errp, err);
+                    xen_pt_config_reg_init(s, reg_grp_entry, regs, errp);
+                    if (*errp) {
+                        error_append_hint(errp, "Failed to init register %d"
+                                          " offsets 0x%x in grp_type = 0x%x (%d/%zu)",
+                                          j,
+                                          regs->offset,
+                                          xen_pt_emu_reg_grps[i].grp_type,
+                                          i, ARRAY_SIZE(xen_pt_emu_reg_grps));
                         xen_pt_config_delete(s);
                         return;
                     }
-- 
2.21.0


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

Re: [Xen-devel] [PATCH v7 00/11] error: auto propagated local_err part I

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200131130118.1716-1-vsementsov@virtuozzo.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Xen-devel] [PATCH v7 00/11] error: auto propagated local_err part I
Message-id: 20200131130118.1716-1-vsementsov@virtuozzo.com
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

From https://github.com/patchew-project/qemu
   9281736..adcd6e9  master     -> master
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20200131130118.1716-1-vsementsov@virtuozzo.com -> patchew/20200131130118.1716-1-vsementsov@virtuozzo.com
Switched to a new branch 'test'
b0f4834 xen: introduce ERRP_AUTO_PROPAGATE
c704561 nbd: introduce ERRP_AUTO_PROPAGATE
238b4d2 TPM: introduce ERRP_AUTO_PROPAGATE
56af634 virtio-9p: introduce ERRP_AUTO_PROPAGATE
8dd497a fw_cfg: introduce ERRP_AUTO_PROPAGATE
cd54750 pflash: introduce ERRP_AUTO_PROPAGATE
031d7ed SD (Secure Card): introduce ERRP_AUTO_PROPAGATE
7dc91c5 hw/sd/ssi-sd: fix error handling in ssi_sd_realize
57435c4 scripts: add coccinelle script to use auto propagated errp
0883f29 error: auto propagated local_err
df0db83 qapi/error: add (Error **errp) cleaning APIs

=== OUTPUT BEGIN ===
1/11 Checking commit df0db83cd18a (qapi/error: add (Error **errp) cleaning APIs)
2/11 Checking commit 0883f296ed8f (error: auto propagated local_err)
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#139: FILE: include/qapi/error.h:427:
+#define ERRP_AUTO_PROPAGATE()                                  \
+    g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp};  \
+    errp = ((errp == NULL || *errp == error_fatal)             \
+            ? &_auto_errp_prop.local_err : errp)

total: 1 errors, 0 warnings, 101 lines checked

Patch 2/11 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

3/11 Checking commit 57435c473bf1 (scripts: add coccinelle script to use auto propagated errp)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#34: 
new file mode 100644

total: 0 errors, 1 warnings, 167 lines checked

Patch 3/11 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
4/11 Checking commit 7dc91c560263 (hw/sd/ssi-sd: fix error handling in ssi_sd_realize)
5/11 Checking commit 031d7eda4bbb (SD (Secure Card): introduce ERRP_AUTO_PROPAGATE)
6/11 Checking commit cd54750748f4 (pflash: introduce ERRP_AUTO_PROPAGATE)
7/11 Checking commit 8dd497a506bc (fw_cfg: introduce ERRP_AUTO_PROPAGATE)
8/11 Checking commit 56af634941d2 (virtio-9p: introduce ERRP_AUTO_PROPAGATE)
9/11 Checking commit 238b4d2c886f (TPM: introduce ERRP_AUTO_PROPAGATE)
10/11 Checking commit c7045610d28d (nbd: introduce ERRP_AUTO_PROPAGATE)
11/11 Checking commit b0f483460534 (xen: introduce ERRP_AUTO_PROPAGATE)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200131130118.1716-1-vsementsov@virtuozzo.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v7 00/11] error: auto propagated local_err part I

[Vladimir Sementsov-Ogievskiy]

31.01.2020 16:12, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200131130118.1716-1-vsementsov@virtuozzo.com/
> 
> 
> 
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Subject: [Xen-devel] [PATCH v7 00/11] error: auto propagated local_err part I
> Message-id: 20200131130118.1716-1-vsementsov@virtuozzo.com
> Type: series
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> git rev-parse base > /dev/null || exit 0
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> git config --local diff.algorithm histogram
> ./scripts/checkpatch.pl --mailback base..
> === TEST SCRIPT END ===
> 
>  From https://github.com/patchew-project/qemu
>     9281736..adcd6e9  master     -> master
>  From https://github.com/patchew-project/qemu
>   * [new tag]         patchew/20200131130118.1716-1-vsementsov@virtuozzo.com -> patchew/20200131130118.1716-1-vsementsov@virtuozzo.com
> Switched to a new branch 'test'
> b0f4834 xen: introduce ERRP_AUTO_PROPAGATE
> c704561 nbd: introduce ERRP_AUTO_PROPAGATE
> 238b4d2 TPM: introduce ERRP_AUTO_PROPAGATE
> 56af634 virtio-9p: introduce ERRP_AUTO_PROPAGATE
> 8dd497a fw_cfg: introduce ERRP_AUTO_PROPAGATE
> cd54750 pflash: introduce ERRP_AUTO_PROPAGATE
> 031d7ed SD (Secure Card): introduce ERRP_AUTO_PROPAGATE
> 7dc91c5 hw/sd/ssi-sd: fix error handling in ssi_sd_realize
> 57435c4 scripts: add coccinelle script to use auto propagated errp
> 0883f29 error: auto propagated local_err
> df0db83 qapi/error: add (Error **errp) cleaning APIs
> 
> === OUTPUT BEGIN ===
> 1/11 Checking commit df0db83cd18a (qapi/error: add (Error **errp) cleaning APIs)
> 2/11 Checking commit 0883f296ed8f (error: auto propagated local_err)
> ERROR: Macros with multiple statements should be enclosed in a do - while loop
> #139: FILE: include/qapi/error.h:427:
> +#define ERRP_AUTO_PROPAGATE()                                  \
> +    g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp};  \
> +    errp = ((errp == NULL || *errp == error_fatal)             \
> +            ? &_auto_errp_prop.local_err : errp)


It worth it.

> 
> total: 1 errors, 0 warnings, 101 lines checked
> 
> Patch 2/11 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> 3/11 Checking commit 57435c473bf1 (scripts: add coccinelle script to use auto propagated errp)
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #34:
> new file mode 100644
> 
> total: 0 errors, 1 warnings, 167 lines checked
> 
> Patch 3/11 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 4/11 Checking commit 7dc91c560263 (hw/sd/ssi-sd: fix error handling in ssi_sd_realize)
> 5/11 Checking commit 031d7eda4bbb (SD (Secure Card): introduce ERRP_AUTO_PROPAGATE)
> 6/11 Checking commit cd54750748f4 (pflash: introduce ERRP_AUTO_PROPAGATE)
> 7/11 Checking commit 8dd497a506bc (fw_cfg: introduce ERRP_AUTO_PROPAGATE)
> 8/11 Checking commit 56af634941d2 (virtio-9p: introduce ERRP_AUTO_PROPAGATE)
> 9/11 Checking commit 238b4d2c886f (TPM: introduce ERRP_AUTO_PROPAGATE)
> 10/11 Checking commit c7045610d28d (nbd: introduce ERRP_AUTO_PROPAGATE)
> 11/11 Checking commit b0f483460534 (xen: introduce ERRP_AUTO_PROPAGATE)
> === OUTPUT END ===
> 
> Test command exited with code: 1
> 
> 
> The full log is available at
> http://patchew.org/logs/20200131130118.1716-1-vsementsov@virtuozzo.com/testing.checkpatch/?type=message.
> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-devel@redhat.com
> 


-- 
Best regards,
Vladimir

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

Re: [Xen-devel] [PATCH v7 00/11] error: auto propagated local_err part I

[Markus Armbruster]

Hi Vladimir,

I've come to rather like your ERRP_AUTO_PROPAGATE() idea.  What I
wouldn't like is a protracted conversion.

Once we're happy with PATCH 1-3, it's a matter of running Coccinelle and
reviewing its output.  I'm confident we can converge on PATCH 1-3.

It's two weeks until soft freeze.  We need to decide whether to pursue a
partial conversion for 5.0 (basically this series plus the two patches
we identified in review of PATCH 1), or delay until 5.1.  In either
case, I want the conversion to be finished in 5.1.

Please do not feel pressured to make the 5.0 deadline.

I can queue up patches for 5.1 during the freeze.

How would you like to proceed?


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

Re: [Xen-devel] [PATCH v7 00/11] error: auto propagated local_err part I

[Vladimir Sementsov-Ogievskiy]

03.03.2020 11:01, Markus Armbruster wrote:
> Hi Vladimir,
> 
> I've come to rather like your ERRP_AUTO_PROPAGATE() idea.  What I
> wouldn't like is a protracted conversion.
> 
> Once we're happy with PATCH 1-3, it's a matter of running Coccinelle and
> reviewing its output.  I'm confident we can converge on PATCH 1-3.
> 
> It's two weeks until soft freeze.  We need to decide whether to pursue a
> partial conversion for 5.0 (basically this series plus the two patches
> we identified in review of PATCH 1), or delay until 5.1.  In either
> case, I want the conversion to be finished in 5.1.
> 
> Please do not feel pressured to make the 5.0 deadline.
> 
> I can queue up patches for 5.1 during the freeze.
> 
> How would you like to proceed?
> 

Hi Markus! Funny coincidence: exactly now (less than 1 hour ago), I've
started working for the next version for these series. So, I'm going to
resend today. Of course, I'd prefer to merge something to 5.0 if at all
possible.


-- 
Best regards,
Vladimir

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

Re: [Xen-devel] [PATCH v7 00/11] error: auto propagated local_err part I

[Markus Armbruster]

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

> 03.03.2020 11:01, Markus Armbruster wrote:
>> Hi Vladimir,
>>
>> I've come to rather like your ERRP_AUTO_PROPAGATE() idea.  What I
>> wouldn't like is a protracted conversion.
>>
>> Once we're happy with PATCH 1-3, it's a matter of running Coccinelle and
>> reviewing its output.  I'm confident we can converge on PATCH 1-3.
>>
>> It's two weeks until soft freeze.  We need to decide whether to pursue a
>> partial conversion for 5.0 (basically this series plus the two patches
>> we identified in review of PATCH 1), or delay until 5.1.  In either
>> case, I want the conversion to be finished in 5.1.
>>
>> Please do not feel pressured to make the 5.0 deadline.
>>
>> I can queue up patches for 5.1 during the freeze.
>>
>> How would you like to proceed?
>>
>
> Hi Markus! Funny coincidence: exactly now (less than 1 hour ago), I've
> started working for the next version for these series. So, I'm going to
> resend today. Of course, I'd prefer to merge something to 5.0 if at all
> possible.

That was v8, followed by v9.  We're clearly converging.  However, the
soft freeze is tomorrow already.

You've persevered with this idea for quite a while; some impatience
would be quite excusable now.  Still, I doubt part I making 5.0 matters.
The hand-written part is likely to rebase easily, and the generated part
should be regenerated instead of rebased anyway.

What actually matters is *finishing* the job.  What does that take?

* Consensus on the hand-written part.  I think we're basically there, we
  just want to work in a few more tweaks.

* Split the generated part into reviewable batches, regenerating patches
  as necessary.  Solicit review.  First batch is part of this series,
  and v9 looks ready to me.  I assume you'll prepare the remaining
  batches.

* Queue up batches as they become ready, post pull requests.  I can do
  that.

* Update the QAPI code generator to the new Error usage.  I can do that.


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

Re: [Xen-devel] [PATCH v7 00/11] error: auto propagated local_err part I

[Vladimir Sementsov-Ogievskiy]

16.03.2020 17:40, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> 03.03.2020 11:01, Markus Armbruster wrote:
>>> Hi Vladimir,
>>>
>>> I've come to rather like your ERRP_AUTO_PROPAGATE() idea.  What I
>>> wouldn't like is a protracted conversion.
>>>
>>> Once we're happy with PATCH 1-3, it's a matter of running Coccinelle and
>>> reviewing its output.  I'm confident we can converge on PATCH 1-3.
>>>
>>> It's two weeks until soft freeze.  We need to decide whether to pursue a
>>> partial conversion for 5.0 (basically this series plus the two patches
>>> we identified in review of PATCH 1), or delay until 5.1.  In either
>>> case, I want the conversion to be finished in 5.1.
>>>
>>> Please do not feel pressured to make the 5.0 deadline.
>>>
>>> I can queue up patches for 5.1 during the freeze.
>>>
>>> How would you like to proceed?
>>>
>>
>> Hi Markus! Funny coincidence: exactly now (less than 1 hour ago), I've
>> started working for the next version for these series. So, I'm going to
>> resend today. Of course, I'd prefer to merge something to 5.0 if at all
>> possible.
> 
> That was v8, followed by v9.  We're clearly converging.  However, the
> soft freeze is tomorrow already.
> 
> You've persevered with this idea for quite a while; some impatience
> would be quite excusable now.  Still, I doubt part I making 5.0 matters.

Not a problem. I hope, I'll resend soon, then it will be up to you.

> The hand-written part is likely to rebase easily, and the generated part
> should be regenerated instead of rebased anyway.
> 
> What actually matters is *finishing* the job.  What does that take?
> 
> * Consensus on the hand-written part.  I think we're basically there, we
>    just want to work in a few more tweaks.

I'll resend today, most probably it would be your version of coccinelle (if I will not find real sense in fn inheritance).

> 
> * Split the generated part into reviewable batches, regenerating patches
>    as necessary.  Solicit review.  First batch is part of this series,
>    and v9 looks ready to me.  I assume you'll prepare the remaining
>    batches.

Yes I will. This is the reward for this work: send one hundred generated patches :))

> 
> * Queue up batches as they become ready, post pull requests.  I can do
>    that.
> 
> * Update the QAPI code generator to the new Error usage.  I can do that.
> 

Great!

-- 
Best regards,
Vladimir

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