| Message ID | 20200916153718.582-18-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | multipath-tools: shutdown, libdevmapper races, globals | expand |
On Wed, Sep 16, 2020 at 05:37:16PM +0200, mwilck@suse.com wrote: > From: Martin Wilck <mwilck@suse.com> > > With these symbols added, applications using libmultipath don't > need to define global variables "udev" and "logsink" any more. > This comes at the cost of having to call an init function. > Currently, libmultipath_init() does nothing but initialize > "udev". > > The linker's symbol lookup order still allows applications to use > their own "logsink" and "udev" variables, which will take precendence > over libmultipath's internal ones. In this case, calling > libmultipath_init() can be skipped, but like before, > udev should be initialized (using udev_new()) before making any > libmultipath calls. > > Signed-off-by: Martin Wilck <mwilck@suse.com> > --- > libmultipath/config.c | 22 ++++++++++++++++++++++ > libmultipath/config.h | 4 +++- > libmultipath/debug.c | 2 ++ > 3 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/libmultipath/config.c b/libmultipath/config.c > index b83e5cd..4b48b27 100644 > --- a/libmultipath/config.c > +++ b/libmultipath/config.c > @@ -27,6 +27,28 @@ > #include "mpath_cmd.h" > #include "propsel.h" > > +static pthread_once_t _udev_once = PTHREAD_ONCE_INIT; > +struct udev *udev; > + > +void _udev_init(void) > +{ > + udev = udev_new(); > + if (!udev) > + condlog(0, "%s: failed to initialize udev", __func__); > +} > + > +int libmultipath_init(void) > +{ > + if (!udev) > + pthread_once(&_udev_once, _udev_init); > + return udev ? 0 : 1; > +} > + > +void libmultipath_exit(void) > +{ > + udev_unref(udev); > +} After calling libmultipath_exit(), you can never reinitialized the udev device. That seems fine, but it should probably set udev to null, so that future calls to libmultipath_init() don't return success. Either that or multipath_init() should use a mutex instead of pthread_once() to avoid races, so that you can reinitialize udev after a call to libmultipath_exit(). -Ben > + > static struct config __internal_config; > struct config *libmp_get_multipath_config(void) > { > diff --git a/libmultipath/config.h b/libmultipath/config.h > index 5997b71..541b2e4 100644 > --- a/libmultipath/config.h > +++ b/libmultipath/config.h > @@ -232,7 +232,9 @@ struct config { > char *enable_foreign; > }; > > -extern struct udev * udev; > +extern struct udev *udev; > +int libmultipath_init(void); > +void libmultipath_exit(void); > > int find_hwe (const struct _vector *hwtable, > const char * vendor, const char * product, const char *revision, > diff --git a/libmultipath/debug.c b/libmultipath/debug.c > index 4128cb9..b3a1de9 100644 > --- a/libmultipath/debug.c > +++ b/libmultipath/debug.c > @@ -15,6 +15,8 @@ > #include "defaults.h" > #include "debug.h" > > +int logsink; > + > void dlog (int sink, int prio, const char * fmt, ...) > { > va_list ap; > -- > 2.28.0 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Mon, 2020-09-21 at 15:10 -0500, Benjamin Marzinski wrote: > > After calling libmultipath_exit(), you can never reinitialized the > udev > device. That seems fine, but it should probably set udev to null, so > that future calls to libmultipath_init() don't return success. Either > that or multipath_init() should use a mutex instead of pthread_once() > to > avoid races, so that you can reinitialize udev after a call to > libmultipath_exit(). I've been thinking about this some more. It makes a lot of sense to move more cleanup code into libmultipath_exit() in the future; thus this function will do more than just clean up "udev". I believe calling libmultipath_exit() will become the only supported way to clean up libmultipath, and basically mandatory, rather sooner than later. The handling of "udev" is the main cause of headache, because we don't know whether the application wants to continue to use the variable after libmultipath_exit(). In libmultipath_exit(), we can't determine if "udev" is the symbol from libmultipath or from some other object file. It's also impossible to tell by the return value of udev_unref() whether or not it destroyed the variable. Setting udev to NULL is dangerous if the uevent listener thread is still running, or if the application needs to use the variable further. So this is my current idea for a "robust" design: 1. libmultipath_init() initializes udev if it's NULL; otherwise, it simply takes an additional ref. IOW, applications may (but don't have to) initialize and use udev before calling libmultipath_init(). 2. libmultipath_exit() calls udev_unref(udev), without nullifying it. Thus applications may continue using udev afterwards, if they own an additional reference. 3. libmultipath_init() always fails after calling libmultipath_exit(). 4. Other libmultipath calls after libmultipath_exit() may cause undefined behavior. 5. Normal usage would be to call libmultipath_exit() at program exit. 6. Calling libmultipath_init() is currently only mandatory for programs that don't initialize "udev" themselves. This may change in the future. 7. Calling libmultipath_exit() will be mandatory for programs that wish to avoid memory leaks. The only downside that I see is that the application can't test whether "udev" is valid by checking if it's NULL. But that's by design of udev_unref(). The application needs to track its own references. There is no rigorous reason for (3.). In principle, we could just handle re-initialization like (1.). But I don't think it's worth the effort of figuring out all possible ways in which re-initialization could go wrong, in particular if we want to initialize more stuff later. Does this make sense? Martin -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Wed, Sep 23, 2020 at 10:16:48AM +0200, Martin Wilck wrote: > On Mon, 2020-09-21 at 15:10 -0500, Benjamin Marzinski wrote: > > > > After calling libmultipath_exit(), you can never reinitialized the > > udev > > device. That seems fine, but it should probably set udev to null, so > > that future calls to libmultipath_init() don't return success. Either > > that or multipath_init() should use a mutex instead of pthread_once() > > to > > avoid races, so that you can reinitialize udev after a call to > > libmultipath_exit(). > > I've been thinking about this some more. It makes a lot of sense to > move more cleanup code into libmultipath_exit() in the future; thus > this function will do more than just clean up "udev". I believe calling > libmultipath_exit() will become the only supported way to clean up > libmultipath, and basically mandatory, rather sooner than later. > > The handling of "udev" is the main cause of headache, because we don't > know whether the application wants to continue to use the variable > after libmultipath_exit(). In libmultipath_exit(), we can't determine > if "udev" is the symbol from libmultipath or from some other object > file. It's also impossible to tell by the return value of udev_unref() > whether or not it destroyed the variable. Setting udev to NULL is > dangerous if the uevent listener thread is still running, or if the > application needs to use the variable further. > > So this is my current idea for a "robust" design: > > 1. libmultipath_init() initializes udev if it's NULL; otherwise, it > simply takes an additional ref. IOW, applications may (but > don't have to) initialize and use udev before calling > libmultipath_init(). > 2. libmultipath_exit() calls udev_unref(udev), without nullifying it. > Thus applications may continue using udev afterwards, if they own an > additional reference. > 3. libmultipath_init() always fails after calling libmultipath_exit(). > 4. Other libmultipath calls after libmultipath_exit() may cause > undefined behavior. > 5. Normal usage would be to call libmultipath_exit() at program exit. > 6. Calling libmultipath_init() is currently only mandatory for > programs that don't initialize "udev" themselves. This may change > in the future. > 7. Calling libmultipath_exit() will be mandatory for programs that > wish to avoid memory leaks. > > The only downside that I see is that the application can't test whether > "udev" is valid by checking if it's NULL. But that's by design of > udev_unref(). The application needs to track its own references. > > There is no rigorous reason for (3.). In principle, we could just > handle re-initialization like (1.). But I don't think it's worth the > effort of figuring out all possible ways in which re-initialization > could go wrong, in particular if we want to initialize more stuff > later. > > Does this make sense? Yeah. After I sent off my message, I realized that there's no way to know if you dropped the last reference, which means that NULLing out udev doesn't make sense. -Ben > Martin > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/libmultipath/config.c b/libmultipath/config.c index b83e5cd..4b48b27 100644 --- a/libmultipath/config.c +++ b/libmultipath/config.c @@ -27,6 +27,28 @@ #include "mpath_cmd.h" #include "propsel.h" +static pthread_once_t _udev_once = PTHREAD_ONCE_INIT; +struct udev *udev; + +void _udev_init(void) +{ + udev = udev_new(); + if (!udev) + condlog(0, "%s: failed to initialize udev", __func__); +} + +int libmultipath_init(void) +{ + if (!udev) + pthread_once(&_udev_once, _udev_init); + return udev ? 0 : 1; +} + +void libmultipath_exit(void) +{ + udev_unref(udev); +} + static struct config __internal_config; struct config *libmp_get_multipath_config(void) { diff --git a/libmultipath/config.h b/libmultipath/config.h index 5997b71..541b2e4 100644 --- a/libmultipath/config.h +++ b/libmultipath/config.h @@ -232,7 +232,9 @@ struct config { char *enable_foreign; }; -extern struct udev * udev; +extern struct udev *udev; +int libmultipath_init(void); +void libmultipath_exit(void); int find_hwe (const struct _vector *hwtable, const char * vendor, const char * product, const char *revision, diff --git a/libmultipath/debug.c b/libmultipath/debug.c index 4128cb9..b3a1de9 100644 --- a/libmultipath/debug.c +++ b/libmultipath/debug.c @@ -15,6 +15,8 @@ #include "defaults.h" #include "debug.h" +int logsink; + void dlog (int sink, int prio, const char * fmt, ...) { va_list ap;