[GEDI] [PATCH v4 23/31] block: Fix error_append_hint/error_prepend usage
Vladimir Sementsov-Ogievskiy
vsementsov at virtuozzo.com
Tue Oct 1 19:12:27 UTC 2019
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 at kaod.org>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov at 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)
--
Best regards,
Vladimir
More information about the integration
mailing list