[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] virtfs: error out gracefully when mandatory sub
From: |
Michael Roth |
Subject: |
Re: [Qemu-devel] [PATCH] virtfs: error out gracefully when mandatory suboptions are missing |
Date: |
Tue, 05 Sep 2017 10:20:22 -0500 |
User-agent: |
alot/0.5.1 |
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