Re: [Bug 219203] New: xfsprogs-6.10.0: missing cast in /usr/include/xfs/xfs_fs.h(xfs_getparents_next_rec) causes error in C++ compilations

["Darrick J. Wong" <djwong@kernel.org>]

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@kernel.org 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@kernel-bugs.kernel.org
> >           Reporter: kernel@mattwhitlock.name
> >         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@fromorbit.com
>