[dm-devel] [PATCH 0/4] remove dangerous cleanup __attribute__ uses
Benjamin Marzinski
bmarzins at redhat.com
Thu Oct 20 22:57:02 UTC 2022
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(-)
> >
More information about the dm-devel
mailing list