| Message ID | 20170517154309.17787-6-bart.vanassche@sandisk.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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;
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(-)