[PATCH 0/7] Seven small multipath-tools patches

[PATCH 0/7] Seven small multipath-tools patches

[Bart Van Assche]

Hello Christoph,

The seven patches in this series are minor changes to a Makefile,
header files and source files that do not change any functionality
but make the code easier to maintain.

Please consider the patches in this series for inclusion in the
multipath-tools project.

Thanks,

Bart.

Bart Van Assche (7):
  Makefile: Remove assignments to unused variables
  libmultipath/configure.h: Add a forward declaration
  libmpathpersist: Add two missing #include directives
  Remove mpath_reverse_8bytes_order()
  Move the declaration of mpath_mx_alloc_len to a header file
  kpartx: Fix a compiler warning
  Remove a superfluous "extern" keyword

 Makefile                         | 27 +--------------------------
 kpartx/Makefile                  |  4 ++++
 kpartx/sysmacros.h               |  6 ++++++
 libmpathpersist/mpath_persist.h  |  3 +++
 libmpathpersist/mpath_pr_ioctl.c | 15 ---------------
 libmpathpersist/mpath_updatepr.c |  1 +
 libmpathpersist/mpathpr.h        |  2 ++
 libmultipath/configure.h         |  2 ++
 libmultipath/propsel.c           |  3 +--
 mpathpersist/main.c              |  1 -
 multipathd/main.c                |  2 --
 11 files changed, 20 insertions(+), 46 deletions(-)

-- 
2.12.2

[PATCH 1/7] Makefile: Remove assignments to unused variables

[Bart Van Assche]

No multipath-tools source file or Makefile imports anything directly
from the kernel source tree. Hence remove the variables that refer to
the kernel tree.

Since the $(VERSION) Makefile macro is not used, remove it. Note:
the version number is defined as a macro in libmultipath/version.h.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 Makefile | 27 +--------------------------
 1 file changed, 1 insertion(+), 26 deletions(-)

diff --git a/Makefile b/Makefile
index cfee0d07..bfb168fb 100644
--- a/Makefile
+++ b/Makefile
@@ -2,28 +2,6 @@
 # Copyright (C) 2003 Christophe Varoqui, <christophe.varoqui@opensvc.com>
 #
 
-#
-# Try to supply the linux kernel headers.
-#
-ifeq ($(KRNLSRC),)
-	KRNLLIB = /lib/modules/$(shell uname -r)
-	ifeq ($(shell test -r $(KRNLLIB)/source && echo 1),1)
-		KRNLSRC = $(KRNLLIB)/source
-		KRNLOBJ = $(KRNLLIB)/build
-	else
-		KRNLSRC = $(KRNLLIB)/build
-		KRNLOBJ = $(KRNLLIB)/build
-	endif
-	export KRNLSRC
-	export KRNLOBJ
-endif
-
-ifeq ($(MULTIPATH_VERSION),)
-	VERSION = $(shell basename ${PWD} | cut -d'-' -f3)
-else
-	VERSION = $(MULTIPATH_VERSION)
-endif
-
 BUILDDIRS = \
 	libmpathcmd \
 	libmultipath \
@@ -43,10 +21,7 @@ endif
 all: recurse
 
 recurse:
-	@for dir in $(BUILDDIRS); do \
-	$(MAKE) -C $$dir VERSION=$(VERSION) \
-		KRNLSRC=$(KRNLSRC) KRNLOBJ=$(KRNLOBJ) || exit $?; \
-	done
+	@for dir in $(BUILDDIRS); do $(MAKE) -C $$dir || exit $?; done
 
 recurse_clean:
 	@for dir in $(BUILDDIRS); do \
-- 
2.12.2

[PATCH 2/7] libmultipath/configure.h: Add a forward declaration

[Bart Van Assche]

