Re: [Qemu-devel] checkpatch.pl question

[Stefan Weil]

Am 07.06.2014 16:58, schrieb Peter Maydell:
> On 6 June 2014 08:17, Markus Armbruster <address@hidden> wrote:
>> --debug values=1 produces
>>
>> 188 > . extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
>> 188 > EEVVVVVVVTTTTVVVVVVVVVVVVVVVVVVVVNTTTTTTTTTTTTTTVVCCTTTTTTTTVVVVVVVCC
>>
>> which suggests it recognizes the declaration just fine.
>>
>> Copying a few possible victims^Wexperts.
> 
> It's clearly something to do with it getting confused by the type name,
> because you can suppress the warning by just changing it so it has
> an "_t" suffix, for instance. In particular notice that the set of allowed
> type names constructed by the build_types() subroutine is extremely
> limited: it looks to me as if the script makes assumptions based on
> kernel style (where 'struct foo' is preferred over a typedef and plain
> 'foo') that allow it to assume that if it's not one of a very few allowed
> formats then it's not a type name.
> 
> I find this script extremely hard to understand, though.
> 
> thanks
> -- PMM
> 

Yes, but that's only part of the story. checkpatch.pl contains some
special handling for the standard C data types and also knows some Linux
data type patterns. It also handles the above case correctly in most
circumstances because there is special code for "*" used in function
argument lists.

Here checkpatch.pl gets confused by a totally different line of the patch:

 type_init(register_vfio_pci_dev_type)

Simply adding a semicolon at the end of that line would help, but is of
course not correct (note that there is already some QEMU code which adds
such a semicolon and which should be fixed). It's generally very
difficult or even impossible for code analysers with a limited view to
analyse macro calls correctly. A human reviewer has the same problem.

checkpatch.pl could be improved to handle the patch correctly: the
type_init line and the line which raises the error message belong to
different files. Obviously some global state variables are not reset
when checkpatch.pl detects patch code belonging to a new file. I'm still
searching which ones. A similar problem might also occur when more than
one patch is checked: this also does not reset all relevant variables
when switching from one patch file to the next one. The original Linux
version of checkpatch.pl shows the same bug.

Regards
Stefan

PS. I just sent a first patch for checkpatch.pl which is totally
unrelated to the above problem, but which I noticed while I investigated it.