| Message ID | 20191001155319.8066-1-vsementsov@virtuozzo.com |
|---|---|
| Headers | show |
| Series | error: auto propagated local_err | expand |
Patchew URL: https://patchew.org/QEMU/20191001155319.8066-1-vsementsov@virtuozzo.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20191001155319.8066-1-vsementsov@virtuozzo.com
Subject: [PATCH v4 00/31] error: auto propagated local_err
=== 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 ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
f0134cb ivshmem: Fix error_append_hint/error_prepend usage
927e8d8 PVRDMA: Fix error_append_hint/error_prepend usage
c901c43 nbd: Fix error_append_hint/error_prepend usage
a990496 Sockets: Fix error_append_hint/error_prepend usage
7e9b7be Migration: Fix error_append_hint/error_prepend usage
c37742f QOM: Fix error_append_hint/error_prepend usage
e2d8729 cmdline: Fix error_append_hint/error_prepend usage
111a147 chardev: Fix error_append_hint/error_prepend usage
7f7095d block: Fix error_append_hint/error_prepend usage
42992e2 XIVE: Fix error_append_hint/error_prepend usage
aef0d72 virtio-9p: Fix error_append_hint/error_prepend usage
fadcc18 virtio: Fix error_append_hint/error_prepend usage
0b46fe6 vhost: Fix error_append_hint/error_prepend usage
2b5a95f VFIO: Fix error_append_hint/error_prepend usage
17cff57 USB: Fix error_append_hint/error_prepend usage
b8c7bd2 SCSI: Fix error_append_hint/error_prepend usage
2833abc PCI: Fix error_append_hint/error_prepend usage
61a303e PowerNV (Non-Virtualized): Fix error_append_hint/error_prepend usage
c287bb4 Boston: Fix error_append_hint/error_prepend usage
163ff63 ASPEED BMCs: Fix error_append_hint/error_prepend usage
1eb85cb SmartFusion2: Fix error_append_hint/error_prepend usage
a29bb2d arm: Fix error_append_hint/error_prepend usage
af781f2 PowerPC TCG CPUs: Fix error_append_hint/error_prepend usage
00ceb2b ARM TCG CPUs: Fix error_append_hint/error_prepend usage
a8ac50c s390: Fix error_append_hint/error_prepend usage
82e8c8b python: add commit-per-subsystem.py
a97cab0 scripts: add script to fix error_append_hint/error_prepend usage
987fd13 error: auto propagated local_err
b209564 net/net: fix local variable shadowing in net_client_init
896b2dd hw/core/loader-fit: fix freeing errp in fit_load_fdt
cd953ee errp: rename errp to errp_in where it is IN-argument
=== OUTPUT BEGIN ===
1/31 Checking commit cd953ee1bb8c (errp: rename errp to errp_in where it is IN-argument)
2/31 Checking commit 896b2dde85c4 (hw/core/loader-fit: fix freeing errp in fit_load_fdt)
3/31 Checking commit b209564a3bf7 (net/net: fix local variable shadowing in net_client_init)
4/31 Checking commit 987fd13e64fc (error: auto propagated local_err)
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#71: FILE: include/qapi/error.h:357:
+#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, 43 lines checked
Patch 4/31 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
5/31 Checking commit a97cab09ecd5 (scripts: add script to fix error_append_hint/error_prepend usage)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#16:
new file mode 100644
total: 0 errors, 1 warnings, 28 lines checked
Patch 5/31 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
6/31 Checking commit 82e8c8bfa5e9 (python: add commit-per-subsystem.py)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#13:
new file mode 100755
total: 0 errors, 1 warnings, 69 lines checked
Patch 6/31 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
7/31 Checking commit a8ac50cc35ed (s390: Fix error_append_hint/error_prepend usage)
8/31 Checking commit 00ceb2bad505 (ARM TCG CPUs: Fix error_append_hint/error_prepend usage)
9/31 Checking commit af781f20f6a2 (PowerPC TCG CPUs: Fix error_append_hint/error_prepend usage)
10/31 Checking commit a29bb2dcb2b9 (arm: Fix error_append_hint/error_prepend usage)
11/31 Checking commit 1eb85cb68ba5 (SmartFusion2: Fix error_append_hint/error_prepend usage)
12/31 Checking commit 163ff63668f9 (ASPEED BMCs: Fix error_append_hint/error_prepend usage)
13/31 Checking commit c287bb4ab3b2 (Boston: Fix error_append_hint/error_prepend usage)
14/31 Checking commit 61a303eb291b (PowerNV (Non-Virtualized): Fix error_append_hint/error_prepend usage)
15/31 Checking commit 2833abc764d5 (PCI: Fix error_append_hint/error_prepend usage)
16/31 Checking commit b8c7bd233ee3 (SCSI: Fix error_append_hint/error_prepend usage)
17/31 Checking commit 17cff57051b2 (USB: Fix error_append_hint/error_prepend usage)
18/31 Checking commit 2b5a95f33325 (VFIO: Fix error_append_hint/error_prepend usage)
19/31 Checking commit 0b46fe629163 (vhost: Fix error_append_hint/error_prepend usage)
20/31 Checking commit fadcc18dfb3d (virtio: Fix error_append_hint/error_prepend usage)
21/31 Checking commit aef0d72aef21 (virtio-9p: Fix error_append_hint/error_prepend usage)
22/31 Checking commit 42992e22ff24 (XIVE: Fix error_append_hint/error_prepend usage)
23/31 Checking commit 7f7095d05e79 (block: Fix error_append_hint/error_prepend usage)
24/31 Checking commit 111a1472baf3 (chardev: Fix error_append_hint/error_prepend usage)
25/31 Checking commit e2d872961ff8 (cmdline: Fix error_append_hint/error_prepend usage)
26/31 Checking commit c37742f15571 (QOM: Fix error_append_hint/error_prepend usage)
27/31 Checking commit 7e9b7bef90c8 (Migration: Fix error_append_hint/error_prepend usage)
28/31 Checking commit a9904966d137 (Sockets: Fix error_append_hint/error_prepend usage)
29/31 Checking commit c901c432d406 (nbd: Fix error_append_hint/error_prepend usage)
30/31 Checking commit 927e8d8a6586 (PVRDMA: Fix error_append_hint/error_prepend usage)
31/31 Checking commit f0134cb1157d (ivshmem: Fix error_append_hint/error_prepend usage)
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20191001155319.8066-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
Reviewing this series is finally getting close to the head of my work queue. I apologize for the delay.
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > Hi all! > > Here is a proposal of auto propagation for local_err, to not call > error_propagate on every exit point, when we deal with local_err. > > There are also two issues with errp: > > 1. error_fatal & error_append_hint/error_prepend: user can't see this > additional info, because exit() happens in error_setg earlier than info > is added. [Reported by Greg Kurz] How is this series related to Greg's "[PATCH 00/17] Fix usage of error_append_hint()"? Do we need both? > 2. error_abort & error_propagate: when we wrap > error_abort by local_err+error_propagate, resulting coredump will > refer to error_propagate and not to the place where error happened. > (the macro itself don't fix the issue, but it allows to [3.] drop all > local_err+error_propagate pattern, which will definitely fix the issue) > [Reported by Kevin Wolf] > > Still, applying new macro to all errp-functions is a huge task, which is > impossible to solve in one series. > > So, here is a minimum: solve only [1.], by adding new macro to all > errp-functions which wants to call error_append_hint. > > v4; > 02: - check errp to be not NULL > - drop Eric's r-b > 03: add Eric's r-b > 04: - rename macro to ERRP_AUTO_PROPAGATE [Kevin] > - improve comment and commit msg, mention > error_prepend > 05: - handle error_prepend too > - use new macro name > - drop empty line at the end > > commit message for auto-generated commits updated, > commits regenerated. > > I'll use cc-cmd to cc appropriate recipients per patch, still > cover-letter together with 04-06 patches should be interesting for > all: [...] Big series; let me guess its structure. > Vladimir Sementsov-Ogievskiy (31): > errp: rename errp to errp_in where it is IN-argument > hw/core/loader-fit: fix freeing errp in fit_load_fdt > net/net: fix local variable shadowing in net_client_init Preparations. > error: auto propagated local_err The new idea. > scripts: add script to fix error_append_hint/error_prepend usage > python: add commit-per-subsystem.py Scripts to help you apply it. > s390: Fix error_append_hint/error_prepend usage > ARM TCG CPUs: Fix error_append_hint/error_prepend usage > PowerPC TCG CPUs: Fix error_append_hint/error_prepend usage > arm: Fix error_append_hint/error_prepend usage > SmartFusion2: Fix error_append_hint/error_prepend usage > ASPEED BMCs: Fix error_append_hint/error_prepend usage > Boston: Fix error_append_hint/error_prepend usage > PowerNV (Non-Virtualized): Fix error_append_hint/error_prepend usage > PCI: Fix error_append_hint/error_prepend usage > SCSI: Fix error_append_hint/error_prepend usage > USB: Fix error_append_hint/error_prepend usage > VFIO: Fix error_append_hint/error_prepend usage > vhost: Fix error_append_hint/error_prepend usage > virtio: Fix error_append_hint/error_prepend usage > virtio-9p: Fix error_append_hint/error_prepend usage > XIVE: Fix error_append_hint/error_prepend usage > block: Fix error_append_hint/error_prepend usage > chardev: Fix error_append_hint/error_prepend usage > cmdline: Fix error_append_hint/error_prepend usage > QOM: Fix error_append_hint/error_prepend usage > Migration: Fix error_append_hint/error_prepend usage > Sockets: Fix error_append_hint/error_prepend usage > nbd: Fix error_append_hint/error_prepend usage > PVRDMA: Fix error_append_hint/error_prepend usage > ivshmem: Fix error_append_hint/error_prepend usage Applying it. Correct? [...]
08.10.2019 10:30, Markus Armbruster wrote: > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > >> Hi all! >> >> Here is a proposal of auto propagation for local_err, to not call >> error_propagate on every exit point, when we deal with local_err. >> >> There are also two issues with errp: >> >> 1. error_fatal & error_append_hint/error_prepend: user can't see this >> additional info, because exit() happens in error_setg earlier than info >> is added. [Reported by Greg Kurz] > > How is this series related to Greg's "[PATCH 00/17] Fix usage of > error_append_hint()"? Do we need both? These series is a substitution for Greg's. Still, there are problems with automation, which Greg pointed in 21/31, and I don't know what to do next. May be, just continue to review patches and fix them by hand. May be try to improve automation... > >> 2. error_abort & error_propagate: when we wrap >> error_abort by local_err+error_propagate, resulting coredump will >> refer to error_propagate and not to the place where error happened. >> (the macro itself don't fix the issue, but it allows to [3.] drop all >> local_err+error_propagate pattern, which will definitely fix the issue) >> [Reported by Kevin Wolf] >> >> Still, applying new macro to all errp-functions is a huge task, which is >> impossible to solve in one series. >> >> So, here is a minimum: solve only [1.], by adding new macro to all >> errp-functions which wants to call error_append_hint. >> >> v4; >> 02: - check errp to be not NULL >> - drop Eric's r-b >> 03: add Eric's r-b >> 04: - rename macro to ERRP_AUTO_PROPAGATE [Kevin] >> - improve comment and commit msg, mention >> error_prepend >> 05: - handle error_prepend too >> - use new macro name >> - drop empty line at the end >> >> commit message for auto-generated commits updated, >> commits regenerated. >> >> I'll use cc-cmd to cc appropriate recipients per patch, still >> cover-letter together with 04-06 patches should be interesting for >> all: > [...] > > Big series; let me guess its structure. > >> Vladimir Sementsov-Ogievskiy (31): >> errp: rename errp to errp_in where it is IN-argument >> hw/core/loader-fit: fix freeing errp in fit_load_fdt >> net/net: fix local variable shadowing in net_client_init > > Preparations. > >> error: auto propagated local_err > > The new idea. > >> scripts: add script to fix error_append_hint/error_prepend usage >> python: add commit-per-subsystem.py > > Scripts to help you apply it. > >> s390: Fix error_append_hint/error_prepend usage >> ARM TCG CPUs: Fix error_append_hint/error_prepend usage >> PowerPC TCG CPUs: Fix error_append_hint/error_prepend usage >> arm: Fix error_append_hint/error_prepend usage >> SmartFusion2: Fix error_append_hint/error_prepend usage >> ASPEED BMCs: Fix error_append_hint/error_prepend usage >> Boston: Fix error_append_hint/error_prepend usage >> PowerNV (Non-Virtualized): Fix error_append_hint/error_prepend usage >> PCI: Fix error_append_hint/error_prepend usage >> SCSI: Fix error_append_hint/error_prepend usage >> USB: Fix error_append_hint/error_prepend usage >> VFIO: Fix error_append_hint/error_prepend usage >> vhost: Fix error_append_hint/error_prepend usage >> virtio: Fix error_append_hint/error_prepend usage >> virtio-9p: Fix error_append_hint/error_prepend usage >> XIVE: Fix error_append_hint/error_prepend usage >> block: Fix error_append_hint/error_prepend usage >> chardev: Fix error_append_hint/error_prepend usage >> cmdline: Fix error_append_hint/error_prepend usage >> QOM: Fix error_append_hint/error_prepend usage >> Migration: Fix error_append_hint/error_prepend usage >> Sockets: Fix error_append_hint/error_prepend usage >> nbd: Fix error_append_hint/error_prepend usage >> PVRDMA: Fix error_append_hint/error_prepend usage >> ivshmem: Fix error_append_hint/error_prepend usage > > Applying it. > > Correct? > Yes
On Tue, 8 Oct 2019 08:41:08 +0000 Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > 08.10.2019 10:30, Markus Armbruster wrote: > > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > > > >> Hi all! > >> > >> Here is a proposal of auto propagation for local_err, to not call > >> error_propagate on every exit point, when we deal with local_err. > >> > >> There are also two issues with errp: > >> > >> 1. error_fatal & error_append_hint/error_prepend: user can't see this > >> additional info, because exit() happens in error_setg earlier than info > >> is added. [Reported by Greg Kurz] > > > > How is this series related to Greg's "[PATCH 00/17] Fix usage of > > error_append_hint()"? Do we need both? > > These series is a substitution for Greg's. Still, there are problems with > automation, which Greg pointed in 21/31, and I don't know what to do next. > > May be, just continue to review patches and fix them by hand. May be try to > improve automation... > The feeling I have after working on my series is that the lines that deal with errors are mixed up with the functional code in a variety of ways. That makes it very difficult if not impossible to come with code patterns suitable for a 100% automated solution IMHO. My questioning is more around the semantics of error_fatal actually. What does passing &error_fatal gives us over passing &local_err and calling error_report_err()+exit(), apart from breaking error_append_hint() and error_prepend() ? > > > >> 2. error_abort & error_propagate: when we wrap > >> error_abort by local_err+error_propagate, resulting coredump will > >> refer to error_propagate and not to the place where error happened. > >> (the macro itself don't fix the issue, but it allows to [3.] drop all > >> local_err+error_propagate pattern, which will definitely fix the issue) > >> [Reported by Kevin Wolf] > >> > >> Still, applying new macro to all errp-functions is a huge task, which is > >> impossible to solve in one series. > >> > >> So, here is a minimum: solve only [1.], by adding new macro to all > >> errp-functions which wants to call error_append_hint. > >> > >> v4; > >> 02: - check errp to be not NULL > >> - drop Eric's r-b > >> 03: add Eric's r-b > >> 04: - rename macro to ERRP_AUTO_PROPAGATE [Kevin] > >> - improve comment and commit msg, mention > >> error_prepend > >> 05: - handle error_prepend too > >> - use new macro name > >> - drop empty line at the end > >> > >> commit message for auto-generated commits updated, > >> commits regenerated. > >> > >> I'll use cc-cmd to cc appropriate recipients per patch, still > >> cover-letter together with 04-06 patches should be interesting for > >> all: > > [...] > > > > Big series; let me guess its structure. > > > >> Vladimir Sementsov-Ogievskiy (31): > >> errp: rename errp to errp_in where it is IN-argument > >> hw/core/loader-fit: fix freeing errp in fit_load_fdt > >> net/net: fix local variable shadowing in net_client_init > > > > Preparations. > > > >> error: auto propagated local_err > > > > The new idea. > > > >> scripts: add script to fix error_append_hint/error_prepend usage > >> python: add commit-per-subsystem.py > > > > Scripts to help you apply it. > > > >> s390: Fix error_append_hint/error_prepend usage > >> ARM TCG CPUs: Fix error_append_hint/error_prepend usage > >> PowerPC TCG CPUs: Fix error_append_hint/error_prepend usage > >> arm: Fix error_append_hint/error_prepend usage > >> SmartFusion2: Fix error_append_hint/error_prepend usage > >> ASPEED BMCs: Fix error_append_hint/error_prepend usage > >> Boston: Fix error_append_hint/error_prepend usage > >> PowerNV (Non-Virtualized): Fix error_append_hint/error_prepend usage > >> PCI: Fix error_append_hint/error_prepend usage > >> SCSI: Fix error_append_hint/error_prepend usage > >> USB: Fix error_append_hint/error_prepend usage > >> VFIO: Fix error_append_hint/error_prepend usage > >> vhost: Fix error_append_hint/error_prepend usage > >> virtio: Fix error_append_hint/error_prepend usage > >> virtio-9p: Fix error_append_hint/error_prepend usage > >> XIVE: Fix error_append_hint/error_prepend usage > >> block: Fix error_append_hint/error_prepend usage > >> chardev: Fix error_append_hint/error_prepend usage > >> cmdline: Fix error_append_hint/error_prepend usage > >> QOM: Fix error_append_hint/error_prepend usage > >> Migration: Fix error_append_hint/error_prepend usage > >> Sockets: Fix error_append_hint/error_prepend usage > >> nbd: Fix error_append_hint/error_prepend usage > >> PVRDMA: Fix error_append_hint/error_prepend usage > >> ivshmem: Fix error_append_hint/error_prepend usage > > > > Applying it. > > > > Correct? > > > > Yes > >
08.10.2019 12:39, Greg Kurz wrote: > On Tue, 8 Oct 2019 08:41:08 +0000 > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > >> 08.10.2019 10:30, Markus Armbruster wrote: >>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: >>> >>>> Hi all! >>>> >>>> Here is a proposal of auto propagation for local_err, to not call >>>> error_propagate on every exit point, when we deal with local_err. >>>> >>>> There are also two issues with errp: >>>> >>>> 1. error_fatal & error_append_hint/error_prepend: user can't see this >>>> additional info, because exit() happens in error_setg earlier than info >>>> is added. [Reported by Greg Kurz] >>> >>> How is this series related to Greg's "[PATCH 00/17] Fix usage of >>> error_append_hint()"? Do we need both? >> >> These series is a substitution for Greg's. Still, there are problems with >> automation, which Greg pointed in 21/31, and I don't know what to do next. >> >> May be, just continue to review patches and fix them by hand. May be try to >> improve automation... >> > > The feeling I have after working on my series is that the lines that deal > with errors are mixed up with the functional code in a variety of ways. > That makes it very difficult if not impossible to come with code patterns > suitable for a 100% automated solution IMHO. > > My questioning is more around the semantics of error_fatal actually. What > does passing &error_fatal gives us over passing &local_err and calling > error_report_err()+exit(), apart from breaking error_append_hint() and > error_prepend() ? As I understand, the only benefit is one line instead of four: func(..., &error_fatal); instead of func(..., &local_err); if (local_err) { exit(1); } But, keeping in mind all difficulties about these series... We can consider conversion error_fatal -> local_err too. It seems simple to do with a coccinelle script, I can send another automatic series to look at it. Hmm, some ideas around this: 1. We can generate _fatal versions of functions by python script (we'll call it from Makefile, we have a lot of generated code anyway). and convert func(..., &local_err); to func_fatal(...); 2. Use macro like #define FATAL(func, ...) do { Error *__fatal_err = NULL; func(__VA_ARGS__ __VA_OPT(,), &__fatal_err); if (__fatal_err) { error_report(__fatal_err); exit(1); } } while (0) and convert func(..., &local_err); to FATAL(func, ...); > >>> >>>> 2. error_abort & error_propagate: when we wrap >>>> error_abort by local_err+error_propagate, resulting coredump will >>>> refer to error_propagate and not to the place where error happened. >>>> (the macro itself don't fix the issue, but it allows to [3.] drop all >>>> local_err+error_propagate pattern, which will definitely fix the issue) >>>> [Reported by Kevin Wolf] >>>> >>>> Still, applying new macro to all errp-functions is a huge task, which is >>>> impossible to solve in one series. >>>> >>>> So, here is a minimum: solve only [1.], by adding new macro to all >>>> errp-functions which wants to call error_append_hint. >>>> >>>> v4; >>>> 02: - check errp to be not NULL >>>> - drop Eric's r-b >>>> 03: add Eric's r-b >>>> 04: - rename macro to ERRP_AUTO_PROPAGATE [Kevin] >>>> - improve comment and commit msg, mention >>>> error_prepend >>>> 05: - handle error_prepend too >>>> - use new macro name >>>> - drop empty line at the end >>>> >>>> commit message for auto-generated commits updated, >>>> commits regenerated. >>>> >>>> I'll use cc-cmd to cc appropriate recipients per patch, still >>>> cover-letter together with 04-06 patches should be interesting for >>>> all: >>> [...] >>> >>> Big series; let me guess its structure. >>> >>>> Vladimir Sementsov-Ogievskiy (31): >>>> errp: rename errp to errp_in where it is IN-argument >>>> hw/core/loader-fit: fix freeing errp in fit_load_fdt >>>> net/net: fix local variable shadowing in net_client_init >>> >>> Preparations. >>> >>>> error: auto propagated local_err >>> >>> The new idea. >>> >>>> scripts: add script to fix error_append_hint/error_prepend usage >>>> python: add commit-per-subsystem.py >>> >>> Scripts to help you apply it. >>> >>>> s390: Fix error_append_hint/error_prepend usage >>>> ARM TCG CPUs: Fix error_append_hint/error_prepend usage >>>> PowerPC TCG CPUs: Fix error_append_hint/error_prepend usage >>>> arm: Fix error_append_hint/error_prepend usage >>>> SmartFusion2: Fix error_append_hint/error_prepend usage >>>> ASPEED BMCs: Fix error_append_hint/error_prepend usage >>>> Boston: Fix error_append_hint/error_prepend usage >>>> PowerNV (Non-Virtualized): Fix error_append_hint/error_prepend usage >>>> PCI: Fix error_append_hint/error_prepend usage >>>> SCSI: Fix error_append_hint/error_prepend usage >>>> USB: Fix error_append_hint/error_prepend usage >>>> VFIO: Fix error_append_hint/error_prepend usage >>>> vhost: Fix error_append_hint/error_prepend usage >>>> virtio: Fix error_append_hint/error_prepend usage >>>> virtio-9p: Fix error_append_hint/error_prepend usage >>>> XIVE: Fix error_append_hint/error_prepend usage >>>> block: Fix error_append_hint/error_prepend usage >>>> chardev: Fix error_append_hint/error_prepend usage >>>> cmdline: Fix error_append_hint/error_prepend usage >>>> QOM: Fix error_append_hint/error_prepend usage >>>> Migration: Fix error_append_hint/error_prepend usage >>>> Sockets: Fix error_append_hint/error_prepend usage >>>> nbd: Fix error_append_hint/error_prepend usage >>>> PVRDMA: Fix error_append_hint/error_prepend usage >>>> ivshmem: Fix error_append_hint/error_prepend usage >>> >>> Applying it. >>> >>> Correct? >>> >> >> Yes >> >> >
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > 08.10.2019 12:39, Greg Kurz wrote: >> On Tue, 8 Oct 2019 08:41:08 +0000 >> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: >> >>> 08.10.2019 10:30, Markus Armbruster wrote: >>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: >>>> >>>>> Hi all! >>>>> >>>>> Here is a proposal of auto propagation for local_err, to not call >>>>> error_propagate on every exit point, when we deal with local_err. >>>>> >>>>> There are also two issues with errp: >>>>> >>>>> 1. error_fatal & error_append_hint/error_prepend: user can't see this >>>>> additional info, because exit() happens in error_setg earlier than info >>>>> is added. [Reported by Greg Kurz] >>>> >>>> How is this series related to Greg's "[PATCH 00/17] Fix usage of >>>> error_append_hint()"? Do we need both? >>> >>> These series is a substitution for Greg's. Still, there are problems with >>> automation, which Greg pointed in 21/31, and I don't know what to do next. >>> >>> May be, just continue to review patches and fix them by hand. May be try to >>> improve automation... >>> >> >> The feeling I have after working on my series is that the lines that deal >> with errors are mixed up with the functional code in a variety of ways. >> That makes it very difficult if not impossible to come with code patterns >> suitable for a 100% automated solution IMHO. >> >> My questioning is more around the semantics of error_fatal actually. What >> does passing &error_fatal gives us over passing &local_err and calling >> error_report_err()+exit(), apart from breaking error_append_hint() and >> error_prepend() ? > > As I understand, the only benefit is one line instead of four: Brevity matters when it comes to error handling. > func(..., &error_fatal); > > instead of > > func(..., &local_err); > if (local_err) { error_report_err(local_err); > exit(1); > } [...]
08.10.2019 13:09, Vladimir Sementsov-Ogievskiy wrote: > 08.10.2019 12:39, Greg Kurz wrote: >> On Tue, 8 Oct 2019 08:41:08 +0000 >> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: >> >>> 08.10.2019 10:30, Markus Armbruster wrote: >>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: >>>> >>>>> Hi all! >>>>> >>>>> Here is a proposal of auto propagation for local_err, to not call >>>>> error_propagate on every exit point, when we deal with local_err. >>>>> >>>>> There are also two issues with errp: >>>>> >>>>> 1. error_fatal & error_append_hint/error_prepend: user can't see this >>>>> additional info, because exit() happens in error_setg earlier than info >>>>> is added. [Reported by Greg Kurz] >>>> >>>> How is this series related to Greg's "[PATCH 00/17] Fix usage of >>>> error_append_hint()"? Do we need both? >>> >>> These series is a substitution for Greg's. Still, there are problems with >>> automation, which Greg pointed in 21/31, and I don't know what to do next. >>> >>> May be, just continue to review patches and fix them by hand. May be try to >>> improve automation... >>> >> >> The feeling I have after working on my series is that the lines that deal >> with errors are mixed up with the functional code in a variety of ways. >> That makes it very difficult if not impossible to come with code patterns >> suitable for a 100% automated solution IMHO. >> >> My questioning is more around the semantics of error_fatal actually. What >> does passing &error_fatal gives us over passing &local_err and calling >> error_report_err()+exit(), apart from breaking error_append_hint() and >> error_prepend() ? > > As I understand, the only benefit is one line instead of four: > > func(..., &error_fatal); > > instead of > > func(..., &local_err); > if (local_err) { > exit(1); > } > > But, keeping in mind all difficulties about these series... We can consider > conversion error_fatal -> local_err too. It seems simple to do with a coccinelle > script, I can send another automatic series to look at it. > > > Hmm, some ideas around this: > > 1. We can generate _fatal versions of functions by python script (we'll call it from Makefile, we have a lot of generated code anyway). > > and convert > func(..., &local_err); to > > func_fatal(...); > > 2. Use macro like > > #define FATAL(func, ...) do { Error *__fatal_err = NULL; func(__VA_ARGS__ __VA_OPT(,), &__fatal_err); if (__fatal_err) { error_report(__fatal_err); exit(1); } } while (0) > > and convert > func(..., &local_err); to > > FATAL(func, ...); Now, I think, that what these series do is better, as func(..., &fatal_err) and one macro invocation at function start is better than too many generated functions.
Hi all! Here is a proposal of auto propagation for local_err, to not call error_propagate on every exit point, when we deal with local_err. There are also two issues with errp: 1. error_fatal & error_append_hint/error_prepend: user can't see this additional info, because exit() happens in error_setg earlier than info is added. [Reported by Greg Kurz] 2. error_abort & error_propagate: when we wrap error_abort by local_err+error_propagate, resulting coredump will refer to error_propagate and not to the place where error happened. (the macro itself don't fix the issue, but it allows to [3.] drop all local_err+error_propagate pattern, which will definitely fix the issue) [Reported by Kevin Wolf] Still, applying new macro to all errp-functions is a huge task, which is impossible to solve in one series. So, here is a minimum: solve only [1.], by adding new macro to all errp-functions which wants to call error_append_hint. v4; 02: - check errp to be not NULL - drop Eric's r-b 03: add Eric's r-b 04: - rename macro to ERRP_AUTO_PROPAGATE [Kevin] - improve comment and commit msg, mention error_prepend 05: - handle error_prepend too - use new macro name - drop empty line at the end commit message for auto-generated commits updated, commits regenerated. I'll use cc-cmd to cc appropriate recipients per patch, still cover-letter together with 04-06 patches should be interesting for all: CC: kwolf@redhat.com CC: mreitz@redhat.com CC: jsnow@redhat.com CC: fam@euphon.net CC: sw@weilnetz.de CC: codyprime@gmail.com CC: marcandre.lureau@redhat.com CC: pbonzini@redhat.com CC: groug@kaod.org CC: sundeep.lkml@gmail.com CC: peter.maydell@linaro.org CC: stefanha@redhat.com CC: pburton@wavecomp.com CC: arikalo@wavecomp.com CC: berrange@redhat.com CC: ehabkost@redhat.com CC: david@gibson.dropbear.id.au CC: clg@kaod.org CC: mst@redhat.com CC: marcel.apfelbaum@gmail.com CC: mark.cave-ayland@ilande.co.uk CC: yuval.shaia@oracle.com CC: cohuck@redhat.com CC: farman@linux.ibm.com CC: rth@twiddle.net CC: david@redhat.com CC: pasic@linux.ibm.com CC: borntraeger@de.ibm.com CC: kraxel@redhat.com CC: alex.williamson@redhat.com CC: andrew@aj.id.au CC: joel@jms.id.au CC: eblake@redhat.com CC: armbru@redhat.com CC: mdroth@linux.vnet.ibm.com CC: quintela@redhat.com CC: dgilbert@redhat.com CC: jasowang@redhat.com CC: qemu-block@nongnu.org CC: integration@gluster.org CC: qemu-arm@nongnu.org CC: qemu-ppc@nongnu.org CC: qemu-s390x@nongnu.org Vladimir Sementsov-Ogievskiy (31): errp: rename errp to errp_in where it is IN-argument hw/core/loader-fit: fix freeing errp in fit_load_fdt net/net: fix local variable shadowing in net_client_init error: auto propagated local_err scripts: add script to fix error_append_hint/error_prepend usage python: add commit-per-subsystem.py s390: Fix error_append_hint/error_prepend usage ARM TCG CPUs: Fix error_append_hint/error_prepend usage PowerPC TCG CPUs: Fix error_append_hint/error_prepend usage arm: Fix error_append_hint/error_prepend usage SmartFusion2: Fix error_append_hint/error_prepend usage ASPEED BMCs: Fix error_append_hint/error_prepend usage Boston: Fix error_append_hint/error_prepend usage PowerNV (Non-Virtualized): Fix error_append_hint/error_prepend usage PCI: Fix error_append_hint/error_prepend usage SCSI: Fix error_append_hint/error_prepend usage USB: Fix error_append_hint/error_prepend usage VFIO: Fix error_append_hint/error_prepend usage vhost: Fix error_append_hint/error_prepend usage virtio: Fix error_append_hint/error_prepend usage virtio-9p: Fix error_append_hint/error_prepend usage XIVE: Fix error_append_hint/error_prepend usage block: Fix error_append_hint/error_prepend usage chardev: Fix error_append_hint/error_prepend usage cmdline: Fix error_append_hint/error_prepend usage QOM: Fix error_append_hint/error_prepend usage Migration: Fix error_append_hint/error_prepend usage Sockets: Fix error_append_hint/error_prepend usage nbd: Fix error_append_hint/error_prepend usage PVRDMA: Fix error_append_hint/error_prepend usage ivshmem: Fix error_append_hint/error_prepend usage include/block/nbd.h | 1 + include/monitor/hmp.h | 2 +- include/qapi/error.h | 39 +++++++++++- ui/vnc.h | 2 +- block.c | 3 + block/backup.c | 1 + block/dirty-bitmap.c | 1 + block/file-posix.c | 4 ++ block/gluster.c | 2 + block/qcow.c | 1 + block/qcow2-bitmap.c | 1 + block/qcow2.c | 3 + block/vdi.c | 1 + block/vhdx-log.c | 1 + block/vmdk.c | 1 + block/vpc.c | 1 + chardev/spice.c | 1 + hw/9pfs/9p-local.c | 1 + hw/9pfs/9p-proxy.c | 1 + hw/9pfs/9p.c | 1 + hw/arm/msf2-soc.c | 1 + hw/arm/virt.c | 2 + hw/block/dataplane/virtio-blk.c | 1 + hw/core/loader-fit.c | 7 ++- hw/core/qdev-properties-system.c | 1 + hw/intc/arm_gicv3_kvm.c | 1 + hw/intc/pnv_xive.c | 1 + hw/intc/xive.c | 3 + hw/misc/ivshmem.c | 1 + hw/misc/msf2-sysreg.c | 1 + hw/pci-bridge/pcie_root_port.c | 1 + hw/ppc/mac_newworld.c | 1 + hw/ppc/pnv_lpc.c | 1 + hw/ppc/pnv_occ.c | 1 + hw/ppc/spapr.c | 1 + hw/ppc/spapr_irq.c | 1 + hw/ppc/spapr_pci.c | 1 + hw/rdma/vmw/pvrdma_main.c | 1 + hw/s390x/s390-ccw.c | 1 + hw/scsi/scsi-disk.c | 1 + hw/scsi/scsi-generic.c | 1 + hw/scsi/vhost-scsi.c | 1 + hw/usb/ccid-card-emulated.c | 1 + hw/vfio/common.c | 3 + hw/vfio/pci-quirks.c | 1 + hw/vfio/pci.c | 3 + hw/vfio/platform.c | 1 + hw/virtio/vhost-vsock.c | 1 + hw/virtio/virtio-pci.c | 2 + hw/watchdog/wdt_aspeed.c | 1 + migration/migration.c | 1 + migration/savevm.c | 2 + monitor/hmp-cmds.c | 8 +-- nbd/client.c | 5 ++ nbd/server.c | 4 ++ net/net.c | 17 +++-- qdev-monitor.c | 2 + target/ppc/kvm.c | 2 + target/s390x/cpu_models.c | 2 + ui/vnc.c | 10 +-- util/error.c | 8 +-- util/qemu-option.c | 2 + util/qemu-sockets.c | 2 + python/commit-per-subsystem.py | 69 +++++++++++++++++++++ scripts/coccinelle/fix-error-add-info.cocci | 28 +++++++++ 65 files changed, 247 insertions(+), 27 deletions(-) create mode 100755 python/commit-per-subsystem.py create mode 100644 scripts/coccinelle/fix-error-add-info.cocci