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

[Michael Roth]

Quoting Greg Kurz (2017-09-05 09:24:25)
> On Tue, 5 Sep 2017 15:35:34 +0200
> Thomas Huth <address@hidden> 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 <address@hidden>
> > > Signed-off-by: Greg Kurz <address@hidden>
> > > ---
> > >  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?

We're in freeze as of yesterday with release set for Thursday, so it's a
bit late. OTOH I don't have test coverage for virtfs and the patch seems
isolated enough that I don't see any harm in slipping it in if it
lands in master by tomorrow. Otherwise it'll have to wait for 2.10.1,
which I'm hoping to get out this month.

> 
> Cheers,
> 
> --
> Greg
> 
> >  Thomas