[Qemu-devel] [BUGFIX][PATCH for 2.2 v5 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen

[Qemu-devel] [BUGFIX][PATCH for 2.2 v5 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen

[Don Slutz]

c/s 9b23cfb76b3a5e9eb5cc899eaf2f46bc46d33ba4

or

c/s b154537ad07598377ebf98252fb7d2aff127983b

moved the testing of xen_enabled() from pc_init1() to
pc_machine_initfn().

xen_enabled() does not return the correct value in
pc_machine_initfn().

Changed vmport from a bool to an enum.  Added the value "auto" to do
the old way.

Signed-off-by: Don Slutz <dslutz@verizon.com>
Acked-by: Eric Blake <eblake@redhat.com>
---

v5:
  Eduardo Habkost:
    What about changing pc_machine->vmport here instead of using a
    no_vmport variable, so the actual vmport configuration may be
    queried by anybody later using the QOM property? It would even
    make the code shorter.
      Done.
  Eric Blake:
    I've only reviewed the qapi/common.json and qemu-options.hx
    files for QMP interface (and will leave the rest of the patch to
    others), but I'm okay with the changes to those files. I guess
    that means no R-b, since I didn't do a full review, so here's a
    weaker acked-by.

v4:
  Michael S. Tsirkin, Eric Blake, Eduardo Habkost:
    Rename vmport to OnOffAuto and move to qapi/common.json
  Eduardo Habkost:
    Simpler convert of enum to no_vmport.
  Michael S. Tsirkin:
    Add assert for ON_OFF_AUTO_MAX.

 hw/i386/pc.c         | 23 ++++++++++++++---------
 hw/i386/pc_piix.c    |  7 ++++++-
 hw/i386/pc_q35.c     |  7 ++++++-
 include/hw/i386/pc.h |  2 +-
 qapi/common.json     | 15 +++++++++++++++
 qemu-options.hx      |  8 +++++---
 vl.c                 |  2 +-
 7 files changed, 48 insertions(+), 16 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 1205db8..15fba60 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1711,18 +1711,23 @@ static void pc_machine_set_max_ram_below_4g(Object *obj, Visitor *v,
     pcms->max_ram_below_4g = value;
 }
 
-static bool pc_machine_get_vmport(Object *obj, Error **errp)
+static void pc_machine_get_vmport(Object *obj, Visitor *v, void *opaque,
+                                  const char *name, Error **errp)
 {
     PCMachineState *pcms = PC_MACHINE(obj);
+    int vmport = pcms->vmport;
 
-    return pcms->vmport;
+    visit_type_enum(v, &vmport, OnOffAuto_lookup, NULL, name, errp);
 }
 
-static void pc_machine_set_vmport(Object *obj, bool value, Error **errp)
+static void pc_machine_set_vmport(Object *obj, Visitor *v, void *opaque,
+                                  const char *name, Error **errp)
 {
     PCMachineState *pcms = PC_MACHINE(obj);
+    int vmport;
 
-    pcms->vmport = value;
+    visit_type_enum(v, &vmport, OnOffAuto_lookup, NULL, name, errp);
+    pcms->vmport = vmport;
 }
 
 static void pc_machine_initfn(Object *obj)
@@ -1737,11 +1742,11 @@ static void pc_machine_initfn(Object *obj)
                         pc_machine_get_max_ram_below_4g,
                         pc_machine_set_max_ram_below_4g,
                         NULL, NULL, NULL);
-    pcms->vmport = !xen_enabled();
-    object_property_add_bool(obj, PC_MACHINE_VMPORT,
-                             pc_machine_get_vmport,
-                             pc_machine_set_vmport,
-                             NULL);
+    pcms->vmport = ON_OFF_AUTO_AUTO;
+    object_property_add(obj, PC_MACHINE_VMPORT, "str",
+                        pc_machine_get_vmport,
+                        pc_machine_set_vmport,
+                        NULL, NULL, NULL);
 }
 
 static void pc_machine_class_init(ObjectClass *oc, void *data)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 7bb97a4..fffaab7 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -234,9 +234,14 @@ static void pc_init1(MachineState *machine,
 
     pc_vga_init(isa_bus, pci_enabled ? pci_bus : NULL);
 
+    assert(pc_machine->vmport != ON_OFF_AUTO_MAX);
+    if (pc_machine->vmport == ON_OFF_AUTO_AUTO) {
+        pc_machine->vmport = xen_enabled() ? ON_OFF_AUTO_OFF : ON_OFF_AUTO_ON;
+    }
+
     /* init basic PC hardware */
     pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy,
-                         !pc_machine->vmport, 0x4);
+                         (pc_machine->vmport != ON_OFF_AUTO_ON), 0x4);
 
     pc_nic_init(isa_bus, pci_bus);
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 598e679..88cee93 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -242,9 +242,14 @@ static void pc_q35_init(MachineState *machine)
 
     pc_register_ferr_irq(gsi[13]);
 
