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

[Greg Kurz]

On Tue, 05 Sep 2017 10:20:22 -0500
Michael Roth  wrote:
[...]
> > 
> > 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.
> 

The fix just got merged.

commit 32b6943699948f7adc35ada233fbd25daffad5e9
Author: Greg Kurz 
Date:   Mon Sep 4 09:59:01 2017 +0200

virtfs: error out gracefully when mandatory suboptions are missing

Cheers,

--
Greg


pgpy_SQx0FNfV.pgp
Description: OpenPGP digital signature

Re: [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  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 
> > > Signed-off-by: Greg Kurz 
> > > ---
> > >  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"), 
> > > _abort);
> > > -qemu_opt_set(fsdev, "path", qemu_opt_get(opts, "path"),
> > > - _abort);
> > > -qemu_opt_set(fsdev, "security_model",
> > > - qemu_opt_get(opts, "security_model"),
> > > - _abort);
> > > +path = qemu_opt_get(opts, "path");
> > > +if (path) {
> > > +qemu_opt_set(fsdev, "path", path, _abort);
> > > +}
> > > +security_model = qemu_opt_get(opts, "security_model");
> > > +if (security_model) {
> > > +qemu_opt_set(fsdev, "security_model", security_model,
> > > + _abort);
> > > +}
> > >  socket = qemu_opt_get(opts, "socket");
> > >  if (socket) {
> > >  qemu_opt_set(fsdev, "socket", socket, _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

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

[Greg Kurz]

On Tue, 5 Sep 2017 15:35:34 +0200
Thomas Huth  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 
> > Signed-off-by: Greg Kurz 
> > ---
> >  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"), _abort);
> > -qemu_opt_set(fsdev, "path", qemu_opt_get(opts, "path"),
> > - _abort);
> > -qemu_opt_set(fsdev, "security_model",
> > - qemu_opt_get(opts, "security_model"),
> > - _abort);
> > +path = qemu_opt_get(opts, "path");
> > +if (path) {
> > +qemu_opt_set(fsdev, "path", path, _abort);
> > +}
> > +security_model = qemu_opt_get(opts, "security_model");
> > +if (security_model) {
> > +qemu_opt_set(fsdev, "security_model", security_model,
> > + _abort);
> > +}
> >  socket = qemu_opt_get(opts, "socket");
> >  if (socket) {
> >  qemu_opt_set(fsdev, "socket", socket, _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


pgpVCdCgkL7WY.pgp
Description: OpenPGP digital signature

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

[Thomas Huth]

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 
> Signed-off-by: Greg Kurz 
> ---
>  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"), _abort);
> -qemu_opt_set(fsdev, "path", qemu_opt_get(opts, "path"),
> - _abort);
> -qemu_opt_set(fsdev, "security_model",
> - qemu_opt_get(opts, "security_model"),
> - _abort);
> +path = qemu_opt_get(opts, "path");
> +if (path) {
> +qemu_opt_set(fsdev, "path", path, _abort);
> +}
> +security_model = qemu_opt_get(opts, "security_model");
> +if (security_model) {
> +qemu_opt_set(fsdev, "security_model", security_model,
> + _abort);
> +}
>  socket = qemu_opt_get(opts, "socket");
>  if (socket) {
>  qemu_opt_set(fsdev, "socket", socket, _abort);

By the way, I think this should be CC:-ed to stable, too.

 Thomas

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

[Thomas Huth]

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 
> Signed-off-by: Greg Kurz 
> ---
>  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"), _abort);
> -qemu_opt_set(fsdev, "path", qemu_opt_get(opts, "path"),
> - _abort);
> -qemu_opt_set(fsdev, "security_model",
> - qemu_opt_get(opts, "security_model"),
> - _abort);
> +path = qemu_opt_get(opts, "path");
> +if (path) {
> +qemu_opt_set(fsdev, "path", path, _abort);
> +}
> +security_model = qemu_opt_get(opts, "security_model");
> +if (security_model) {
> +qemu_opt_set(fsdev, "security_model", security_model,
> + _abort);
> +}
>  socket = qemu_opt_get(opts, "socket");
>  if (socket) {
>  qemu_opt_set(fsdev, "socket", socket, _abort);
> 

Looks good to me.

Reviewed-by: Thomas Huth 

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

[Greg Kurz]

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 
Signed-off-by: Greg Kurz 
---
 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"), _abort);
-qemu_opt_set(fsdev, "path", qemu_opt_get(opts, "path"),
- _abort);
-qemu_opt_set(fsdev, "security_model",
- qemu_opt_get(opts, "security_model"),
- _abort);
+path = qemu_opt_get(opts, "path");
+if (path) {
+qemu_opt_set(fsdev, "path", path, _abort);
+}
+security_model = qemu_opt_get(opts, "security_model");
+if (security_model) {
+qemu_opt_set(fsdev, "security_model", security_model,
+ _abort);
+}
 socket = qemu_opt_get(opts, "socket");
 if (socket) {
 qemu_opt_set(fsdev, "socket", socket, _abort);