Re: [Qemu-devel] [PATCH 1/4] e1000e: Use regular DEFINE_PROP_ macros for properties

[Eduardo Habkost]

On Thu, Oct 27, 2016 at 05:13:34PM +0200, Igor Mammedov wrote:
> On Thu, 27 Oct 2016 09:25:27 -0200
> Eduardo Habkost  wrote:
> 
> > On Thu, Oct 27, 2016 at 09:38:28AM +0300, Dmitry Fleytman wrote:
> > >   
> > > > On 27 Oct 2016, at 06:12 AM, Jason Wang  wrote:
> > > > 
> > > > 
> > > > 
> > > > On 2016年10月27日 03:21, Eduardo Habkost wrote:  
> > > >> Instead of hacking custom PropertyInfo structs, use the regular
> > > >> DEFINE_PROP_ macros for the e1000e properties.
> > > >> 
> > > >> This also fixes a bug in the disable_vnet_hdr property
> > > >> definition, that was using qdev_prop_uint8 for a bool field.
> > > >> 
> > > >> Signed-off-by: Eduardo Habkost 
> > > >> ---
> > > >>  hw/net/e1000e.c | 25 +++--
> > > >>  1 file changed, 3 insertions(+), 22 deletions(-)
> > > >> 
> > > >> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> > > >> index 4994e1c..df24e55 100644
> > > >> --- a/hw/net/e1000e.c
> > > >> +++ b/hw/net/e1000e.c
> > > >> @@ -638,19 +638,11 @@ static const VMStateDescription e1000e_vmstate = 
> > > >> {
> > > >>  }
> > > >>  };
> > > >>  -static PropertyInfo e1000e_prop_disable_vnet,
> > > >> -e1000e_prop_subsys_ven,
> > > >> -e1000e_prop_subsys;
> > > >> -
> > > >>  static Property e1000e_properties[] = {
> > > >>  DEFINE_NIC_PROPERTIES(E1000EState, conf),
> > > >> -DEFINE_PROP_DEFAULT("disable_vnet_hdr", E1000EState, 
> > > >> disable_vnet, false,
> > > >> -e1000e_prop_disable_vnet, bool),
> > > >> -DEFINE_PROP_DEFAULT("subsys_ven", E1000EState, subsys_ven,
> > > >> -PCI_VENDOR_ID_INTEL,
> > > >> -e1000e_prop_subsys_ven, uint16_t),
> > > >> -DEFINE_PROP_DEFAULT("subsys", E1000EState, subsys, 0,
> > > >> -e1000e_prop_subsys, uint16_t),
> > > >> +DEFINE_PROP_BOOL("disable_vnet_hdr", E1000EState, disable_vnet, 
> > > >> false),
> > > >> +DEFINE_PROP_UINT16("subsys_ven", E1000EState, subsys_ven, 
> > > >> PCI_VENDOR_ID_INTEL),
> > > >> +DEFINE_PROP_UINT16("subsys", E1000EState, subsys, 0),
> > > >>  DEFINE_PROP_END_OF_LIST(),
> > > >>  };
> > > >>  @@ -673,17 +665,6 @@ static void e1000e_class_init(ObjectClass 
> > > >> *class, void *data)
> > > >>  dc->vmsd = _vmstate;
> > > >>  dc->props = e1000e_properties;
> > > >>  -e1000e_prop_disable_vnet = qdev_prop_uint8;
> > > >> -e1000e_prop_disable_vnet.description = "Do not use virtio 
> > > >> headers, "
> > > >> -   "perform SW offloads 
> > > >> emulation "
> > > >> -   "instead";
> > > >> -
> > > >> -e1000e_prop_subsys_ven = qdev_prop_uint16;
> > > >> -e1000e_prop_subsys_ven.description = "PCI device Subsystem Vendor 
> > > >> ID";
> > > >> -
> > > >> -e1000e_prop_subsys = qdev_prop_uint16;
> > > >> -e1000e_prop_subsys.description = "PCI device Subsystem ID";
> > > >> -
> > > >>  set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
> > > >>  }
> > > >>
> > > > 
> > > > Is there a way to keep the description here? At least for "vnet" I 
> > > > believe.  
> > > 
> > > Actually, ability to add description was the only reason for having 
> > > custom PropertyInfo structs.
> > > Does DEFINE_PROP_… macros set worth an improvement?  
> > 
> > I think it would be reasonable to add a descrition field to
> > struct Property.
> > 
> > Alternatively, you can call object_property_set_description() on
> > instance_init.
> that would look sort of convoluted in case of class properties.
> description should be set at the same time the property is added.