+    assert(pc_machine->vmport != ON_OFF_AUTO_MAX);
+    if (pc_machine->vmport == ON_OFF_AUTO_AUTO) {
+        pc_machine->vmport = xen_enabled() ? ON_OFF_AUTO_OFF : ON_OFF_AUTO_ON;
+    }
+
     /* init basic PC hardware */
     pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy,
-                         !pc_machine->vmport, 0xff0104);
+                         (pc_machine->vmport != ON_OFF_AUTO_ON), 0xff0104);
 
     /* connect pm stuff to lpc */
     ich9_lpc_pm_init(lpc);
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 7c3731f..7f7d2d4 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -37,7 +37,7 @@ struct PCMachineState {
     ISADevice *rtc;
 
     uint64_t max_ram_below_4g;
-    bool vmport;
+    OnOffAuto vmport;
 };
 
 #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
diff --git a/qapi/common.json b/qapi/common.json
index 4e9a21f..63ef3b4 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -87,3 +87,18 @@
 ##
 { 'command': 'query-commands', 'returns': ['CommandInfo'] }
 
+##
+# @OnOffAuto
+#
+# An enumeration of three options: on, off, and auto
+#
+# @auto: QEMU selects the value between on and off
+#
+# @on: Enabled
+#
+# @off: Disabled
+#
+# Since: 2.2
+##
+{ 'enum': 'OnOffAuto',
+  'data': [ 'auto', 'on', 'off' ] }
diff --git a/qemu-options.hx b/qemu-options.hx
index da9851d..64af16d 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -33,7 +33,7 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
     "                property accel=accel1[:accel2[:...]] selects accelerator\n"
     "                supported accelerators are kvm, xen, tcg (default: tcg)\n"
     "                kernel_irqchip=on|off controls accelerated irqchip support\n"
-    "                vmport=on|off controls emulation of vmport (default: on)\n"
+    "                vmport=on|off|auto controls emulation of vmport (default: auto)\n"
     "                kvm_shadow_mem=size of KVM shadow MMU\n"
     "                dump-guest-core=on|off include guest memory in a core dump (default=on)\n"
     "                mem-merge=on|off controls memory merge support (default: on)\n"
@@ -52,8 +52,10 @@ than one accelerator specified, the next one is used if the previous one fails
 to initialize.
 @item kernel_irqchip=on|off
 Enables in-kernel irqchip support for the chosen accelerator when available.
-@item vmport=on|off
-Enables emulation of VMWare IO port, for vmmouse etc. (enabled by default)
+@item vmport=on|off|auto
+Enables emulation of VMWare IO port, for vmmouse etc. auto says to select the
+value based on accel. For accel=xen the default is off otherwise the default
+is on.
 @item kvm_shadow_mem=size
 Defines the size of the KVM shadow MMU.
 @item dump-guest-core=on|off
