Re: [dm-devel] [RFC PATCH 17/20] libmultipath/foreign: nvme foreign library

2018-03-02 Thread Benjamin Marzinski
On Fri, Mar 02, 2018 at 05:04:28PM +0100, Martin Wilck wrote:
> On Wed, 2018-02-28 at 21:14 -0600, Benjamin Marzinski wrote:
> > On Tue, Feb 20, 2018 at 02:26:55PM +0100, Martin Wilck wrote:
> > > +void cleanup(struct context *ctx)
> > > +{
> > > + if (ctx == NULL)
> > > + return;
> > > +
> > > + (void)delete_all(ctx);
> > > +
> > > + lock(ctx);
> > > + pthread_cleanup_push(unlock, ctx);
> > > + vector_free(ctx->mpvec);
> > > + pthread_cleanup_pop(1);
> > > + pthread_mutex_destroy(>mutex);
> > > +
> > > + free(ctx);
> > > +}
> > 
> > Would you mind either removing the locking, or adding a comment
> > explaining that it's not necessary here.  If you really did need to
> > lock
> > ctx in order guard against someone accessing ctx->mpvec in cleanup(),
> > then not setting it to NULL after its freed, and immediately freeing
> > ctx
> > afterwards would clearly be broken, so seeing the locking makes it
> > look
> > like this function is wrong.
> 
> I don't understand, sorry. I need to lock because some other entity may
> still be using ctx->mpvec when cleanup() is called, and I should wait
> until that other entity has released the lock before I free it.

Who else could be using ctx->mpvec?  You only call cleanup on code paths
through init_foreign and cleanup_foreign.  There are no seperate threads
running in parallel in multipath, and in multipathd, you call
init_foreign before any other threads except the log writer thread is
created, and call cleanup_foreign after all the main threads except for
the log writer thread have been waited for? I admit, there could be a
rogue checker or prio thread that has been cancelled, but still not
returned, but those don't touch the foreign ctx code. If I'm missing
something, or you want to make sure that this code is future-proofed,
against possible future threads, you need some ref counting. Otherwise,
you could get into a situation where the thread doing the cleanup locks
the ctx->mutex and then another thread blocks on it. When the cleanup
thread drops the mutex and frees ctx, the other thread will grab a lock
to freed memory.

> I'm
> also pretty sure that checkers such as coverity would complain if I
> free a structure here without locking which I access only under the
> lock in other places.

Yeah. That's why I said you could just add a comment.
 
> I agree, though, that I should nullify the data and add checks in the
> other functions. I'll also add some locking in the foreign.c file,
> didn't occur to me before.

-Ben

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel


Re: [dm-devel] [RFC PATCH 17/20] libmultipath/foreign: nvme foreign library