In the case of class properties, we have
object_class_property_set_description().

It's easier to redo this series on top of the
qdev-class-properties series, to avoid conflicts. I will do that.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/4] e1000e: Use regular DEFINE_PROP_ macros for properties

[Igor Mammedov]

On Thu, 27 Oct 2016 09:25:27 -0200
Eduardo Habkost  wrote:

> On Thu, Oct 27, 2016 at 09:38:28AM +0300, Dmitry Fleytman wrote:
> >   
> > > On 27 Oct 2016, at 06:12 AM, Jason Wang  wrote:
> > > 
> > > 
> > > 
> > > On 2016年10月27日 03:21, Eduardo Habkost wrote:  
> > >> Instead of hacking custom PropertyInfo structs, use the regular
> > >> DEFINE_PROP_ macros for the e1000e properties.
> > >> 
> > >> This also fixes a bug in the disable_vnet_hdr property
> > >> definition, that was using qdev_prop_uint8 for a bool field.
> > >> 
> > >> Signed-off-by: Eduardo Habkost 
> > >> ---
> > >>  hw/net/e1000e.c | 25 +++--
> > >>  1 file changed, 3 insertions(+), 22 deletions(-)
> > >> 
> > >> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> > >> index 4994e1c..df24e55 100644
> > >> --- a/hw/net/e1000e.c
> > >> +++ b/hw/net/e1000e.c
> > >> @@ -638,19 +638,11 @@ static const VMStateDescription e1000e_vmstate = {
> > >>  }
> > >>  };
> > >>  -static PropertyInfo e1000e_prop_disable_vnet,
> > >> -e1000e_prop_subsys_ven,
> > >> -e1000e_prop_subsys;
> > >> -
> > >>  static Property e1000e_properties[] = {
> > >>  DEFINE_NIC_PROPERTIES(E1000EState, conf),
> > >> -DEFINE_PROP_DEFAULT("disable_vnet_hdr", E1000EState, disable_vnet, 
> > >> false,
> > >> -e1000e_prop_disable_vnet, bool),
> > >> -DEFINE_PROP_DEFAULT("subsys_ven", E1000EState, subsys_ven,
> > >> -PCI_VENDOR_ID_INTEL,
> > >> -e1000e_prop_subsys_ven, uint16_t),
> > >> -DEFINE_PROP_DEFAULT("subsys", E1000EState, subsys, 0,
> > >> -e1000e_prop_subsys, uint16_t),
> > >> +DEFINE_PROP_BOOL("disable_vnet_hdr", E1000EState, disable_vnet, 
> > >> false),
> > >> +DEFINE_PROP_UINT16("subsys_ven", E1000EState, subsys_ven, 
> > >> PCI_VENDOR_ID_INTEL),
> > >> +DEFINE_PROP_UINT16("subsys", E1000EState, subsys, 0),
> > >>  DEFINE_PROP_END_OF_LIST(),
> > >>  };
> > >>  @@ -673,17 +665,6 @@ static void e1000e_class_init(ObjectClass *class, 
> > >> void *data)
> > >>  dc->vmsd = _vmstate;
> > >>  dc->props = e1000e_properties;
> > >>  -e1000e_prop_disable_vnet = qdev_prop_uint8;
> > >> -e1000e_prop_disable_vnet.description = "Do not use virtio headers, "
> > >> -   "perform SW offloads 
> > >> emulation "
> > >> -   "instead";
> > >> -
> > >> -e1000e_prop_subsys_ven = qdev_prop_uint16;
> > >> -e1000e_prop_subsys_ven.description = "PCI device Subsystem Vendor 
> > >> ID";
> > >> -
> > >> -e1000e_prop_subsys = qdev_prop_uint16;
> > >> -e1000e_prop_subsys.description = "PCI device Subsystem ID";
> > >> -
> > >>  set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
> > >>  }
> > >>
> > > 
> > > Is there a way to keep the description here? At least for "vnet" I 
> > > believe.  
> > 
> > Actually, ability to add description was the only reason for having custom 
> > PropertyInfo structs.
> > Does DEFINE_PROP_… macros set worth an improvement?  
> 
> I think it would be reasonable to add a descrition field to
> struct Property.
> 
> Alternatively, you can call object_property_set_description() on
> instance_init.
that would look sort of convoluted in case of class properties.
description should be set at the same time the property is added.

> 

Re: [Qemu-devel] [PATCH 1/4] e1000e: Use regular DEFINE_PROP_ macros for properties

[Eduardo Habkost]

On Thu, Oct 27, 2016 at 09:38:28AM +0300, Dmitry Fleytman wrote:
> 
> > On 27 Oct 2016, at 06:12 AM, Jason Wang  wrote:
> > 
> > 
> > 
> > On 2016年10月27日 03:21, Eduardo Habkost wrote:
> >> Instead of hacking custom PropertyInfo structs, use the regular
> >> DEFINE_PROP_ macros for the e1000e properties.
> >> 
> >> This also fixes a bug in the disable_vnet_hdr property
> >> definition, that was using qdev_prop_uint8 for a bool field.
> >> 
> >> Signed-off-by: Eduardo Habkost 
> >> ---
> >>  hw/net/e1000e.c | 25 +++--
> >>  1 file changed, 3 insertions(+), 22 deletions(-)
> >> 
> >> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> >> index 4994e1c..df24e55 100644
> >> --- a/hw/net/e1000e.c
> >> +++ b/hw/net/e1000e.c
> >> @@ -638,19 +638,11 @@ static const VMStateDescription e1000e_vmstate = {
> >>  }
> >>  };
> >>  -static PropertyInfo e1000e_prop_disable_vnet,
> >> -e1000e_prop_subsys_ven,
> >> -e1000e_prop_subsys;
> >> -
> >>  static Property e1000e_properties[] = {
> >>  DEFINE_NIC_PROPERTIES(E1000EState, conf),
> >> -DEFINE_PROP_DEFAULT("disable_vnet_hdr", E1000EState, disable_vnet, 
> >> false,
> >> -e1000e_prop_disable_vnet, bool),
> >> -DEFINE_PROP_DEFAULT("subsys_ven", E1000EState, subsys_ven,
> >> -PCI_VENDOR_ID_INTEL,
> >> -e1000e_prop_subsys_ven, uint16_t),
> >> -DEFINE_PROP_DEFAULT("subsys", E1000EState, subsys, 0,
> >> -e1000e_prop_subsys, uint16_t),
> >> +DEFINE_PROP_BOOL("disable_vnet_hdr", E1000EState, disable_vnet, 
> >> false),
> >> +DEFINE_PROP_UINT16("subsys_ven", E1000EState, subsys_ven, 
> >> PCI_VENDOR_ID_INTEL),
> >> +DEFINE_PROP_UINT16("subsys", E1000EState, subsys, 0),
> >>  DEFINE_PROP_END_OF_LIST(),
> >>  };
> >>  @@ -673,17 +665,6 @@ static void e1000e_class_init(ObjectClass *class, 
> >> void *data)
> >>  dc->vmsd = _vmstate;
> >>  dc->props = e1000e_properties;
> >>  -e1000e_prop_disable_vnet = qdev_prop_uint8;
> >> -e1000e_prop_disable_vnet.description = "Do not use virtio headers, "
> >> -   "perform SW offloads emulation 
> >> "
> >> -   "instead";
> >> -
> >> -e1000e_prop_subsys_ven = qdev_prop_uint16;
> >> -e1000e_prop_subsys_ven.description = "PCI device Subsystem Vendor ID";
> >> -
> >> -e1000e_prop_subsys = qdev_prop_uint16;
> >> -e1000e_prop_subsys.description = "PCI device Subsystem ID";
> >> -
> >>  set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
> >>  }
> >>  
> > 
> > Is there a way to keep the description here? At least for "vnet" I believe.
> 
> Actually, ability to add description was the only reason for having custom 
> PropertyInfo structs.
> Does DEFINE_PROP_… macros set worth an improvement?

I think it would be reasonable to add a descrition field to
struct Property.

Alternatively, you can call object_property_set_description() on
instance_init.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/4] e1000e: Use regular DEFINE_PROP_ macros for properties

