Re: [Qemu-devel] [PATCH] virtfs: error out gracefully when mandatory suboptions are missing

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 3132 bytes --]

On Tue, 5 Sep 2017 15:35:34 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 04.09.2017 09:59, Greg Kurz wrote:
> > We internally convert -virtfs to -fsdev/-device. If the user doesn't
> > provide the path or security_model suboptions, and the fsdev backend
> > requires them, we hit an assertion when populating the internal -fsdev
> > option:
> > 
> > util/qemu-option.c:547: opt_set: Assertion `opt->str' failed.
> > Aborted (core dumped)
> > 
> > Let's test the suboption presence on the command line before trying
> > to set it in the internal -fsdev option, and let the backend code
> > error out gracefully (ie, like it already does when the user passes
> > -fsdev on the command line).
> > 
> > Reported-by: Thomas Huth <thuth@redhat.com>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  vl.c |   16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/vl.c b/vl.c
> > index 8e247cc2a239..d63269332fed 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -3557,7 +3557,7 @@ int main(int argc, char **argv, char **envp)
> >              case QEMU_OPTION_virtfs: {
> >                  QemuOpts *fsdev;
> >                  QemuOpts *device;
> > -                const char *writeout, *sock_fd, *socket;
> > +                const char *writeout, *sock_fd, *socket, *path, *security_model;
> >  
> >                  olist = qemu_find_opts("virtfs");
> >                  if (!olist) {
> > @@ -3596,11 +3596,15 @@ int main(int argc, char **argv, char **envp)
> >                  }
> >                  qemu_opt_set(fsdev, "fsdriver",
> >                               qemu_opt_get(opts, "fsdriver"), &error_abort);
> > -                qemu_opt_set(fsdev, "path", qemu_opt_get(opts, "path"),
> > -                             &error_abort);
> > -                qemu_opt_set(fsdev, "security_model",
> > -                             qemu_opt_get(opts, "security_model"),
> > -                             &error_abort);
> > +                path = qemu_opt_get(opts, "path");
> > +                if (path) {
> > +                    qemu_opt_set(fsdev, "path", path, &error_abort);
> > +                }
> > +                security_model = qemu_opt_get(opts, "security_model");
> > +                if (security_model) {
> > +                    qemu_opt_set(fsdev, "security_model", security_model,
> > +                                 &error_abort);
> > +                }
> >                  socket = qemu_opt_get(opts, "socket");
> >                  if (socket) {
> >                      qemu_opt_set(fsdev, "socket", socket, &error_abort);  
> 
> By the way, I think this should be CC:-ed to stable, too.
> 

Well, it's been there forever and it isn't a critical fix... but the
thread on qemu-discuss the other day showed that it was confusing
people, and the fix is trivial. So I guess it doesn't hurt to have
this in stable. :)

Michael,

I'll send a pull req later today. If it's not too late, maybe you can add
this patch to 2.9.1 when it's merged in master?

Cheers,

--
Greg

>  Thomas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]