Better fix for the empty notification issue by keszybz · Pull Request #4242 · systemd/systemd

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

keszybz

This undoes 531ac2b. I acked that patch without looking at the code
carefully enough. There are two problems:
- we want to process the fds anyway
- in principle empty notification messages are valid, and we should
  process them as usual, including logging using log_unit_debug().
It's probably easier to diagnose a bad notification message if the
contents are printed. But still, do anything only if debugging is on.

fbuihuu

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I don't get the "we want to process the fds anyway" part.

To process an fds, isn't the service supposed to pass "FDSTORE=1" ?

@keszybz

If you want them stored, then yes. But from systemd side, we want to process and do something with them: if we are not going to handle the message we should close the fds.

@fbuihuu

Hmm you mean that a service may want to send a fds with an empty message and can expect that systemd will close them ? If so I don't see how this can be useful and therefore why we would want to support this.

BTW how are the remaining fds which are not handled by the message closed exactly ?

@keszybz

Remaining fds are cleanup up by the auto-cleanup function.

It's not about being useful, but about proper cleanup.

martinpitt

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense, and Zbigniew also tested/verified this with python-systemd. This will get a proper test case soon, but let's push this out ASAP. Thanks!

Reviewers

@fbuihuu fbuihuu fbuihuu left review comments

@martinpitt martinpitt martinpitt approved these changes