Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Gonglei (Arei)]

Hi,

> >>>> This doesn't change the fact that ObjectProperty is a generic struct,
> >>>> and adding alias-specific fields there is wrong.
> >>>
> >>> OK, Maybe I should find other ways to attach this purpose and
> >>> avoid layering violation. Thanks!
> >>
> >> Unfortunately I cannot think of any.
> >>
> >> We could add a description field to ObjectProperty, and replace
> >> legacy_name with a description.  The output then would be
> >>
> >> virtio-blk.drive=str (drive)

There is a question that the QOM properties are added dynamically.
When we call qdev_alias_all_properties() adding alias properties to
the source object all qdev properties on the target DeviceState, how do we
judge the property's name and set the value of corresponding description field?
Such as setting virtio-blk-pci.drive.description = "drive".

The only way I think of is using the property' name as the description. In 
object_property_add_alias() add below code:

+   op->description = g_strdup(name);

After those handling we can get results:

virtio-blk-pci.physical_block_size=uint16 (physical_block_size)
virtio-blk-pci.logical_block_size=uint16 (logical_block_size)
virtio-blk-pci.drive=str (drive)

virtio-net-pci.netdev=str (netdev)
virtio-net-pci.vlan=int (vlan)
virtio-net-pci.mac=str (mac)

But if using this way, we just need simply modify make_device_property_info()
like below patch (just assure the new output resemble the old, don't need change
ObjectProperty struct).  Am I right? Thanks :)


diff --git a/qmp.c b/qmp.c
index c6767c4..1da3e25 100644
--- a/qmp.c
+++ b/qmp.c
@@ -446,6 +446,7 @@ static DevicePropertyInfo 
*make_device_property_info(ObjectClass *klass,
 {
     DevicePropertyInfo *info;
     Property *prop;
+    char *type_desc = NULL;
 
     do {
         for (prop = DEVICE_CLASS(klass)->props; prop && prop->name; prop++) {
@@ -474,7 +475,9 @@ static DevicePropertyInfo 
*make_device_property_info(ObjectClass *klass,
     /* Not a qdev property, use the default type */
     info = g_malloc0(sizeof(*info));
     info->name = g_strdup(name);
-    info->type = g_strdup(default_type);
+    type_desc = g_strdup_printf("%s (%s)", default_type, name);
+    info->type = g_strdup(type_desc);
+    g_free(type_desc);
     return info;
 }

Best regards,
-Gonglei

> >> That's a bit hackish, but perhaps it would be good enough for mst and
> >> Markus?
> >
> > I would just drop "str" and replace it with drive.
> 
> In the above example, the format would be
> 
>    CLASS.PROPERTY=TYPE (DESCRIPTION)
> 
> where I just put "drive" as the description to resemble the old output.
> 
> You cannot just have
> 
>    foo.drive=drive
> 
> because the type is *not* "drive", it's "str" (if anything, we could
> make it link<BlockBackend> which does not really show the connection
> between BlockBackend and -drive).
> 
> The problem is that "drive" as the type (the legacy_name) is simply not
> available for the virtio-blk-pci.drive property.  It's hidden beneath an
> opaque pointer.  Gonglei's patches were adding fields to ObjectProperty
> just to avoid visiting that opaque pointer, which I consider a layering
> violation.
> 
> > I seems to convey zero information.
> > Description should be something more informative, e.g.
> > (ID of a drive to use as a backend)
> 
> That would of course be fine too.  The problem is then that we have 644
> properties (DEFINE_PROP_* macro invocations) that someone must add a
> description to.
> 
> > Help for -device could add "Must match ID of a -drive option".
> > Help for device HMP command could add "Must match ID of a drive
> command."
> 
> You are turning a bug that nobody noticed until now, and that really
> doesn't break anything, into a month or more worth of work.
> 
> Paolo