This change avoids that the "#include "configure.h"" statements that
are added by the next patch trigger a compiler warning.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 libmultipath/configure.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libmultipath/configure.h b/libmultipath/configure.h
index fb078a61..fd7f581d 100644
--- a/libmultipath/configure.h
+++ b/libmultipath/configure.h
@@ -26,6 +26,8 @@ enum actions {
 #define FLUSH_ONE 1
 #define FLUSH_ALL 2
 
+struct vectors;
+
 int setup_map (struct multipath * mpp, char * params, int params_size );
 int domap (struct multipath * mpp, char * params, int is_daemon);
 int reinstate_paths (struct multipath *mpp);
-- 
2.12.2

[PATCH 3/7] libmpathpersist: Add two missing #include directives

[Bart Van Assche]

Make sure that including mpathpr.h as the first header file does not
fail. Since mpath_updatepr.c defines functions declared in mpathpr.h,
include that header file in that .c file such that the compiler can
check consistency of the .h and the .c file.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 libmpathpersist/mpath_updatepr.c | 1 +
 libmpathpersist/mpathpr.h        | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/libmpathpersist/mpath_updatepr.c b/libmpathpersist/mpath_updatepr.c
index 5af2e032..b3701b28 100644
--- a/libmpathpersist/mpath_updatepr.c
+++ b/libmpathpersist/mpath_updatepr.c
@@ -15,6 +15,7 @@
 #include "mpath_cmd.h"
 #include "uxsock.h"
 #include "memory.h"
+#include "mpathpr.h"
 
 
 int update_prflag(char * arg1, char * arg2, int noisy)
diff --git a/libmpathpersist/mpathpr.h b/libmpathpersist/mpathpr.h
index e6c2dedf..99e641b7 100644
--- a/libmpathpersist/mpathpr.h
+++ b/libmpathpersist/mpathpr.h
@@ -1,6 +1,8 @@
 #ifndef MPATHPR_H
 #define MPATHPR_H
 
+#include "structs.h" /* FILE_NAME_SIZE */
+
 struct prin_param {
 	char dev[FILE_NAME_SIZE];
 	int rq_servact;
-- 
2.12.2

[PATCH 4/7] Remove mpath_reverse_8bytes_order()

[Bart Van Assche]

Since this function is not used, remove it.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 libmpathpersist/mpath_pr_ioctl.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/libmpathpersist/mpath_pr_ioctl.c b/libmpathpersist/mpath_pr_ioctl.c
index 31b2fe6d..29df8c6f 100644
--- a/libmpathpersist/mpath_pr_ioctl.c
+++ b/libmpathpersist/mpath_pr_ioctl.c
@@ -502,21 +502,6 @@ void mpath_reverse_uint32_byteorder(uint32_t *num)
 	*num = ((byte0 << 24) | (byte1 << 16) | (byte2 << 8) | (byte3 << 0));
 }
 
-void mpath_reverse_8bytes_order(char * var)
-{
-	char byte[8];
-
-	int i;
-	for(i=0 ; i < 8 ; i++ )
-	{
-		byte[i] = var[i];
-	}
-	for(i=0 ; i < 8 ; i++ )
-	{
-		var[7 - i] = byte[i];
-	}
-}
-
 void
 dumpHex(const char* str, int len, int log)
 {
-- 
2.12.2

[PATCH 5/7] Move the declaration of mpath_mx_alloc_len to a header file

[Bart Van Assche]

This allows the compiler to verify consistency of declaration and
definition.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 libmpathpersist/mpath_persist.h | 3 +++
 mpathpersist/main.c             | 1 -
 multipathd/main.c               | 2 --
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/libmpathpersist/mpath_persist.h b/libmpathpersist/mpath_persist.h
index 79de5b5b..3faa8c9e 100644
--- a/libmpathpersist/mpath_persist.h
+++ b/libmpathpersist/mpath_persist.h
@@ -81,6 +81,9 @@ extern "C" {
 
 
 
+extern unsigned int mpath_mx_alloc_len;
+
+
 
 struct prin_readdescr
 {
diff --git a/mpathpersist/main.c b/mpathpersist/main.c
index 2e0aba3c..9de52b98 100644
--- a/mpathpersist/main.c
+++ b/mpathpersist/main.c
@@ -40,7 +40,6 @@ void mpath_print_transport_id(struct prin_fulldescr *fdesc);
 int construct_transportid(const char * inp, struct transportid transid[], int num_transportids);
 
 int logsink;
-unsigned int mpath_mx_alloc_len;
 struct config *multipath_conf;
 
 struct config *get_multipath_config(void)
diff --git a/multipathd/main.c b/multipathd/main.c
index b167cb4c..81c76cab 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -104,8 +104,6 @@ struct mpath_event_param
 	struct multipath *mpp;
 };
 
-unsigned int mpath_mx_alloc_len;
-
 int logsink;
 int verbosity;
 int bindings_read_only;
-- 
2.12.2

[PATCH 6/7] kpartx: Fix a compiler warning

[Bart Van Assche]

Avoid that building against glibc >= 2.25 triggers the following
compiler warning:

lopart.c: In function 'is_loop_device':
lopart.c:93:13: warning: In the GNU C Library, "major" is defined
 by <sys/sysmacros.h>. For historical compatibility, it is
 currently defined by <sys/types.h> as well, but we plan to
 remove this soon. To use "major", include <sys/sysmacros.h>
 directly. If you did not intend to use a system-defined macro
 "major", you should undefine it after including <sys/types.h>.
   major(statbuf.st_rdev) == loopmajor);

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 kpartx/Makefile    | 4 ++++
 kpartx/sysmacros.h | 6 ++++++
 2 files changed, 10 insertions(+)

diff --git a/kpartx/Makefile b/kpartx/Makefile
index 9441a2ba..2d2a249e 100644
--- a/kpartx/Makefile
+++ b/kpartx/Makefile
@@ -10,6 +10,10 @@ LIBDEPS += -ldevmapper
 ifneq ($(call check_func,dm_task_set_cookie,/usr/include/libdevmapper.h),0)
 	CFLAGS += -DLIBDM_API_COOKIE
 endif
+ifneq ($(call check_func,major,/usr/include/sys/sysmacros.h),0)
+	CFLAGS += -DHAVE_SYS_SYSMACROS_H
+endif
+
 
 OBJS = bsd.o dos.o kpartx.o solaris.o unixware.o dasd.o sun.o \
 	gpt.o mac.o ps3.o crc32.o lopart.o xstrncpy.o devmapper.o
diff --git a/kpartx/sysmacros.h b/kpartx/sysmacros.h
index 171b33d4..dc5077ec 100644
--- a/kpartx/sysmacros.h
+++ b/kpartx/sysmacros.h
@@ -1,5 +1,10 @@
 /* versions to be used with > 16-bit dev_t - leave unused for now */
 
+#ifdef HAVE_SYS_SYSMACROS_H
+#include <sys/sysmacros.h>
+#else
+#include <sys/types.h>
+
 #ifndef major
 #define major(dev)	((dev) >> 8)
 #endif
@@ -7,3 +12,4 @@
 #ifndef minor
 #define minor(dev)	((dev) & 0xff)
 #endif
+#endif
-- 
2.12.2

[PATCH 7/7] Remove a superfluous "extern" keyword

[Bart Van Assche]

Since it is not necessary to use the "extern" keyword in front of
a function definition, remove that keyword.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 libmultipath/propsel.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 09fe7282..3c057237 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -744,8 +744,7 @@ out:
 	return 0;
 }
 
-extern int
-select_max_sectors_kb (struct config *conf, struct multipath * mp)
+int select_max_sectors_kb(struct config *conf, struct multipath * mp)
 {
 	char *origin;
 
-- 
2.12.2

Re: [PATCH 6/7] kpartx: Fix a compiler warning

[Christophe Varoqui]

[-- Attachment #1.1: Type: text/plain, Size: 2072 bytes --]

This patch is dropped, due to
Martin's 8b9cda51f5c64bc869188ca0b2e1ac0e7eb7c667 patch switching to
sys/sysmacros.h

Otherwise, the rest of the set is now merged.
Thanks.

On Wed, May 17, 2017 at 5:43 PM, Bart Van Assche <bart.vanassche@sandisk.com
> wrote:

> Avoid that building against glibc >= 2.25 triggers the following
> compiler warning:
>
> lopart.c: In function 'is_loop_device':
> lopart.c:93:13: warning: In the GNU C Library, "major" is defined
>  by <sys/sysmacros.h>. For historical compatibility, it is
>  currently defined by <sys/types.h> as well, but we plan to
>  remove this soon. To use "major", include <sys/sysmacros.h>
>  directly. If you did not intend to use a system-defined macro
>  "major", you should undefine it after including <sys/types.h>.
>    major(statbuf.st_rdev) == loopmajor);
>
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> ---
>  kpartx/Makefile    | 4 ++++
>  kpartx/sysmacros.h | 6 ++++++
>  2 files changed, 10 insertions(+)
>
> diff --git a/kpartx/Makefile b/kpartx/Makefile
> index 9441a2ba..2d2a249e 100644
> --- a/kpartx/Makefile
> +++ b/kpartx/Makefile
> @@ -10,6 +10,10 @@ LIBDEPS += -ldevmapper
>  ifneq ($(call check_func,dm_task_set_cookie,/usr/include/libdevmapper.h),
> 0)
>         CFLAGS += -DLIBDM_API_COOKIE
>  endif
> +ifneq ($(call check_func,major,/usr/include/sys/sysmacros.h),0)
> +       CFLAGS += -DHAVE_SYS_SYSMACROS_H
> +endif
> +
>
>  OBJS = bsd.o dos.o kpartx.o solaris.o unixware.o dasd.o sun.o \
>         gpt.o mac.o ps3.o crc32.o lopart.o xstrncpy.o devmapper.o
> diff --git a/kpartx/sysmacros.h b/kpartx/sysmacros.h
> index 171b33d4..dc5077ec 100644
> --- a/kpartx/sysmacros.h
> +++ b/kpartx/sysmacros.h
> @@ -1,5 +1,10 @@
>  /* versions to be used with > 16-bit dev_t - leave unused for now */
>
> +#ifdef HAVE_SYS_SYSMACROS_H
> +#include <sys/sysmacros.h>
> +#else
> +#include <sys/types.h>
> +
>  #ifndef major
>  #define major(dev)     ((dev) >> 8)
>  #endif
> @@ -7,3 +12,4 @@
>  #ifndef minor
>  #define minor(dev)     ((dev) & 0xff)
>  #endif
> +#endif
> --
> 2.12.2
>
>

[-- Attachment #1.2: Type: text/html, Size: 2836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 5/7] Move the declaration of mpath_mx_alloc_len to a header file

[Benjamin Marzinski]

On Wed, May 17, 2017 at 08:43:07AM -0700, Bart Van Assche wrote:
> This allows the compiler to verify consistency of declaration and
> definition.

I get why you did this, but I'm not totally happy with where the extern
declaration is.  libmpathpersist is actually a public library, unlike
libmultipath, and mpath_persist.h is that header for that library.  As
such, I'd rather not add things to it that users aren't supposed to mess
with.  So, is your intention to let users set the max alloc length. If
so, we should probably give them a function, or at the very least some
comments telling them what this variable does. If we only want the
mpathpersist program to be able to change this, then we probably
should probably put the declaration in a different header file, perhaps
mpath_pr_ioctl.c. Now that I'm looking at this, I see that this option
(-l) isn't even documented in the mpathpersist manpage. So right now
this seems likely a completely unused feature. I guess since it exists,
I'd lean towards actually documenting it and putting it in the
libmpathpersist API in a more useful way.

-Ben

> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> ---
>  libmpathpersist/mpath_persist.h | 3 +++
>  mpathpersist/main.c             | 1 -
>  multipathd/main.c               | 2 --
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/libmpathpersist/mpath_persist.h b/libmpathpersist/mpath_persist.h
> index 79de5b5b..3faa8c9e 100644
> --- a/libmpathpersist/mpath_persist.h
> +++ b/libmpathpersist/mpath_persist.h
> @@ -81,6 +81,9 @@ extern "C" {
>  
>  
>  
> +extern unsigned int mpath_mx_alloc_len;
> +
> +
>  
>  struct prin_readdescr
>  {
> diff --git a/mpathpersist/main.c b/mpathpersist/main.c
> index 2e0aba3c..9de52b98 100644
> --- a/mpathpersist/main.c
> +++ b/mpathpersist/main.c
> @@ -40,7 +40,6 @@ void mpath_print_transport_id(struct prin_fulldescr *fdesc);
>  int construct_transportid(const char * inp, struct transportid transid[], int num_transportids);
>  
>  int logsink;
> -unsigned int mpath_mx_alloc_len;
>  struct config *multipath_conf;
>  
>  struct config *get_multipath_config(void)
> diff --git a/multipathd/main.c b/multipathd/main.c
> index b167cb4c..81c76cab 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -104,8 +104,6 @@ struct mpath_event_param
>  	struct multipath *mpp;
>  };
>  
> -unsigned int mpath_mx_alloc_len;
> -
>  int logsink;
>  int verbosity;
>  int bindings_read_only;
> -- 
> 2.12.2
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 5/7] Move the declaration of mpath_mx_alloc_len to a header file

[Bart Van Assche]

On Thu, 2017-05-18 at 15:06 -0500, Benjamin Marzinski wrote:
> On Wed, May 17, 2017 at 08:43:07AM -0700, Bart Van Assche wrote:
> > This allows the compiler to verify consistency of declaration and
> > definition.
> > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> > ---
> >  libmpathpersist/mpath_persist.h | 3 +++
> >  mpathpersist/main.c             | 1 -
> >  multipathd/main.c               | 2 --
> >  3 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libmpathpersist/mpath_persist.h b/libmpathpersist/mpath_persist.h
> > index 79de5b5b..3faa8c9e 100644
> > --- a/libmpathpersist/mpath_persist.h
> > +++ b/libmpathpersist/mpath_persist.h
> > @@ -81,6 +81,9 @@ extern "C" {
> >  
> >  
> >  
> > +extern unsigned int mpath_mx_alloc_len;
> > +
> > +
> >  
> >  struct prin_readdescr
> >  {
> > diff --git a/mpathpersist/main.c b/mpathpersist/main.c
> > index 2e0aba3c..9de52b98 100644
> > --- a/mpathpersist/main.c
> > +++ b/mpathpersist/main.c
> > @@ -40,7 +40,6 @@ void mpath_print_transport_id(struct prin_fulldescr *fdesc);
> >  int construct_transportid(const char * inp, struct transportid transid[], int num_transportids);
> >  
> >  int logsink;
> > -unsigned int mpath_mx_alloc_len;
> >  struct config *multipath_conf;
> >  
> >  struct config *get_multipath_config(void)
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index b167cb4c..81c76cab 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -104,8 +104,6 @@ struct mpath_event_param
> >  	struct multipath *mpp;
> >  };
> >  
> > -unsigned int mpath_mx_alloc_len;
> > -
> >  int logsink;
> >  int verbosity;
> >  int bindings_read_only; 
>
> I get why you did this, but I'm not totally happy with where the extern
> declaration is.  libmpathpersist is actually a public library, unlike
> libmultipath, and mpath_persist.h is that header for that library.  As
> such, I'd rather not add things to it that users aren't supposed to mess
> with.  So, is your intention to let users set the max alloc length. If
> so, we should probably give them a function, or at the very least some
> comments telling them what this variable does. If we only want the
> mpathpersist program to be able to change this, then we probably
> should probably put the declaration in a different header file, perhaps
> mpath_pr_ioctl.c. Now that I'm looking at this, I see that this option
> (-l) isn't even documented in the mpathpersist manpage. So right now
> this seems likely a completely unused feature. I guess since it exists,
> I'd lean towards actually documenting it and putting it in the
> libmpathpersist API in a more useful way.

(changed top-posting into bottom-posting)

Hello Ben,

It was not my intention to make the mpath_mx_alloc_len variable accessible
to users of the libmpathpersist library. From what I can see in
libmpathpersist/Makefile the only public header file of the libmpathpersist
library is mpath_persist.h. Would you agree to move that declaration into
mpathpr.h?

Bart.

Re: [PATCH 5/7] Move the declaration of mpath_mx_alloc_len to a header file

[Benjamin Marzinski]

On Thu, May 18, 2017 at 08:36:10PM +0000, Bart Van Assche wrote:
> On Thu, 2017-05-18 at 15:06 -0500, Benjamin Marzinski wrote:
> > On Wed, May 17, 2017 at 08:43:07AM -0700, Bart Van Assche wrote:
> > > This allows the compiler to verify consistency of declaration and
> > > definition.
> > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> > > ---
> > >  libmpathpersist/mpath_persist.h | 3 +++
> > >  mpathpersist/main.c             | 1 -
> > >  multipathd/main.c               | 2 --
> > >  3 files changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/libmpathpersist/mpath_persist.h b/libmpathpersist/mpath_persist.h
> > > index 79de5b5b..3faa8c9e 100644
> > > --- a/libmpathpersist/mpath_persist.h
> > > +++ b/libmpathpersist/mpath_persist.h
> > > @@ -81,6 +81,9 @@ extern "C" {
> > >  
> > >  
> > >  
> > > +extern unsigned int mpath_mx_alloc_len;
> > > +
> > > +
> > >  
> > >  struct prin_readdescr
> > >  {
> > > diff --git a/mpathpersist/main.c b/mpathpersist/main.c
> > > index 2e0aba3c..9de52b98 100644
> > > --- a/mpathpersist/main.c
> > > +++ b/mpathpersist/main.c
> > > @@ -40,7 +40,6 @@ void mpath_print_transport_id(struct prin_fulldescr *fdesc);
> > >  int construct_transportid(const char * inp, struct transportid transid[], int num_transportids);
> > >  
> > >  int logsink;
> > > -unsigned int mpath_mx_alloc_len;
> > >  struct config *multipath_conf;
> > >  
> > >  struct config *get_multipath_config(void)
> > > diff --git a/multipathd/main.c b/multipathd/main.c
> > > index b167cb4c..81c76cab 100644
> > > --- a/multipathd/main.c
> > > +++ b/multipathd/main.c
> > > @@ -104,8 +104,6 @@ struct mpath_event_param
> > >  	struct multipath *mpp;
> > >  };
> > >  
> > > -unsigned int mpath_mx_alloc_len;
> > > -
> > >  int logsink;
> > >  int verbosity;
> > >  int bindings_read_only; 
> >
> > I get why you did this, but I'm not totally happy with where the extern
> > declaration is.  libmpathpersist is actually a public library, unlike
> > libmultipath, and mpath_persist.h is that header for that library.  As
> > such, I'd rather not add things to it that users aren't supposed to mess
> > with.  So, is your intention to let users set the max alloc length. If
> > so, we should probably give them a function, or at the very least some
> > comments telling them what this variable does. If we only want the
> > mpathpersist program to be able to change this, then we probably
> > should probably put the declaration in a different header file, perhaps
> > mpath_pr_ioctl.c. Now that I'm looking at this, I see that this option
> > (-l) isn't even documented in the mpathpersist manpage. So right now
> > this seems likely a completely unused feature. I guess since it exists,
> > I'd lean towards actually documenting it and putting it in the
> > libmpathpersist API in a more useful way.
> 
> (changed top-posting into bottom-posting)
> 
> Hello Ben,
> 
> It was not my intention to make the mpath_mx_alloc_len variable accessible
> to users of the libmpathpersist library. From what I can see in
> libmpathpersist/Makefile the only public header file of the libmpathpersist
> library is mpath_persist.h. Would you agree to move that declaration into
> mpathpr.h?

Sure

-Ben

> 
> Bart.

Re: [PATCH 5/7] Move the declaration of mpath_mx_alloc_len to a header file

[Bart Van Assche]

On Thu, 2017-05-18 at 16:00 -0500, Benjamin Marzinski wrote:
> On Thu, May 18, 2017 at 08:36:10PM +0000, Bart Van Assche wrote:
> > On Thu, 2017-05-18 at 15:06 -0500, Benjamin Marzinski wrote:
> > > On Wed, May 17, 2017 at 08:43:07AM -0700, Bart Van Assche wrote:
> > > > This allows the compiler to verify consistency of declaration and
> > > > definition.
> > > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> > > > ---
> > > >  libmpathpersist/mpath_persist.h | 3 +++
> > > >  mpathpersist/main.c             | 1 -
> > > >  multipathd/main.c               | 2 --
> > > >  3 files changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/libmpathpersist/mpath_persist.h b/libmpathpersist/mpath_persist.h
> > > > index 79de5b5b..3faa8c9e 100644
> > > > --- a/libmpathpersist/mpath_persist.h
> > > > +++ b/libmpathpersist/mpath_persist.h
> > > > @@ -81,6 +81,9 @@ extern "C" {
> > > >  
> > > >  
> > > >  
> > > > +extern unsigned int mpath_mx_alloc_len;
> > > > +
> > > > +
> > > >  
> > > >  struct prin_readdescr
> > > >  {
> > > > diff --git a/mpathpersist/main.c b/mpathpersist/main.c
> > > > index 2e0aba3c..9de52b98 100644
> > > > --- a/mpathpersist/main.c
> > > > +++ b/mpathpersist/main.c
> > > > @@ -40,7 +40,6 @@ void mpath_print_transport_id(struct prin_fulldescr *fdesc);
> > > >  int construct_transportid(const char * inp, struct transportid transid[], int num_transportids);
> > > >  
> > > >  int logsink;
> > > > -unsigned int mpath_mx_alloc_len;
> > > >  struct config *multipath_conf;
> > > >  
> > > >  struct config *get_multipath_config(void)
> > > > diff --git a/multipathd/main.c b/multipathd/main.c
> > > > index b167cb4c..81c76cab 100644
> > > > --- a/multipathd/main.c
> > > > +++ b/multipathd/main.c
> > > > @@ -104,8 +104,6 @@ struct mpath_event_param
> > > >  	struct multipath *mpp;
> > > >  };
> > > >  
> > > > -unsigned int mpath_mx_alloc_len;
> > > > -
> > > >  int logsink;
> > > >  int verbosity;
> > > >  int bindings_read_only; 
> > > 
> > > I get why you did this, but I'm not totally happy with where the extern
> > > declaration is.  libmpathpersist is actually a public library, unlike
> > > libmultipath, and mpath_persist.h is that header for that library.  As
> > > such, I'd rather not add things to it that users aren't supposed to mess
> > > with.  So, is your intention to let users set the max alloc length. If
> > > so, we should probably give them a function, or at the very least some
> > > comments telling them what this variable does. If we only want the
> > > mpathpersist program to be able to change this, then we probably
> > > should probably put the declaration in a different header file, perhaps
> > > mpath_pr_ioctl.c. Now that I'm looking at this, I see that this option
> > > (-l) isn't even documented in the mpathpersist manpage. So right now
> > > this seems likely a completely unused feature. I guess since it exists,
> > > I'd lean towards actually documenting it and putting it in the
> > > libmpathpersist API in a more useful way.
> > 
> > (changed top-posting into bottom-posting)
> > 
> > Hello Ben,
> > 
> > It was not my intention to make the mpath_mx_alloc_len variable accessible
> > to users of the libmpathpersist library. From what I can see in
> > libmpathpersist/Makefile the only public header file of the libmpathpersist
> > library is mpath_persist.h. Would you agree to move that declaration into
> > mpathpr.h?
> 
> Sure

Hello Ben,

When I tried to implement what I proposed I noticed that there is already a user
of the libmpathpersist library that modifies the mpath_mx_alloc_len variable,
namely the mpathpersist executable. So what I proposed is not possible because it
would break the build of the mpathpersist executable.

Bart.