Message ID | 20190923161231.22028-1-vsementsov@virtuozzo.com (mailing list archive) |
---|---|
Headers | show |
Series | error: auto propagated local_err | expand |
On 9/23/19 11:12 AM, Vladimir Sementsov-Ogievskiy wrote: > 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. > > It also fixes two issues: > 1. Fix issue with error_fatal & error_append_hint: user can't see these > hints, because exit() happens in error_setg earlier than hint is > appended. [Reported by Greg Kurz] > > 2. Fix issue with 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 doesn't > local_err+error_propagate pattern, which will definitely fix the issue) > [Reported by Kevin Wolf] > > It's still an RFC, due to the following reasons: > > 1. I'm new to coccinella, so I failed to do the following pattern: > > <... > - goto out; > + return; > ...> > - out: > - error_propagate(errp, local_err) > > So, here is compilation fix 08.. Who can help with it? If nobody, 08 is > to be merged to 07 by hand. I'm not sure either; but I agree that if we can't figure out how to make Coccinelle do quite what we want, that we are better off squashing in compile fixes. Also, while I like Coccinelle for automating the conversion, it's harder to get everyone to run it; it would be nice if we could also figure out a patch to scripts/checkpatch.pl that for any instance of 'Error **errp)\n{\n' not followed by either } or the new macro, we flag that as a checkpatch warning or error. > > 2. Question about using new macro in empty stub functions - see 09 It would be nice if we could exempt empty functions - no need to use the macro if there is no function body otherwise. I'm not sure if Coccinelle can do that filtering during the conversion, or if we clean up by hand after the fact. > > 3. What to do with huge auto-generated commit 07? Should I split it > per-maintainer or per-subsystem, or leave it as-is? It's big. I'd split it into multiple patches (and reduce the cc - except for the cover letter, the rest of the patches can be limited to the actual maintainer/subsystem affected rather than everyone involved anywhere else in the series. With the current large cc, anyone that replies gets several mail bounces about "too many recipients"). It may be easier to split along directory boundaries than by maintainer boundaries. Markus has applied large tree-wide Coccinelle cleanups before, maybe he has some advice. > > 4. Also, checkpatch has some complains about 07 patch: > - using tabs.. (hmm exactly stubs functions..) > - empty ifs > Again, I don't see any ways to fix it other that by hand and merge to > 07.. Hand cleanups for formatting or compilation fixes to Coccinelle's work is not an uncommon issue after large patches; thankfully it's also not very difficult (and surprisingly needed in very few places compared to how much actually gets touched). > > ================== > > Also, if we decide, that this all is too huge, here is plan B: > > 1. apply 01 > 2. fix only functions that don't use local_err and use > error_append_hint, by just invocation of new macro at function start - > it will substitute Greg's series with no pain. > 3[optional]. Do full update for some subsystems, for example, only for > block* and nbd* Even if we go with plan B, it's still worth checking in a Coccinelle script that we can periodically run to make sure we aren't missing out on the use of the macro where it is needed. > > Vladimir Sementsov-Ogievskiy (9): > error: auto propagated local_err > qapi/error: add (Error **errp) cleaning APIs > 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 > scripts: add coccinelle script to use auto propagated errp > Use auto-propagated errp > fix-compilation: empty goto > fix-compilation: includes > > include/hw/pci-host/spapr.h | 2 + > include/monitor/hmp.h | 2 +- > include/qapi/error.h | 61 ++++- > target/ppc/kvm_ppc.h | 3 + > target/s390x/cpu_models.h | 3 + > ui/vnc.h | 2 +- > vl.c | 13 +- > scripts/coccinelle/auto-propagated-errp.cocci | 82 +++++++ > 319 files changed, 2729 insertions(+), 4245 deletions(-) > create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci The diffstat is huge, but promising.
23.09.2019 22:47, Eric Blake wrote: > On 9/23/19 11:12 AM, Vladimir Sementsov-Ogievskiy wrote: >> 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. >> >> It also fixes two issues: >> 1. Fix issue with error_fatal & error_append_hint: user can't see these >> hints, because exit() happens in error_setg earlier than hint is >> appended. [Reported by Greg Kurz] >> >> 2. Fix issue with 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 > > doesn't > >> local_err+error_propagate pattern, which will definitely fix the issue) >> [Reported by Kevin Wolf] >> >> It's still an RFC, due to the following reasons: >> >> 1. I'm new to coccinella, so I failed to do the following pattern: >> >> <... >> - goto out; >> + return; >> ...> >> - out: >> - error_propagate(errp, local_err) >> >> So, here is compilation fix 08.. Who can help with it? If nobody, 08 is >> to be merged to 07 by hand. > > I'm not sure either; but I agree that if we can't figure out how to make > Coccinelle do quite what we want, that we are better off squashing in > compile fixes. > > Also, while I like Coccinelle for automating the conversion, it's harder > to get everyone to run it; it would be nice if we could also figure out > a patch to scripts/checkpatch.pl that for any instance of 'Error > **errp)\n{\n' not followed by either } or the new macro, we flag that as > a checkpatch warning or error. > >> >> 2. Question about using new macro in empty stub functions - see 09 > > It would be nice if we could exempt empty functions - no need to use the > macro if there is no function body otherwise. I'm not sure if > Coccinelle can do that filtering during the conversion, or if we clean > up by hand after the fact. > >> >> 3. What to do with huge auto-generated commit 07? Should I split it >> per-maintainer or per-subsystem, or leave it as-is? > > It's big. I'd split it into multiple patches (and reduce the cc - except > for the cover letter, the rest of the patches can be limited to the > actual maintainer/subsystem affected rather than everyone involved > anywhere else in the series. With the current large cc, anyone that > replies gets several mail bounces about "too many recipients"). It may > be easier to split along directory boundaries than by maintainer > boundaries. Markus has applied large tree-wide Coccinelle cleanups > before, maybe he has some advice. If split by subsystem it would be 200+ patches: git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | head -1; done | sort | uniq | wc -l 205 Try to look at larger subsystem: git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | tail -2 | head -1; done | sort | uniq | wc -l 139 still too many.. Or is it OK? > >> >> 4. Also, checkpatch has some complains about 07 patch: >> - using tabs.. (hmm exactly stubs functions..) >> - empty ifs >> Again, I don't see any ways to fix it other that by hand and merge to >> 07.. > > Hand cleanups for formatting or compilation fixes to Coccinelle's work > is not an uncommon issue after large patches; thankfully it's also not > very difficult (and surprisingly needed in very few places compared to > how much actually gets touched). > >> >> ================== >> >> Also, if we decide, that this all is too huge, here is plan B: >> >> 1. apply 01 >> 2. fix only functions that don't use local_err and use >> error_append_hint, by just invocation of new macro at function start - >> it will substitute Greg's series with no pain. >> 3[optional]. Do full update for some subsystems, for example, only for >> block* and nbd* > > Even if we go with plan B, it's still worth checking in a Coccinelle > script that we can periodically run to make sure we aren't missing out > on the use of the macro where it is needed. > >> >> Vladimir Sementsov-Ogievskiy (9): >> error: auto propagated local_err >> qapi/error: add (Error **errp) cleaning APIs >> 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 >> scripts: add coccinelle script to use auto propagated errp >> Use auto-propagated errp >> fix-compilation: empty goto >> fix-compilation: includes >> >> include/hw/pci-host/spapr.h | 2 + >> include/monitor/hmp.h | 2 +- >> include/qapi/error.h | 61 ++++- >> target/ppc/kvm_ppc.h | 3 + >> target/s390x/cpu_models.h | 3 + >> ui/vnc.h | 2 +- > >> vl.c | 13 +- >> scripts/coccinelle/auto-propagated-errp.cocci | 82 +++++++ >> 319 files changed, 2729 insertions(+), 4245 deletions(-) >> create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci > > The diffstat is huge, but promising. >
On 9/24/19 9:12 AM, Vladimir Sementsov-Ogievskiy wrote: >>> 3. What to do with huge auto-generated commit 07? Should I split it >>> per-maintainer or per-subsystem, or leave it as-is? >> >> It's big. I'd split it into multiple patches (and reduce the cc - except >> for the cover letter, the rest of the patches can be limited to the >> actual maintainer/subsystem affected rather than everyone involved >> anywhere else in the series. With the current large cc, anyone that >> replies gets several mail bounces about "too many recipients"). It may >> be easier to split along directory boundaries than by maintainer >> boundaries. Markus has applied large tree-wide Coccinelle cleanups >> before, maybe he has some advice. > > > If split by subsystem it would be 200+ patches: > git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | head -1; done | sort | uniq | wc -l > 205 > > > Try to look at larger subsystem: > git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | tail -2 | head -1; done | sort | uniq | wc -l > 139 > > still too many.. Or is it OK? Hmm - that becomes a tradeoff in length of the series (where individual patches may be reviewed fast, but where the overall process may be bogged down by sheer length), vs. length of individual emails (where the email itself is daunting, but as the review is mechanical and done by automation, it becomes a matter of spot-checking if we trust that the automation was done correctly). You can probably group it in fewer patches, by joining smaller patches across a couple of subsystems. It's an art form, there's probably several ways to do it that would work, and it comes down to a judgment call on how much work you want to do to try and reduce other's work in reviewing it. Maybe even an off-hand split of gathering files until you reach about 500 or so lines per diff. I wish I had easier advice on how to tackle this sort of project in the way that will get the fastest response time. >>> vl.c | 13 +- >>> scripts/coccinelle/auto-propagated-errp.cocci | 82 +++++++ >>> 319 files changed, 2729 insertions(+), 4245 deletions(-) >>> create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci >> >> The diffstat is huge, but promising. We also learned in reviews of 7/9 that the diffstat here is misleading, the number of insertions will definitely be increasing once the Coccinelle script is fixed to insert the macro in more functions, but hopefully it's still a net reduction in overall lines.
24.09.2019 18:28, Eric Blake wrote: > On 9/24/19 9:12 AM, Vladimir Sementsov-Ogievskiy wrote: > >>>> 3. What to do with huge auto-generated commit 07? Should I split it >>>> per-maintainer or per-subsystem, or leave it as-is? >>> >>> It's big. I'd split it into multiple patches (and reduce the cc - except >>> for the cover letter, the rest of the patches can be limited to the >>> actual maintainer/subsystem affected rather than everyone involved >>> anywhere else in the series. With the current large cc, anyone that >>> replies gets several mail bounces about "too many recipients"). It may >>> be easier to split along directory boundaries than by maintainer >>> boundaries. Markus has applied large tree-wide Coccinelle cleanups >>> before, maybe he has some advice. >> >> >> If split by subsystem it would be 200+ patches: >> git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | head -1; done | sort | uniq | wc -l >> 205 >> >> >> Try to look at larger subsystem: >> git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | tail -2 | head -1; done | sort | uniq | wc -l >> 139 >> >> still too many.. Or is it OK? > > Hmm - that becomes a tradeoff in length of the series (where individual > patches may be reviewed fast, but where the overall process may be > bogged down by sheer length), vs. length of individual emails (where the > email itself is daunting, but as the review is mechanical and done by > automation, it becomes a matter of spot-checking if we trust that the > automation was done correctly). You can probably group it in fewer > patches, by joining smaller patches across a couple of subsystems. It's > an art form, there's probably several ways to do it that would work, and > it comes down to a judgment call on how much work you want to do to try > and reduce other's work in reviewing it. Maybe even an off-hand split > of gathering files until you reach about 500 or so lines per diff. I > wish I had easier advice on how to tackle this sort of project in the > way that will get the fastest response time. > > >>>> vl.c | 13 +- >>>> scripts/coccinelle/auto-propagated-errp.cocci | 82 +++++++ >>>> 319 files changed, 2729 insertions(+), 4245 deletions(-) >>>> create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci >>> >>> The diffstat is huge, but promising. > > We also learned in reviews of 7/9 that the diffstat here is misleading, > the number of insertions will definitely be increasing once the > Coccinelle script is fixed to insert the macro in more functions, but > hopefully it's still a net reduction in overall lines. > No hope for us: with fixed script I now see 919 files changed, 6425 insertions(+), 4234 deletions(-)
24.09.2019 18:44, Vladimir Sementsov-Ogievskiy wrote: > 24.09.2019 18:28, Eric Blake wrote: >> On 9/24/19 9:12 AM, Vladimir Sementsov-Ogievskiy wrote: >> >>>>> 3. What to do with huge auto-generated commit 07? Should I split it >>>>> per-maintainer or per-subsystem, or leave it as-is? >>>> >>>> It's big. I'd split it into multiple patches (and reduce the cc - except >>>> for the cover letter, the rest of the patches can be limited to the >>>> actual maintainer/subsystem affected rather than everyone involved >>>> anywhere else in the series. With the current large cc, anyone that >>>> replies gets several mail bounces about "too many recipients"). It may >>>> be easier to split along directory boundaries than by maintainer >>>> boundaries. Markus has applied large tree-wide Coccinelle cleanups >>>> before, maybe he has some advice. >>> >>> >>> If split by subsystem it would be 200+ patches: >>> git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | head -1; done | sort | uniq | wc -l >>> 205 >>> >>> >>> Try to look at larger subsystem: >>> git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | tail -2 | head -1; done | sort | uniq | wc -l >>> 139 >>> >>> still too many.. Or is it OK? >> >> Hmm - that becomes a tradeoff in length of the series (where individual >> patches may be reviewed fast, but where the overall process may be >> bogged down by sheer length), vs. length of individual emails (where the >> email itself is daunting, but as the review is mechanical and done by >> automation, it becomes a matter of spot-checking if we trust that the >> automation was done correctly). You can probably group it in fewer >> patches, by joining smaller patches across a couple of subsystems. It's >> an art form, there's probably several ways to do it that would work, and >> it comes down to a judgment call on how much work you want to do to try >> and reduce other's work in reviewing it. Maybe even an off-hand split >> of gathering files until you reach about 500 or so lines per diff. I >> wish I had easier advice on how to tackle this sort of project in the >> way that will get the fastest response time. >> >> >>>>> vl.c | 13 +- >>>>> scripts/coccinelle/auto-propagated-errp.cocci | 82 +++++++ >>>>> 319 files changed, 2729 insertions(+), 4245 deletions(-) >>>>> create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci >>>> >>>> The diffstat is huge, but promising. >> >> We also learned in reviews of 7/9 that the diffstat here is misleading, >> the number of insertions will definitely be increasing once the >> Coccinelle script is fixed to insert the macro in more functions, but >> hopefully it's still a net reduction in overall lines. >> > > No hope for us: with fixed script I now see > > 919 files changed, 6425 insertions(+), 4234 deletions(-) > Also, I have add include "qapi/error.h" to files, where errp only passed to called functions (or for functions, which are not simple stubs): # git diff | grep '+#include' | wc -l 253
24.09.2019 18:46, Vladimir Sementsov-Ogievskiy wrote: > 24.09.2019 18:44, Vladimir Sementsov-Ogievskiy wrote: >> 24.09.2019 18:28, Eric Blake wrote: >>> On 9/24/19 9:12 AM, Vladimir Sementsov-Ogievskiy wrote: >>> >>>>>> 3. What to do with huge auto-generated commit 07? Should I split it >>>>>> per-maintainer or per-subsystem, or leave it as-is? >>>>> >>>>> It's big. I'd split it into multiple patches (and reduce the cc - except >>>>> for the cover letter, the rest of the patches can be limited to the >>>>> actual maintainer/subsystem affected rather than everyone involved >>>>> anywhere else in the series. With the current large cc, anyone that >>>>> replies gets several mail bounces about "too many recipients"). It may >>>>> be easier to split along directory boundaries than by maintainer >>>>> boundaries. Markus has applied large tree-wide Coccinelle cleanups >>>>> before, maybe he has some advice. >>>> >>>> >>>> If split by subsystem it would be 200+ patches: >>>> git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | head -1; done | sort | uniq | wc -l >>>> 205 >>>> >>>> >>>> Try to look at larger subsystem: >>>> git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | tail -2 | head -1; done | sort | uniq | wc -l >>>> 139 >>>> >>>> still too many.. Or is it OK? >>> >>> Hmm - that becomes a tradeoff in length of the series (where individual >>> patches may be reviewed fast, but where the overall process may be >>> bogged down by sheer length), vs. length of individual emails (where the >>> email itself is daunting, but as the review is mechanical and done by >>> automation, it becomes a matter of spot-checking if we trust that the >>> automation was done correctly). You can probably group it in fewer >>> patches, by joining smaller patches across a couple of subsystems. It's >>> an art form, there's probably several ways to do it that would work, and >>> it comes down to a judgment call on how much work you want to do to try >>> and reduce other's work in reviewing it. Maybe even an off-hand split >>> of gathering files until you reach about 500 or so lines per diff. I >>> wish I had easier advice on how to tackle this sort of project in the >>> way that will get the fastest response time. git diff | wc -l 48941 so, by 500 lines it would be 97 patches. Seems, we'll never be able to review this thing :( Any ideas? May be, separate big patch, which just add ERRP_FUNCTION_BEGIN() to all errp functions? >>> >>> >>>>>> vl.c | 13 +- >>>>>> scripts/coccinelle/auto-propagated-errp.cocci | 82 +++++++ >>>>>> 319 files changed, 2729 insertions(+), 4245 deletions(-) >>>>>> create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci >>>>> >>>>> The diffstat is huge, but promising. >>> >>> We also learned in reviews of 7/9 that the diffstat here is misleading, >>> the number of insertions will definitely be increasing once the >>> Coccinelle script is fixed to insert the macro in more functions, but >>> hopefully it's still a net reduction in overall lines. >>> >> >> No hope for us: with fixed script I now see >> >> 919 files changed, 6425 insertions(+), 4234 deletions(-) >> > > Also, I have add include "qapi/error.h" to files, where errp only passed > to called functions (or for functions, which are not simple stubs): > > # git diff | grep '+#include' | wc -l > 253 > >
24.09.2019 18:49, Vladimir Sementsov-Ogievskiy wrote: > 24.09.2019 18:46, Vladimir Sementsov-Ogievskiy wrote: >> 24.09.2019 18:44, Vladimir Sementsov-Ogievskiy wrote: >>> 24.09.2019 18:28, Eric Blake wrote: >>>> On 9/24/19 9:12 AM, Vladimir Sementsov-Ogievskiy wrote: >>>> >>>>>>> 3. What to do with huge auto-generated commit 07? Should I split it >>>>>>> per-maintainer or per-subsystem, or leave it as-is? >>>>>> >>>>>> It's big. I'd split it into multiple patches (and reduce the cc - except >>>>>> for the cover letter, the rest of the patches can be limited to the >>>>>> actual maintainer/subsystem affected rather than everyone involved >>>>>> anywhere else in the series. With the current large cc, anyone that >>>>>> replies gets several mail bounces about "too many recipients"). It may >>>>>> be easier to split along directory boundaries than by maintainer >>>>>> boundaries. Markus has applied large tree-wide Coccinelle cleanups >>>>>> before, maybe he has some advice. >>>>> >>>>> >>>>> If split by subsystem it would be 200+ patches: >>>>> git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | head -1; done | sort | uniq | wc -l >>>>> 205 >>>>> >>>>> >>>>> Try to look at larger subsystem: >>>>> git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | tail -2 | head -1; done | sort | uniq | wc -l >>>>> 139 >>>>> >>>>> still too many.. Or is it OK? >>>> >>>> Hmm - that becomes a tradeoff in length of the series (where individual >>>> patches may be reviewed fast, but where the overall process may be >>>> bogged down by sheer length), vs. length of individual emails (where the >>>> email itself is daunting, but as the review is mechanical and done by >>>> automation, it becomes a matter of spot-checking if we trust that the >>>> automation was done correctly). You can probably group it in fewer >>>> patches, by joining smaller patches across a couple of subsystems. It's >>>> an art form, there's probably several ways to do it that would work, and >>>> it comes down to a judgment call on how much work you want to do to try >>>> and reduce other's work in reviewing it. Maybe even an off-hand split >>>> of gathering files until you reach about 500 or so lines per diff. I >>>> wish I had easier advice on how to tackle this sort of project in the >>>> way that will get the fastest response time. > > git diff | wc -l > 48941 > > so, by 500 lines it would be 97 patches. > > Seems, we'll never be able to review this thing :( > > Any ideas? > > May be, separate big patch, which just add ERRP_FUNCTION_BEGIN() to all errp functions? checked: for remaining improvements: git diff | wc -l 20218 git diff --name-only | while read f; do scripts/get_maintainer.pl -f $f --subsystem --no-rolestats 2>/dev/null | grep -v @ | tail -2 | head -1; done | sort | uniq | wc -l 90 Still, too much.. I think we should switch to plan B, at least as something that may be merged to 4.2 > >>>> >>>> >>>>>>> vl.c | 13 +- >>>>>>> scripts/coccinelle/auto-propagated-errp.cocci | 82 +++++++ >>>>>>> 319 files changed, 2729 insertions(+), 4245 deletions(-) >>>>>>> create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci >>>>>> >>>>>> The diffstat is huge, but promising. >>>> >>>> We also learned in reviews of 7/9 that the diffstat here is misleading, >>>> the number of insertions will definitely be increasing once the >>>> Coccinelle script is fixed to insert the macro in more functions, but >>>> hopefully it's still a net reduction in overall lines. >>>> >>> >>> No hope for us: with fixed script I now see >>> >>> 919 files changed, 6425 insertions(+), 4234 deletions(-) >>> >> >> Also, I have add include "qapi/error.h" to files, where errp only passed >> to called functions (or for functions, which are not simple stubs): >> >> # git diff | grep '+#include' | wc -l >> 253 >> >> > >