@dmnks commented on this pull request.
> if (fnamedPipe)
Fclose(fnamedPipe);
-if (pid)
- waitpid(pid, , 0);
+if (gpgPid)
+ waitpid(gpgPid, , 0);
Hopefully resolved in the second commit :)
--
You are receiving this because you are subscribed to this
@dmnks pushed 2 commits.
f1df9c9bd2f7e9955932a63930c008e97440c9e8 GPG: Switch back to pipe(7) for
signing
285d1823ca30f4a19bf7058b248d2dfba428a11b GPG: refactor: exit label
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
@dmnks pushed 1 commit.
787f80c2340544fd55c0a1eb7dcdb6adcbe199e1 Stop blocking when GPG process dies
prematurely (RhBug:1746353)
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
@dmnks pushed 1 commit.
7ba7f3a0d2a9a2edfb03dda2e045e8f570487514 Stop blocking when GPG process dies
prematurely (RhBug:1746353)
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
@dmnks pushed 2 commits.
1cd747fa9599424fcb0151518f4ff50e116c993e Exit
7b2074b61be01dba7568e4a8bafb8956693aa49f Stop blocking when GPG process dies
prematurely (RhBug:1746353)
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
@dmnks pushed 2 commits.
b9b42ef226facad6a632404aa385dffb30c28794 Exit
ad7d340e08d1e43a692805fc3077c33cad12e6f9 Stop blocking when GPG process dies
prematurely (RhBug:1746353)
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
@dmnks pushed 1 commit.
f343661ecb7eedf95c2206de57ad83ec71a0af3c Stop blocking when GPG process dies
prematurely (RhBug:1746353)
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
@dmnks pushed 1 commit.
a1c384cd841bc6da2f2805fb4000b3a2e24254d8 Stop blocking when GPG process dies
prematurely (RhBug:1746353)
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
@dmnks pushed 1 commit.
5f583ba5136d94f5189fe2d044c0c215c9a69f7e Stop blocking when GPG process dies
prematurely (RhBug:1746353)
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
@dmnks pushed 2 commits.
7730c3f31e73e2ae290f8aab2caee33fca905361 Rename
b98ad2d140a924f6794dbff2eb3c842cd79f200e Stop blocking when GPG process dies
prematurely (RhBug:1746353)
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
@dmnks pushed 2 commits.
3173e9a82f1a7a66264d0e092c1a338be3a1456a Rename
1a1bf9a336294539befd81c6a4519bda2e45f843 Stop blocking when GPG process dies
prematurely (RhBug:1746353)
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
@dmnks pushed 1 commit.
a1a343679f089380af6192f6fa07f73b1e7310e8 Stop blocking when GPG process dies
prematurely (RhBug:1746353)
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
pmatilai commented on this pull request.
> if (fnamedPipe)
Fclose(fnamedPipe);
-if (pid)
- waitpid(pid, , 0);
+if (gpgPid)
+ waitpid(gpgPid, , 0);
Not a problem introduced in this patch so not something you need to address,
just noting: the exit section
pmatilai commented on this pull request.
> @@ -307,8 +337,8 @@ static int runGPG(sigTarget sigt, const char *sigfile)
Fclose(fnamedPipe);
fnamedPipe = NULL;
-(void) waitpid(pid, , 0);
-pid = 0;
+(void) waitpid(gpgPid, , 0);
Hmm, what if we get a SIGCHLD in the
pmatilai commented on this pull request.
> @@ -276,7 +296,17 @@ static int runGPG(sigTarget sigt, const char *sigfile)
rpmPopMacro(NULL, "__plaintext_filename");
rpmPopMacro(NULL, "__signature_filename");
-fnamedPipe = Fopen(namedPipeName, "w");
+while (1) {
Looping while
pmatilai commented on this pull request.
> @@ -276,7 +296,17 @@ static int runGPG(sigTarget sigt, const char *sigfile)
rpmPopMacro(NULL, "__plaintext_filename");
rpmPopMacro(NULL, "__signature_filename");
-fnamedPipe = Fopen(namedPipeName, "w");
+while (1) {
+errno =
pmatilai commented on this pull request.
> @@ -238,11 +239,25 @@ static rpmtd makeSigTag(Header sigh, int ishdr, uint8_t
> *pkt, size_t pktlen)
return sigtd;
}
+static pid_t gpgPid = 0;
Okay, thinking about it with hopefully somewhat fresher head, I suppose this
should be `volatile`
dmnks commented on this pull request.
> @@ -238,11 +239,25 @@ static rpmtd makeSigTag(Header sigh, int ishdr, uint8_t
> *pkt, size_t pktlen)
return sigtd;
}
+pid_t gpgPid = 0;
Indeed. No rush!
Declaration fixed.
--
You are receiving this because you are subscribed to this thread.
@dmnks pushed 1 commit.
f4d2e9ebf21660ece2986879d6602fc7fa88aea3 Stop blocking when GPG process dies
prematurely (RhBug:1746353)
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
pmatilai commented on this pull request.
> @@ -238,11 +239,25 @@ static rpmtd makeSigTag(Header sigh, int ishdr, uint8_t
> *pkt, size_t pktlen)
return sigtd;
}
+pid_t gpgPid = 0;
I don't trust myself to do a proper review this late on Friday afternoon (and
neither should you) :smile:
Sorry for the delay; I just pushed a reworked version of this patch, please
re-review.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
@dmnks pushed 1 commit.
1c92f7bbd135e60468dda7ff0e79cdc284dbeaa1 Stop blocking when GPG process dies
prematurely (RhBug:1746353)
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
It does block on `waitpid()`. But that could be a different problem. 'll try to
investigate. I would go with this kind of solution too, for its simplicity.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
Doing it in the child (i.e. before the execve call) should not block. It just
does the same open that gpg does later on.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
I also tried putting the `open()` call in the forked process just before we
replace it with GPG with `execve()`, but the result was the same as above.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
FTR, I can make the RPM process unblock (and fail gracefully) when I use the
following instead:
`open(namedPipeName, O_NONBLOCK, "r");`
However, this only helps in the GPG failure case (expired keys). If the keys
are OK and GPG gets around to opening the pipe eventually, the RPM process
hangs
> Regarding opening the pipe for reading: oh yeah, that's right. You can't do
> it before the fork. But it should work if you do it after the fork.
Actually it does not! :) I tried opening it after the fork (in the RPM
process), just before the writer end is opened:
```
Fopen(namedPipeName,
Regarding opening the pipe for reading: oh yeah, that's right. You can't do it
before the fork. But it should work if you do it after the fork.
It would be much saner if the gpg call would just read from stdin, i.e. if we
use a normal pipe instead of that named pipe. But it's a bit late to
Corrected patch in progress, will push tomorrow.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/938#issuecomment-555122938___
Good point @mlschroe , we're still in a library here and the calling process
may have forked children we don't know about, and whose termination should not
affect our behavior. So we should only abort if the SIGCHLD originated from our
forked gpg process, and probably use SA_NOCLDSTOP to ignore
> (A different fix that might also work (untested) would be to simply open the
> named pipe for reading before doing the exec call. You can even do that
> before the fork(). The opened file descriptor will not be used by gpg, but
> that shouldn't hurt.)
Turns out this won't work. When calling
> (A different fix that might also work (untested) would be to simply open the
> named pipe for reading before doing the exec call. You can even do that
> before the fork(). The opened file descriptor will not be used by gpg, but
> that shouldn't hurt.)
This actually sounds pretty good. I'll
> A SIGCHLD can happen all the time for whatever reason, you need at least to
> check if the process still exists and retry the Fopen if it does and the
> error is EINTR.
Could you please elaborate? The thing is, what we're trying to do here is to
actually get Fopen unblock when a SIGCHLD
(A different fix that might also work (untested) would be to simply open the
named pipe for reading before doing the exec call. You can even do that before
the fork(). The opened file descriptor will not be used by gpg, but that
shouldn't hurt.)
--
You are receiving this because you are
A SIGCHLD can happen all the time for whatever reason, you need at least to
check if the process still exists and retry the Fopen if it does and the error
is EINTR.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
pmatilai commented on this pull request.
> @@ -276,7 +282,26 @@ static int runGPG(sigTarget sigt, const char *sigfile)
rpmPopMacro(NULL, "__plaintext_filename");
rpmPopMacro(NULL, "__signature_filename");
+/* The child GPG process may terminate without ever opening the pipe
dmnks commented on this pull request.
> @@ -276,7 +282,26 @@ static int runGPG(sigTarget sigt, const char *sigfile)
rpmPopMacro(NULL, "__plaintext_filename");
rpmPopMacro(NULL, "__signature_filename");
+/* The child GPG process may terminate without ever opening the pipe (such
+
pmatilai commented on this pull request.
> @@ -276,7 +282,26 @@ static int runGPG(sigTarget sigt, const char *sigfile)
rpmPopMacro(NULL, "__plaintext_filename");
rpmPopMacro(NULL, "__signature_filename");
+/* The child GPG process may terminate without ever opening the pipe
dmnks commented on this pull request.
> @@ -276,7 +282,26 @@ static int runGPG(sigTarget sigt, const char *sigfile)
rpmPopMacro(NULL, "__plaintext_filename");
rpmPopMacro(NULL, "__signature_filename");
+/* The child GPG process may terminate without ever opening the pipe (such
+
dmnks commented on this pull request.
> @@ -276,7 +282,26 @@ static int runGPG(sigTarget sigt, const char *sigfile)
rpmPopMacro(NULL, "__plaintext_filename");
rpmPopMacro(NULL, "__signature_filename");
+/* The child GPG process may terminate without ever opening the pipe (such
+
pmatilai requested changes on this pull request.
Trivia aside, save-and-restore of the previous SIGCHLD handler is a must.
> @@ -276,7 +282,26 @@ static int runGPG(sigTarget sigt, const char *sigfile)
rpmPopMacro(NULL, "__plaintext_filename");
rpmPopMacro(NULL, "__signature_filename");
@dmnks pushed 1 commit.
394a50b2eeaba90256d54cc26c44b8aa67cff827 Stop blocking when GPG process dies
prematurely (RhBug:1746353)
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
@dmnks pushed 1 commit.
124cb1c74587e26afc43882a8468088dd0862de0 Stop blocking when GPG process dies
prematurely (RhBug:1746353)
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
dmnks commented on this pull request.
> fnamedPipe = Fopen(namedPipeName, "w");
+
+if (errno == EINTR) {
+ /* We got interrupted while waiting on the reader side to open the
Hmm, seems like the mix of tabs and spaces confuses github's diff viewer :)
--
You are receiving this
44 matches
Mail list logo
