From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59172) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WkVan-0000j5-Ml for qemu-devel@nongnu.org; Wed, 14 May 2014 05:35:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WkVai-0002kX-Sp for qemu-devel@nongnu.org; Wed, 14 May 2014 05:35:17 -0400 Date: Wed, 14 May 2014 10:34:49 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20140514093448.GM2381@work-vm> References: <1399374955-29082-1-git-send-email-dgilbert@redhat.com> <87lhu4hg36.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87lhu4hg36.fsf@blackfin.pond.sub.org> Subject: Re: [Qemu-devel] [PATCH] Fix 'name' option to work with -readconfig List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-trivial@nongnu.org, qemu-devel@nongnu.org, wdauchy@gmail.com * Markus Armbruster (armbru@redhat.com) wrote: > "Dr. David Alan Gilbert (git)" writes: > > > From: "Dr. David Alan Gilbert" > > > > 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 > > Reported-by: William Dauchy > > --- > > 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 Thanks. Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK