| Message ID | 1561066174-13144-1-git-send-email-sandeen@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | xfsprogs: remove unneeded #includes | expand |
On Thu, Jun 20, 2019 at 04:29:23PM -0500, Eric Sandeen wrote: > This is the result of a mechanical process and ... may have a few > oddities, for example removing "init.h" from some utils made me > realize that we inherit it from libxfs and also have it in local We do? That'd be really, really broken if we did - including local header files from a global header files is not a good idea. /me goes looking, can't find where libxfs.h includes init.h libxfs/init.h is private to libxfs/, it's not a global include file, and it is included directly in all the libxfs/*.c files that need it, which is 3 files - init.c, rdwr.c and util.c. > headers; libxfs has a global but so does scrub, etc. So that stuff > can/should be fixed up, but in the meantime, this zaps out a ton > of header dependencies, and seems worthwhile. IMO, this doesn't improve the tangle of header files in userspace. All it does is make the include patterns inconsistent across files because of the tangled mess of the libxfs/ vs include/ header files that was never completely resolved when libxfs was created as a shared kernel library.... IOWs, the include pattern I was originally aiming for with the libxfs/ shared userspace/kernel library was: #include "libxfs_priv.h" <include shared kernel header files> And for things outside libxfs/ that use libxfs: #include "libxfs.h" <include local header files> IOWs, "libxfs_priv.h" contained the includes for all the local userspace libxfs includes and defines and non-shared support structures, and it would export on build all the header files that external code needs to build into include/ via symlinks. This is incomplete - stuff like include/xfs_mount.h, xfs_inode.h, etc needs to move into libxfs as private header files (similar to how they are private in the kernel) and then exported at build time. Likewise, "libxfs.h" should only contain global include files and those exported from libxfs, and that's all the external code should include to use /anything/ from libxfs. i.e. a single include forms the external interface to libxfs. AFAIC, nothing should be including platform or build dependent things like platform_defs.h, because that should be pulled in by libxfs.h or libxfs_priv.h. And nothing external should need to pull in, say, xfs_format.h or xfs_mount.h, because they are all pulled in by include/libxfs.h (which it mostly does already). Hence I'd prefer we finish untangling the header file include mess before we cull unneceesary includes. Otherwise we are going to end up culling the wrong includes and then have to clean up that mess as well to bring the code back to being clean and consistent.... Cheers, Dave.
The whole series looks good to me:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On 6/20/19 5:06 PM, Dave Chinner wrote: > On Thu, Jun 20, 2019 at 04:29:23PM -0500, Eric Sandeen wrote: >> This is the result of a mechanical process and ... may have a few >> oddities, for example removing "init.h" from some utils made me >> realize that we inherit it from libxfs and also have it in local > > We do? That'd be really, really broken if we did - including local > header files from a global header files is not a good idea. > > /me goes looking, can't find where libxfs.h includes init.h > > libxfs/init.h is private to libxfs/, it's not a global include file, > and it is included directly in all the libxfs/*.c files that need > it, which is 3 files - init.c, rdwr.c and util.c. > >> headers; libxfs has a global but so does scrub, etc. So that stuff >> can/should be fixed up, but in the meantime, this zaps out a ton >> of header dependencies, and seems worthwhile. > > IMO, this doesn't improve the tangle of header files in userspace. > All it does is make the include patterns inconsistent across files > because of the tangled mess of the libxfs/ vs include/ header files > that was never completely resolved when libxfs was created as a > shared kernel library.... > > IOWs, the include pattern I was originally aiming for with the > libxfs/ shared userspace/kernel library was: > > #include "libxfs_priv.h" > <include shared kernel header files> > > And for things outside libxfs/ that use libxfs: > > #include "libxfs.h" > <include local header files> > > IOWs, "libxfs_priv.h" contained the includes for all the local > userspace libxfs includes and defines and non-shared support > structures, and it would export on build all the header files that > external code needs to build into include/ via symlinks. This is > incomplete - stuff like include/xfs_mount.h, xfs_inode.h, etc needs > to move into libxfs as private header files (similar to how they are > private in the kernel) and then exported at build time. > > Likewise, "libxfs.h" should only contain global include files and > those exported from libxfs, and that's all the external code should > include to use /anything/ from libxfs. i.e. a single include forms > the external interface to libxfs. > > AFAIC, nothing should be including platform or build dependent > things like platform_defs.h, because that should be pulled in by > libxfs.h or libxfs_priv.h. And nothing external should need to pull > in, say, xfs_format.h or xfs_mount.h, because they are all pulled in > by include/libxfs.h (which it mostly does already). > > Hence I'd prefer we finish untangling the header file include mess > before we cull unneceesary includes. Otherwise we are going to end > up culling the wrong includes and then have to clean up that mess as > well to bring the code back to being clean and consistent.... Dave, what if I reworked this to only remove unneeded system include files for now. Would you be more comfortable with that? I assume there's no argument with i.e. diff --git a/estimate/xfs_estimate.c b/estimate/xfs_estimate.c index 9e01cce..189bb6c 100644 --- a/estimate/xfs_estimate.c +++ b/estimate/xfs_estimate.c @@ -10,7 +10,6 @@ * XXX: assumes dirv1 format. */ #include "libxfs.h" -#include <sys/stat.h> #include <ftw.h> right?
On Thu, Jan 30, 2020 at 10:59:22AM -0600, Eric Sandeen wrote: > On 6/20/19 5:06 PM, Dave Chinner wrote: > > On Thu, Jun 20, 2019 at 04:29:23PM -0500, Eric Sandeen wrote: > >> This is the result of a mechanical process and ... may have a few > >> oddities, for example removing "init.h" from some utils made me > >> realize that we inherit it from libxfs and also have it in local > > > > We do? That'd be really, really broken if we did - including local > > header files from a global header files is not a good idea. > > > > /me goes looking, can't find where libxfs.h includes init.h > > > > libxfs/init.h is private to libxfs/, it's not a global include file, > > and it is included directly in all the libxfs/*.c files that need > > it, which is 3 files - init.c, rdwr.c and util.c. > > > >> headers; libxfs has a global but so does scrub, etc. So that stuff > >> can/should be fixed up, but in the meantime, this zaps out a ton > >> of header dependencies, and seems worthwhile. > > > > IMO, this doesn't improve the tangle of header files in userspace. > > All it does is make the include patterns inconsistent across files > > because of the tangled mess of the libxfs/ vs include/ header files > > that was never completely resolved when libxfs was created as a > > shared kernel library.... > > > > IOWs, the include pattern I was originally aiming for with the > > libxfs/ shared userspace/kernel library was: > > > > #include "libxfs_priv.h" > > <include shared kernel header files> > > > > And for things outside libxfs/ that use libxfs: > > > > #include "libxfs.h" > > <include local header files> > > > > IOWs, "libxfs_priv.h" contained the includes for all the local > > userspace libxfs includes and defines and non-shared support > > structures, and it would export on build all the header files that > > external code needs to build into include/ via symlinks. This is > > incomplete - stuff like include/xfs_mount.h, xfs_inode.h, etc needs > > to move into libxfs as private header files (similar to how they are > > private in the kernel) and then exported at build time. > > > > Likewise, "libxfs.h" should only contain global include files and > > those exported from libxfs, and that's all the external code should > > include to use /anything/ from libxfs. i.e. a single include forms > > the external interface to libxfs. > > > > AFAIC, nothing should be including platform or build dependent > > things like platform_defs.h, because that should be pulled in by > > libxfs.h or libxfs_priv.h. And nothing external should need to pull > > in, say, xfs_format.h or xfs_mount.h, because they are all pulled in > > by include/libxfs.h (which it mostly does already). > > > > Hence I'd prefer we finish untangling the header file include mess > > before we cull unneceesary includes. Otherwise we are going to end > > up culling the wrong includes and then have to clean up that mess as > > well to bring the code back to being clean and consistent.... > > Dave, what if I reworked this to only remove unneeded system include files > for now. Would you be more comfortable with that? I assume there's no > argument with i.e. > > diff --git a/estimate/xfs_estimate.c b/estimate/xfs_estimate.c > index 9e01cce..189bb6c 100644 > --- a/estimate/xfs_estimate.c > +++ b/estimate/xfs_estimate.c > @@ -10,7 +10,6 @@ > * XXX: assumes dirv1 format. > */ > #include "libxfs.h" > -#include <sys/stat.h> > #include <ftw.h> > > right? *nod* That sort of thing can easily be cleaned up. -Dave.