BUG: minor python pydoc fixes

[skrap]

No description provided.

[skrap]

(First time contributing to the project — hopefully I'm doing this properly!)

[coveralls]

Coverage remained the same at 89.794% when pulling 213ae7f on skrap:master into 2b406e3 on seccomp:master.

[tyhicks]

Hey @skrap! I'm not a libseccomp maintainer but I took a quick look at this PR. The changes look sensible to me but I had one question that I left in-line. Thanks!

UPDATE: I noticed that you missed your signed-off-by. Please see the "Sign Your Work" section of CONTRIBUTING.md.

[tyhicks]

Why is rt_sigreturn removed here?

[skrap]

I think it's largely platform differences — either python or underlying libs (I'd suspect glibc or friends) use rt_sigreturn in some versions, and not in others. I think it's probably safer to leave it in! I'll fix up.

[skrap]

Oh, actually it was just moved above. Sorry, my memory of this patch is a bit hazy apparently.

[pcmoore]

Welcome to libseccomp @skrap and thanks for the patches :) I have the same question as @tyhicks regarding rt_sigreturn, could you please explain that change? Also as Tyler pointed out, please take a closer look at CONTRIBUTING.md, thanks!

[skrap]

You know, I took it out because it wasn't needed on my platform to complete a trivial python program, whereas the others were. I suppose it could be glibc differences, or python differences... or maybe rt_sigreturn should be added back? What do you think?

It's actually pretty hard to find a minimal yet useful set of seccomp filters!

[tyhicks]

I think that if rt_sigaction is required, rt_sigreturn should also be included to return from the signal handler that was configured by rt_sigaction. Your small test program is likely getting "lucky" in that a signal is not being delivered during the program's short lifetime.

I think there's two things left for you to do to get these patches merged:

1. Update the second patch to leave rt_sigreturn in the example
   • You probably should move that add_rule() up to the block of minimum required system calls that you added
2. Update both the first and second patch to include your signed-off-by, as documented in CONTRIBUTING.md

[pcmoore]

I agree with @tyhicks, it's best to leave rt_sigaction in the syscall list. I would simply add the exit_group, brk, and rt_sigaction to the existing block of syscalls; there is no need to put them in a separate block. If I wanted to be picky, I would suggest the following order:

• open
• close
• read
• write
• brk
• rt_sigreturn
• rt_sigaction
• exit_group

Beyond that all you need to do is follow the guidance in CONTRIBUTING.md as previously discussed.

[pcmoore]

Hi @skrap, are you able to make the requested changes (see above)? I'd like to include this in the v2.5 release, and it would be nice if you could fixup the PR so you get proper credit :)

[skrap]

I believe an earlier commit addressed the concerns about rt_sigaction. I have added the SOB, and consolidated into a single commit. Thanks for the poke on this!

[drakenclimber]

I looked through the changes (and ran them, too.) I still think @tyhicks comment about re-adding rt_sigreturn is correct. What makes you think it isn't needed? I wasn't able to find a definitive comment why it can be safely removed.

Other than that, I think it looks good.

[skrap]

I still think @tyhicks comment about re-adding rt_sigreturn is correct.

@drakenclimber: Fixed, thanks for the feedback!

[drakenclimber]

Thanks! Looks good to me.

Acked-by: Tom Hromatka <[email protected]>

[pcmoore]

Merged via 853a24c - thanks everyone!