Re: [PATCH v4 23/31] block: Fix error_append_hint/error_prepend usage

[Eric Blake <eblake@redhat.com>]

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.

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