| Message ID | 20220318223339.4226-7-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | multipathd: fix __delayed_reconfig logic | expand |
On Fri, Mar 18, 2022 at 5:33 PM <mwilck@suse.com> wrote: > > From: Martin Wilck <mwilck@suse.com> > > This callback is be used by multipathd to unblock pending > reconfigure requests if a map is removed that multipathd is currently > waiting for. > > Signed-off-by: Martin Wilck <mwilck@suse.com> > --- > libmultipath/libmultipath.version | 3 ++- > libmultipath/structs_vec.c | 6 ++++++ > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version > index 216f0ee..8132df7 100644 > --- a/libmultipath/libmultipath.version > +++ b/libmultipath/libmultipath.version > @@ -31,7 +31,7 @@ > * The new version inherits the previous ones. > */ > > -LIBMULTIPATH_14.0.0 { > +LIBMULTIPATH_14.1.0 { > global: > /* symbols referenced by multipath and multipathd */ > add_foreign; > @@ -164,6 +164,7 @@ global: > remember_wwid; > remove_map; > remove_map_by_alias; > + remove_map_callback; > remove_maps; > remove_wwid; > replace_wwids; > diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c > index 6c23df8..a69f064 100644 > --- a/libmultipath/structs_vec.c > +++ b/libmultipath/structs_vec.c > @@ -336,11 +336,17 @@ void set_path_removed(struct path *pp) > pp->initialized = INIT_REMOVED; > } > > +void remove_map_callback(struct multipath *mpp __attribute__((unused))) > +{ > +} > + Does this work? I thought that unless you specifically declared the symbol weak, the call in remove_map() would have already gotten resolved to point to the existing remove_map_callback() when the shared library was getting created. Is it because the function is empty? Am I just misunderstanding something? -Ben > void > remove_map(struct multipath *mpp, vector pathvec, vector mpvec) > { > int i; > > + remove_map_callback(mpp); > + > free_pathvec(mpp->paths, KEEP_PATHS); > free_pgvec(mpp->pg, KEEP_PATHS); > mpp->paths = mpp->pg = NULL; > -- > 2.35.1 > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Mon, 2022-03-21 at 19:28 -0500, Benjamin Marzinski wrote: > On Fri, Mar 18, 2022 at 5:33 PM <mwilck@suse.com> wrote: > > > > From: Martin Wilck <mwilck@suse.com> > > > > This callback is be used by multipathd to unblock pending > > reconfigure requests if a map is removed that multipathd is > > currently > > waiting for. > > > > Signed-off-by: Martin Wilck <mwilck@suse.com> > > --- > > libmultipath/libmultipath.version | 3 ++- > > libmultipath/structs_vec.c | 6 ++++++ > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/libmultipath/libmultipath.version > > b/libmultipath/libmultipath.version > > index 216f0ee..8132df7 100644 > > --- a/libmultipath/libmultipath.version > > +++ b/libmultipath/libmultipath.version > > @@ -31,7 +31,7 @@ > > * The new version inherits the previous ones. > > */ > > > > -LIBMULTIPATH_14.0.0 { > > +LIBMULTIPATH_14.1.0 { > > global: > > /* symbols referenced by multipath and multipathd */ > > add_foreign; > > @@ -164,6 +164,7 @@ global: > > remember_wwid; > > remove_map; > > remove_map_by_alias; > > + remove_map_callback; > > remove_maps; > > remove_wwid; > > replace_wwids; > > diff --git a/libmultipath/structs_vec.c > > b/libmultipath/structs_vec.c > > index 6c23df8..a69f064 100644 > > --- a/libmultipath/structs_vec.c > > +++ b/libmultipath/structs_vec.c > > @@ -336,11 +336,17 @@ void set_path_removed(struct path *pp) > > pp->initialized = INIT_REMOVED; > > } > > > > +void remove_map_callback(struct multipath *mpp > > __attribute__((unused))) > > +{ > > +} > > + > > Does this work? I thought that unless you specifically declared the > symbol weak, the call in remove_map() would have already gotten > resolved to point to the existing remove_map_callback() when the > shared library was getting created. Is it because the function is > empty? Am I just misunderstanding something? This works because I added the symbol to libmultipath.version, assigning it "global" visibility. To be consistent, we could do the same thing with get_multipath_config() et al., but I didn't want to change that just now. We (or actually, users and distro integrators) have to be somewhat careful with adding linker flags. As discussed e.g. in https://github.com/opensvc/multipath-tools/issues/26 flags like "-Bsymbolic-functions" would mess this up, because this flag overrides the settings from our linker script. But declaring the symbol "weak" wouldn't protect against -Bsymbolic mess-up, either. I had a long discussion with our toolchain experts about this, which lead to the conclusion above. I am pretty positive about it. Feel free to ask the RH experts, too ;-) Regards Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Tue, Mar 22, 2022 at 3:35 AM Martin Wilck <mwilck@suse.com> wrote: > > On Mon, 2022-03-21 at 19:28 -0500, Benjamin Marzinski wrote: > > On Fri, Mar 18, 2022 at 5:33 PM <mwilck@suse.com> wrote: > > > > > > From: Martin Wilck <mwilck@suse.com> > > > > > > This callback is be used by multipathd to unblock pending > > > reconfigure requests if a map is removed that multipathd is > > > currently > > > waiting for. > > > > > > Signed-off-by: Martin Wilck <mwilck@suse.com> > > > --- > > > libmultipath/libmultipath.version | 3 ++- > > > libmultipath/structs_vec.c | 6 ++++++ > > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > > > diff --git a/libmultipath/libmultipath.version > > > b/libmultipath/libmultipath.version > > > index 216f0ee..8132df7 100644 > > > --- a/libmultipath/libmultipath.version > > > +++ b/libmultipath/libmultipath.version > > > @@ -31,7 +31,7 @@ > > > * The new version inherits the previous ones. > > > */ > > > > > > -LIBMULTIPATH_14.0.0 { > > > +LIBMULTIPATH_14.1.0 { > > > global: > > > /* symbols referenced by multipath and multipathd */ > > > add_foreign; > > > @@ -164,6 +164,7 @@ global: > > > remember_wwid; > > > remove_map; > > > remove_map_by_alias; > > > + remove_map_callback; > > > remove_maps; > > > remove_wwid; > > > replace_wwids; > > > diff --git a/libmultipath/structs_vec.c > > > b/libmultipath/structs_vec.c > > > index 6c23df8..a69f064 100644 > > > --- a/libmultipath/structs_vec.c > > > +++ b/libmultipath/structs_vec.c > > > @@ -336,11 +336,17 @@ void set_path_removed(struct path *pp) > > > pp->initialized = INIT_REMOVED; > > > } > > > > > > +void remove_map_callback(struct multipath *mpp > > > __attribute__((unused))) > > > +{ > > > +} > > > + > > > > Does this work? I thought that unless you specifically declared the > > symbol weak, the call in remove_map() would have already gotten > > resolved to point to the existing remove_map_callback() when the > > shared library was getting created. Is it because the function is > > empty? Am I just misunderstanding something? > > This works because I added the symbol to libmultipath.version, > assigning it "global" visibility. To be consistent, we could do the > same thing with get_multipath_config() et al., but I didn't want to > change that just now. So all of the exported symbols from libmultipath are weak? Good to know. Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com> > We (or actually, users and distro integrators) have to be somewhat > careful with adding linker flags. As discussed e.g. in > https://github.com/opensvc/multipath-tools/issues/26 > flags like "-Bsymbolic-functions" would mess this up, because this flag > overrides the settings from our linker script. But declaring the symbol > "weak" wouldn't protect against -Bsymbolic mess-up, either. > > I had a long discussion with our toolchain experts about this, which > lead to the conclusion above. I am pretty positive about it. > Feel free to ask the RH experts, too ;-) > > Regards > Martin > > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Tue, 2022-03-22 at 09:59 -0500, Benjamin Marzinski wrote: > > So all of the exported symbols from libmultipath are weak? Good to > know. No, "weak" has a special meaning. For dynamic linking, it's default behavior to resolve symbols by name and use the first definition encountered. This means that a symbol in a shared library will always be overridden by a symbol of the same name in the main executable. "When resolving symbolic references, the dynamic linker examines the symbol tables with a breadth-first search. That is, it first looks at the symbol table of the executable program itself, then at the symbol tables of the DT_NEEDED entries (in order), then at the second level DT_NEEDED entries, and so on." ([1], III, p. 2-12). The "weak" attribute only matters during static linking ([1], III, p. 1-5). See also the description of LD_DYNAMIC_WEAK in ld.so(8) [2]. I didn't understand this clearly before. And the documentation, in particular the explanation of the "weak" attribute in the gcc docs, is misleading, because it doesn't explain the difference between static and dynamic linking [3]. Regards Martin [1] https://refspecs.linuxfoundation.org/elf/elf.pdf [2] https://man7.org/linux/man-pages/man8/ld.so.8.html [3] https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version index 216f0ee..8132df7 100644 --- a/libmultipath/libmultipath.version +++ b/libmultipath/libmultipath.version @@ -31,7 +31,7 @@ * The new version inherits the previous ones. */ -LIBMULTIPATH_14.0.0 { +LIBMULTIPATH_14.1.0 { global: /* symbols referenced by multipath and multipathd */ add_foreign; @@ -164,6 +164,7 @@ global: remember_wwid; remove_map; remove_map_by_alias; + remove_map_callback; remove_maps; remove_wwid; replace_wwids; diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c index 6c23df8..a69f064 100644 --- a/libmultipath/structs_vec.c +++ b/libmultipath/structs_vec.c @@ -336,11 +336,17 @@ void set_path_removed(struct path *pp) pp->initialized = INIT_REMOVED; } +void remove_map_callback(struct multipath *mpp __attribute__((unused))) +{ +} + void remove_map(struct multipath *mpp, vector pathvec, vector mpvec) { int i; + remove_map_callback(mpp); + free_pathvec(mpp->paths, KEEP_PATHS); free_pgvec(mpp->pg, KEEP_PATHS); mpp->paths = mpp->pg = NULL;