diff --git a/vl.c b/vl.c
index f4a6e5e..eb89d62 100644
--- a/vl.c
+++ b/vl.c
@@ -381,7 +381,7 @@ static QemuOptsList qemu_machine_opts = {
             .help = "maximum ram below the 4G boundary (32bit boundary)",
         }, {
             .name = PC_MACHINE_VMPORT,
-            .type = QEMU_OPT_BOOL,
+            .type = QEMU_OPT_STRING,
             .help = "Enable vmport (pc & q35)",
         },{
             .name = "iommu",
-- 
1.8.4

Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v5 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen

[Eduardo Habkost]

On Thu, Nov 20, 2014 at 05:32:56PM -0500, Don Slutz wrote:
[...]
> @@ -1711,18 +1711,23 @@ static void pc_machine_set_max_ram_below_4g(Object *obj, Visitor *v,
>      pcms->max_ram_below_4g = value;
>  }
>  
> -static bool pc_machine_get_vmport(Object *obj, Error **errp)
> +static void pc_machine_get_vmport(Object *obj, Visitor *v, void *opaque,
> +                                  const char *name, Error **errp)
>  {
>      PCMachineState *pcms = PC_MACHINE(obj);
> +    int vmport = pcms->vmport;
>  
> -    return pcms->vmport;
> +    visit_type_enum(v, &vmport, OnOffAuto_lookup, NULL, name, errp);

A visit_type_OnOffAuto() function is automatically generated by the QAPI
schema, so you don't need to deal with the low level visit_type_enum()
function.

>  }
>  
> -static void pc_machine_set_vmport(Object *obj, bool value, Error **errp)
> +static void pc_machine_set_vmport(Object *obj, Visitor *v, void *opaque,
> +                                  const char *name, Error **errp)
>  {
>      PCMachineState *pcms = PC_MACHINE(obj);
> +    int vmport;
>  
> -    pcms->vmport = value;
> +    visit_type_enum(v, &vmport, OnOffAuto_lookup, NULL, name, errp);
> +    pcms->vmport = vmport;

'vmport' may be undefined in case the visitor return an error, and in
this case you shouldn't change pcms->vmport. This won't be a problem if
you just call:

    visit_type_OnOffAuto(v, &pcms->vmport, name, errp);

>  }
>  
>  static void pc_machine_initfn(Object *obj)
> @@ -1737,11 +1742,11 @@ static void pc_machine_initfn(Object *obj)
>                          pc_machine_get_max_ram_below_4g,
>                          pc_machine_set_max_ram_below_4g,
>                          NULL, NULL, NULL);
> -    pcms->vmport = !xen_enabled();
> -    object_property_add_bool(obj, PC_MACHINE_VMPORT,
> -                             pc_machine_get_vmport,
> -                             pc_machine_set_vmport,
> -                             NULL);
> +    pcms->vmport = ON_OFF_AUTO_AUTO;
> +    object_property_add(obj, PC_MACHINE_VMPORT, "str",

I believe "OnOffAuto" is a valid type name, if it is defined in the QAPI
schema.

> +                        pc_machine_get_vmport,
> +                        pc_machine_set_vmport,
> +                        NULL, NULL, NULL);
>  }
>  
[...]

-- 
Eduardo

Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v5 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen

[Don Slutz]

