On Wed, Feb 27, 2019 at 08:21:40PM +0100, Markus Armbruster wrote: > Daniel P. Berrangé <berra...@redhat.com> 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 <da...@gibson.dropbear.id.au> > >> > >> > '-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 :|