| Message ID | 1372943363-24081-4-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 4 July 2013 14:09, Markus Armbruster <armbru@redhat.com> wrote: > > +/** > + * Get machine options > + * > + * Returns: machine options (never null). > + */ > +QemuOpts *qemu_get_machine_opts(void) > +{ > + QemuOptsList *list; > + QemuOpts *opts; > + > + list = qemu_find_opts("machine"); > + assert(list); > + opts = qemu_opts_find(list, NULL); > + if (!opts) { > + opts = qemu_opts_create_nofail(list); > + } > + return opts; > +} This looks a bit odd -- why are we creating new options in a function that claims to only be querying them? thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 4 July 2013 14:09, Markus Armbruster <armbru@redhat.com> wrote: >> >> +/** >> + * Get machine options >> + * >> + * Returns: machine options (never null). >> + */ >> +QemuOpts *qemu_get_machine_opts(void) >> +{ >> + QemuOptsList *list; >> + QemuOpts *opts; >> + >> + list = qemu_find_opts("machine"); >> + assert(list); >> + opts = qemu_opts_find(list, NULL); >> + if (!opts) { >> + opts = qemu_opts_create_nofail(list); >> + } >> + return opts; >> +} > > This looks a bit odd -- why are we creating new > options in a function that claims to only be querying > them? So we never return null. If it bothers you, I can initialize the options to empty somewhere else, and assert they exist here.
On 4 July 2013 16:03, Markus Armbruster <armbru@redhat.com> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: >> This looks a bit odd -- why are we creating new >> options in a function that claims to only be querying >> them? > > So we never return null. If it bothers you, I can initialize the > options to empty somewhere else, and assert they exist here. The other option would be to modify qemu_opt_get and friends to accept a NULL QemuOpts* as meaning "return the default". That seems cleaner to me than having "machine" opts be a special case. -- PMM
Am 04.07.2013 16:38, schrieb Peter Maydell: > On 4 July 2013 14:09, Markus Armbruster <armbru@redhat.com> wrote: >> >> +/** >> + * Get machine options >> + * >> + * Returns: machine options (never null). >> + */ >> +QemuOpts *qemu_get_machine_opts(void) >> +{ >> + QemuOptsList *list; >> + QemuOpts *opts; >> + >> + list = qemu_find_opts("machine"); >> + assert(list); >> + opts = qemu_opts_find(list, NULL); >> + if (!opts) { >> + opts = qemu_opts_create_nofail(list); >> + } >> + return opts; >> +} > > This looks a bit odd -- why are we creating new > options in a function that claims to only be querying > them? It's called the Singleton pattern. :P We use it for the /machine Object, for instance. Andreas
Peter Maydell <peter.maydell@linaro.org> writes: > On 4 July 2013 16:03, Markus Armbruster <armbru@redhat.com> wrote: >> Peter Maydell <peter.maydell@linaro.org> writes: >>> This looks a bit odd -- why are we creating new >>> options in a function that claims to only be querying >>> them? >> >> So we never return null. If it bothers you, I can initialize the >> options to empty somewhere else, and assert they exist here. > > The other option would be to modify qemu_opt_get and > friends to accept a NULL QemuOpts* as meaning "return > the default". I considered it, but it's more involved, and it'll sweep accidental null opts arguments under the carpet (not sure that's worth worrying about). > That seems cleaner to me than having > "machine" opts be a special case. "machine" opts are a special case, because unlike most options, they're a singleton. Anyway, what do you guys want me to do? (1) Create empty machine options on the fly (this is what the current patch does) (2) Initialize machine options elsewhere (3) Make QemuOpts consistently treat NULL like empty options (possibly quite some work) I don't mind (1) or (2), but (3) feels like a bit more than I bargained for. I just want to fix the bug that bit me, not rework QemuOpts.
On 4 July 2013 17:11, Markus Armbruster <armbru@redhat.com> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: > >> On 4 July 2013 16:03, Markus Armbruster <armbru@redhat.com> wrote: >>> Peter Maydell <peter.maydell@linaro.org> writes: >>>> This looks a bit odd -- why are we creating new >>>> options in a function that claims to only be querying >>>> them? >>> >>> So we never return null. If it bothers you, I can initialize the >>> options to empty somewhere else, and assert they exist here. >> >> The other option would be to modify qemu_opt_get and >> friends to accept a NULL QemuOpts* as meaning "return >> the default". > > I considered it, but it's more involved, and it'll sweep accidental null > opts arguments under the carpet (not sure that's worth worrying about). > >> That seems cleaner to me than having >> "machine" opts be a special case. > > "machine" opts are a special case, because unlike most options, they're > a singleton. So are boot-opts and smp-opts, right? If you deal with "machine" opts as a special case you'll miss those (and any other singleton options we add later). > (3) Make QemuOpts consistently treat NULL like empty options (possibly > quite some work) I think we could reasonably change just qemu_opt_find(), qemu_opt_has_help_opt() and qemu_opt_foreach() to allow NULL; the rationale being that these are the "query" end of the options API rather than the "set" end. The 'set' end needs to provide a non-NULL opts because we can't just create one on-demand. If you don't want to do that then we could have a point in vl.c where we force all our singleton opts to exist. -- PMM
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index 2fb71af..d85bdc0 100644 --- a/include/sysemu/sysemu.h +++ b/include/sysemu/sysemu.h @@ -185,6 +185,8 @@ char *get_boot_devices_list(size_t *size); DeviceState *get_boot_device(uint32_t position); +QemuOpts *qemu_get_machine_opts(void); + bool usb_enabled(bool default_usb); extern QemuOptsList qemu_drive_opts; diff --git a/vl.c b/vl.c index 6d9fd7d..e68d19c 100644 --- a/vl.c +++ b/vl.c @@ -516,6 +516,25 @@ static QemuOptsList qemu_realtime_opts = { }, }; +/** + * Get machine options + * + * Returns: machine options (never null). + */ +QemuOpts *qemu_get_machine_opts(void) +{ + QemuOptsList *list; + QemuOpts *opts; + + list = qemu_find_opts("machine"); + assert(list); + opts = qemu_opts_find(list, NULL); + if (!opts) { + opts = qemu_opts_create_nofail(list); + } + return opts; +} + const char *qemu_get_vm_name(void) { return qemu_name;
To be used in the next few commits to fix or clean up queries of "machine" options (-machine and its sugared forms). Signed-off-by: Markus Armbruster <armbru@redhat.com> --- include/sysemu/sysemu.h | 2 ++ vl.c | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+)