On 11/20/14 21:01, Eduardo Habkost wrote:
> On Thu, Nov 20, 2014 at 05:32:56PM -0500, Don Slutz wrote:
> [...]
>> @@ -1711,18 +1711,23 @@ static void pc_machine_set_max_ram_below_4g(Object *obj, Visitor *v,
>>       pcms->max_ram_below_4g = value;
>>   }
>>   
>> -static bool pc_machine_get_vmport(Object *obj, Error **errp)
>> +static void pc_machine_get_vmport(Object *obj, Visitor *v, void *opaque,
>> +                                  const char *name, Error **errp)
>>   {
>>       PCMachineState *pcms = PC_MACHINE(obj);
>> +    int vmport = pcms->vmport;
>>   
>> -    return pcms->vmport;
>> +    visit_type_enum(v, &vmport, OnOffAuto_lookup, NULL, name, errp);
> A visit_type_OnOffAuto() function is automatically generated by the QAPI
> schema, so you don't need to deal with the low level visit_type_enum()
> function.

Ok. Will switch.

>>   }
>>   
>> -static void pc_machine_set_vmport(Object *obj, bool value, Error **errp)
>> +static void pc_machine_set_vmport(Object *obj, Visitor *v, void *opaque,
>> +                                  const char *name, Error **errp)
>>   {
>>       PCMachineState *pcms = PC_MACHINE(obj);
>> +    int vmport;
>>   
>> -    pcms->vmport = value;
>> +    visit_type_enum(v, &vmport, OnOffAuto_lookup, NULL, name, errp);
>> +    pcms->vmport = vmport;
> 'vmport' may be undefined in case the visitor return an error, and in
> this case you shouldn't change pcms->vmport. This won't be a problem if
> you just call:
>
>      visit_type_OnOffAuto(v, &pcms->vmport, name, errp);

Will do.

>>   }
>>   
>>   static void pc_machine_initfn(Object *obj)
>> @@ -1737,11 +1742,11 @@ static void pc_machine_initfn(Object *obj)
>>                           pc_machine_get_max_ram_below_4g,
>>                           pc_machine_set_max_ram_below_4g,
>>                           NULL, NULL, NULL);
>> -    pcms->vmport = !xen_enabled();
>> -    object_property_add_bool(obj, PC_MACHINE_VMPORT,
>> -                             pc_machine_get_vmport,
>> -                             pc_machine_set_vmport,
>> -                             NULL);
>> +    pcms->vmport = ON_OFF_AUTO_AUTO;
>> +    object_property_add(obj, PC_MACHINE_VMPORT, "str",
> I believe "OnOffAuto" is a valid type name, if it is defined in the QAPI
> schema.

I can only find:

qapi-types.h:typedef enum OnOffAuto
qapi-types.h:} OnOffAuto;

Which I use to define pcms->vmport.  The best I can translate this is
that "str" is what you are looking to replace.

So I plan no change here.

     -Don Slutz


>> +                        pc_machine_get_vmport,
>> +                        pc_machine_set_vmport,
>> +                        NULL, NULL, NULL);
>>   }
>>   
> [...]
>

Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v5 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen

[Eduardo Habkost]

On Fri, Nov 21, 2014 at 08:30:58AM -0500, Don Slutz wrote:
> On 11/20/14 21:01, Eduardo Habkost wrote:
> >On Thu, Nov 20, 2014 at 05:32:56PM -0500, Don Slutz wrote:
> >[...]
> >>@@ -1711,18 +1711,23 @@ static void pc_machine_set_max_ram_below_4g(Object *obj, Visitor *v,
> >>      pcms->max_ram_below_4g = value;
> >>  }
> >>-static bool pc_machine_get_vmport(Object *obj, Error **errp)
> >>+static void pc_machine_get_vmport(Object *obj, Visitor *v, void *opaque,
> >>+                                  const char *name, Error **errp)
> >>  {
> >>      PCMachineState *pcms = PC_MACHINE(obj);
> >>+    int vmport = pcms->vmport;
> >>-    return pcms->vmport;
> >>+    visit_type_enum(v, &vmport, OnOffAuto_lookup, NULL, name, errp);
> >A visit_type_OnOffAuto() function is automatically generated by the QAPI
> >schema, so you don't need to deal with the low level visit_type_enum()
> >function.
> 
> Ok. Will switch.
> 
> >>  }
> >>-static void pc_machine_set_vmport(Object *obj, bool value, Error **errp)
> >>+static void pc_machine_set_vmport(Object *obj, Visitor *v, void *opaque,
> >>+                                  const char *name, Error **errp)
> >>  {
> >>      PCMachineState *pcms = PC_MACHINE(obj);
> >>+    int vmport;
> >>-    pcms->vmport = value;
> >>+    visit_type_enum(v, &vmport, OnOffAuto_lookup, NULL, name, errp);
> >>+    pcms->vmport = vmport;
> >'vmport' may be undefined in case the visitor return an error, and in
> >this case you shouldn't change pcms->vmport. This won't be a problem if
> >you just call:
> >
> >     visit_type_OnOffAuto(v, &pcms->vmport, name, errp);
> 
> Will do.
> 
> >>  }
> >>  static void pc_machine_initfn(Object *obj)
> >>@@ -1737,11 +1742,11 @@ static void pc_machine_initfn(Object *obj)
> >>                          pc_machine_get_max_ram_below_4g,
> >>                          pc_machine_set_max_ram_below_4g,
> >>                          NULL, NULL, NULL);
> >>-    pcms->vmport = !xen_enabled();
> >>-    object_property_add_bool(obj, PC_MACHINE_VMPORT,
> >>-                             pc_machine_get_vmport,
> >>-                             pc_machine_set_vmport,
> >>-                             NULL);
> >>+    pcms->vmport = ON_OFF_AUTO_AUTO;
> >>+    object_property_add(obj, PC_MACHINE_VMPORT, "str",
> >I believe "OnOffAuto" is a valid type name, if it is defined in the QAPI
> >schema.
> 
> I can only find:
> 
> qapi-types.h:typedef enum OnOffAuto
> qapi-types.h:} OnOffAuto;

