Re: [Qemu-devel] [PATCH 1/1] sandbox: avoid to compile options if CONFIG_SECCOMP undefined

[Ján Tomko]

On Mon, May 07, 2018 at 01:04:17PM -0500, Eric Blake wrote:
> On 05/06/2018 10:32 PM, Yi Min Zhao wrote:
>
> In the subject line: s/avoid to compile/avoid compiling/
>
> > If CONFIG_SECCOMP is undefined, the option 'elevatorprivileges' remains
>
> s/elevator/elevate/
>
> > complied. This would make libvirt set the corresponding capability and
>
> s/complied/compiled/
>
> > then trigger the guest startup fails. So let's wrap the options with
> > CONFIG_SECCOMP.
> >
> > Signed-off-by: Yi Min Zhao <address@hidden>
> > ---
> >   vl.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/vl.c b/vl.c
> > index fce1fd12d8..cb07b19c02 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -268,6 +268,7 @@ static QemuOptsList qemu_sandbox_opts = {
> >               .name = "enable",
> >               .type = QEMU_OPT_BOOL,
> >           },
> > +#ifdef CONFIG_SECCOMP
> >           {
> >               .name = "obsolete",
> >               .type = QEMU_OPT_STRING,
> > @@ -284,6 +285,7 @@ static QemuOptsList qemu_sandbox_opts = {
> >               .name = "resourcecontrol",
> >               .type = QEMU_OPT_STRING,
> >           },
> > +#endif
>
> The commit message mentions only 'elevateprivileges' (once the typo is
> fixed), but you are also crippling 'obsolete', 'spawn', and
> 'resourcecontrol'.  Perhaps the commit message should call that out
> better?  Or, since libvirt is looking at just 'elevateprivileges', per
> this line in libvirt's qemu_capabilities.c:
>
> src/qemu/qemu_capabilities.c:    { "sandbox", "elevateprivileges",
> QEMU_CAPS_SECCOMP_BLACKLIST },
>
> is it sufficient to just mask out that one option?

That would be inconsistent. I picked one option randomly, because they
were added at the same time, but compiling out just one out of four
is just odd. And with leaving them in even though the functionality is
compiled out, libvirt has no way to tell upfront whether it's usable.

By that logic, removing the -sandbox option without CONFIG_SECCOMP makes
sense, but libvirt already assumes this option is present on all supported
QEMUs (>= 1.5.0), so please cc: me on that change if you decide to
remove it as well.

Jano

signature.asc
Description: Digital signature