Skip to content
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

Better fix for the empty notification issue #4242

Merged
merged 2 commits into from Sep 29, 2016

Conversation

keszybz
Copy link
Member

@keszybz keszybz commented Sep 29, 2016

No description provided.

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.
Copy link
Contributor

@fbuihuu fbuihuu left a comment

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
Copy link
Member Author

keszybz commented Sep 29, 2016

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
Copy link
Contributor

fbuihuu commented Sep 29, 2016

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
Copy link
Member Author

keszybz commented Sep 29, 2016

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

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

Copy link
Contributor

@martinpitt martinpitt left a comment

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!

@martinpitt martinpitt merged commit a86b767 into systemd:master Sep 29, 2016
@keszybz keszybz deleted the notifications branch October 5, 2016 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants