[ndctl] ndctl/monitor: Drop 'struct ndctl_ctx *' casts

Commit Message

Dan Williams Dec. 23, 2018, 6:56 p.m. UTC

Functions that take a 'struct ndctl_ctx *' are fine to take the 'void *'
argument representing a library context that is passed to all commands.
However, the shared logging macros need a defined type to dereference
and retrieve the log_ctx. Introduce _gen_ctx to generically represent
'struct ndctl_ctx *' and 'struct daxctl_ctx *' to the log helpers, and
drop the remaining casts in ndctl/monitor.c. Add a couple BUILD_BUG_ON()
statements to backup the log_ctx expectations.

Cc: QI Fuli <[e-mail]>
Signed-off-by: Dan Williams <[e-mail]>
---
 daxctl/lib/libdaxctl.c |    4 ++++
 ndctl/lib/libndctl.c   |    4 ++++
 ndctl/monitor.c        |   22 +++++++++++-----------
 util/log.h             |   16 ++++++++++++----
 4 files changed, 31 insertions(+), 15 deletions(-)

Comments

Dan Williams Dec. 27, 2018, 6:07 p.m. UTC | #1

On Wed, Dec 26, 2018 at 4:03 PM Verma, Vishal L
<[e-mail]> wrote:
>
> On Sun, 2018-12-23 at 10:56 -0800, Dan Williams wrote:
> > Functions that take a 'struct ndctl_ctx *' are fine to take the 'void *'
> > argument representing a library context that is passed to all commands.
> > However, the shared logging macros need a defined type to dereference
> > and retrieve the log_ctx. Introduce _gen_ctx to generically represent
> > 'struct ndctl_ctx *' and 'struct daxctl_ctx *' to the log helpers, and
> > drop the remaining casts in ndctl/monitor.c. Add a couple BUILD_BUG_ON()
> > statements to backup the log_ctx expectations.
> >
> > Cc: QI Fuli <[e-mail]>
> > Signed-off-by: Dan Williams <[e-mail]>
> > ---
> >  daxctl/lib/libdaxctl.c |    4 ++++
> >  ndctl/lib/libndctl.c   |    4 ++++
> >  ndctl/monitor.c        |   22 +++++++++++-----------
> >  util/log.h             |   16 ++++++++++++----
> >  4 files changed, 31 insertions(+), 15 deletions(-)
> >
>
> <..>
>
> > diff --git a/util/log.h b/util/log.h
> > index 495e0d33c7f5..881971522690 100644
> > --- a/util/log.h
> > +++ b/util/log.h
> > @@ -56,10 +56,18 @@ do { \
> >  #  define log_notice(ctx, arg...) log_null(ctx, ## arg)
> >  #endif
> >
> > -#define dbg(x, arg...) log_dbg(&(x)->ctx, ## arg)
> > -#define info(x, arg...) log_info(&(x)->ctx, ## arg)
> > -#define err(x, arg...) log_err(&(x)->ctx, ## arg)
> > -#define notice(x, arg...) log_notice(&(x)->ctx, ## arg)
> > +struct _gen_ctx {
> > +     /*
> > +      * Requires ndctl_ctx and daxctl_ctx keep their log_ctx as their
> > +      * first element
> > +      */
> > +     struct log_ctx ctx;
> > +};
> > +
> > +#define dbg(x, arg...) log_dbg(&((struct _gen_ctx *)(x))->ctx, ## arg)
> > +#define info(x, arg...) log_info(&((struct _gen_ctx *)(x))->ctx, ## arg)
> > +#define err(x, arg...) log_err(&((struct _gen_ctx *)(x))->ctx, ## arg)
> > +#define notice(x, arg...) log_notice(&((struct _gen_ctx *)(x))->ctx, ## arg)
>
> Looks like this breaks ndctl/check.c's (ab)use of log_ctx directly,
> causing a segfault when this is called.

Whoops.

> I'm guessing this should be
> fixed by passing the actual ndctl_ctx to the check function, and letting
> the logging helpers dereference it correctly.
>
> It is also fixed if I move log_ctx to be the first member of btt_chk.
>
> Any thoughts on whether we should allow the embedding of log_ctx like
> this?

Yeah, I think we should, it's useful.

I'll just cook up a patch to convert the command harness to be
typesafe and drop this "offset-0" trickery.

> Other than that the patch looks good to me.

Good catch.

Patch

diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
index 22f4210a7ea0..7b233b8d43f1 100644
--- a/daxctl/lib/libdaxctl.c
+++ b/daxctl/lib/libdaxctl.c
@@ -23,6 +23,7 @@ 
 #include <ccan/array_size/array_size.h>
 
 #include <util/log.h>
+#include <util/util.h>
 #include <util/sysfs.h>
 #include <daxctl/libdaxctl.h>
 #include "libdaxctl-private.h"
@@ -86,6 +87,9 @@  DAXCTL_EXPORT int daxctl_new(struct daxctl_ctx **ctx)
 {
 	struct daxctl_ctx *c;
 
+	/* Sanity check for the expectations of the shared log functions */
+	BUILD_BUG_ON(offsetof(struct daxctl_ctx, ctx) != 0);
+
 	c = calloc(1, sizeof(struct daxctl_ctx));
 	if (!c)
 		return -ENOMEM;
diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index 0c3a35e5bcc9..77b29454a253 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -31,6 +31,7 @@ 
 #include <ccan/build_assert/build_assert.h>
 
 #include <ndctl.h>
+#include <util/util.h>
 #include <util/sysfs.h>
 #include <ndctl/libndctl.h>
 #include <ndctl/namespace.h>
@@ -287,6 +288,9 @@  NDCTL_EXPORT int ndctl_new(struct ndctl_ctx **ctx)
 	const char *env;
 	int rc = 0;
 
+	/* Sanity check for the expectations of the shared log functions */
+	BUILD_BUG_ON(offsetof(struct ndctl_ctx, ctx) != 0);
+
 	udev = udev_new();
 	if (check_udev(udev) != 0)
 		return -ENXIO;
diff --git a/ndctl/monitor.c b/ndctl/monitor.c
index b92a133b7b94..73d75286e90b 100644
--- a/ndctl/monitor.c
+++ b/ndctl/monitor.c
@@ -623,14 +623,14 @@  int cmd_monitor(int argc, const char **argv, void *ctx)
 		usage_with_options(u, options);
 
 	/* default to log_standard */
-	ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_standard);
+	ndctl_set_log_fn(ctx, log_standard);
 
 	if (monitor.verbose)
-		ndctl_set_log_priority((struct ndctl_ctx *)ctx, LOG_DEBUG);
+		ndctl_set_log_priority(ctx, LOG_DEBUG);
 	else
-		ndctl_set_log_priority((struct ndctl_ctx *)ctx, LOG_INFO);
+		ndctl_set_log_priority(ctx, LOG_INFO);
 
-	rc = read_config_file((struct ndctl_ctx *)ctx, &monitor, &param);
+	rc = read_config_file(ctx, &monitor, &param);
 	if (rc)
 		goto out;
 
@@ -638,7 +638,7 @@  int cmd_monitor(int argc, const char **argv, void *ctx)
 		if (strncmp(monitor.log, "./", 2) != 0)
 			fix_filename(prefix, (const char **)&monitor.log);
 		if (strncmp(monitor.log, "./syslog", 8) == 0)
-			ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_syslog);
+			ndctl_set_log_fn(ctx, log_syslog);
 		else if (strncmp(monitor.log, "./standard", 10) == 0)
 			; /*default, already set */
 		else {
@@ -649,21 +649,21 @@  int cmd_monitor(int argc, const char **argv, void *ctx)
 				goto out;
 			}
 			fclose(f);
