Re: Process isolation support for start-stop-daemon
On Mon, 2014-03-17 at 18:13:51 +0100, Andrew Shadura wrote:
> On Sun, 16 Mar 2014 05:35:41 +0100 Guillem Jover wrote:
> > Then there's several implementation details, here's a short list from
> > a quick skimming (ignoring style issues and similar):
>
> > * The command-line option should always be visible in the help output.
>
> Probably if it's a no-op, there should be a notice about that then?
There's currently no notice for process and I/O scheduling parameters,
but yeah I guess they deserve a mention, I'll improve the --help
output and man pages to note that.
> > * Some system calls are missing proper error checks.
> > * The quiet warnings seem suspect, I'd say they should either be
> > actual errors or normal warnings.
>
> Well, those warnings don't necessarily mean something went wrong, which
> is why I set them to default-quiet mode.
In that case I think they should just be normal (quiet) notices and
not warnings.
> > * Why remount the /dev filesystems? /proc is needed to get the new
> > PID namespace, but the others do not seem needed? And they are
> > problematic as they might change depending on the system, for
> > example /dev/shm is now /run/shm in Debian.
>
> That code it ported from lxc-unshare. I haven't checked if /dev/*
> things are really needed, so left it as is for a while.
From the man page, they don't seem to be, although maybe there's
practical reasons for those.
And now that you mention it, the lxc code seems to be LGPL? If so I'd
like to preserve s-s-d as PD, so try to get inspiration but do not
copy code over, please?
> > * Pointers should get NULL not 0.
> > * The two wait()s in the parent could be collapsed into one, and
> > handle error conditions from normal conditions (EINTR and ECHILD vs
> > the rest for example).
> > * The fatal() belongs with the execve(), which makes the “return;”
> > unnecessary.
>
> Thanks for the comments, I think I may try to address them this week,
> but not sure when exactly :)
No problem, and no hurry. :)
Thanks,
Guillem
Reply to: