On Fri, 2023-06-30 at 17:16 -0500, Benjamin Marzinski wrote:
> On Fri, Jun 30, 2023 at 08:14:07PM +0200, mwilck@xxxxxxxx wrote:
> > From: Martin Wilck <mwilck@xxxxxxxx>
> >
> > If the nvme native multipath driver is enabled, blacklist nvme
> > devices
> > for dm-multipath by default. This is particularly useful with
> > "find_multipaths greedy".
> >
> > Signed-off-by: Martin Wilck <mwilck@xxxxxxxx>
>
> This looks good. I have two small questions below.
>
> > ---
> > libmultipath/blacklist.c | 35 ++++++++++++++++++++++++++++++++---
> > tests/blacklist.c | 30 ++++++++++++++++++++++++++++--
> > 2 files changed, 60 insertions(+), 5 deletions(-)
> >
> > diff --git a/libmultipath/blacklist.c b/libmultipath/blacklist.c
> > index 8d15d2e..75100b2 100644
> > --- a/libmultipath/blacklist.c
> > +++ b/libmultipath/blacklist.c
> > @@ -2,6 +2,8 @@
> > * Copyright (c) 2004, 2005 Christophe Varoqui
> > */
> > #include <stdio.h>
> > +#include <unistd.h>
> > +#include <fcntl.h>
> > #include <libudev.h>
> >
> > #include "checkers.h"
> > @@ -191,6 +193,27 @@ find_blacklist_device (const struct _vector
> > *blist, const char *vendor,
> > return 0;
> > }
> >
> > +/*
> > + * Test if nvme native multipath is enabled. If the sysfs file
> > can't
> > + * be accessed, multipath is either disabled at compile time, or
> > no
> > + * nvme driver is loaded at all. Thus treat errors as "no".
> > + */
> > +static bool nvme_multipath_enabled(void)
> > +{
> > + static const char fn[] =
> > "/sys/module/nvme_core/parameters/multipath";
>
> Is there some benefit that I don't understand to using "static
> const",
> instead of just "const"? Obviously, the amount of memory necessary
> to
> keep storing this string outside of the function is very small, but I
> don't see why we need to at all.
"static const" and "const" aren't the same thing. "static const" makes
sure the variable's life time is the run time of the program, hence the
compiler can place it in the .rodata section of the output. A "const"
variable in function scope without "static" has "automatic" storage
class, and is thus allocated and initialized on the stack.
https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf
(§6.2.4 storage durations of objects)
https://stackoverflow.com/questions/3709207/c-semantics-of-static-const-vs-const
You can see this if you compile a program like this
int check(const char *);
int fn(void) {
/* static */ const char t[] = "0123456789abcdef";
return check(t);
}
with and without "static".
> > diff --git a/tests/blacklist.c b/tests/blacklist.c
> > index 882aa3a..65002eb 100644
> > --- a/tests/blacklist.c
> > +++ b/tests/blacklist.c
> > @@ -17,6 +17,8 @@
> > */
> > #include <stdarg.h>
> > #include <stddef.h>
> > +#include <unistd.h>
> > +#include <fcntl.h>
> > #include <setjmp.h>
> > #include <cmocka.h>
> > #include "globals.c"
> > @@ -220,12 +222,36 @@ static void test_devnode_missing(void
> > **state)
> > MATCH_NOTHING);
> > }
>
> Perhaps we should just use #include "../libmultipath/blacklist.c"
> like
> we do for tests where we need get at a file's private
> variables/functions ("../libmultipath/alias.c" in alias.c for
> instance).
Ok.
Regards
Martin
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