I don't understand what you mean, where did you expect to find it? You added it
to the schema, so it is now a valid type name.

> 
> Which I use to define pcms->vmport.  The best I can translate this is
> that "str" is what you are looking to replace.
> 
> So I plan no change here.

You are not defining a string property, but an OnOffAuto enum property,
so why use "str"?

See, for example, the type of an existing enum property:
ide-hd.bios-chs-trans:

  $ qemu-system-x86_64 -device ide-hd,?
  [...]
  ide-hd.bios-chs-trans=BiosAtaTranslation (Logical CHS translation algorithm, auto/none/lba/large/rechs)
  [...]
  $ 


-- 
Eduardo

Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v5 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen

[Don Slutz]

On 11/21/14 08:53, Eduardo Habkost wrote:
> On Fri, Nov 21, 2014 at 08:30:58AM -0500, Don Slutz wrote:
>> On 11/20/14 21:01, Eduardo Habkost wrote:
>>> On Thu, Nov 20, 2014 at 05:32:56PM -0500, Don Slutz wrote:
>>> [...]
[...]
>>>> +    pcms->vmport = ON_OFF_AUTO_AUTO;
>>>> +    object_property_add(obj, PC_MACHINE_VMPORT, "str",
>>> I believe "OnOffAuto" is a valid type name, if it is defined in the QAPI
>>> schema.
>> I can only find:
>>
>> qapi-types.h:typedef enum OnOffAuto
>> qapi-types.h:} OnOffAuto;
> I don't understand what you mean, where did you expect to find it? You added it
> to the schema, so it is now a valid type name.
>
>> Which I use to define pcms->vmport.  The best I can translate this is
>> that "str" is what you are looking to replace.
>>
>> So I plan no change here.
> You are not defining a string property, but an OnOffAuto enum property,
> so why use "str"?
>
> See, for example, the type of an existing enum property:
> ide-hd.bios-chs-trans:
>
>    $ qemu-system-x86_64 -device ide-hd,?
>    [...]
>    ide-hd.bios-chs-trans=BiosAtaTranslation (Logical CHS translation algorithm, auto/none/lba/large/rechs)
>    [...]
>    $
>
>
Ok, I now understand what you are getting at.  From object.h:

  * @type: the type name of the property.  This namespace is pretty loosely
  *   defined.  Sub namespaces are constructed by using a prefix and then
  *   to angle brackets.  For instance, the type 'virtio-net-pci' in the
  *   'link' namespace would be 'link<virtio-net-pci>'.

It turns out that BiosAtaTranslation is a .name not a .type:

