On 4/10/2023 11:34 PM, Greg Kroah-Hartman wrote:
On Mon, Apr 10, 2023 at 04:32:49PM -0700, John Moon wrote:
According to this tool, it looks like we broke a lot of UAPI
headers in the previous MW (between v6.2 and v6.3-rc1).
That's not ok, and needs to be fixed, otherwise this is useless as no
one can rely on it at all.
Right, there are several classes of false positives that we've documented
and when examining thousands of commits at time, it'll flag many things.
For some comparison, if you run checkpatch on the same changeset
(v6.2..v6.3-rc1), you get 995 errors and 7,313 warnings. Still, checkpatch
is helpful for spot-checks.
checkpatch.pl does not matter, it is a "hint", and many patches
explicitly ignore it (think about patches in the staging tree, you could
fix up one checkpatch issue for a line, but ignore another one as you
are not supposed to mix them up.)
Also for some subsystems, checkpatch does not matter because their
codebase is old and follows different rules. And in some places,
checkpatch is just wrong, because it's a perl script and can not really
parse code.
So NEVER use that as a comparison to the user/kernel abi please. It's a
false comparison.
Fair enough. I was just trying to frame this tool as a "hint" as well. :)
Others do not seem to be intentional:
Addition/use of flex arrays:
- include/uapi/linux/rseq.h (f7b01bb0b57f)
- include/uapi/scsi/scsi_bsg_mpi3mr.h (c6f2e6b6eaaf)
That is not a breakage, that's a tool problem.
Type change:
- include/uapi/scsi/scsi_bsg_ufs.h (3f5145a615238)
Again, not a real breakage, size is still the same.
Would you find the tool more useful if it simply filtered out all
instances where the size of the type did not change? This would filter
out the following which the tool currently flags:
- enum expansions
- reserved field expansions
- expansions of a struct with a flex array at the end
- type changes
- re-ordering of existing members
- ...others?
These changes aren't _always_ safe, but if you assume the kernel
developer is doing something reasonable, then maybe it's okay. Maybe we
could hide these checks behind something like a "--pedantic" flag?
This logic is actually trivial to add. Filtering out issues where the
type size stays the same brings us down to 14 failures in that same MW
changeset. 8 of them are file removals and the 6 remaining flags are
additions to existing structs or intentional removals of APIs.
LMK what you think and I can work this into a v6 patch.
Additions into existing struct:
- include/uapi/drm/amdgpu_drm.h (b299221faf9b)
- include/uapi/linux/perf_event.h (09519ec3b19e)
- include/uapi/linux/virtio_blk.h (95bfec41bd3d)
Adding data to the end of a structure is a well-known way to extend the
api, in SOME instances if it is used properly.
So again, not a break.
I don't know of a way the tool could be smart enough to decide if the
additions are done properly in the proper instances. Seems appropriate
that the tool would flag these changes for closer human review.
Thanks,
John
