"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 :)
