Re: [Qemu-ppc] [Qemu-devel] [PATCH] Allow -sandbox off with --disable-seccomp

[Daniel P . Berrangé]

On Wed, Feb 27, 2019 at 08:21:40PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <address@hidden> writes:
> 
> > On Wed, Feb 27, 2019 at 07:59:11AM -0600, Eric Blake wrote:
> >> On 2/27/19 5:01 AM, Daniel P. Berrangé wrote:
> >> > On Wed, Feb 27, 2019 at 12:21:32PM +1100, David Gibson wrote:
> >> >> At present, when seccomp support is compiled out with --disable-seccomp
> >> >> we fail with an error if the user puts -sandbox on the command line.
> >> >>
> >> >> That kind of makes sense, but it's a bit strange that we reject a 
> >> >> request
> >> >> to disable sandboxing with "-sandbox off" saying we don't support
> >> >> sandboxing.
> >> >>
> >> >> This puts in a small sandbox to (correctly) silently ignore -sandbox off
> >> >> when we don't have sandboxing support compiled in.  This makes life 
> >> >> easier
> >> >> for testcases, since they can safely specify "-sandbox off" without 
> >> >> having
> >> >> to care if the qemu they're using is compiled with sandbox support or 
> >> >> not.
> 
> I can't see such test cases.  Can you give specific examples?
> 
> >> >> Signed-off-by: David Gibson <address@hidden>
> >> 
> >> > '-sandbox off'  is just syntax sugar for '-sandbox enable=off', with
> >> > the default arg name handled by QemuOpts.
> >> 
> >> Except that libvirt probes, via query-command-line-options, whether the
> >> '-sandbox' option supports the 'enable' key.  You risk breaking
> >> introspection (where libvirt knows NOT to use enable=on|off) if -sandbox
> >> enable=off is advertised even when the feature is not compiled in.
> >
> > It is even more complex than that. Libvirt looks for "elevateprivileges"
> > option to decide to enable the sandbox. It only looks for "enable" when
> > libvirt is configured to disable the sandbox, at which point is sets
> > "-sandbox off". So I don't think my suggestion should break it.
> >
> > I do hate the idea of QEMU tailoring its CLI handling to suit the
> > current specific impl of one client app though.
> >
> > If anything I'd suggest we should completely disable any parsing
> > of -sandbox when seccomp is disabled, rather than leaving getopt
> > to parse -sandbox and then raise an error.
> 
> I'm confused.  Are you proposing to silently ignore -sandbox OPTARG
> regardless of OPTARG when CONFIG_SECCOMP is off?  If yes, I object.  If
> a user asks for a sandbox, and we can't give him one, we should at least
> tell him as much.

No, i mean remove "case QEMU_OPTION_sandbox:" from the code, which
will cause us to report '-sandbox' as an unsupported argument.

> Currently, we have both
> 
>     #ifdef CONFIG_FOO
>                 case QEMU_OPTION_FOO:
>                     ...
>                     break;
>     #endif

This is what I was suggesting for -sandbox above.

> 
> and
> 
> 
>                 case QEMU_OPTION_FOO:
>     #ifdef CONFIG_FOO
>                     ...
>                     break;
>     #else
>                     error_report("FOO support is disabled");
>                     exit(1);
>     #endif

This is what -sandbox does currently.

> 
> The patch adds a third variant.  We need fewer variants, not more.
> 
> For what it's worth, conditional QMP commands do not exist when
> disabled, like the first variant above.  In particular, introspection
> doesn't have them.  Nicely obvious.
> 
> QAPIfying the command line (yes, yes, overdue) should make it more like
> QMP.
> 
> Wanting nicer errors than "unknown option / command" is legitimate.
> 
> Wanting to accept "switch FOO off" even though FOO is disabled is also
> legitimate.
> 
> Patches that provide such UI niceties while keeping introspection nicely
> obvious would be acceptable to me, provided they're not too complex.
> But they should be general, not a one-off.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|