[Dmitry Fleytman]

> On 27 Oct 2016, at 06:12 AM, Jason Wang  wrote:
> 
> 
> 
> On 2016年10月27日 03:21, Eduardo Habkost wrote:
>> Instead of hacking custom PropertyInfo structs, use the regular
>> DEFINE_PROP_ macros for the e1000e properties.
>> 
>> This also fixes a bug in the disable_vnet_hdr property
>> definition, that was using qdev_prop_uint8 for a bool field.
>> 
>> Signed-off-by: Eduardo Habkost 
>> ---
>>  hw/net/e1000e.c | 25 +++--
>>  1 file changed, 3 insertions(+), 22 deletions(-)
>> 
>> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
>> index 4994e1c..df24e55 100644
>> --- a/hw/net/e1000e.c
>> +++ b/hw/net/e1000e.c
>> @@ -638,19 +638,11 @@ static const VMStateDescription e1000e_vmstate = {
>>  }
>>  };
>>  -static PropertyInfo e1000e_prop_disable_vnet,
>> -e1000e_prop_subsys_ven,
>> -e1000e_prop_subsys;
>> -
>>  static Property e1000e_properties[] = {
>>  DEFINE_NIC_PROPERTIES(E1000EState, conf),
>> -DEFINE_PROP_DEFAULT("disable_vnet_hdr", E1000EState, disable_vnet, 
>> false,
>> -e1000e_prop_disable_vnet, bool),
>> -DEFINE_PROP_DEFAULT("subsys_ven", E1000EState, subsys_ven,
>> -PCI_VENDOR_ID_INTEL,
>> -e1000e_prop_subsys_ven, uint16_t),
>> -DEFINE_PROP_DEFAULT("subsys", E1000EState, subsys, 0,
>> -e1000e_prop_subsys, uint16_t),
>> +DEFINE_PROP_BOOL("disable_vnet_hdr", E1000EState, disable_vnet, false),
>> +DEFINE_PROP_UINT16("subsys_ven", E1000EState, subsys_ven, 
>> PCI_VENDOR_ID_INTEL),
>> +DEFINE_PROP_UINT16("subsys", E1000EState, subsys, 0),
>>  DEFINE_PROP_END_OF_LIST(),
>>  };
>>  @@ -673,17 +665,6 @@ static void e1000e_class_init(ObjectClass *class, void 
>> *data)
>>  dc->vmsd = _vmstate;
>>  dc->props = e1000e_properties;
>>  -e1000e_prop_disable_vnet = qdev_prop_uint8;
>> -e1000e_prop_disable_vnet.description = "Do not use virtio headers, "
>> -   "perform SW offloads emulation "
>> -   "instead";
>> -
>> -e1000e_prop_subsys_ven = qdev_prop_uint16;
>> -e1000e_prop_subsys_ven.description = "PCI device Subsystem Vendor ID";
>> -
>> -e1000e_prop_subsys = qdev_prop_uint16;
>> -e1000e_prop_subsys.description = "PCI device Subsystem ID";
>> -
>>  set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
>>  }
>>  
> 
> Is there a way to keep the description here? At least for "vnet" I believe.

Actually, ability to add description was the only reason for having custom 
PropertyInfo structs.
Does DEFINE_PROP_… macros set worth an improvement?

> 
> Thanks

Re: [Qemu-devel] [PATCH 1/4] e1000e: Use regular DEFINE_PROP_ macros for properties

[Jason Wang]

On 2016年10月27日 03:21, Eduardo Habkost wrote:

Instead of hacking custom PropertyInfo structs, use the regular
DEFINE_PROP_ macros for the e1000e properties.

This also fixes a bug in the disable_vnet_hdr property
definition, that was using qdev_prop_uint8 for a bool field.

Signed-off-by: Eduardo Habkost 
---
  hw/net/e1000e.c | 25 +++--
  1 file changed, 3 insertions(+), 22 deletions(-)

diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index 4994e1c..df24e55 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -638,19 +638,11 @@ static const VMStateDescription e1000e_vmstate = {
  }
  };
  
-static PropertyInfo e1000e_prop_disable_vnet,

