Re: [Qemu-devel] [PATCH] Fix 'name' option to work with -readconfig
Dr. David Alan Gilbert (git) dgilb...@redhat.com writes:
From: Dr. David Alan Gilbert dgilb...@redhat.com
The 'name' option silently failed when used in config files
( http://lists.gnu.org/archive/html/qemu-devel/2014-04/msg00378.html )
Signed-off-by: Dr. David Alan Gilbert dgilb...@redhat.com
Reported-by: William Dauchy wdau...@gmail.com
---
vl.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/vl.c b/vl.c
index 8411a4a..47c199a 100644
--- a/vl.c
+++ b/vl.c
@@ -965,7 +965,7 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
return 0;
}
-static void parse_name(QemuOpts *opts)
+static int parse_name(QemuOpts *opts, void *opaque)
{
const char *proc_name;
@@ -978,6 +978,8 @@ static void parse_name(QemuOpts *opts)
if (proc_name) {
os_set_proc_name(proc_name);
}
+
+return 0;
}
bool usb_enabled(bool default_usb)
@@ -3780,7 +3782,6 @@ int main(int argc, char **argv, char **envp)
if (!opts) {
exit(1);
}
-parse_name(opts);
break;
case QEMU_OPTION_prom_env:
if (nb_prom_envs = MAX_PROM_ENVS) {
@@ -3955,6 +3956,10 @@ int main(int argc, char **argv, char **envp)
exit(1);
}
+if (qemu_opts_foreach(qemu_find_opts(name), parse_name, NULL, 1)) {
+exit(1);
+}
+
This will never exit, but that's okay.
#ifndef _WIN32
if (qemu_opts_foreach(qemu_find_opts(add-fd), parse_add_fd, NULL, 1)) {
exit(1);
-readconfig stores the configuration read in QemuOpts. Command line
option parsing should do the same, and no more. In particular it should
not act upon the option. That needs to be done separately, where both
command line and -readconfig settings are visible in QemuOpts.
Your patch does exactly that. I think amending the commit message with
the previous paragraph would improve it.
Have you checked command line option parsing (the big switch) for
similar problems?
Reviewed-by: Markus Armbruster arm...@redhat.com
Re: [Qemu-devel] [PATCH] Fix 'name' option to work with -readconfig
* Markus Armbruster (arm...@redhat.com) wrote:
Dr. David Alan Gilbert (git) dgilb...@redhat.com writes:
From: Dr. David Alan Gilbert dgilb...@redhat.com
The 'name' option silently failed when used in config files
( http://lists.gnu.org/archive/html/qemu-devel/2014-04/msg00378.html )
Signed-off-by: Dr. David Alan Gilbert dgilb...@redhat.com
Reported-by: William Dauchy wdau...@gmail.com
---
vl.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/vl.c b/vl.c
index 8411a4a..47c199a 100644
--- a/vl.c
+++ b/vl.c
@@ -965,7 +965,7 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
return 0;
}
-static void parse_name(QemuOpts *opts)
+static int parse_name(QemuOpts *opts, void *opaque)
{
const char *proc_name;
@@ -978,6 +978,8 @@ static void parse_name(QemuOpts *opts)
if (proc_name) {
os_set_proc_name(proc_name);
}
+
+return 0;
}
bool usb_enabled(bool default_usb)
@@ -3780,7 +3782,6 @@ int main(int argc, char **argv, char **envp)
if (!opts) {
exit(1);
}
-parse_name(opts);
break;
case QEMU_OPTION_prom_env:
if (nb_prom_envs = MAX_PROM_ENVS) {
@@ -3955,6 +3956,10 @@ int main(int argc, char **argv, char **envp)
exit(1);
}
+if (qemu_opts_foreach(qemu_find_opts(name), parse_name, NULL, 1)) {
+exit(1);
+}
+
This will never exit, but that's okay.
Ah, because my parse_name currently never fails?
#ifndef _WIN32
if (qemu_opts_foreach(qemu_find_opts(add-fd), parse_add_fd, NULL,
1)) {
exit(1);
-readconfig stores the configuration read in QemuOpts. Command line
option parsing should do the same, and no more. In particular it should
not act upon the option. That needs to be done separately, where both
command line and -readconfig settings are visible in QemuOpts.
Your patch does exactly that. I think amending the commit message with
the previous paragraph would improve it.
Have you checked command line option parsing (the big switch) for
similar problems?
I hadn't, other than fixing up -name; tbh It's taken me a while to understand
how QemuOpts is supposed to work (and hence why I didn't get this in the 1st
patch); I'd seen the qemu_opts_foreach uses at the bottom of the switch, but
since they looked like a loop, I'd assumed they were only for options with
multiple sets of values and not looked any further.
The big switch seems to be a mix of a lot of different ways of doing things;
A quick scan shows rtc, option_rom, usb_device, and others all use
qemu_opts_parse
in the big switch but also taking an action in the switch.
Reviewed-by: Markus Armbruster arm...@redhat.com
Thanks.
Dave
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH] Fix 'name' option to work with -readconfig
Dr. David Alan Gilbert dgilb...@redhat.com writes:
* Markus Armbruster (arm...@redhat.com) wrote:
Dr. David Alan Gilbert (git) dgilb...@redhat.com writes:
From: Dr. David Alan Gilbert dgilb...@redhat.com
The 'name' option silently failed when used in config files
( http://lists.gnu.org/archive/html/qemu-devel/2014-04/msg00378.html )
Signed-off-by: Dr. David Alan Gilbert dgilb...@redhat.com
Reported-by: William Dauchy wdau...@gmail.com
---
vl.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/vl.c b/vl.c
index 8411a4a..47c199a 100644
--- a/vl.c
+++ b/vl.c
@@ -965,7 +965,7 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
return 0;
}
-static void parse_name(QemuOpts *opts)
+static int parse_name(QemuOpts *opts, void *opaque)
{
const char *proc_name;
@@ -978,6 +978,8 @@ static void parse_name(QemuOpts *opts)
if (proc_name) {
os_set_proc_name(proc_name);
}
+
+return 0;
}
bool usb_enabled(bool default_usb)
@@ -3780,7 +3782,6 @@ int main(int argc, char **argv, char **envp)
if (!opts) {
exit(1);
}
-parse_name(opts);
break;
case QEMU_OPTION_prom_env:
if (nb_prom_envs = MAX_PROM_ENVS) {
@@ -3955,6 +3956,10 @@ int main(int argc, char **argv, char **envp)
exit(1);
}
+if (qemu_opts_foreach(qemu_find_opts(name), parse_name, NULL, 1)) {
+exit(1);
+}
+
This will never exit, but that's okay.
Ah, because my parse_name currently never fails?
Yes. But that's a non-local argument, and handling the impossible
failure here doesn't hurt.
#ifndef _WIN32
if (qemu_opts_foreach(qemu_find_opts(add-fd), parse_add_fd, NULL,
1)) {
exit(1);
-readconfig stores the configuration read in QemuOpts. Command line
option parsing should do the same, and no more. In particular it should
not act upon the option. That needs to be done separately, where both
command line and -readconfig settings are visible in QemuOpts.
Your patch does exactly that. I think amending the commit message with
the previous paragraph would improve it.
Have you checked command line option parsing (the big switch) for
similar problems?
I hadn't, other than fixing up -name; tbh It's taken me a while to understand
how QemuOpts is supposed to work (and hence why I didn't get this in the 1st
patch); I'd seen the qemu_opts_foreach uses at the bottom of the switch, but
since they looked like a loop, I'd assumed they were only for options with
multiple sets of values and not looked any further.
The big switch seems to be a mix of a lot of different ways of doing things;
A quick scan shows rtc, option_rom, usb_device, and others all use
qemu_opts_parse
in the big switch but also taking an action in the switch.
Okay. Looks like we better audit this some time.
Reviewed-by: Markus Armbruster arm...@redhat.com
Thanks.
My pleasure :)
[Qemu-devel] [PATCH] Fix 'name' option to work with -readconfig
From: Dr. David Alan Gilbert dgilb...@redhat.com
The 'name' option silently failed when used in config files
( http://lists.gnu.org/archive/html/qemu-devel/2014-04/msg00378.html )
Signed-off-by: Dr. David Alan Gilbert dgilb...@redhat.com
Reported-by: William Dauchy wdau...@gmail.com
---
vl.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/vl.c b/vl.c
index 8411a4a..47c199a 100644
--- a/vl.c
+++ b/vl.c
@@ -965,7 +965,7 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
return 0;
}
-static void parse_name(QemuOpts *opts)
+static int parse_name(QemuOpts *opts, void *opaque)
{
const char *proc_name;
@@ -978,6 +978,8 @@ static void parse_name(QemuOpts *opts)
if (proc_name) {
os_set_proc_name(proc_name);
}
+
+return 0;
}
bool usb_enabled(bool default_usb)
@@ -3780,7 +3782,6 @@ int main(int argc, char **argv, char **envp)
if (!opts) {
exit(1);
}
-parse_name(opts);
break;
case QEMU_OPTION_prom_env:
if (nb_prom_envs = MAX_PROM_ENVS) {
@@ -3955,6 +3956,10 @@ int main(int argc, char **argv, char **envp)
exit(1);
}
+if (qemu_opts_foreach(qemu_find_opts(name), parse_name, NULL, 1)) {
+exit(1);
+}
+
#ifndef _WIN32
if (qemu_opts_foreach(qemu_find_opts(add-fd), parse_add_fd, NULL, 1)) {
exit(1);
--
1.9.0
Re: [Qemu-devel] [PATCH] Fix 'name' option to work with -readconfig
On Tue, May 6, 2014 at 1:15 PM, Dr. David Alan Gilbert (git) dgilb...@redhat.com wrote: From: Dr. David Alan Gilbert dgilb...@redhat.com The 'name' option silently failed when used in config files ( http://lists.gnu.org/archive/html/qemu-devel/2014-04/msg00378.html ) Signed-off-by: Dr. David Alan Gilbert dgilb...@redhat.com Reported-by: William Dauchy wdau...@gmail.com Please use: Reported-by: William Dauchy will...@gandi.net I have also successfully tested the patch Tested-by: William Dauchy will...@gandi.net Thanks, -- William
