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@xxxxxxxxxx https://listman.redhat.com/mailman/listinfo/dm-devel
