[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] Allow -sandbox off with --disable-seccomp
From: |
Daniel P . Berrangé |
Subject: |
Re: [Qemu-devel] [PATCH] Allow -sandbox off with --disable-seccomp |
Date: |
Thu, 28 Feb 2019 10:22:36 +0000 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
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 :|