| Message ID | 20191001155319.8066-24-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | error: auto propagated local_err | expand |
On 10/1/19 10:53 AM, Vladimir Sementsov-Ogievskiy wrote: > 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 == &fatal_err > (the program will exit prior to the error_append_hint() or > error_prepend() call). Fix such cases. > > This commit (together with its neighbors) was generated by > > git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \ > spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \ > --in-place $f; done > > and then > > ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)" > > (auto-msg was a file with this commit message) > > and then by hand, for not maintained changed files: > > git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES> > > Still, for backporting it may be more comfortable to use only the first > command and then do one huge commit. > > Reported-by: Greg Kurz <groug@kaod.org> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > include/block/nbd.h | 1 + > 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 + > 13 files changed, 21 insertions(+) The addition of error_prepend() checking makes this patch larger than in v3. But similar to v3, I was able to come up with a matching grep query: $ git grep -np 'error_\(append_hint\|prepend\)(errp' \ block.c block/ include/block/ \ | grep '\.[ch]=' | wc -l 22 and see that grep found one more instance than coccinelle. Looking closer, I see where my diff found a spot you missed: block/qcow2-bitmap.c=1448=void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) block/qcow2-bitmap.c=1671=fail: [Ouch - there's a REASON that emacs prefers indenting labels at column '2, 5, 9, 13, ...' rather than '1, 5, 9, 13' - that's because anything at column 1 messes up determination of 'grep -p' for which function owns the code, showing the label at 1671 instead of the function qcow2_can_store_new_dirty_bitmap at 1618 - but that's a side point] > +++ b/block/qcow2-bitmap.c > @@ -1618,6 +1618,7 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, > uint32_t granularity, > Error **errp) > { > + ERRP_AUTO_PROPAGATE(); > BDRVQcow2State *s = bs->opaque; > bool found; > Qcow2BitmapList *bm_list; > diff --git a/block/qcow2.c b/block/qcow2.c Your patch is missing a patch to qcow2_store_persistent_dirty_bitmaps(), which calls error_prepend(errp, ...). However, when I manually ran the same spatch command line, I also got the same failure to include a fix in that function. I'm not sure what's wrong with the .cocci script to miss that instance; I've tried fiddling around with the .cocci file to see if I can spot any change to make (for example, using ... instead of <+...), but so far, with no success at getting the second function patched.
01.10.2019 20:09, Eric Blake wrote: > On 10/1/19 10:53 AM, Vladimir Sementsov-Ogievskiy wrote: >> 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 == &fatal_err >> (the program will exit prior to the error_append_hint() or >> error_prepend() call). Fix such cases. >> >> This commit (together with its neighbors) was generated by >> >> git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \ >> spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \ >> --in-place $f; done >> >> and then >> >> ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)" >> >> (auto-msg was a file with this commit message) >> >> and then by hand, for not maintained changed files: >> >> git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES> >> >> Still, for backporting it may be more comfortable to use only the first >> command and then do one huge commit. >> >> Reported-by: Greg Kurz <groug@kaod.org> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> include/block/nbd.h | 1 + >> 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 + >> 13 files changed, 21 insertions(+) > > The addition of error_prepend() checking makes this patch larger than in v3. > > But similar to v3, I was able to come up with a matching grep query: > > $ git grep -np 'error_\(append_hint\|prepend\)(errp' \ > block.c block/ include/block/ \ > | grep '\.[ch]=' | wc -l > 22 > > and see that grep found one more instance than coccinelle. Looking closer, I see where my diff found a spot you missed: > > block/qcow2-bitmap.c=1448=void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) > block/qcow2-bitmap.c=1671=fail: > > [Ouch - there's a REASON that emacs prefers indenting labels at column '2, 5, 9, 13, ...' rather than '1, 5, 9, 13' - that's because anything at column 1 messes up determination of 'grep -p' for which function owns the code, showing the label at 1671 instead of the function qcow2_can_store_new_dirty_bitmap at 1618 - but that's a side point] > > >> +++ b/block/qcow2-bitmap.c >> @@ -1618,6 +1618,7 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, >> uint32_t granularity, >> Error **errp) >> { >> + ERRP_AUTO_PROPAGATE(); >> BDRVQcow2State *s = bs->opaque; >> bool found; >> Qcow2BitmapList *bm_list; >> diff --git a/block/qcow2.c b/block/qcow2.c > > Your patch is missing a patch to qcow2_store_persistent_dirty_bitmaps(), which calls error_prepend(errp, ...). However, when I manually ran the same spatch command line, I also got the same failure to include a fix in that function. > > I'm not sure what's wrong with the .cocci script to miss that instance; I've tried fiddling around with the .cocci file to see if I can spot any change to make (for example, using ... instead of <+...), but so far, with no success at getting the second function patched. > Hmm, interesting. Actually I think that coccinelle is something that just works bad from the beginning of these series. And here: coccinelle works with void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) { error_prepend(errp, "Bitmap '%s' doesn't satisfy the constraints: ", name); } but failes with: void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) { QSIMPLEQ_HEAD(, Qcow2BitmapTable) drop_tables; error_prepend(errp, "Bitmap '%s' doesn't satisfy the constraints: ", name); } So, it can't parse "QSIMPLEQ_HEAD(, Qcow2BitmapTable) drop_tables" thing.. It even can't parse it without comma: void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) { QSIMPLEQ_HEAD(Qcow2BitmapTable) drop_tables; error_prepend(errp, "Bitmap '%s' doesn't satisfy the constraints: ", name); } ---- aha, it works if I add definition of QSIMPLEQ_HEAD. So, we may miss includes? adding --recursive-includes parameter to spatch leads to error: [.. a lot of failures] failed on sys/shm.h failed on sys/uio.h failed on qapi/qapi-types-error.h failed on qapi/qapi-types-crypto.h failed on sys/endian.h failed on machine/bswap.h failed on byteswap.h failed on pthread.h failed on semaphore.h failed on qapi/qapi-builtin-types.h failed on qapi/qapi-types-block-core.h failed on qapi/qapi-types-job.h failed on qcow2.h Impossible: How can diff be null and have not Correct in compare_c? Tag1 ("diff token: QEMU_PACKED VS QEMU_PACKED\nFile \"block/qcow2-bitmap.c\", line 59, column 15, charpos = 2334\n around = 'QEMU_PACKED',\n whole content = typedef struct QEMU_PACKED Qcow2BitmapDirEntry {\nFile \"/tmp/cocci-output-10311-cc4e45-qcow2-bitmap.c\", line 59, column 15, charpos = 2334\n around = 'QEMU_PACKED',\n whole content = typedef struct QEMU_PACKED Qcow2BitmapDirEntry {\n") Aha, we need -I option. Something like spatch --verbose-parsing --verbose-includes -I include -I '.' -I block --recursive-includes --sp-file scripts/coccinelle/fix-error-add-info.cocci block/qcow2-bitmap.c 2>&1 And it just can't parse our includes, queue.h for example.. So many parsing errors. ... including include/qemu/queue.h ERROR-RECOV: found sync end of #define, line 90 parsing pass2: try again ERROR-RECOV: found sync end of #define, line 90 parse error = File "include/qemu/queue.h", line 90, column 15, charpos = 4359 around = '}', whole content = { NULL } badcount: 3 bad: } bad: bad: #define QLIST_HEAD_INITIALIZER(head) \ BAD:!!!!! { NULL } ERROR-RECOV: found sync end of #define, line 150 parsing pass2: try again ERROR-RECOV: found sync end of #define, line 150 parse error = File "include/qemu/queue.h", line 150, column 47, charpos = 7628 around = '', whole content = (var) = ((var)->field.le_next)) badcount: 5 bad: } while (/*CONSTCOND*/0) bad: bad: #define QLIST_FOREACH(var, head, field) \ bad: for ((var) = ((head)->lh_first); \ bad: (var); \ BAD:!!!!! (var) = ((var)->field.le_next)) ERROR-RECOV: found sync end of #define, line 155 parsing pass2: try again ... So, it seems like coccinelle just don't work. At least it don't allow to define initializer macro. Any ideas? The series is still meaningful. Not all bugs are fixed, but at least some bugs are fixed. I'm afraid I can't put more effort on this topic, it already ate a lot of time. As an alternative I can suggest Greg to rebase his series on my patch 04 and forget about error_append and so on. Hmm or may be try some simple regex instead of coccinelle? Something as simple as substitute (^[^{}]+\([^{}]*Error \*\*errp[^{}]*\)\s*^\{)(([^}]|(?<!^)})*error_(prepend|append_hint)\(errp) by \1\n ERRP_AUTO_PROPAGATE();\2 seems work
01.10.2019 21:55, Vladimir Sementsov-Ogievskiy wrote: > 01.10.2019 20:09, Eric Blake wrote: >> On 10/1/19 10:53 AM, Vladimir Sementsov-Ogievskiy wrote: >>> 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 == &fatal_err >>> (the program will exit prior to the error_append_hint() or >>> error_prepend() call). Fix such cases. >>> >>> This commit (together with its neighbors) was generated by >>> >>> git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \ >>> spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \ >>> --in-place $f; done >>> >>> and then >>> >>> ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)" >>> >>> (auto-msg was a file with this commit message) >>> >>> and then by hand, for not maintained changed files: >>> >>> git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES> >>> >>> Still, for backporting it may be more comfortable to use only the first >>> command and then do one huge commit. >>> >>> Reported-by: Greg Kurz <groug@kaod.org> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> --- >>> include/block/nbd.h | 1 + >>> 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 + >>> 13 files changed, 21 insertions(+) >> >> The addition of error_prepend() checking makes this patch larger than in v3. >> >> But similar to v3, I was able to come up with a matching grep query: >> >> $ git grep -np 'error_\(append_hint\|prepend\)(errp' \ >> block.c block/ include/block/ \ >> | grep '\.[ch]=' | wc -l >> 22 >> >> and see that grep found one more instance than coccinelle. Looking closer, I see where my diff found a spot you missed: >> >> block/qcow2-bitmap.c=1448=void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) >> block/qcow2-bitmap.c=1671=fail: >> >> [Ouch - there's a REASON that emacs prefers indenting labels at column '2, 5, 9, 13, ...' rather than '1, 5, 9, 13' - that's because anything at column 1 messes up determination of 'grep -p' for which function owns the code, showing the label at 1671 instead of the function qcow2_can_store_new_dirty_bitmap at 1618 - but that's a side point] >> >> >>> +++ b/block/qcow2-bitmap.c >>> @@ -1618,6 +1618,7 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, >>> uint32_t granularity, >>> Error **errp) >>> { >>> + ERRP_AUTO_PROPAGATE(); >>> BDRVQcow2State *s = bs->opaque; >>> bool found; >>> Qcow2BitmapList *bm_list; >>> diff --git a/block/qcow2.c b/block/qcow2.c >> >> Your patch is missing a patch to qcow2_store_persistent_dirty_bitmaps(), which calls error_prepend(errp, ...). However, when I manually ran the same spatch command line, I also got the same failure to include a fix in that function. >> >> I'm not sure what's wrong with the .cocci script to miss that instance; I've tried fiddling around with the .cocci file to see if I can spot any change to make (for example, using ... instead of <+...), but so far, with no success at getting the second function patched. >> > > > Hmm, interesting. Actually I think that coccinelle is something that just works bad from the beginning of these series. And here: > > coccinelle works with > > void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) > { > error_prepend(errp, "Bitmap '%s' doesn't satisfy the constraints: ", > name); > } > > > but failes with: > > void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) > { > QSIMPLEQ_HEAD(, Qcow2BitmapTable) drop_tables; > > error_prepend(errp, "Bitmap '%s' doesn't satisfy the constraints: ", > name); > } > > So, it can't parse "QSIMPLEQ_HEAD(, Qcow2BitmapTable) drop_tables" thing.. > It even can't parse it without comma: > > void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) > { > QSIMPLEQ_HEAD(Qcow2BitmapTable) drop_tables; > > error_prepend(errp, "Bitmap '%s' doesn't satisfy the constraints: ", > name); > } > > ---- > > aha, it works if I add definition of QSIMPLEQ_HEAD. So, we may miss includes? > > adding --recursive-includes parameter to spatch leads to error: > > [.. a lot of failures] > failed on sys/shm.h > failed on sys/uio.h > failed on qapi/qapi-types-error.h > failed on qapi/qapi-types-crypto.h > failed on sys/endian.h > failed on machine/bswap.h > failed on byteswap.h > failed on pthread.h > failed on semaphore.h > failed on qapi/qapi-builtin-types.h > failed on qapi/qapi-types-block-core.h > failed on qapi/qapi-types-job.h > failed on qcow2.h > Impossible: How can diff be null and have not Correct in compare_c? Tag1 ("diff token: QEMU_PACKED VS QEMU_PACKED\nFile \"block/qcow2-bitmap.c\", line 59, column 15, charpos = 2334\n around = 'QEMU_PACKED',\n whole content = typedef struct QEMU_PACKED Qcow2BitmapDirEntry {\nFile \"/tmp/cocci-output-10311-cc4e45-qcow2-bitmap.c\", line 59, column 15, charpos = 2334\n around = 'QEMU_PACKED',\n whole content = typedef struct QEMU_PACKED Qcow2BitmapDirEntry {\n") > > Aha, we need -I option. Something like > > spatch --verbose-parsing --verbose-includes -I include -I '.' -I block --recursive-includes --sp-file scripts/coccinelle/fix-error-add-info.cocci block/qcow2-bitmap.c 2>&1 > > > And it just can't parse our includes, queue.h for example.. So many parsing errors. > > ... > > including include/qemu/queue.h > ERROR-RECOV: found sync end of #define, line 90 > parsing pass2: try again > ERROR-RECOV: found sync end of #define, line 90 > parse error > = File "include/qemu/queue.h", line 90, column 15, charpos = 4359 > around = '}', > whole content = { NULL } > badcount: 3 > bad: } > bad: > bad: #define QLIST_HEAD_INITIALIZER(head) \ > BAD:!!!!! { NULL } > ERROR-RECOV: found sync end of #define, line 150 > parsing pass2: try again > ERROR-RECOV: found sync end of #define, line 150 > parse error > = File "include/qemu/queue.h", line 150, column 47, charpos = 7628 > around = '', > whole content = (var) = ((var)->field.le_next)) > badcount: 5 > bad: } while (/*CONSTCOND*/0) > bad: > bad: #define QLIST_FOREACH(var, head, field) \ > bad: for ((var) = ((head)->lh_first); \ > bad: (var); \ > BAD:!!!!! (var) = ((var)->field.le_next)) > ERROR-RECOV: found sync end of #define, line 155 > parsing pass2: try again > > ... > > > So, it seems like coccinelle just don't work. At least it don't allow to define initializer macro. > > Any ideas? The series is still meaningful. Not all bugs are fixed, but at least some bugs are fixed. > > I'm afraid I can't put more effort on this topic, it already ate a lot of time. > > As an alternative I can suggest Greg to rebase his series on my patch 04 and forget about error_append > and so on. > > Hmm or may be try some simple regex instead of coccinelle? > > > Something as simple as substitute > (^[^{}]+\([^{}]*Error \*\*errp[^{}]*\)\s*^\{)(([^}]|(?<!^)})*error_(prepend|append_hint)\(errp) > > by > > \1\n ERRP_AUTO_PROPAGATE();\2 > > seems work > Hahahaha. I tried, it works. And it generate exactly same patches as in these series, except only two functions in nbd and block which you've found. It's amazing. I'll drop coccinelle script and resend. My current script looks as follows. Hmm I'd better rewrite regexp in a bit more readable manner. #!/usr/bin/env python import re import sys r = re.compile(r"(^[^{}]+\([^{}]*Error \*\*errp[^{}]*\)\s*^\{)(([^}]|(?<!^)})*error_(prepend|append_hint)\(errp)", re.M) with open(sys.argv[1]) as f: text = f.read() text = r.sub(r'\1\n ERRP_AUTO_PROPAGATE();\2', text) with open(sys.argv[1], 'w') as f: f.write(text)
On 10/1/19 1:55 PM, Vladimir Sementsov-Ogievskiy wrote: >> Your patch is missing a patch to qcow2_store_persistent_dirty_bitmaps(), which calls error_prepend(errp, ...). However, when I manually ran the same spatch command line, I also got the same failure to include a fix in that function. >> >> I'm not sure what's wrong with the .cocci script to miss that instance; I've tried fiddling around with the .cocci file to see if I can spot any change to make (for example, using ... instead of <+...), but so far, with no success at getting the second function patched. >> > > > Hmm, interesting. Actually I think that coccinelle is something that just works bad from the beginning of these series. And here: > > but failes with: > > void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) > { > QSIMPLEQ_HEAD(, Qcow2BitmapTable) drop_tables; > > error_prepend(errp, "Bitmap '%s' doesn't satisfy the constraints: ", > name); > } > > So, it can't parse "QSIMPLEQ_HEAD(, Qcow2BitmapTable) drop_tables" thing.. Generally, when running spatch, you want to include --macro-file scripts/cocci-macro-file.h to help coccinelle get past the worst of the preprocessor macros it can't otherwise handle. But that is rather sad that it ignores the entire function body as soon as it encounters a parse problem, and also sad that scripts/cocci-macro-file.h isn't yet complete enough to help coccinelle past the QSIMPLEQ_HEAD() uses in our sources. (I wonder how many other false negatives we have where we missed a code cleanup because Coccinelle silently gave up on parsing a function or file) > adding --recursive-includes parameter to spatch leads to error: > > [.. a lot of failures] > failed on sys/shm.h > failed on sys/uio.h > failed on qapi/qapi-types-error.h > failed on qapi/qapi-types-crypto.h > failed on sys/endian.h > failed on machine/bswap.h > failed on byteswap.h > failed on pthread.h > failed on semaphore.h > failed on qapi/qapi-builtin-types.h > failed on qapi/qapi-types-block-core.h > failed on qapi/qapi-types-job.h > failed on qcow2.h > Impossible: How can diff be null and have not Correct in compare_c? Tag1 ("diff token: QEMU_PACKED VS QEMU_PACKED\nFile \"block/qcow2-bitmap.c\", line 59, column 15, charpos = 2334\n around = 'QEMU_PACKED',\n whole content = typedef struct QEMU_PACKED Qcow2BitmapDirEntry {\nFile \"/tmp/cocci-output-10311-cc4e45-qcow2-bitmap.c\", line 59, column 15, charpos = 2334\n around = 'QEMU_PACKED',\n whole content = typedef struct QEMU_PACKED Qcow2BitmapDirEntry {\n") Eww - that sounds like a Coccinelle bug that we should report to their upstream. > > Aha, we need -I option. Something like > > spatch --verbose-parsing --verbose-includes -I include -I '.' -I block --recursive-includes --sp-file scripts/coccinelle/fix-error-add-info.cocci block/qcow2-bitmap.c 2>&1 > > > And it just can't parse our includes, queue.h for example.. So many parsing errors. We may not need to parse all our headers, if the --macro-file scripts/cocci-macro-file.h is sufficient. In fact, now that I found that (by reading through git log history of previous Coccinelle scripts we've run), and adding the proper --macro-file command line argument, I didn't have to add a --recursive-includes, but Coccinelle was finally able to fix that last spot in block/qcow2-bitmap.c. > So, it seems like coccinelle just don't work. At least it don't allow to define initializer macro. > > Any ideas? The series is still meaningful. Not all bugs are fixed, but at least some bugs are fixed. > Using --macro-file sscripts/cocci-macro-file.h should get it a lot further. The sad part is I don't have a quantitative way to tell how many functions/files are being silently skipped when Coccinelle runs up against something it doesn't know how to parse. > I'm afraid I can't put more effort on this topic, it already ate a lot of time. > > As an alternative I can suggest Greg to rebase his series on my patch 04 and forget about error_append > and so on. > > Hmm or may be try some simple regex instead of coccinelle? > > > Something as simple as substitute > (^[^{}]+\([^{}]*Error \*\*errp[^{}]*\)\s*^\{)(([^}]|(?<!^)})*error_(prepend|append_hint)\(errp) > > by > > \1\n ERRP_AUTO_PROPAGATE();\2 > > seems work A little more blunt (and as written, not idempotent), but as long as we document whatever pattern we use (whether coccinelle or regex) and the patch is repeatable, that's less of a concern.
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > 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 == &fatal_err > (the program will exit prior to the error_append_hint() or > error_prepend() call). Fix such cases. > > This commit (together with its neighbors) was generated by > > git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \ > spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \ > --in-place $f; done > > and then > > ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)" > > (auto-msg was a file with this commit message) > > and then by hand, for not maintained changed files: > > git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES> > > Still, for backporting it may be more comfortable to use only the first > command and then do one huge commit. > > Reported-by: Greg Kurz <groug@kaod.org> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > include/block/nbd.h | 1 + > 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 + > 13 files changed, 21 insertions(+) > > diff --git a/include/block/nbd.h b/include/block/nbd.h > index 316fd705a9..330f40142a 100644 > --- a/include/block/nbd.h > +++ b/include/block/nbd.h > @@ -360,6 +360,7 @@ void nbd_server_start(SocketAddress *addr, const char *tls_creds, > static inline int nbd_read(QIOChannel *ioc, void *buffer, size_t size, > const char *desc, Error **errp) > { > + ERRP_AUTO_PROPAGATE(); > int ret = qio_channel_read_all(ioc, buffer, size, errp) < 0 ? -EIO : 0; > > if (ret < 0) { This is an example of commit-per-subsystem.py producing a questionable split. MAINTAINERS files include/block/nbd.h under both "Block layer core" and "Network Block Device (NBD)". The script picks "Block layer core" because it comes first.
diff --git a/include/block/nbd.h b/include/block/nbd.h index 316fd705a9..330f40142a 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -360,6 +360,7 @@ void nbd_server_start(SocketAddress *addr, const char *tls_creds, static inline int nbd_read(QIOChannel *ioc, void *buffer, size_t size, const char *desc, Error **errp) { + ERRP_AUTO_PROPAGATE(); int ret = qio_channel_read_all(ioc, buffer, size, errp) < 0 ? -EIO : 0; if (ret < 0) { diff --git a/block.c b/block.c index 5944124845..5dfcfc9b90 100644 --- a/block.c +++ b/block.c @@ -1565,6 +1565,7 @@ fail_opts: static QDict *parse_json_filename(const char *filename, Error **errp) { + ERRP_AUTO_PROPAGATE(); QObject *options_obj; QDict *options; int ret; @@ -2592,6 +2593,7 @@ out: int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options, const char *bdref_key, Error **errp) { + ERRP_AUTO_PROPAGATE(); char *backing_filename = NULL; char *bdref_key_dot; const char *reference = NULL; @@ -2823,6 +2825,7 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs, QDict *snapshot_options, Error **errp) { + ERRP_AUTO_PROPAGATE(); /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */ char *tmp_filename = g_malloc0(PATH_MAX + 1); int64_t total_size; diff --git a/block/backup.c b/block/backup.c index 763f0d7ff6..58488a21fb 100644 --- a/block/backup.c +++ b/block/backup.c @@ -583,6 +583,7 @@ static const BlockJobDriver backup_job_driver = { static int64_t backup_calculate_cluster_size(BlockDriverState *target, Error **errp) { + ERRP_AUTO_PROPAGATE(); int ret; BlockDriverInfo bdi; diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 134e0c9a0c..099ed74dfa 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -237,6 +237,7 @@ static bool bdrv_dirty_bitmap_recording(BdrvDirtyBitmap *bitmap) int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags, Error **errp) { + ERRP_AUTO_PROPAGATE(); if ((flags & BDRV_BITMAP_BUSY) && bdrv_dirty_bitmap_busy(bitmap)) { error_setg(errp, "Bitmap '%s' is currently in use by another" " operation and cannot be used", bitmap->name); diff --git a/block/file-posix.c b/block/file-posix.c index f12c06de2d..db7a5b7e1a 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -320,6 +320,7 @@ static bool raw_is_io_aligned(int fd, void *buf, size_t len) static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) { + ERRP_AUTO_PROPAGATE(); BDRVRawState *s = bs->opaque; char *buf; size_t max_align = MAX(MAX_BLOCKSIZE, getpagesize()); @@ -473,6 +474,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, int bdrv_flags, int open_flags, bool device, Error **errp) { + ERRP_AUTO_PROPAGATE(); BDRVRawState *s = bs->opaque; QemuOpts *opts; Error *local_err = NULL; @@ -817,6 +819,7 @@ static int raw_handle_perm_lock(BlockDriverState *bs, uint64_t new_perm, uint64_t new_shared, Error **errp) { + ERRP_AUTO_PROPAGATE(); BDRVRawState *s = bs->opaque; int ret = 0; Error *local_err = NULL; @@ -2232,6 +2235,7 @@ static int64_t raw_get_allocated_file_size(BlockDriverState *bs) static int coroutine_fn raw_co_create(BlockdevCreateOptions *options, Error **errp) { + ERRP_AUTO_PROPAGATE(); BlockdevCreateOptionsFile *file_opts; Error *local_err = NULL; int fd; diff --git a/block/gluster.c b/block/gluster.c index 64028b2cba..a69e89b42a 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -419,6 +419,7 @@ out: static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf, Error **errp) { + ERRP_AUTO_PROPAGATE(); struct glfs *glfs; int ret; int old_errno; @@ -694,6 +695,7 @@ static int qemu_gluster_parse(BlockdevOptionsGluster *gconf, const char *filename, QDict *options, Error **errp) { + ERRP_AUTO_PROPAGATE(); int ret; if (filename) { ret = qemu_gluster_parse_uri(gconf, filename); diff --git a/block/qcow.c b/block/qcow.c index 5bdf72ba33..0caf94c8f8 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -117,6 +117,7 @@ static QemuOptsList qcow_runtime_opts = { static int qcow_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { + ERRP_AUTO_PROPAGATE(); BDRVQcowState *s = bs->opaque; unsigned int len, i, shift; int ret; diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index b2487101ed..90f2f010f2 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -1618,6 +1618,7 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs, uint32_t granularity, Error **errp) { + ERRP_AUTO_PROPAGATE(); BDRVQcow2State *s = bs->opaque; bool found; Qcow2BitmapList *bm_list; diff --git a/block/qcow2.c b/block/qcow2.c index 4d16393e61..a8f0bf3cae 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1207,6 +1207,7 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options, static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { + ERRP_AUTO_PROPAGATE(); BDRVQcow2State *s = bs->opaque; unsigned int len, i; int ret = 0; @@ -3036,6 +3037,7 @@ static uint64_t qcow2_opt_get_refcount_bits_del(QemuOpts *opts, int version, static int coroutine_fn qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp) { + ERRP_AUTO_PROPAGATE(); BlockdevCreateOptionsQcow2 *qcow2_opts; QDict *options; @@ -3740,6 +3742,7 @@ fail: static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, PreallocMode prealloc, Error **errp) { + ERRP_AUTO_PROPAGATE(); BDRVQcow2State *s = bs->opaque; uint64_t old_length; int64_t new_l1_size; diff --git a/block/vdi.c b/block/vdi.c index 806ba7f53c..5259cce24b 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -735,6 +735,7 @@ nonallocating_write: static int coroutine_fn vdi_co_do_create(BlockdevCreateOptions *create_options, size_t block_size, Error **errp) { + ERRP_AUTO_PROPAGATE(); BlockdevCreateOptionsVdi *vdi_opts; int ret = 0; uint64_t bytes = 0; diff --git a/block/vhdx-log.c b/block/vhdx-log.c index fdd3a7adc3..176e03327b 100644 --- a/block/vhdx-log.c +++ b/block/vhdx-log.c @@ -748,6 +748,7 @@ exit: int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed, Error **errp) { + ERRP_AUTO_PROPAGATE(); int ret = 0; VHDXHeader *hdr; VHDXLogSequence logs = { 0 }; diff --git a/block/vmdk.c b/block/vmdk.c index fed3b50c8a..6b10830d94 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -1078,6 +1078,7 @@ static const char *next_line(const char *s) static int vmdk_parse_extents(const char *desc, BlockDriverState *bs, QDict *options, Error **errp) { + ERRP_AUTO_PROPAGATE(); int ret; int matches; char access[11]; diff --git a/block/vpc.c b/block/vpc.c index 5cd3890780..90d0cae862 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -971,6 +971,7 @@ static int calculate_rounded_image_size(BlockdevCreateOptionsVpc *vpc_opts, static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts, Error **errp) { + ERRP_AUTO_PROPAGATE(); BlockdevCreateOptionsVpc *vpc_opts; BlockBackend *blk = NULL; BlockDriverState *bs = NULL;
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 == &fatal_err (the program will exit prior to the error_append_hint() or error_prepend() call). Fix such cases. This commit (together with its neighbors) was generated by git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \ spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \ --in-place $f; done and then ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)" (auto-msg was a file with this commit message) and then by hand, for not maintained changed files: git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES> Still, for backporting it may be more comfortable to use only the first command and then do one huge commit. Reported-by: Greg Kurz <groug@kaod.org> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- include/block/nbd.h | 1 + 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 + 13 files changed, 21 insertions(+)