On Wed, Aug 28, 2024 at 07:40:15AM +1000, Dave Chinner wrote: > On Tue, Aug 27, 2024 at 09:07:03PM +0000, bugzilla-daemon@xxxxxxxxxx wrote: > > https://bugzilla.kernel.org/show_bug.cgi?id=219203 > > > > Bug ID: 219203 > > Summary: xfsprogs-6.10.0: missing cast in > > /usr/include/xfs/xfs_fs.h(xfs_getparents_next_rec) > > causes error in C++ compilations > > Product: File System > > Version: 2.5 > > Hardware: All > > OS: Linux > > Status: NEW > > Severity: normal > > Priority: P3 > > Component: XFS > > Assignee: filesystem_xfs@xxxxxxxxxxxxxxxxxxxxxx > > Reporter: kernel@xxxxxxxxxxxxxxxxx > > Regression: No > > > > C allows implicit casts from void* to any pointer type, but C++ does not. Thus, > > when including <xfs/xfs_fs.h> in a C++ compilation unit, the compiler raises > > this error: > > > > /usr/include/xfs/xfs_fs.h: In function 'xfs_getparents_rec* > > xfs_getparents_next_rec(xfs_getparents*, xfs_getparents_rec*)': > > /usr/include/xfs/xfs_fs.h:915:16: error: invalid conversion from 'void*' to > > 'xfs_getparents_rec*' [-fpermissive] > > 915 | return next; > > | ^~~~ > > | | > > | void* > > > > > > The return statement in xfs_getparents_next_rec() should have used an explicit > > cast, as the return statement in xfs_getparents_first_rec() does. > > > > --- /usr/include/xfs/xfs_fs.h > > +++ /usr/include/xfs/xfs_fs.h > > @@ -912,7 +912,7 @@ > > if (next >= end) > > return NULL; > > > > - return next; > > + return (struct xfs_getparents_rec *)next; > > } > > We shouldn't be putting static inline code in xfs_fs.h. That header > file is purely for kernel API definitions. Iterator helper functions > aren't part of the kernel API definition - they should be in some > other exported header file if they are needed at all. The helpers > could be defined in the getparents man page in the example code that > uses them rather than exposing the C code to the world... > > I note that we've recently added a static inline function type > checking function to xfs_types.h rather than it being an external > function declaration, so there's more than one header file that > needs cleanup.... XFS has been exporting tons of static inline functions via xfslibs-dev for ages: $ grep static.*inline /usr/include/xfs/ | wc -l 103 And the kernel itself has been doing that for years: $ grep static.*inline /usr/include/linux/ | wc -l 348 ...most of which don't trip g++ errors. This was the first thing that broke a build somewhere, because neither the kernel nor xfsprogs use the C++ compiler. Shouldn't code review have caught these kinds of problems? Why wasn't there any automated testing to identify these issues before they got merged? How many more guardrails do I get to build?? Or: should we add a dummy cpp source file to xfsprogs that includes xfs.h so that developers have some chance of finding potential C++ errors sooner than 6 weeks after the kernel release? So far as I can tell, this fixes the c++ compilation errors, at least with gcc 12: diff --git a/libxfs/xfs_fs.h b/libxfs/xfs_fs.h index 0613239cad13..8fc305cce06b 100644 --- a/libxfs/xfs_fs.h +++ b/libxfs/xfs_fs.h @@ -971,13 +971,13 @@ static inline struct xfs_getparents_rec * xfs_getparents_next_rec(struct xfs_getparents *gp, struct xfs_getparents_rec *gpr) { - void *next = ((void *)gpr + gpr->gpr_reclen); + void *next = ((char *)gpr + gpr->gpr_reclen); void *end = (void *)(uintptr_t)(gp->gp_buffer + gp->gp_bufsize); if (next >= end) return NULL; - return next; + return (struct xfs_getparents_rec *)next; } --D > -Dave. > -- > Dave Chinner > david@xxxxxxxxxxxxx >