-			ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_file);
+			ndctl_set_log_fn(ctx, log_file);
 		}
 	}
 
 	if (monitor.daemon) {
 		if (!monitor.log || strncmp(monitor.log, "./", 2) == 0)
-			ndctl_set_log_fn((struct ndctl_ctx *)ctx, log_syslog);
+			ndctl_set_log_fn(ctx, log_syslog);
 		if (daemon(0, 0) != 0) {
-			err((struct ndctl_ctx *)ctx, "daemon start failed\n");
+			err(ctx, "daemon start failed\n");
 			goto out;
 		}
-		info((struct ndctl_ctx *)ctx, "ndctl monitor daemon started\n");
+		info(ctx, "ndctl monitor daemon started\n");
 	}
 
-	if (parse_monitor_event(&monitor, (struct ndctl_ctx *)ctx))
+	if (parse_monitor_event(&monitor, ctx))
 		goto out;
 
 	fctx.filter_bus = filter_bus;
@@ -681,7 +681,7 @@  int cmd_monitor(int argc, const char **argv, void *ctx)
 		goto out;
 
 	if (!mfa.num_dimm) {
-		dbg((struct ndctl_ctx *)ctx, "no dimms to monitor\n");
+		dbg(ctx, "no dimms to monitor\n");
 		if (!monitor.daemon)
 			rc = -ENXIO;
 		goto out;
diff --git a/util/log.h b/util/log.h
index 495e0d33c7f5..881971522690 100644
--- a/util/log.h
+++ b/util/log.h
@@ -56,10 +56,18 @@  do { \
 #  define log_notice(ctx, arg...) log_null(ctx, ## arg)
 #endif
 
-#define dbg(x, arg...) log_dbg(&(x)->ctx, ## arg)
-#define info(x, arg...) log_info(&(x)->ctx, ## arg)
-#define err(x, arg...) log_err(&(x)->ctx, ## arg)
-#define notice(x, arg...) log_notice(&(x)->ctx, ## arg)
+struct _gen_ctx {
+	/*
+	 * Requires ndctl_ctx and daxctl_ctx keep their log_ctx as their
+	 * first element
+	 */
+	struct log_ctx ctx;
+};
+
+#define dbg(x, arg...) log_dbg(&((struct _gen_ctx *)(x))->ctx, ## arg)
+#define info(x, arg...) log_info(&((struct _gen_ctx *)(x))->ctx, ## arg)
+#define err(x, arg...) log_err(&((struct _gen_ctx *)(x))->ctx, ## arg)
+#define notice(x, arg...) log_notice(&((struct _gen_ctx *)(x))->ctx, ## arg)
 
 #ifndef HAVE_SECURE_GETENV
 #  ifdef HAVE___SECURE_GETENV