Re: [Qemu-devel] [PATCH 1/4] e1000e: Use regular DEFINE_PROP_<type> 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 <address@hidden> wrote:
> > 
> > 
> > 
> > On 2016年10月27日 03:21, Eduardo Habkost wrote:
> >> Instead of hacking custom PropertyInfo structs, use the regular
> >> DEFINE_PROP_<type> 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 <address@hidden>
> >> ---
> >>  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 = &e1000e_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