Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
@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 thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/938#discussion_r433171564___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
@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: https://github.com/rpm-software-management/rpm/pull/938/files/787f80c2340544fd55c0a1eb7dcdb6adcbe199e1..285d1823ca30f4a19bf7058b248d2dfba428a11b ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
@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: https://github.com/rpm-software-management/rpm/pull/938/files/7ba7f3a0d2a9a2edfb03dda2e045e8f570487514..787f80c2340544fd55c0a1eb7dcdb6adcbe199e1 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
@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: https://github.com/rpm-software-management/rpm/pull/938/files/64a58570a8583e35fc2bc8de7872e56e77f6c195..7ba7f3a0d2a9a2edfb03dda2e045e8f570487514 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
@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: https://github.com/rpm-software-management/rpm/pull/938/files/fbbb90ad7411b38a16eb7df47b3eb463a3f39fbc..7b2074b61be01dba7568e4a8bafb8956693aa49f ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
@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: https://github.com/rpm-software-management/rpm/pull/938/files/a1c384cd841bc6da2f2805fb4000b3a2e24254d8..ad7d340e08d1e43a692805fc3077c33cad12e6f9 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
@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: https://github.com/rpm-software-management/rpm/pull/938/files/f2c7b420ec92bc976afce907599bba209943f6d7..f343661ecb7eedf95c2206de57ad83ec71a0af3c ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
@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: https://github.com/rpm-software-management/rpm/pull/938/files/f343661ecb7eedf95c2206de57ad83ec71a0af3c..a1c384cd841bc6da2f2805fb4000b3a2e24254d8 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
@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: https://github.com/rpm-software-management/rpm/pull/938/files/bb59f46c2f322ddd5b006e43e31953d9262c8234..5f583ba5136d94f5189fe2d044c0c215c9a69f7e ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
@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: https://github.com/rpm-software-management/rpm/pull/938/files/1a1bf9a336294539befd81c6a4519bda2e45f843..b98ad2d140a924f6794dbff2eb3c842cd79f200e ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
@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: https://github.com/rpm-software-management/rpm/pull/938/files/a1a343679f089380af6192f6fa07f73b1e7310e8..1a1bf9a336294539befd81c6a4519bda2e45f843 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
@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: https://github.com/rpm-software-management/rpm/pull/938/files/eea946804f2896ec4372f87f60b33bcaa70aad05..a1a343679f089380af6192f6fa07f73b1e7310e8 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
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 duplicates much of what is being done just above it. The whole point of having exit/error labels to jump to is to avoid just that... -- 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#pullrequestreview-32363___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
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 meanwhile? That'd set gpgPid to 0, which would turn the above into waitpid(0, ...) which doesn't seem right. -- 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#pullrequestreview-323627595___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
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 fnamedPipe remains NULL might be a more natural check here,
eternal loops always make me a bit fidgety.
--
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#pullrequestreview-323611419___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
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 = 0;
+fnamedPipe = Fopen(namedPipeName, "w");
+if (errno != EINTR)
Never trust errno alone, it can get set as a side-effect even on success
(generally speaking). You'll want to move the "Fopen failed" check from below
here, and only look at errno if the open failed.
--
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#pullrequestreview-323608669___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
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` in addition to being static. As SIGCHLD is the only registered signal for that it can't be called multiple times so atomicity issues shouldn't come to play. And now my head hurts :smile: -- 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#pullrequestreview-323605958___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
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. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/938#discussion_r349625137___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
@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: https://github.com/rpm-software-management/rpm/pull/938/files/16f8d6a8a7e91560b31a1c28855e84fc67652326..f4d2e9ebf21660ece2986879d6602fc7fa88aea3 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
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: but just a quick comment: this variable needs to be declared static, otherwise it'll end up visible in the library ABI. -- 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#pullrequestreview-321583545___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
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: https://github.com/rpm-software-management/rpm/pull/938#issuecomment-557545096___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
@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: https://github.com/rpm-software-management/rpm/pull/938/files/394a50b2eeaba90256d54cc26c44b8aa67cff827..1c92f7bbd135e60468dda7ff0e79cdc284dbeaa1 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
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: https://github.com/rpm-software-management/rpm/pull/938#issuecomment-06399___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
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: https://github.com/rpm-software-management/rpm/pull/938#issuecomment-555493960___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
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: https://github.com/rpm-software-management/rpm/pull/938#issuecomment-555487055___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
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 again. -- 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-555484709___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
> 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, "r"); // here fnamedPipe = Fopen(namedPipeName, "w"); ``` And it just blocks on the first Fopen. Or am I missing something? > 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 change > that and I guess /dev/stdin does not exist in every system rpm supports. Yeah, probably. But like you said, rewriting it would probably be too risky at this point, with little added benefit. -- 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-555482392___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
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 change that and I guess /dev/stdin does not exist in every system rpm supports. -- 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-555443418___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
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___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
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 stuff that is not our business. -- 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-555023362___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
> (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 `open(2)` on the pipe, the call blocks until the other end is opened as well. This is actually the very reason we're doing this patch. -- 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-555018828___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
> (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 give it a try, thanks! -- 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-554996718___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
> 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 happens. -- 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-554996423___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
(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 subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/938#issuecomment-554958735___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
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: https://github.com/rpm-software-management/rpm/pull/938#issuecomment-554954364___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
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 (such + * as when the key is expired), causing our parent process to get forever + * stuck on the Fopen() call below (see fifo(7) for details). To fix that, + * we need to let the underlying open(2) unblock when SIGCHLD is received, + * by registering a no-op handler, so here we go: */ +struct sigaction act; +memset(, '\0', sizeof(act)); +act.sa_handler = _sa_handler; +act.sa_flags = 0; /* Make sure we don't have SA_RESTART */ +sigaction(SIGCHLD, , NULL); OTOH there might be some other flags that we want to set, at least SA_NOCLDSTOP looks quite relevant. -- 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#discussion_r347255811___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
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 + * as when the key is expired), causing our parent process to get forever + * stuck on the Fopen() call below (see fifo(7) for details). To fix that, + * we need to let the underlying open(2) unblock when SIGCHLD is received, + * by registering a no-op handler, so here we go: */ Wow, that's great feedback, thanks. I'll definitely check out the link. -- 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#discussion_r347255137___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
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 (such + * as when the key is expired), causing our parent process to get forever + * stuck on the Fopen() call below (see fifo(7) for details). To fix that, + * we need to let the underlying open(2) unblock when SIGCHLD is received, + * by registering a no-op handler, so here we go: */ On another purely stylistic note, this comment is on the verbose side. Some general guidelines on the topic: - use comments sparingly, subtle and non-obvious things (this certainly classifies) - try to stick to one-liners for in-line comments, unless it's something *really* special - the preferred place for comments is at head of functions, and longer comments are okay there - ...but story-telling belongs to commit messages (where OTOH this sort of background info is golden) The kernel coding style differs in various aspects and some things just don't apply, but in general it's usually excellent advice and on comments it's totally spot-on in rpm too: https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting -- 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#pullrequestreview-318168130___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
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 + * as when the key is expired), causing our parent process to get forever + * stuck on the Fopen() call below (see fifo(7) for details). To fix that, + * we need to let the underlying open(2) unblock when SIGCHLD is received, + * by registering a no-op handler, so here we go: */ +struct sigaction act; +memset(, '\0', sizeof(act)); +act.sa_handler = _sa_handler; +act.sa_flags = 0; /* Make sure we don't have SA_RESTART */ +sigaction(SIGCHLD, , NULL); Oh, restoring the handler is something that I just didn't realize at all. Thanks for that. Yeah, it really is superfluous :) I wanted to signify that `SA_RESTART` is not set, but since we `memset()` the whole struct right above, it's really not needed and I'll remove it as well. -- 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#discussion_r347250293___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
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 + * as when the key is expired), causing our parent process to get forever + * stuck on the Fopen() call below (see fifo(7) for details). To fix that, + * we need to let the underlying open(2) unblock when SIGCHLD is received, + * by registering a no-op handler, so here we go: */ +struct sigaction act; +memset(, '\0', sizeof(act)); Oh OK, thanks for pointing this out, I'll fix this. -- 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#discussion_r347249103___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
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"); +/* The child GPG process may terminate without ever opening the pipe (such + * as when the key is expired), causing our parent process to get forever + * stuck on the Fopen() call below (see fifo(7) for details). To fix that, + * we need to let the underlying open(2) unblock when SIGCHLD is received, + * by registering a no-op handler, so here we go: */ +struct sigaction act; +memset(, '\0', sizeof(act)); memset() second argument is an int, not a character. Which seems kinda bizarre if you start thinking about bytes and all, but that's the way it is, and so the argument should be just plain zero, not the nul character. Not that it makes any difference functionality-wise. > @@ -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 + * as when the key is expired), causing our parent process to get forever + * stuck on the Fopen() call below (see fifo(7) for details). To fix that, + * we need to let the underlying open(2) unblock when SIGCHLD is received, + * by registering a no-op handler, so here we go: */ +struct sigaction act; +memset(, '\0', sizeof(act)); +act.sa_handler = _sa_handler; +act.sa_flags = 0; /* Make sure we don't have SA_RESTART */ +sigaction(SIGCHLD, , NULL); As we're in a shared library here, we'll need to take care to save the old handler and restore it after we're done. Pay attention to error paths. Specifically setting sa_flags to 0 seems a bit superfluous as we just memset() the struct to all zero two lines earlier. Not saying it's wrong, sometimes it's useful to signify something this way, whether that's called for here I dunno. -- 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#pullrequestreview-318142487___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
@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: https://github.com/rpm-software-management/rpm/pull/938/files/124cb1c74587e26afc43882a8468088dd0862de0..394a50b2eeaba90256d54cc26c44b8aa67cff827 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
@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: https://github.com/rpm-software-management/rpm/pull/938/files/6caa812a71087cb4949685b0154e742bee1ce807..124cb1c74587e26afc43882a8468088dd0862de0 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)
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 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#pullrequestreview-317665488___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