2018-03-02 Thread Martin Wilck
On Wed, 2018-02-28 at 21:14 -0600, Benjamin Marzinski wrote:
> On Tue, Feb 20, 2018 at 02:26:55PM +0100, Martin Wilck wrote:
> > This still contains stubs for path handling and checking, but it's
> > functional
> > for printing already.
> > 
> > Signed-off-by: Martin Wilck 
> > ---
> >  Makefile  |   1 +
> >  libmultipath/foreign/Makefile |  30 +++
> >  libmultipath/foreign/nvme.c   | 444
> > ++
> >  3 files changed, 475 insertions(+)
> >  create mode 100644 libmultipath/foreign/Makefile
> >  create mode 100644 libmultipath/foreign/nvme.c
> > 
> > diff --git a/libmultipath/foreign/nvme.c
> > b/libmultipath/foreign/nvme.c
> > new file mode 100644
> > index ..4e9c3a52d03c
> > --- /dev/null
> > +++ b/libmultipath/foreign/nvme.c
> > 
> > +static void cleanup_nvme_map(struct nvme_map *map)
> > +{
> > +   if (map->udev)
> > +   udev_device_unref(map->udev);
> > +   if (map->subsys)
> > +   udev_device_unref(map->subsys);
> 
> According to the libudev reference manual, udev devices returned by
> udev_device_get_parent_with_subsystem_devtype are attached to their
> child and free along with them, so this unref shouldn't be necessary

I'd figured that myself and fixed it in patch 20/20. It must have
excaped my attention when I cleaned up. I'll fix the sequence.

> +
> > +void cleanup(struct context *ctx)
> > +{
> > +   if (ctx == NULL)
> > +   return;
> > +
> > +   (void)delete_all(ctx);
> > +
> > +   lock(ctx);
> > +   pthread_cleanup_push(unlock, ctx);
> > +   vector_free(ctx->mpvec);
> > +   pthread_cleanup_pop(1);
> > +   pthread_mutex_destroy(>mutex);
> > +
> > +   free(ctx);
> > +}
> 
> Would you mind either removing the locking, or adding a comment
> explaining that it's not necessary here.  If you really did need to
> lock
> ctx in order guard against someone accessing ctx->mpvec in cleanup(),
> then not setting it to NULL after its freed, and immediately freeing
> ctx
> afterwards would clearly be broken, so seeing the locking makes it
> look
> like this function is wrong.

I don't understand, sorry. I need to lock because some other entity may
still be using ctx->mpvec when cleanup() is called, and I should wait
until that other entity has released the lock before I free it. I'm
also pretty sure that checkers such as coverity would complain if I
free a structure here without locking which I access only under the
lock in other places.

I agree, though, that I should nullify the data and add checks in the
other functions. I'll also add some locking in the foreign.c file,
didn't occur to me before.

> > +static int _add_map(struct context *ctx, struct udev_device *ud,
> > +   struct udev_device *subsys)
> > +{
> > +   dev_t devt = udev_device_get_devnum(ud);
> > +   struct nvme_map *map;
> > +
> > +   if (_find_nvme_map_by_devt(ctx, devt) != NULL)
> > +   return FOREIGN_OK;
> > +
> > +   map = calloc(1, sizeof(*map));
> > +   if (map == NULL)
> > +   return FOREIGN_ERR;
> > +
> > +   map->devt = devt;
> > +   map->udev = udev_device_ref(ud);
> > +   map->subsys = udev_device_ref(subsys);
> 
> You shouldn't need to get a reference here, since map->udev will keep
> this device around.

See above.

Regards,
Martin

> 
> -Ben
> 
> > +   map->gen.ops = _map_ops;
> > +
> > +   if (vector_alloc_slot(ctx->mpvec) == NULL) {
> > +   cleanup_nvme_map(map);
> > +   return FOREIGN_ERR;
> > +   }
> > +
> > +   vector_set_slot(ctx->mpvec, map);
> > +
> > +   return FOREIGN_CLAIMED;
> > +}
> > +
> > +int add(struct context *ctx, struct udev_device *ud)
> > +{
> > +   struct udev_device *subsys;
> > +   int rc;
> > +
> > +   condlog(5, "%s called for \"%s\"", __func__, THIS);
> > +
> > +   if (ud == NULL)
> > +   return FOREIGN_ERR;
> > +   if (strcmp("disk", udev_device_get_devtype(ud)))
> > +   return FOREIGN_IGNORED;
> > +
> > +   subsys = udev_device_get_parent_with_subsystem_devtype(ud,
> > +  "nv
> > me-subsystem",
> > +  NUL
> > L);
> > +   if (subsys == NULL)
> > +   return FOREIGN_IGNORED;
> > +
> > +   lock(ctx);
> > +   pthread_cleanup_push(unlock, ctx);
> > +   rc = _add_map(ctx, ud, subsys);
> > +   pthread_cleanup_pop(1);
> > +
> > +   if (rc == FOREIGN_CLAIMED)
> > +   condlog(3, "%s: %s: added map %s", __func__, THIS,
> > +   udev_device_get_sysname(ud));
> > +   else if (rc != FOREIGN_OK)
> > +   condlog(1, "%s: %s: retcode %d adding %s",
> > +   __func__, THIS, rc,
> > udev_device_get_sysname(ud));
> > +
> > +   return rc;
> > +}
> > +
> > +int change(struct context *ctx, struct udev_device *ud)
> > +{
> > +   condlog(5, "%s called for \"%s\"", __func__, THIS);
> > +   return FOREIGN_IGNORED;
> > +}
> > +
> > +static int _delete_map(struct context *ctx, struct udev_device

Re: [dm-devel] [RFC PATCH 17/20] libmultipath/foreign: nvme foreign library

2018-02-28 Thread Benjamin Marzinski
On Tue, Feb 20, 2018 at 02:26:55PM +0100, Martin Wilck wrote:
> This still contains stubs for path handling and checking, but it's functional
> for printing already.
> 
> Signed-off-by: Martin Wilck 
> ---
>  Makefile  |   1 +
>  libmultipath/foreign/Makefile |  30 +++
>  libmultipath/foreign/nvme.c   | 444 
> ++
>  3 files changed, 475 insertions(+)
>  create mode 100644 libmultipath/foreign/Makefile
>  create mode 100644 libmultipath/foreign/nvme.c
> 
> diff --git a/Makefile b/Makefile
> index 11c46eb4dbc9..4b145c593605 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -7,6 +7,7 @@ BUILDDIRS = \
>   libmultipath \
>   libmultipath/prioritizers \
>   libmultipath/checkers \
> + libmultipath/foreign \
>   libmpathpersist \
>   multipath \
>   multipathd \
> diff --git a/libmultipath/foreign/Makefile b/libmultipath/foreign/Makefile
> new file mode 100644
> index ..dfba11e86d76
> --- /dev/null
> +++ b/libmultipath/foreign/Makefile
> @@ -0,0 +1,30 @@
> +#
> +# Copyright (C) 2003 Christophe Varoqui, 
> +#
> +include ../../Makefile.inc
> +
> +CFLAGS += $(LIB_CFLAGS) -I..
> +
> +# If you add or remove a checker also update multipath/multipath.conf.5
> +LIBS= \
> + libforeign-nvme.so
> +
> +all: $(LIBS)
> +
> +libforeign-%.so: %.o
> + $(CC) $(LDFLAGS) $(SHARED_FLAGS) -o $@ $^
> +
> +install:
> + $(INSTALL_PROGRAM) -m 755 $(LIBS) $(DESTDIR)$(libdir)
> +
> +uninstall:
> + for file in $(LIBS); do $(RM) $(DESTDIR)$(libdir)/$$file; done
> +
> +clean: dep_clean
> + $(RM) core *.a *.o *.gz *.so
> +
> +OBJS := $(LIBS:libforeign-%.so=%.o)
> +include $(wildcard $(OBJS:.o=.d))
> +
> +dep_clean:
> + $(RM) $(OBJS:.o=.d)
> diff --git a/libmultipath/foreign/nvme.c b/libmultipath/foreign/nvme.c
> new file mode 100644
> index ..4e9c3a52d03c
> --- /dev/null
> +++ b/libmultipath/foreign/nvme.c
> @@ -0,0 +1,444 @@
> +/*
> +  Copyright (c) 2018 Martin Wilck, SUSE Linux GmbH
> +
> +  This program is free software; you can redistribute it and/or
> +  modify it under the terms of the GNU General Public License
> +  as published by the Free Software Foundation; either version 2
> +  of the License, or (at your option) any later version.
> +
> +  This program is distributed in the hope that it will be useful,
> +  but WITHOUT ANY WARRANTY; without even the implied warranty of
> +  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +  GNU General Public License for more details.
> +
> +  You should have received a copy of the GNU General Public License
> +  along with this program; if not, write to the Free Software
> +  Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301,
> +  USA.
> +*/
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "vector.h"
> +#include "generic.h"
> +#include "foreign.h"
> +#include "debug.h"
> +
> +const char *THIS;
> +
> +struct nvme_map {
> + struct gen_multipath gen;
> + struct udev_device *udev;
> + struct udev_device *subsys;
> + dev_t devt;
> +};
> +
> +#define NAME_LEN 64 /* buffer length temp model name */
> +#define const_gen_mp_to_nvme(g) ((const struct nvme_map*)(g))
> +#define gen_mp_to_nvme(g) ((struct nvme_map*)(g))
> +#define nvme_mp_to_gen(n) &((n)->gen)
> +
> +static void cleanup_nvme_map(struct nvme_map *map)
> +{
> + if (map->udev)
> + udev_device_unref(map->udev);
> + if (map->subsys)
> + udev_device_unref(map->subsys);

According to the libudev reference manual, udev devices returned by
udev_device_get_parent_with_subsystem_devtype are attached to their
child and free along with them, so this unref shouldn't be necessary

> + free(map);
> +}
> +
> +static const struct _vector*
> +nvme_mp_get_pgs(const struct gen_multipath *gmp) {
> + return NULL;
> +}
> +
> +static void
> +nvme_mp_rel_pgs(const struct gen_multipath *gmp, const struct _vector *v)
> +{
> +}
> +
> +static void rstrip(char *str)
> +{
> + int n;
> +
> + for (n = strlen(str) - 1; n >= 0 && str[n] == ' '; n--);
> + str[n+1] = '\0';
> +}
> +
> +static int snprint_nvme_map(const struct gen_multipath *gmp,
> + char *buff, int len, char wildcard)
> +{
> + const struct nvme_map *nvm = const_gen_mp_to_nvme(gmp);
> + static const char nvme_vendor[] = "NVMe";
> + char fld[NAME_LEN];
> + const char *val;
> +
> + switch (wildcard) {
> + case 'd':
> + return snprintf(buff, len, "%s",
> + udev_device_get_sysname(nvm->udev));
> + case 'n':
> + return snprintf(buff, len, "%s:NQN:%s",
> + udev_device_get_sysname(nvm->subsys),
> + udev_device_get_sysattr_value(nvm->subsys,
> +   

[dm-devel] [RFC PATCH 17/20] libmultipath/foreign: nvme foreign library

2018-02-20 Thread Martin Wilck
This still contains stubs for path handling and checking, but it's functional
for printing already.

Signed-off-by: Martin Wilck 
---
 Makefile  |   1 +
 libmultipath/foreign/Makefile |  30 +++
 libmultipath/foreign/nvme.c   | 444 ++
 3 files changed, 475 insertions(+)
 create mode 100644 libmultipath/foreign/Makefile
 create mode 100644 libmultipath/foreign/nvme.c

diff --git a/Makefile b/Makefile
index 11c46eb4dbc9..4b145c593605 100644
--- a/Makefile
+++ b/Makefile
@@ -7,6 +7,7 @@ BUILDDIRS = \
libmultipath \
libmultipath/prioritizers \
libmultipath/checkers \
+   libmultipath/foreign \
libmpathpersist \
multipath \
multipathd \
diff --git a/libmultipath/foreign/Makefile b/libmultipath/foreign/Makefile
new file mode 100644
index ..dfba11e86d76
--- /dev/null
+++ b/libmultipath/foreign/Makefile
@@ -0,0 +1,30 @@
+#
+# Copyright (C) 2003 Christophe Varoqui, 
+#
+include ../../Makefile.inc
+
+CFLAGS += $(LIB_CFLAGS) -I..
+
+# If you add or remove a checker also update multipath/multipath.conf.5
+LIBS= \
+   libforeign-nvme.so
+
+all: $(LIBS)
+
+libforeign-%.so: %.o
+   $(CC) $(LDFLAGS) $(SHARED_FLAGS) -o $@ $^
+
+install:
+   $(INSTALL_PROGRAM) -m 755 $(LIBS) $(DESTDIR)$(libdir)
+
+uninstall:
+   for file in $(LIBS); do $(RM) $(DESTDIR)$(libdir)/$$file; done
+
+clean: dep_clean
+   $(RM) core *.a *.o *.gz *.so
+
+OBJS := $(LIBS:libforeign-%.so=%.o)
+include $(wildcard $(OBJS:.o=.d))
+
+dep_clean:
+   $(RM) $(OBJS:.o=.d)
diff --git a/libmultipath/foreign/nvme.c b/libmultipath/foreign/nvme.c
new file mode 100644
index ..4e9c3a52d03c
--- /dev/null
+++ b/libmultipath/foreign/nvme.c
@@ -0,0 +1,444 @@
+/*
+  Copyright (c) 2018 Martin Wilck, SUSE Linux GmbH
+
+  This program is free software; you can redistribute it and/or
+  modify it under the terms of the GNU General Public License
+  as published by the Free Software Foundation; either version 2
+  of the License, or (at your option) any later version.
+
+  This program is distributed in the hope that it will be useful,
+  but WITHOUT ANY WARRANTY; without even the implied warranty of
+  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+  GNU General Public License for more details.
+
+  You should have received a copy of the GNU General Public License
+  along with this program; if not, write to the Free Software
+  Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301,
+  USA.
+*/
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "vector.h"
+#include "generic.h"
+#include "foreign.h"
+#include "debug.h"
+
+const char *THIS;
+
+struct nvme_map {
+   struct gen_multipath gen;
+   struct udev_device *udev;
+   struct udev_device *subsys;
+   dev_t devt;
+};
+
+#define NAME_LEN 64 /* buffer length temp model name */
+#define const_gen_mp_to_nvme(g) ((const struct nvme_map*)(g))
+#define gen_mp_to_nvme(g) ((struct nvme_map*)(g))
+#define nvme_mp_to_gen(n) &((n)->gen)
+
+static void cleanup_nvme_map(struct nvme_map *map)
+{
+   if (map->udev)
+   udev_device_unref(map->udev);
+   if (map->subsys)
+   udev_device_unref(map->subsys);
+   free(map);
+}
+
+static const struct _vector*
+nvme_mp_get_pgs(const struct gen_multipath *gmp) {
+   return NULL;
+}
+
+static void
+nvme_mp_rel_pgs(const struct gen_multipath *gmp, const struct _vector *v)
+{
+}
+
+static void rstrip(char *str)
+{
+   int n;
+
+   for (n = strlen(str) - 1; n >= 0 && str[n] == ' '; n--);
+   str[n+1] = '\0';
+}
+
+static int snprint_nvme_map(const struct gen_multipath *gmp,
+   char *buff, int len, char wildcard)
+{
+   const struct nvme_map *nvm = const_gen_mp_to_nvme(gmp);
+   static const char nvme_vendor[] = "NVMe";
+   char fld[NAME_LEN];
+   const char *val;
+
+   switch (wildcard) {
+   case 'd':
+   return snprintf(buff, len, "%s",
+   udev_device_get_sysname(nvm->udev));
+   case 'n':
+   return snprintf(buff, len, "%s:NQN:%s",
+   udev_device_get_sysname(nvm->subsys),
+   udev_device_get_sysattr_value(nvm->subsys,
+ "subsysnqn"));
+   case 'w':
+   return snprintf(buff, len, "%s",
+   udev_device_get_sysattr_value(nvm->udev,
+ "wwid"));
+   case 'S':
+   return snprintf(buff, len, "%s",
+   udev_device_get_sysattr_value(nvm->udev,
+ "size"));
+   case 'v':
+   return snprintf(buff, len, "%s",