Patchwork [3/7] vl: New qemu_get_machine_opts()

login
register
mail settings
Submitter Markus Armbruster
Date July 4, 2013, 1:09 p.m.
Message ID <1372943363-24081-4-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/256893/
State New
Headers show

Comments

Markus Armbruster - July 4, 2013, 1:09 p.m.
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(+)
Peter Maydell - July 4, 2013, 2:38 p.m.
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
Markus Armbruster - July 4, 2013, 3:03 p.m.
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.
Peter Maydell - July 4, 2013, 3:11 p.m.
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
Andreas Färber - July 4, 2013, 3:14 p.m.
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
Markus Armbruster - July 4, 2013, 4:11 p.m.
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.
Peter Maydell - July 4, 2013, 4:46 p.m.
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

Patch

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;