* [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.