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

systemd-notify: Always pass a valid pid to sd_pid_notify #1315

Merged
merged 1 commit into from Sep 21, 2015
Merged

systemd-notify: Always pass a valid pid to sd_pid_notify #1315

merged 1 commit into from Sep 21, 2015

Conversation

dvdhrm
Copy link
Contributor

@dvdhrm dvdhrm commented Sep 21, 2015

If the option --pid was used, take the pid from this option, unless take
the parend pid. Using 0 as pid (ucred of systemd-notify) will result 99% of the
time in a failure with this error: "Cannot find unit for notify message of PID"

Shouldn't we use always the ppid, since the MAINPID is something else ?

Signed-off-by: Benjamin Robin dev@benjarobin.fr

If the option --pid was used, take the pid from this option, unless take
the parend pid. Using 0 as pid (ucred of systemd-notify) will result 99% of the
time in a failure with this error: "Cannot find unit for notify message of PID"

Shouldn't we use always the ppid, since the MAINPID is something else ?

Signed-off-by: Benjamin Robin <dev@benjarobin.fr>
@poettering
Copy link
Member

Looks OK to me. Let's see if Semaphore agrees too!

@dvdhrm
Copy link
Contributor Author

dvdhrm commented Sep 21, 2015

This is imported from the ML. I wonder whether we should also set MAINPID correctly?

@dvdhrm
Copy link
Contributor Author

dvdhrm commented Sep 21, 2015

That is, make parse_argv() set arg_pid = arg_pid ? : getppid();.

@poettering
Copy link
Member

Hmm, that's a very good question. But I think it should not do that.

--pid= sets the main PID, and then sends the message on its behalf.

If --pid= is not set, then this would mean, we send the message from our parent, (which is a safer choice, since it's going to be around for longer than our tool), but this should not make it the main pid of the daemon, because we don't know if it is.

Note that --pid also may be invoked without arguments, in which case the main PID is set to getppid(). Thus we cover all three cases already:

  1. no argument sets no main PID, and sends on behalf of the PPID
  2. --pid without argument sets the PPID as main PID and sends on its behalf
  3. --pid= with argument sets the specified PID as main PID and sends on its behalf

The user hence has a nice choice and the main PID is only set if explicitly requested, otherwise we don't do that.

Or in other words: patch looks good to merge, still.

poettering added a commit that referenced this pull request Sep 21, 2015
…-git-send-email-dev@benjarobin.fr

systemd-notify: Always pass a valid pid to sd_pid_notify
@poettering poettering merged commit 35bb188 into systemd:master Sep 21, 2015
@haraldh haraldh deleted the 1442692671-10134-1-git-send-email-dev@benjarobin.fr branch July 21, 2018 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants