Skip to content

Better fix for the empty notification issue#4242

Merged
martinpitt merged 2 commits intosystemd:masterfrom
keszybz:notifications
Sep 29, 2016
Merged

Better fix for the empty notification issue#4242
martinpitt merged 2 commits intosystemd:masterfrom
keszybz:notifications

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.

3 participants

Comments