Tails

#1 Updated by intrigeri 2015-03-31 09:01:55

• blocks #8538 added

#2 Updated by intrigeri 2015-03-31 09:02:03

• Target version set to Tails_1.4

#4 Updated by intrigeri 2015-03-31 12:33:22

• related to Bug #8707: The automated test suite does not clean up when it's finished added

#5 Updated by intrigeri 2015-04-07 21:02:19

• Priority changed from Normal to Elevated

#6 Updated by anonym 2015-04-14 19:01:46

• Status changed from Confirmed to In Progress

Applied in changeset commit:226371d7b1be5d04f5389c303ce0d23454df7ea0.

#8 Updated by anonym 2015-04-14 19:22:00

• Assignee changed from anonym to kytv
• % Done changed from 0 to 50
• QA Check set to Ready for QA
• Feature Branch set to bugfix/9139-get-rid-of-virt-viewer-zombies

kytv, you could reproduce this issue (on Jessie), right? If so, please review’n’merge. otherwise reassign to intrigeri.

Note: I also snuck in some unrelated fixes. I hope that’s no problem. Otherwise let me know and I split them out.

#10 Updated by kytv 2015-04-14 22:09:26

• Assignee changed from kytv to intrigeri

Testing passes and the code looks good. Assigning to intrigeri since he was able to reproduce the problem.

#11 Updated by intrigeri 2015-04-15 07:28:56

• Assignee changed from intrigeri to anonym

That bug can be reproduced on lizard: when I’ve seen it, I’ve pointed anonym there. So, I’ll let anonym check if this branch indeed fixes the problem there.

#12 Updated by anonym 2015-04-15 08:21:41

• Assignee changed from anonym to intrigeri

intrigeri wrote:
> That bug can be reproduced on lizard: when I’ve seen it, I’ve pointed anonym there. So, I’ll let anonym check if this branch indeed fixes the problem there.

I ran checks.feature with the stable branch checked out, and mulitple zombies spawned. Running the same feature with this branch checked out spawned none.

#14 Updated by intrigeri 2015-04-15 13:16:17

• Assignee changed from intrigeri to anonym
• QA Check changed from Ready for QA to Info Needed

I’m glad you’ve found a solution!

Is commit:226371d enough to fix the bug this ticket and branch are about? If so, perhaps we can take this one and postpone the non-consensual other commits:

• I suspect that commit:e264b2f replaces an “awkward syntax” that has the advantage of not passing the command-line through a shell, with another syntax that isn’t as safe. Were the security and robustness implications taken into account? (Also, as a result you had to add commit:dd8a417 on top.)
• Related to that, commit:1b9c94d also fixes a problem the wrong way IMO. As is now clear, I don’t like to work around the “oh I spawned a useless shell” mistake by adding exec’s.
• commit:582b3e1 hides virt-viewer’s stdout and stderr, but doesn’t explain why. I suspect it’ll make future debugging and bug reporting harder. So, why do we need/want to do that? E.g. if we merely want to hide one specific message, perhaps we can do that, instead of hiding them all. In any case, perhaps these messages shouldn’t be hidden when running under --debug?
• I don’t understand the rationale behind commit:edd5e44, and the commit message doesn’t help me much.

#17 Updated by anonym 2015-04-17 07:58:57

• Assignee changed from anonym to intrigeri
• QA Check changed from Info Needed to Ready for QA

intrigeri wrote:
> I’m glad you’ve found a solution!
>
> Is commit:226371d enough to fix the bug this ticket and branch are about? If so, perhaps we can take this one and postpone the non-consensual other commits:

Well, it’s commit:a1fa3a8 (“Close IO object for good measure”) too. I’ve rewritten the branch to contain only these two commits, and I’ll deal with the rest in Bug #9253.

> * I suspect that commit:e264b2f replaces an “awkward syntax” that has the advantage of not passing the command-line through a shell, with another syntax that isn’t as safe. Were the security and robustness implications taken into account? (Also, as a result you had to add commit:dd8a417 on top.)

If you look again, the list was join:ed into a single string. At the time this code was written Ruby’s IO.popen didn’t support the syntax you are thinking of (which bypasses the shell) but now it does, so we should switch to that.

> * Related to that, commit:1b9c94d also fixes a problem the wrong way IMO. As is now clear, I don’t like to work around the “oh I spawned a useless shell” mistake by adding exec’s.

Right.

> * commit:582b3e1 hides virt-viewer’s stdout and stderr, but doesn’t explain why. I suspect it’ll make future debugging and bug reporting harder. So, why do we need/want to do that? E.g. if we merely want to hide one specific message, perhaps we can do that, instead of hiding them all. In any case, perhaps these messages shouldn’t be hidden when running under --debug?

I’ve never seen virt-viewer print anything useful except when you feed it bad commands. OTOH, at least since we moved to Debian Jessie it has started to spam the following to stderr all the time:

(virt-viewer:17826): Gdk-CRITICAL **: gdk_window_get_width: assertion 'GDK_IS_WINDOW (window)' failed

which I find incredibly annoying. I think suppressing this is crucial.

> * I don’t understand the rationale behind commit:edd5e44, and the commit message doesn’t help me much.

The xprop hack was a workaround when grabbing the pid wasn’t possible (see commit:eb9ea20). OTOH, looking for the pid isn’t particularly robust either (even though I seriously doubt we’d get into trouble in reality) so, whatever, let’s drop that part.

#19 Updated by intrigeri 2015-04-17 12:29:25

• Status changed from In Progress to Fix committed
• % Done changed from 50 to 100

Applied in changeset commit:7d7104152a1370e98065087ff3e1e31ab77f964e.

#20 Updated by intrigeri 2015-04-17 12:30:39

• Assignee deleted (intrigeri)
• QA Check changed from Ready for QA to Pass

Great, thanks!

#21 Updated by BitingBird 2015-05-12 18:39:29

• Status changed from Fix committed to Resolved