* [Qemu-devel] [PATCH] virtfs: error out gracefully when mandatory suboptions are missing
@ 2017-09-04 7:59 Greg Kurz
2017-09-05 11:50 ` Thomas Huth
2017-09-05 13:35 ` Thomas Huth
0 siblings, 2 replies; 6+ messages in thread
From: Greg Kurz @ 2017-09-04 7:59 UTC (permalink / raw)
To: qemu-devel; +Cc: Thomas Huth, Paolo Bonzini
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);
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] virtfs: error out gracefully when mandatory suboptions are missing
2017-09-04 7:59 [Qemu-devel] [PATCH] virtfs: error out gracefully when mandatory suboptions are missing Greg Kurz
@ 2017-09-05 11:50 ` Thomas Huth
2017-09-05 13:35 ` Thomas Huth
1 sibling, 0 replies; 6+ messages in thread
From: Thomas Huth @ 2017-09-05 11:50 UTC (permalink / raw)
To: Greg Kurz, qemu-devel; +Cc: Paolo Bonzini
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);
>
Looks good to me.
Reviewed-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] virtfs: error out gracefully when mandatory suboptions are missing
2017-09-04 7:59 [Qemu-devel] [PATCH] virtfs: error out gracefully when mandatory suboptions are missing Greg Kurz
2017-09-05 11:50 ` Thomas Huth
@ 2017-09-05 13:35 ` Thomas Huth
2017-09-05 14:24 ` Greg Kurz
1 sibling, 1 reply; 6+ messages in thread
From: Thomas Huth @ 2017-09-05 13:35 UTC (permalink / raw)
To: Greg Kurz, qemu-devel; +Cc: qemu-stable
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.
Thomas
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] virtfs: error out gracefully when mandatory suboptions are missing
2017-09-05 13:35 ` Thomas Huth
@ 2017-09-05 14:24 ` Greg Kurz
2017-09-05 15:20 ` Michael Roth
0 siblings, 1 reply; 6+ messages in thread
From: Greg Kurz @ 2017-09-05 14:24 UTC (permalink / raw)
To: Thomas Huth; +Cc: qemu-devel, qemu-stable, mdroth
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] virtfs: error out gracefully when mandatory suboptions are missing
2017-09-05 14:24 ` Greg Kurz
@ 2017-09-05 15:20 ` Michael Roth
2017-09-05 17:00 ` Greg Kurz
0 siblings, 1 reply; 6+ messages in thread
From: Michael Roth @ 2017-09-05 15:20 UTC (permalink / raw)
To: Greg Kurz, Thomas Huth; +Cc: qemu-devel, qemu-stable
Quoting Greg Kurz (2017-09-05 09:24:25)
> 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?
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] virtfs: error out gracefully when mandatory suboptions are missing
2017-09-05 15:20 ` Michael Roth
@ 2017-09-05 17:00 ` Greg Kurz
0 siblings, 0 replies; 6+ messages in thread
From: Greg Kurz @ 2017-09-05 17:00 UTC (permalink / raw)
To: Michael Roth; +Cc: Thomas Huth, qemu-devel, qemu-stable
[-- Attachment #1: Type: text/plain, Size: 1095 bytes --]
On Tue, 05 Sep 2017 10:20:22 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> 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 <groug@kaod.org>
Date: Mon Sep 4 09:59:01 2017 +0200
virtfs: error out gracefully when mandatory suboptions are missing
Cheers,
--
Greg
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-09-05 17:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-04 7:59 [Qemu-devel] [PATCH] virtfs: error out gracefully when mandatory suboptions are missing Greg Kurz
2017-09-05 11:50 ` Thomas Huth
2017-09-05 13:35 ` Thomas Huth
2017-09-05 14:24 ` Greg Kurz
2017-09-05 15:20 ` Michael Roth
2017-09-05 17:00 ` Greg Kurz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.