compat: Adds more platform compatibility headers and mem compatibility.#3310
Conversation
bgoing-micron-oss
commented
Apr 23, 2026
bgoing-micron-oss
commented
- Adds compatibility headers that provides functionality missing from Windows headers and cross-platform compatibility macros.
- Adds memory management compatibility for aligned memory allocation and freeing. Windows has a seprate aligned free that needs to be used for aligned memory.
|
I'll try find some time to dive into these patches soon but I need to prepare for next week LSFMM and am not completely done with my POC yet. |
| #include <direct.h> | ||
|
|
||
| /* Windows mkdir doesn't take the mode parameter */ | ||
| #define mkdir(path, mode) _mkdir(path) |
There was a problem hiding this comment.
There is no user of mkdir in the library. The only users are in the plugins, thus we should not add this helper here.
There was a problem hiding this comment.
Thanks for the feedback. Do you have a recommendation for where you would like to see this type of compatibility definition if not here? It is used across multiple plugins, and there isn't currently a pattern for shared utilities or definitions specifically for plugins.
There was a problem hiding this comment.
So my aim is first to analyze if we can replace/rewrite the code so we don't need any special wrappers. If not possible, a wrapper has to be placed to the library (private if possible). If only nvme-cli is using it we should add something there. Generally, I am looking for the smallest impact radius.
Not sure yet how to handle this one but it looks like only the plugins are using mkdir and I wonder what these plugins are doing...
Anyway, I am running out of steam for today.
There was a problem hiding this comment.
Since this are no users in the library, I ignore this problem for now. First goal so sort out all the problems in the library
| fputs(buffer, stderr); | ||
| else if (fd == STDOUT_FILENO) | ||
| fputs(buffer, stdout); | ||
| return result; |
There was a problem hiding this comment.
fd could be also a normal file descriptor. Let's not worry about this and just replace dprintf with asprintf and write.
There was a problem hiding this comment.
Good point. Are you referring to using asprintf and write here in a compatibility implementation, or in log.c, which is the one place it is currently used?
There was a problem hiding this comment.
IIRC, I picked dprintf just because it was simplest. Since we arleady use apsrintf in this function to format the message I don't see why not to use one more time. The final message can then just written out using the write command (with retry on EINTR etc).
There was a problem hiding this comment.
I've got this covered in my next PR based on this work.
| } | ||
|
|
||
| /* open_memstream workaround for Windows - returns a temporary file instead */ | ||
| static inline FILE *open_memstream(char **ptr, size_t *sizeloc) |
There was a problem hiding this comment.
I've refactored the only user of this function so it doesn't use open_memstream anymore.
There was a problem hiding this comment.
and it's back with the new nvme top feature. Let's see what we have to do with this one. Maybe we have to make this feature an compile time option in the beginning because I don't think I am able to cheat with this one.
There was a problem hiding this comment.
It looks like the new nvme top feature is very linux-specific, and will require a separate effort to port, if possible. Making it a compile-time option for now sounds like a good idea.
| * Simplified signal handling using Windows signal() function | ||
| * This is sufficient for handling SIGINT with no mask or flags. | ||
| */ | ||
| static inline int sigaction(int signum, const struct sigaction *act, |
There was a problem hiding this comment.
The library itself doesn't do any signal stuff which also makes sense, a library should never interfere with this API. So we should take this to nvme-cli. I think we have to implement util/sighdl.h so it's portable, thus we might need to either introduce util/signhdl-linux.c/util/signhdl-win.c or have some ifdefery there.
There was a problem hiding this comment.
@igaw I should have noticed during my review yesterday, but these got pulled into mem-win.c. Only getpagesize is used there, so it doesn't do any good to have the others there. gethostname, fsync, and getpagesize are still all used elsewhere in the code, which is why they were in a unistd compatibility header. Do you have concerns about handling compatibility in this way? I do see that most of the uses, particularly of fsync, are within nvme-cli, not libnvme, but since it was used across both this seemed like a good way to handle it.
There was a problem hiding this comment.
I really want to avoid exposing compatibility functions in the public library API as much as possible. There are dragons there (been there, done that). So my plan is to go through each function and figure out what the options are.
- fsync: used only in nvme-cli, so it should go there somewhere
- getpagesize: there are only a few callsites left, let's review them and see what is really necessary
- gethostname: looks like this one is only used for fabrics. that means we should move it to new util-fabrics.c
Ideally, every change improves the code base for all users and avoids complex indirection and other difficult abstractions to handle.
There was a problem hiding this comment.
@igaw What do you think of common.h as a potential home for things like fsync, getpagesize and mkdir that are used across nvme-cli and plugins? It seems to align with the mmio compatibility methods that are already there, and is already included everywhere those methods are needed.
- Adds compatibility headers that provides functionality missing from Windows headers and adds cross-platform compatibility macros. Signed-off-by: Broc Going <bgoing@micron.com>
bgoing-micron-oss
force-pushed
the
bgoing/additional-compatibility-includes
branch
from
b7b325c to
f4730b4
Compare
2 participants
