Re: [Qemu-devel] [PATCH v2 1/1] sandbox: disable -sandbox if CONFIG_SECCOMP undefined

[Yi Min Zhao]

在 2018/5/17 下午8:41, Eduardo Otubo 写道:
> On 15/05/2018 - 19:33:48, Yi Min Zhao wrote:
> > If CONFIG_SECCOMP is undefined, the option 'elevateprivileges' remains
> > compiled. This would make libvirt set the corresponding capability and
> > then trigger the guest startup fails. So this patch excludes the code
> > regarding seccomp staff if CONFIG_SECCOMP is undefined.
> Just a sugestion for the next patch you send: If it's a single patch, you don't
> need to format it with a cover-letter. Just put all the description in the body,
> or if you need to add a text that shouldn't be included in the commit message,
> just add it after the "---" after Signed-off-by.
OK. Thanks for your suggestion.
> > Signed-off-by: Yi Min Zhao <address@hidden>
> > ---
> >   vl.c | 13 ++++++++-----
> >   1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/vl.c b/vl.c
> > index 806eec2ef6..b22d158f5f 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -257,6 +257,7 @@ static QemuOptsList qemu_rtc_opts = {
> >       },
> >   };
> >   
> > +#ifdef CONFIG_SECCOMP
> >   static QemuOptsList qemu_sandbox_opts = {
> >       .name = "sandbox",
> >       .implied_opt_name = "enable",
> > @@ -285,6 +286,7 @@ static QemuOptsList qemu_sandbox_opts = {
> >           { /* end of list */ }
> >       },
> >   };
> > +#endif
> >   
> >   static QemuOptsList qemu_option_rom_opts = {
> >       .name = "option-rom",
> > @@ -1041,10 +1043,10 @@ static int bt_parse(const char *opt)
> >       return 1;
> >   }
> >   
> > +#ifdef CONFIG_SECCOMP
> >   static int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
> >   {
> >       if (qemu_opt_get_bool(opts, "enable", false)) {
> > -#ifdef CONFIG_SECCOMP
> >           uint32_t seccomp_opts = QEMU_SECCOMP_SET_DEFAULT
> >                   | QEMU_SECCOMP_SET_OBSOLETE;
> >           const char *value = NULL;
> > @@ -1114,14 +1116,11 @@ static int parse_sandbox(void *opaque, QemuOpts *opts, 
> > Error **errp)
> >                            "in the kernel");
> >               return -1;
> >           }
> > -#else
> > -        error_report("seccomp support is disabled");
> > -        return -1;
> > -#endif
> Any reason not to keep the error message on the new #endif location?
If error report is originally wrapped in CONFIG_SECCOMP undefined.
This patch excludes the entire function if CONFIG_SECCOMP is undefined.
So the error report is not needed.
> >       }
> >   
> >       return 0;
> >   }
> > +#endif
> >   
> >   static int parse_name(void *opaque, QemuOpts *opts, Error **errp)
> >   {
> > @@ -3074,7 +3073,9 @@ int main(int argc, char **argv, char **envp)
> >       qemu_add_opts(&qemu_mem_opts);
> >       qemu_add_opts(&qemu_smp_opts);
> >       qemu_add_opts(&qemu_boot_opts);
> > +#ifdef CONFIG_SECCOMP
> >       qemu_add_opts(&qemu_sandbox_opts);
> > +#endif
> >       qemu_add_opts(&qemu_add_fd_opts);
> >       qemu_add_opts(&qemu_object_opts);
> >       qemu_add_opts(&qemu_tpmdev_opts);
> > @@ -4071,10 +4072,12 @@ int main(int argc, char **argv, char **envp)
> >           exit(1);
> >       }
> >   
> > +#ifdef CONFIG_SECCOMP
> >       if (qemu_opts_foreach(qemu_find_opts("sandbox"),
> >                             parse_sandbox, NULL, NULL)) {
> >           exit(1);
> >       }
> > +#endif
> >   
> >       if (qemu_opts_foreach(qemu_find_opts("name"),
> >                             parse_name, NULL, NULL)) {
> > --
> > Yi Min
> I just wanted a review from Ján, since he is the author of the original libvirt
> patch. Does this breaks libvirt logic in any way? If not, ACK on this patch.
OK.