Some missing.h polish#443
Conversation
The missing.h header is aimed to be used as fallback for platforms/setups where relevant functionality is missing. On the other hand, we explicitly avoid the kernel API header - as mentioned in the comment. Move the defines and tweak the comment a bit. Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
Drop the round-about resolution of basename and finit_module via the libkmod-internal.h include. Instead pull the header missing.h where it's actually used... This high-lights that we're missing an errno.h include in a couple of places, so let's fix that while we're here. Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| /* | ||
| * (Re-)define some linux/module.h macros. | ||
| * | ||
| * In practice, most people/distros do not have the relevant package/header during the |
There was a problem hiding this comment.
I'm not sure this comment is true anymore... we could probably just require the header
| #include <sys/stat.h> | ||
| #include <sys/types.h> | ||
|
|
||
| #include <shared/missing.h> // For basename |
There was a problem hiding this comment.
I don't understand the reason for doing this. The "missing.h" is exactly the thing you don't want to include explicitly : the idea is that you only include the headers you actually need and if the libc is missing something, it gets resolved by this header.
Also, this patch does something I don't really like: add a comment "// For xyz...". This is not really maintainable - what happens when this file also needs something else from missing.h?
There's another approach I like for this kind of stuff that spreads the definitions to the headers they are expected on in an additional include like this:
// shared/missing/linux/module.h
#include_next <linux/module.h>
#ifndef ....
#define ...
#endif
for basename:
// shared/missing/libgen.h
#error ...
// shared/missing/string.h
#include_next <string.h>
#if !HAVE_DECL_BASENAME
static inline const char *basename(const char *s)
{
const char *p = strrchr(s, '/');
return p ? p + 1 : s;
}
#endif
And then add shared/missing/ early in the include path. But I'm not sure it's worth it for our small use missing definitions.
There was a problem hiding this comment.
Fwiw looking at missing.h reminds be of gnulib and some interesting PTSD, of sorts 😅
IMHO missing.h focuses on the wrong aspect - availability - instead of the platform (OS or c runtime), standard or the header which provides the functionality. In addition, it actively masks missing includes and makes things more fragile.
In particular, I've been itching to have macOS/windows build of depmod and modinfo. Plus would be an interesting exercise to have the Android build also covered - AOSP is lagging at kmod v32.
Having standalone headers was my other option, let me try that. Thanks
2 participants
While moving the openssl code, I've noticed that the
missing.hhandling could be flattened/improved a bit. So here it is:MODULE_INIT_*defines to their sole usermissing.hinclusion where it's used and fixing the falloutAside: some v7.1-rc5 commits promote more of the module data/structs that we use to UAPI headers... Which means we could reuse it - either via
missing.h, pulling a local copy or otherwise - in the not too distant future. Aka somewhat related to #428