Re: [dm-devel] [PATCH 0/4] remove dangerous cleanup __attribute__ uses

[Benjamin Marzinski <bmarzins@redhat.com>]

On Thu, Oct 20, 2022 at 04:03:46PM +0000, Martin Wilck wrote:
> On Tue, 2022-10-11 at 16:52 -0500, Benjamin Marzinski wrote:
> > the cleanup __attribute__ is only run when a variable goes out of
> > scope
> > normally. It is not run on pthread cancellation. This means that
> > multipathd could leak whatever resources were supposed to be cleaned
> > up
> > if the thread was cancelled in a function using variables with the
> > cleanup __attribute__. This patchset removes all these uses in cases
> > where the code is run by multipathd and includes a cancellation point
> > in the variables scope (usually condlog(), which calls fprintf(), a
> > cancellation point, the way multipathd is usually run).
> 
> I have to say I don't like this. 
> 
> Cleaning up resources is certainly  very important, but in most of
> cases it's only about freeing memory on exit: memory which is going to
> be freed by the system anyway when multipathd terminates. Resource
> cleanup matters much more during runtime than on exit. The only threads
> that are cancelled during multipathd runtime are the TUR threads.
> It's nice to have valgrind show zero leaks when we kill multipathd out
> if the sudden. But we should weigh this against the side effects it
> has, which is ugly, hard-to-maintain code.
> 
> pthread_cleanup_push()/pop() calls contribute a lot to the bad
> readability and maintainability of much of the multipath code base, not
> to mention the weird errors some gcc versions throw for
> pthread_cleanup() constructs. I'd rather try to get rid of as much of
> them as we can. I know it won't be possible everywhere, because some
> resources (like file descriptors) really need to be cleaned up, but I
> am really unsure whether we need pthread cleanup for every free()
> operation.
> 
> __attribute__((cleanup())), on the contrary, goes a long way to make
> code more readable and easier to review. It actually helps a lot to
> ensure resources are properly cleaned up, considering how easily a
> "goto cleanup;" statement may be forgotten. Replacing it by
> pthread_cleanup() and goto statements is undesirable, IMO.
> 
> If your concern is really condlog(), let's discuss if we can find a way
> to convert this such that it is no cancellation point any more. I can
> imagine converting the locking in log_safe() from a mutex into some
> lockless scheme, using atomic variables, and using the log thread not
> only for syslog, but also for the fprintf() case.

Actually, I've never been a huge fan of all the work that we do to keep
multipathd from leaking memory when it's killed, but I though that was
the goal. If you don't care about these leaks, then I'm fine with
ignoring them.  How about just taking the first and third patches?

libmultipath: don't print garbage keywords 
libmultipath: use regular array for field widths

-Ben
 
> Regards
> Martin
> 
> 
> > 
> > Benjamin Marzinski (4):
> >   libmultipath: don't print garbage keywords
> >   libmultipath: avoid STRBUF_ON_STACK with cancellation points
> >   libmultipath: use regular array for field widths
> >   libmultipath: avoid cleanup __attribute__ with cancellation points
> > 
> >  libmpathutil/parser.c                    | 13 ++--
> >  libmpathutil/strbuf.h                    |  4 +-
> >  libmultipath/alias.c                     | 59 ++++++++++-------
> >  libmultipath/blacklist.c                 |  4 +-
> >  libmultipath/configure.c                 |  6 +-
> >  libmultipath/discovery.c                 | 34 ++++++----
> >  libmultipath/dmparser.c                  | 23 +++----
> >  libmultipath/foreign.c                   |  9 +--
> >  libmultipath/generic.c                   | 14 ++--
> >  libmultipath/libmultipath.version        |  4 +-
> >  libmultipath/print.c                     | 82 ++++++++++++++--------
> > --
> >  libmultipath/print.h                     |  4 +-
> >  libmultipath/prioritizers/weightedpath.c | 22 ++++---
> >  libmultipath/propsel.c                   | 76 ++++++++++++++++------
> >  libmultipath/sysfs.h                     | 11 +---
> >  libmultipath/uevent.c                    |  6 +-
> >  multipath/main.c                         |  5 +-
> >  multipathd/cli_handlers.c                | 52 +++++++--------
> >  multipathd/main.c                        | 49 ++++++++------
> >  19 files changed, 286 insertions(+), 191 deletions(-)
> > 
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel