Header cleanups for Windows support#3270
Conversation
40ed5f3 to
ae90d22
Compare
|
The Windows build includes the I consider this a major milestone. Nevertheless there is still some more cleanups necessary, e.g. moving the fabrics code into fabrics.c. Currently, it's just behind some ugly ifdefs. This is all in the c files and not in the headers. The headers should be in good state. So when reviewing concentrate on the headers. They are the important step right now. |
|
@igaw - Everything looks good. Disregard my earlier comment. You made the right change to |
When the code was switched to use nvme_scan_topology, the introduced error check was incorrect. Fixes: 7daa1e9 ("plugins: replace nvme_scan with nvme_scan_topology") Signed-off-by: Daniel Wagner <wagi@kernel.org>
All other files in libnvme use a dash. Rename the compiler attributes header accordingly. Signed-off-by: Daniel Wagner <wagi@kernel.org>
The cleanup helpers for addrinfo and uri depend on features that are not always available, such as network support or fabrics. Therefore, they cannot be part of the generic cleanup header file. Also, they are only used in a single location each. Move them to the C files that use them. Signed-off-by: Daniel Wagner <wagi@kernel.org>
|
I missed to update the headers when updating the code generator. While at it also improved the commit message a bit. It should be a bit more standard English :) |
There was a problem hiding this comment.
Pull request overview
This PR continues the Windows portability work by removing Linux-specific header dependencies from shared code, introducing/renaming libnvme public headers, and gating MI-related code behind a Meson feature option to keep Windows builds clean.
Changes:
- Replace direct
linux/types.h/endian.husage with libnvme-provided portable headers (<nvme/types.h>,<nvme/endian.h>,<nvme/nvme-types.h>,<nvme/nvme-cmds.h>). - Introduce a Meson
mifeature flag, add MI stubs when disabled, and wire MI into build/install logic (including examples/tests). - Standardize logging levels away from syslog constants to
LIBNVME_LOG_*across nvme-cli + libnvme.
Reviewed changes
Copilot reviewed 93 out of 96 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| util/types.h | Remove Linux-only types include; rely on libnvme-provided types. |
| util/argconfig.h | Switch to portable <nvme/types.h> include. |
| types.h | Use <nvme/nvme-types.h> for NVMe type definitions. |
| scripts/build.sh | Disable MI in minimal static Meson configuration. |
| plugins/zns/zns.c | Use LIBNVME_DEFAULT_LOGLEVEL and correct ctx null-check flow. |
| plugins/solidigm/solidigm-smart.c | Remove Linux-only types include. |
| plugins/solidigm/solidigm-garbage-collection.c | Remove Linux-only types include. |
| plugins/ocp/ocp-telemetry-decode.c | Remove Linux-only types include. |
| plugins/ocp/ocp-smart-extended-log.h | Replace Linux types include with <nvme/types.h>. |
| plugins/ocp/ocp-hardware-component-log.c | Replace syslog log levels with LIBNVME_LOG_*. |
| plugins/ocp/ocp-fw-activation-history.h | Remove Linux-only types include. |
| plugins/netapp/netapp-nvme.c | Use LIBNVME_DEFAULT_LOGLEVEL. |
| plugins/memblaze/memblaze-nvme.c | Remove Linux-only types include. |
| plugins/huawei/huawei-nvme.c | Use LIBNVME_DEFAULT_LOGLEVEL. |
| nvme.h | Remove <endian.h> dependency. |
| nvme.c | Remove redundant internal includes; use LIBNVME_LOG_DEBUG/INFO. |
| nvme-print-stdout.c | Switch debug check to LIBNVME_LOG_DEBUG. |
| nvme-cmds.h | Switch to <nvme/nvme-cmds.h> include path. |
| meson_options.txt | Add new mi feature option. |
| meson.build | Add want_mi, CONFIG_MI, and gate dbus dependency on MI. |
| logging.h | Switch macros to LIBNVME_LOG_* and include <nvme/lib.h>. |
| logging.c | Remove Linux/syslog types, update log mapping and log level constants. |
| libnvme/tools/generator/generate-accessors.py | Generate portable includes; rename compiler attributes header. |
| libnvme/tools/generator/generate-accessors.md | Doc update to match generator output. |
| libnvme/test/zns.c | Use LIBNVME_DEFAULT_LOGLEVEL. |
| libnvme/test/tree.c | Use LIBNVME_LOG_* levels in tests. |
| libnvme/test/test.c | Use LIBNVME_DEFAULT_LOGLEVEL. |
| libnvme/test/sysfs/tree-dump.c | Use LIBNVME_LOG_ERR. |
| libnvme/test/psk.c | Use LIBNVME_DEFAULT_LOGLEVEL. |
| libnvme/test/mi.c | Include new MI private header; use LIBNVME_DEFAULT_LOGLEVEL. |
| libnvme/test/mi-mctp.c | Include new MI private header; use LIBNVME_DEFAULT_LOGLEVEL. |
| libnvme/test/meson.build | Update header list to new names (endian, nvme-cmds, nvme-types). |
| libnvme/test/ioctl/zns.c | Use LIBNVME_DEFAULT_LOGLEVEL. |
| libnvme/test/ioctl/misc.c | Use LIBNVME_DEFAULT_LOGLEVEL. |
| libnvme/test/ioctl/logs.c | Use LIBNVME_DEFAULT_LOGLEVEL. |
| libnvme/test/ioctl/identify.c | Use LIBNVME_DEFAULT_LOGLEVEL. |
| libnvme/test/ioctl/features.c | Use LIBNVME_DEFAULT_LOGLEVEL. |
| libnvme/test/ioctl/discovery.c | Use LIBNVME_DEFAULT_LOGLEVEL. |
| libnvme/test/ioctl/ana.c | Use LIBNVME_DEFAULT_LOGLEVEL. |
| libnvme/test/cpp.cc | Use LIBNVME_DEFAULT_LOGLEVEL. |
| libnvme/test/config/psk-json.c | Use LIBNVME_LOG_ERR. |
| libnvme/test/config/hostnqn-order.c | Use LIBNVME_LOG_ERR. |
| libnvme/test/config/config-dump.c | Use LIBNVME_LOG_ERR. |
| libnvme/src/nvme/util.h | Switch to <nvme/nvme-types.h>. |
| libnvme/src/nvme/util.c | Add Linux cleanup header; adjust includes and log levels; add addrinfo cleanup. |
| libnvme/src/nvme/tree.h | Switch to <nvme/nvme-types.h>. |
| libnvme/src/nvme/tree.c | Gate fabrics networking headers; add cleanup-linux; convert log levels. |
| libnvme/src/nvme/private.h | Gate fabrics/MI fields; move MI internals out; remove uring stub block. |
| libnvme/src/nvme/private-mi.h | New internal MI header split out of private.h. |
| libnvme/src/nvme/private-fabrics.h | Remove Linux types include. |
| libnvme/src/nvme/nvme-cmds.h | Use portable <nvme/endian.h> + <nvme/nvme-types.h>. |
| libnvme/src/nvme/nvme-cmds.c | Switch to compiler-attributes.h; update log level constant. |
| libnvme/src/nvme/no-uring.c | New no-liburing implementation file. |
| libnvme/src/nvme/no-mi.c | New no-MI stub implementation file. |
| libnvme/src/nvme/nbft.h | Remove Linux types include. |
| libnvme/src/nvme/nbft.c | Switch to compiler-attributes.h; convert log levels. |
| libnvme/src/nvme/mi.c | Include private-mi.h; convert log levels. |
| libnvme/src/nvme/mi-types.h | Use <nvme/endian.h> + <nvme/nvme-types.h>. |
| libnvme/src/nvme/mi-mctp.c | Include private-mi.h; convert log levels and defaults. |
| libnvme/src/nvme/mi-mctp-compat.h | Switch to <nvme/types.h>. |
| libnvme/src/nvme/log.c | Switch to compiler-attributes.h; remove syslog include. |
| libnvme/src/nvme/linux.c | Add cleanup-linux; convert log levels; add <netdb.h>. |
| libnvme/src/nvme/lib.h | Introduce enum libnvme_log_level + LIBNVME_DEFAULT_LOGLEVEL. |
| libnvme/src/nvme/lib.c | Add fabrics/MI guards; include MI private header; cleanup logic updates. |
| libnvme/src/nvme/lib-types.h | Switch to <nvme/types.h>. |
| libnvme/src/nvme/json.c | Convert log levels to LIBNVME_LOG_*. |
| libnvme/src/nvme/ioctl.c | Switch to compiler-attributes.h. |
| libnvme/src/nvme/filters.c | Switch to compiler-attributes.h. |
| libnvme/src/nvme/fabrics.c | Add cleanup-linux; local __cleanup_uri; convert log levels. |
| libnvme/src/nvme/endian.h | New portable endian header for Windows. |
| libnvme/src/nvme/compiler-attributes.h | New compiler attribute helpers with __public/__weak. |
| libnvme/src/nvme/cleanup.h | Reduce to core cleanup helpers; move POSIX helpers out. |
| libnvme/src/nvme/cleanup-linux.h | New Linux/POSIX cleanup helpers header. |
| libnvme/src/nvme/accessors.h | Generated header switches to <nvme/types.h>. |
| libnvme/src/nvme/accessors.c | Switch to compiler-attributes.h. |
| libnvme/src/nvme/accessors-fabrics.h | Generated header switches to <nvme/types.h>. |
| libnvme/src/nvme/accessors-fabrics.c | Switch to compiler-attributes.h. |
| libnvme/src/meson.build | Split MI build/install and add no-mi/no-uring sources. |
| libnvme/src/libnvme.h.in | Update exported include set/order for new headers. |
| libnvme/src/libnvme-mi.h | Update includes to new nvme-types / nvme-cmds. |
| libnvme/libnvme/tests/test-objects.py | Update accepted log level strings. |
| libnvme/libnvme/nvme.i | Update log level mapping; SWIG include points to nvme-types.h. |
| libnvme/examples/telemetry-listen.c | Use LIBNVME_DEFAULT_LOGLEVEL. |
| libnvme/examples/mi-mctp.c | Use LIBNVME_DEFAULT_LOGLEVEL. |
| libnvme/examples/mi-mctp-csi-test.c | Use LIBNVME_DEFAULT_LOGLEVEL. |
| libnvme/examples/mi-mctp-ae.c | Use LIBNVME_DEFAULT_LOGLEVEL. |
| libnvme/examples/mi-conf.c | Use LIBNVME_DEFAULT_LOGLEVEL. |
| libnvme/examples/meson.build | Gate MI examples on want_mi (and dbus for mi-conf). |
| libnvme/examples/display-tree.c | Use LIBNVME_DEFAULT_LOGLEVEL. |
| libnvme/examples/display-columnar.c | Use LIBNVME_DEFAULT_LOGLEVEL. |
| libnvme/examples/discover-loop.c | Use LIBNVME_DEFAULT_LOGLEVEL. |
| libnvme/doc/meson.build | Document new headers (nvme-cmds.h, nvme-types.h). |
| fabrics.c | Remove syslog + Linux types includes. |
| .github/workflows/build.yml | Adjust Windows GCC path and artifact log path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The main cleanup header file should only include generic headers to simplify porting. Signed-off-by: Daniel Wagner <wagi@kernel.org>
Keep header files readable by avoiding static inline dummy function implementations. Signed-off-by: Daniel Wagner <wagi@kernel.org>
As a first step, mark all fabrics-related parts of the global context as optional. Once all fabrics-related functions are clearly identified, they can be moved to the appropriate location. Signed-off-by: Daniel Wagner <wagi@kernel.org>
The MI code depends on Linux-specific interfaces (networking), which makes porting to other platforms more difficult. Make it optional. This also helps reduce the footprint for minimal setups. Signed-off-by: Daniel Wagner <wagi@kernel.org>
The ioctl APIs have been moved out of the private header. No need to include it. Signed-off-by: Daniel Wagner <wagi@kernel.org>
The library uses these only for log levels, so the remaining syslog constants are unnecessary. Furthermore, removing the dependency on syslog simplifies porting the project to other operating systems. Signed-off-by: Daniel Wagner <wagi@kernel.org>
Use library prefix to avoid colliding with syslog.h definitions. Signed-off-by: Daniel Wagner <wagi@kernel.org>
Follow the recently introduced naming pattern by prefixing the types and cmds headers. This also allows introducing a global types.h for the entire library later. Signed-off-by: Daniel Wagner <wagi@kernel.org>
Not all platforms provide an endian.h. in those cases, define the byte-order helper functions. Signed-off-by: Daniel Wagner <wagi@kernel.org>
Not all platforms define the base types used throughout the library. Inthat case, define them. Signed-off-by: Daniel Wagner <wagi@kernel.org>
MSYS gcc (/usr/bin/gcc) is not the native Windows compiler. Select the UCRT64 compiler explicitly. Without this, the header guards in endian.h and types.h do not work. Signed-off-by: Daniel Wagner <wagi@kernel.org>
Refactor and restructure the code base so the generic code is not platform dependent. The platform dependent part should be in separate files which are added via the build environment.
The fabric and MI patch need some more love to avoid the ifdeffery but it's not too bad luckily.
The last two patches add some window specific code. Not sure yet if this is the best idea but it seems it's the only platform depended code in the generic code base.The TODO list from #3237