hyper-0-21-52:~/qemu>git grep BiosAtaTranslation
hw/core/qdev-properties.c:QEMU_BUILD_BUG_ON(sizeof(BiosAtaTranslation) 
!= sizeof(int));
hw/core/qdev-properties.c:    .name = "BiosAtaTranslation",
hw/core/qdev-properties.c:    .enum_table = BiosAtaTranslation_lookup,
qapi/block.json:# BiosAtaTranslation:
qapi/block.json:{ 'enum': 'BiosAtaTranslation',


so to me there are a few options:

1) "enum"
2) "OnOffAuto"
3) "enum<OnOffAuto>"
4) "enum OnOffAuto"
5) "enum-OnOffAuto"

I think #2 was what you were thinking of, but I am not sure.  Please advise.

    -Don Slutz

Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v5 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen

[Eduardo Habkost]

On Fri, Nov 21, 2014 at 09:56:33AM -0500, Don Slutz wrote:
> On 11/21/14 08:53, Eduardo Habkost wrote:
> >On Fri, Nov 21, 2014 at 08:30:58AM -0500, Don Slutz wrote:
> >>On 11/20/14 21:01, Eduardo Habkost wrote:
> >>>On Thu, Nov 20, 2014 at 05:32:56PM -0500, Don Slutz wrote:
> >>>[...]
> [...]
> >>>>+    pcms->vmport = ON_OFF_AUTO_AUTO;
> >>>>+    object_property_add(obj, PC_MACHINE_VMPORT, "str",
> >>>I believe "OnOffAuto" is a valid type name, if it is defined in the QAPI
> >>>schema.
> >>I can only find:
> >>
> >>qapi-types.h:typedef enum OnOffAuto
> >>qapi-types.h:} OnOffAuto;
> >I don't understand what you mean, where did you expect to find it? You added it
> >to the schema, so it is now a valid type name.
> >
> >>Which I use to define pcms->vmport.  The best I can translate this is
> >>that "str" is what you are looking to replace.
> >>
> >>So I plan no change here.
> >You are not defining a string property, but an OnOffAuto enum property,
> >so why use "str"?
> >
> >See, for example, the type of an existing enum property:
> >ide-hd.bios-chs-trans:
> >
> >   $ qemu-system-x86_64 -device ide-hd,?
> >   [...]
> >   ide-hd.bios-chs-trans=BiosAtaTranslation (Logical CHS translation algorithm, auto/none/lba/large/rechs)
> >   [...]
> >   $
> >
> >
> Ok, I now understand what you are getting at.  From object.h:
> 
>  * @type: the type name of the property.  This namespace is pretty loosely
>  *   defined.  Sub namespaces are constructed by using a prefix and then
>  *   to angle brackets.  For instance, the type 'virtio-net-pci' in the
>  *   'link' namespace would be 'link<virtio-net-pci>'.
> 
> It turns out that BiosAtaTranslation is a .name not a .type:
> 

"-device ...,?" above is showing the property type. See
qdev_device_help().

> hyper-0-21-52:~/qemu>git grep BiosAtaTranslation
> hw/core/qdev-properties.c:QEMU_BUILD_BUG_ON(sizeof(BiosAtaTranslation) !=
> sizeof(int));
> hw/core/qdev-properties.c:    .name = "BiosAtaTranslation",
> hw/core/qdev-properties.c:    .enum_table = BiosAtaTranslation_lookup,
> qapi/block.json:# BiosAtaTranslation:
> qapi/block.json:{ 'enum': 'BiosAtaTranslation',

PropertyInfo.name is what is used as the 'type' parameter to
object_property_add() when qdev adds a static property to the object.
See qdev_property_add_static().

> 
> 
> so to me there are a few options:
> 
> 1) "enum"
> 2) "OnOffAuto"
> 3) "enum<OnOffAuto>"
> 4) "enum OnOffAuto"
> 5) "enum-OnOffAuto"
> 
> I think #2 was what you were thinking of, but I am not sure.  Please advise.

