On Wed, Sep 24, 2025 at 04:10:21PM -0700, Dan Williams wrote: > Alison Schofield wrote: > > When the --media-errors option was added to cxl list it inadvertently > > changed the optional libtracefs requirement into a mandatory one. > > Ndctl versions 80,81,82 no longer build without libtracefs. > > > > Remove that dependency. > > > > When libtracefs is disabled the user will see a 'Notice' level > > message, like this: > > $ cxl list -r region0 --media-errors --targets > > cxl list: cmd_list: --media-errors support disabled at build time > > > > ...followed by the region listing including the output for any other > > valid command line options, like --targets in the example above. > > > > When libtracefs is disabled the cxl-poison.sh unit test is omitted. > > > > The man page gets a note: > > The media-error option is only available with -Dlibtracefs=enabled. > > > > Reported-by: Andreas Hasenack <[email protected]> > > Fixes: d7532bb049e0 ("cxl/list: add --media-errors option to cxl list") > > Closes: https://github.com/pmem/ndctl/issues/289 > > Signed-off-by: Alison Schofield <[email protected]> > > --- > > > > Changes in v2: > > - Notify and continue when --media-error info is unavailable (Dan) > > > > > > Documentation/cxl/cxl-list.txt | 2 ++ > > config.h.meson | 2 +- > > cxl/json.c | 15 ++++++++++++++- > > cxl/list.c | 6 ++++++ > > test/meson.build | 9 +++++++-- > > 5 files changed, 30 insertions(+), 4 deletions(-) > > > > diff --git a/Documentation/cxl/cxl-list.txt b/Documentation/cxl/cxl-list.txt > > index 9a9911e7dd9b..0595638ee054 100644 > > --- a/Documentation/cxl/cxl-list.txt > > +++ b/Documentation/cxl/cxl-list.txt > > @@ -425,6 +425,8 @@ OPTIONS > > "source:" is one of: External, Internal, Injected, Vendor Specific, > > or Unknown, as defined in CXL Specification v3.1 Table 8-140. > > > > +The media-errors option is only available with '-Dlibtracefs=enabled'. > > + > > ---- > > # cxl list -m mem9 --media-errors -u > > { > > diff --git a/config.h.meson b/config.h.meson > > index f75db3e6360f..e8539f8d04df 100644 > > --- a/config.h.meson > > +++ b/config.h.meson > > @@ -19,7 +19,7 @@ > > /* ndctl test support */ > > #mesondefine ENABLE_TEST > > > > -/* cxl monitor support */ > > +/* cxl monitor and cxl list --media-errors support */ > > #mesondefine ENABLE_LIBTRACEFS > > > > /* Define to 1 if big-endian-arch */ > > diff --git a/cxl/json.c b/cxl/json.c > > index e65bd803b706..a75928bf43ed 100644 > > --- a/cxl/json.c > > +++ b/cxl/json.c > > @@ -9,12 +9,15 @@ > > #include <json-c/json.h> > > #include <json-c/printbuf.h> > > #include <ccan/short_types/short_types.h> > > + > > +#ifdef ENABLE_LIBTRACEFS > > #include <tracefs.h> > > +#include "../util/event_trace.h" > > +#endif > > Maybe this is my kernel taste leaking through, but I am allergic to > ifdef in ".c" files. In this case you could move the include of > tracefs.h into event_trace.h and then ifdef guard all the parts of > event_trace.h that need tracefs.h. > > > > > #include "filter.h" > > #include "json.h" > > #include "../daxctl/json.h" > > -#include "../util/event_trace.h" > > > > #define CXL_FW_VERSION_STR_LEN 16 > > #define CXL_FW_MAX_SLOTS 4 > > @@ -575,6 +578,7 @@ err_jobj: > > return NULL; > > } > > > > +#ifdef ENABLE_LIBTRACEFS > > /* CXL Spec 3.1 Table 8-140 Media Error Record */ > > #define CXL_POISON_SOURCE_MAX 7 > > static const char *const poison_source[] = { "Unknown", "External", > > "Internal", > > @@ -753,6 +757,15 @@ err_free: > > tracefs_instance_free(inst); > > return jpoison; > > } > > +#else > > +static struct json_object * > > +util_cxl_poison_list_to_json(struct cxl_region *region, > > + struct cxl_memdev *memdev, > > + unsigned long flags) > > +{ > > + return NULL; > > +} > > +#endif > > This would move a new conditionally compiled ".c" file for just these > trace helpers. > > > struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev, > > unsigned long flags) > > diff --git a/cxl/list.c b/cxl/list.c > > index 0b25d78248d5..48bd1ebc3c0e 100644 > > --- a/cxl/list.c > > +++ b/cxl/list.c > > @@ -146,6 +146,12 @@ int cmd_list(int argc, const char **argv, struct > > cxl_ctx *ctx) > > param.ctx.log_priority = LOG_DEBUG; > > } > > > > +#ifndef ENABLE_LIBTRACEFS > > + if (param.media_errors) { > > + notice(¶m, "--media-errors support disabled at build > > time\n"); > > + param.media_errors = false; > > + } > > +#endif > > This would be a static inline helper in a header file that optionally > reports the problem. > > All that said, I am not the maintainer so go with what you want, but > leaking ifdef in ".c" is, in my opinion, a slow walk into an > increasingly unreadable code base.
I can do what you suggest, with bells on! See you in v3 :)