-e1000e_prop_subsys_ven,
-e1000e_prop_subsys;
-
  static Property e1000e_properties[] = {
  DEFINE_NIC_PROPERTIES(E1000EState, conf),
-DEFINE_PROP_DEFAULT("disable_vnet_hdr", E1000EState, disable_vnet, false,
-e1000e_prop_disable_vnet, bool),
-DEFINE_PROP_DEFAULT("subsys_ven", E1000EState, subsys_ven,
-PCI_VENDOR_ID_INTEL,
-e1000e_prop_subsys_ven, uint16_t),
-DEFINE_PROP_DEFAULT("subsys", E1000EState, subsys, 0,
-e1000e_prop_subsys, uint16_t),
+DEFINE_PROP_BOOL("disable_vnet_hdr", E1000EState, disable_vnet, false),
+DEFINE_PROP_UINT16("subsys_ven", E1000EState, subsys_ven, 
PCI_VENDOR_ID_INTEL),
+DEFINE_PROP_UINT16("subsys", E1000EState, subsys, 0),
  DEFINE_PROP_END_OF_LIST(),
  };
  
@@ -673,17 +665,6 @@ static void e1000e_class_init(ObjectClass *class, void *data)

  dc->vmsd = _vmstate;
  dc->props = e1000e_properties;
  
-e1000e_prop_disable_vnet = qdev_prop_uint8;

-e1000e_prop_disable_vnet.description = "Do not use virtio headers, "
-   "perform SW offloads emulation "
-   "instead";
-
-e1000e_prop_subsys_ven = qdev_prop_uint16;
-e1000e_prop_subsys_ven.description = "PCI device Subsystem Vendor ID";
-
-e1000e_prop_subsys = qdev_prop_uint16;
-e1000e_prop_subsys.description = "PCI device Subsystem ID";
-
  set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
  }
  


Is there a way to keep the description here? At least for "vnet" I believe.

Thanks

[Qemu-devel] [PATCH 1/4] e1000e: Use regular DEFINE_PROP_ macros for properties

[Eduardo Habkost]

Instead of hacking custom PropertyInfo structs, use the regular
DEFINE_PROP_ macros for the e1000e properties.

This also fixes a bug in the disable_vnet_hdr property
definition, that was using qdev_prop_uint8 for a bool field.

Signed-off-by: Eduardo Habkost 
---
 hw/net/e1000e.c | 25 +++--
 1 file changed, 3 insertions(+), 22 deletions(-)

diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index 4994e1c..df24e55 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -638,19 +638,11 @@ static const VMStateDescription e1000e_vmstate = {
 }
 };
 
-static PropertyInfo e1000e_prop_disable_vnet,
-e1000e_prop_subsys_ven,
-e1000e_prop_subsys;
-
 static Property e1000e_properties[] = {
 DEFINE_NIC_PROPERTIES(E1000EState, conf),
-DEFINE_PROP_DEFAULT("disable_vnet_hdr", E1000EState, disable_vnet, false,
-e1000e_prop_disable_vnet, bool),
-DEFINE_PROP_DEFAULT("subsys_ven", E1000EState, subsys_ven,
-PCI_VENDOR_ID_INTEL,
-e1000e_prop_subsys_ven, uint16_t),
-DEFINE_PROP_DEFAULT("subsys", E1000EState, subsys, 0,
-e1000e_prop_subsys, uint16_t),
+DEFINE_PROP_BOOL("disable_vnet_hdr", E1000EState, disable_vnet, false),
+DEFINE_PROP_UINT16("subsys_ven", E1000EState, subsys_ven, 
PCI_VENDOR_ID_INTEL),
+DEFINE_PROP_UINT16("subsys", E1000EState, subsys, 0),
 DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -673,17 +665,6 @@ static void e1000e_class_init(ObjectClass *class, void 
*data)
 dc->vmsd = _vmstate;
 dc->props = e1000e_properties;
 
-e1000e_prop_disable_vnet = qdev_prop_uint8;
-e1000e_prop_disable_vnet.description = "Do not use virtio headers, "
-   "perform SW offloads emulation "
-   "instead";
-
-e1000e_prop_subsys_ven = qdev_prop_uint16;
-e1000e_prop_subsys_ven.description = "PCI device Subsystem Vendor ID";
-
-e1000e_prop_subsys = qdev_prop_uint16;
-e1000e_prop_subsys.description = "PCI device Subsystem ID";
-
 set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
 }
 
-- 
2.7.4