The documentation is unclear about the set of valid type names. But as
existing enum properties simply use the enum type name directly, I
suggest "OnOffAuto" to follow existing practice.

-- 
Eduardo

Re: [Qemu-devel] [BUGFIX][PATCH for 2.2 v5 1/1] -machine vmport=auto: Fix handling of VMWare ioport emulation for xen

[Don Slutz]

On 11/21/14 10:10, Eduardo Habkost wrote:
> On Fri, Nov 21, 2014 at 09:56:33AM -0500, Don Slutz wrote:
>> On 11/21/14 08:53, Eduardo Habkost wrote:
>>> On Fri, Nov 21, 2014 at 08:30:58AM -0500, Don Slutz wrote:
>>>> On 11/20/14 21:01, Eduardo Habkost wrote:
>>>>> On Thu, Nov 20, 2014 at 05:32:56PM -0500, Don Slutz wrote:
>>>>> [...]
>> [...]
>>>>>> +    pcms->vmport = ON_OFF_AUTO_AUTO;
>>>>>> +    object_property_add(obj, PC_MACHINE_VMPORT, "str",
>>>>> I believe "OnOffAuto" is a valid type name, if it is defined in the QAPI
>>>>> schema.
>>>> I can only find:
>>>>
>>>> qapi-types.h:typedef enum OnOffAuto
>>>> qapi-types.h:} OnOffAuto;
>>> I don't understand what you mean, where did you expect to find it? You added it
>>> to the schema, so it is now a valid type name.
>>>
>>>> Which I use to define pcms->vmport.  The best I can translate this is
>>>> that "str" is what you are looking to replace.
>>>>
>>>> So I plan no change here.
>>> You are not defining a string property, but an OnOffAuto enum property,
>>> so why use "str"?
>>>
>>> See, for example, the type of an existing enum property:
>>> ide-hd.bios-chs-trans:
>>>
>>>    $ qemu-system-x86_64 -device ide-hd,?
>>>    [...]
>>>    ide-hd.bios-chs-trans=BiosAtaTranslation (Logical CHS translation algorithm, auto/none/lba/large/rechs)
>>>    [...]
>>>    $
>>>
>>>
>> Ok, I now understand what you are getting at.  From object.h:
>>
>>   * @type: the type name of the property.  This namespace is pretty loosely
>>   *   defined.  Sub namespaces are constructed by using a prefix and then
>>   *   to angle brackets.  For instance, the type 'virtio-net-pci' in the
>>   *   'link' namespace would be 'link<virtio-net-pci>'.
>>
>> It turns out that BiosAtaTranslation is a .name not a .type:
>>
> "-device ...,?" above is showing the property type. See
> qdev_device_help().
>
>> hyper-0-21-52:~/qemu>git grep BiosAtaTranslation
>> hw/core/qdev-properties.c:QEMU_BUILD_BUG_ON(sizeof(BiosAtaTranslation) !=
>> sizeof(int));
>> hw/core/qdev-properties.c:    .name = "BiosAtaTranslation",
>> hw/core/qdev-properties.c:    .enum_table = BiosAtaTranslation_lookup,
>> qapi/block.json:# BiosAtaTranslation:
>> qapi/block.json:{ 'enum': 'BiosAtaTranslation',
> PropertyInfo.name is what is used as the 'type' parameter to
> object_property_add() when qdev adds a static property to the object.
> See qdev_property_add_static().
>
>>
>> so to me there are a few options:
>>
>> 1) "enum"
>> 2) "OnOffAuto"
>> 3) "enum<OnOffAuto>"
>> 4) "enum OnOffAuto"
>> 5) "enum-OnOffAuto"
>>
>> I think #2 was what you were thinking of, but I am not sure.  Please advise.
> The documentation is unclear about the set of valid type names. But as
> existing enum properties simply use the enum type name directly, I
> suggest "OnOffAuto" to follow existing practice.
>

Fine.  I went with this.  v6 posted.

    -Don Slutz