Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
Paolo Bonzini writes: > Il 22/09/2014 13:43, Markus Armbruster ha scritto: >> Paolo Bonzini writes: >> >>> Il 22/09/2014 10:34, Michael S. Tsirkin ha scritto: Basically this patch brings back historical HMP behaviour. As far as I could tell, it wasn't changed intentionally. >>> >>> It was changed intentionally. Or rather, the change was known at the >>> time Stefan made it. >> >> Commit hash? > > There are three commits involved. > > The first is commit f4eb32b (qmp: show QOM properties in > device-list-properties, 2014-05-20). It was done in preparation for the > change of virtio-blk-pci.drive from qdev property to QOM property, to > avoid breaking device-list-properties. Yes. Unless there were *no* QOM properties then (other than the implicit ones listed in the commit message), it also made the help complete again, which I'd regard as regression fix. > The actual change took some more time to review, so it went in one month > later. Commit caffdac (virtio-blk: use aliases instead of duplicate > qdev properties, 2014-06-18) is what changed virtio-blk-pci.drive from > qdev property to QOM property. This changed the type from 'drive' to > 'str' in device-list-properties. (Note that this patch fixed an actual > bug, it was not just for the sake of cleanup). > > I cannot find any reference to the change; perhaps it was discussed only > on IRC. However, I'm quite certain that I knew about it. If memory serves, I didn't. > Now one thing did slip through; commit caffdac actually dropped > virtio-blk-pci.drive from -device virtio-blk-pci,help. Either I > misremembered that the first commit fixed command-line help too, or I > just assumed that -device foo,help was implemented on top of > qmp_device_list_properties. Which wasn't the case, Happens. > so the third commit > (9ef52358, qdev-monitor: include QOM properties in -device FOO, help > output, 2014-07-09) did the right thing and brought back You mean commit ef52358. > virtio-blk-pci.drive into the help message. > > Now, the cover latter for that patch finally has a hint that the change > was known. http://thread.gmane.org/gmane.comp.emulators.qemu/285819 has > this text: > >We simply need to update -device FOO,help code to use both qdev and >QOM properties. Note that types change because a 'drive' qdev type >is actually a 'str' QOM type. We're moving more and more to QOM >properties where the final type for this property would be >'link' or similar. > > It is only in the cover letter and thus not part of any commit message. I missed it. Of course I might have missed it even if the commit message had spelled it out. The cover letter shows the people involved in the patch were aware of the help change. The clause "the final type for this property would be 'link' or similar" can perhaps be read as "yes, the change is a degradation, but it's a temporary one". Sounds sensible, except our path to this "final type" is still unclear, and we have no ETA. Makes the degradation rather less temporary. >> I haven't got this part of the code present in my mind at the moment, >> but I'm willing to take your assertion of a layering violation for >> granted. Is this violation necessary for fixing the regression, or is >> it just an artifact of this particular fix? > > I guess we could always add some ad-hoc mechanism for > device-list-properties to get to the "drive" string, and smuggle it as a > QOM feature. Then, aliases would just forward that mechanism to the > aliased property, which would just work. > > Of course, with every new feature we would most likely have yet another > unfinished transition. In the lack of a clear user complaint (or even > of a clear indication that human users ever used -device foo,help...) > the tempation to pass the buck is strong. I use it all the time, both interactively to get help, and programmatically to answer questions about the code or a configuration. I think elsewhere in this thread, we found a way to improve help that doesn't involve ad hoc hackery to object. It does start yet another transition, though. Thanks for your explanation, and your patience.
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
Paolo Bonzini writes: > Il 23/09/2014 05:09, Gonglei (Arei) ha scritto: >> 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". > > You use the legacy_name field of PropertyInfo to set the description of > a qdev property, and then let object_property_add_alias() copy the > description. Gets us part of the way to what I described in my reply. I'm fine with that.
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
> Subject: Re: [PATCH 0/3] Fix confused output for alias properties > > Il 23/09/2014 05:09, Gonglei (Arei) ha scritto: > > 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". > > You use the legacy_name field of PropertyInfo to set the description of > a qdev property, and then let object_property_add_alias() copy the > description. > > Paolo Got it. Thanks for your point. :) Best regards, -Gonglei
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
> -Original Message- > From: Markus Armbruster [mailto:arm...@redhat.com] > Sent: Tuesday, September 23, 2014 5:06 PM > To: Gonglei (Arei) > Cc: Paolo Bonzini; Michael S. Tsirkin; ag...@suse.de; Huangweidong (C); > stefa...@redhat.com; Huangpeng (Peter); qemu-devel@nongnu.org; > aligu...@amazon.com; lcapitul...@redhat.com > Subject: Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties > > "Gonglei (Arei)" writes: > > > 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 :) > > Adding descriptions to properties is a big, but useful task. The > descriptions can serve as documentation in the code, and they can be > used to provide better help. This applies both to qdev and to object > properties. > > Completing this task in one go is unfortunately not practical. We need > to add descriptions incrementally. Until we're done, some properties > will lack descriptions (null pointer rather than a descriptive string). > > Reusing the property name as description just to have a description adds > no information. > > What about this: add a description string (optional for now) both to > ObjectProperty and to PropertyInfo. Either add a description string > parameter to object property constructors like object_property_add() and > object_property_add_alias, or, for less churn, add a separate function > to set an object property's description. Update > qdev_alias_all_properties() to set the object property's description to > the qdev property's. > > Then you can fix the help regression by giving all the regressed > properties a useful description. > > That fix should also permit retiring legacy_name. Great! Thanks, I will work for this :) Best regards, -Gonglei
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
On Tue, Sep 23, 2014 at 11:06:02AM +0200, Markus Armbruster wrote: > "Gonglei (Arei)" writes: > > > 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 :) > > Adding descriptions to properties is a big, but useful task. The > descriptions can serve as documentation in the code, and they can be > used to provide better help. This applies both to qdev and to object > properties. > > Completing this task in one go is unfortunately not practical. We need > to add descriptions incrementally. Until we're done, some properties > will lack descriptions (null pointer rather than a descriptive string). > > Reusing the property name as description just to have a description adds > no information. > > What about this: add a description string (optional for now) both to > ObjectProperty and to PropertyInfo. Either add a description string > parameter to object property constructors like object_property_add() and > object_property_add_alias, or, for less churn, add a separate function > to set an object property's description. Update > qdev_alias_all_properties() to set the object property's description to > the qdev property's. > > Then you can fix the help regression by giving all the regressed > properties a useful description. > > That fix should also permit retiring legacy_name. Ack. Sounds good to me.
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
"Gonglei (Arei)" writes: > 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 :) Adding descriptions to properties is a big, but useful task. The descriptions can serve as documentation in the code, and they can be used to provide better help. This applies both to qdev and to object properties. Completing this task in one go is unfortunately not practical. We need to add descriptions incrementally. Until we're done, some properties will lack descriptions (null pointer rather than a descriptive string). Reusing the property name as description just to have a description adds no information. What about this: add a description string (optional for now) both to ObjectProperty and to PropertyInfo. Either add a description string parameter to object property constructors like object_property_add() and object_property_add_alias, or, for less churn, add a separate function to set an object property's description. Update qdev_alias_all_properties() to set the object property's description to the qdev property's. Then you can fix the help regression by giving all the regressed properties a useful description. That fix should also permit retiring legacy_name.
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
Il 23/09/2014 05:09, Gonglei (Arei) ha scritto: > 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". You use the legacy_name field of PropertyInfo to set the description of a qdev property, and then let object_property_add_alias() copy the description. Paolo
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
On Mon, Sep 22, 2014 at 03:08:09PM +0200, Paolo Bonzini wrote: > Of course, with every new feature we would most likely have yet another > unfinished transition. In the lack of a clear user complaint (or even > of a clear indication that human users ever used -device foo,help...) > the tempation to pass the buck is strong. > > Paolo Well I use it. Or try to, each time I need to build QEMU command line from scratch. In the end, I usually give up, dig out some old script and copy/paste from it. -- MST
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
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 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
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
> Subject: Re: [PATCH 0/3] Fix confused output for alias properties > > Il 22/09/2014 15:36, Gonglei (Arei) ha scritto: > >> If the field is called "opaque", it's opaque. > > > > Sorry? > > The field is called "opaque" because no one, except the callbacks, > should look into that field. Casting it to AliasProperty breaks that rule. > > Paolo OK. Thanks for your explanation. I have no idea too. :( Best regards, -Gonglei
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
Il 22/09/2014 15:36, Gonglei (Arei) ha scritto: >> If the field is called "opaque", it's opaque. > > Sorry? The field is called "opaque" because no one, except the callbacks, should look into that field. Casting it to AliasProperty breaks that rule. Paolo
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
> Subject: Re: [PATCH 0/3] Fix confused output for alias properties > > Il 22/09/2014 15:13, Gonglei (Arei) ha scritto: > > (I don't add alias-sepecific filed into ObjectProperty > > struct, just add a bool property. ) > > > > diff --git a/include/qom/object.h b/include/qom/object.h > > index 8a05a81..8b8ded5 100644 > > --- a/include/qom/object.h > > +++ b/include/qom/object.h > > @@ -334,6 +334,11 @@ typedef void (ObjectPropertyRelease)(Object *obj, > > const char *name, > > void *opaque); > > > > +typedef struct { > > +Object *target_obj; > > +const char *target_name; > > +} AliasProperty; > > + > > typedef struct ObjectProperty > > { > > gchar *name; > > @@ -344,6 +349,8 @@ typedef struct ObjectProperty > > ObjectPropertyRelease *release; > > void *opaque; > > > > +bool is_alias; > > + > > QTAILQ_ENTRY(ObjectProperty) node; > > } ObjectProperty; > > I don't think this is right. If the field is called "opaque", it's > opaque. Sorry? > We already had cases where we were deriving the type of the > opaque, and they caused bugs (though it was using the const char *type > field; admittedly having a separate bool is a bit cleaner). > Sorry for my poor English. I can't understand your mean clearly. :( Would you explain it for me? Thanks a lot! Best regards, -Gonglei
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
Il 22/09/2014 15:13, Gonglei (Arei) ha scritto: > (I don't add alias-sepecific filed into ObjectProperty > struct, just add a bool property. ) > > diff --git a/include/qom/object.h b/include/qom/object.h > index 8a05a81..8b8ded5 100644 > --- a/include/qom/object.h > +++ b/include/qom/object.h > @@ -334,6 +334,11 @@ typedef void (ObjectPropertyRelease)(Object *obj, > const char *name, > void *opaque); > > +typedef struct { > +Object *target_obj; > +const char *target_name; > +} AliasProperty; > + > typedef struct ObjectProperty > { > gchar *name; > @@ -344,6 +349,8 @@ typedef struct ObjectProperty > ObjectPropertyRelease *release; > void *opaque; > > +bool is_alias; > + > QTAILQ_ENTRY(ObjectProperty) node; > } ObjectProperty; I don't think this is right. If the field is called "opaque", it's opaque. We already had cases where we were deriving the type of the opaque, and they caused bugs (though it was using the const char *type field; admittedly having a separate bool is a bit cleaner). Paolo
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
> > From: Paolo Bonzini [mailto:paolo.bonz...@gmail.com] On Behalf Of Paolo > > Bonzini > > Sent: Monday, September 22, 2014 8:07 PM > > To: Gonglei (Arei); Michael S. Tsirkin > > Cc: ag...@suse.de; Huangweidong (C); aligu...@amazon.com; Huangpeng > > (Peter); qemu-devel@nongnu.org; stefa...@redhat.com; > > lcapitul...@redhat.com > > Subject: Re: [PATCH 0/3] Fix confused output for alias properties > > > > Il 22/09/2014 13:22, Gonglei (Arei) ha scritto: > > > > 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. > > > > What about this below way? Thanks! > > (I don't add alias-sepecific filed into ObjectProperty > struct, just add a bool property. ) > Then we can cast "void *opaque" of ObjectPropery to AliasProperty struct. :) > diff --git a/include/qom/object.h b/include/qom/object.h > index 8a05a81..8b8ded5 100644 > --- a/include/qom/object.h > +++ b/include/qom/object.h > @@ -334,6 +334,11 @@ typedef void (ObjectPropertyRelease)(Object *obj, > const char *name, > void *opaque); > > +typedef struct { > +Object *target_obj; > +const char *target_name; > +} AliasProperty; > + > typedef struct ObjectProperty > { > gchar *name; > @@ -344,6 +349,8 @@ typedef struct ObjectProperty > ObjectPropertyRelease *release; > void *opaque; > > +bool is_alias; > + > QTAILQ_ENTRY(ObjectProperty) node; > } ObjectProperty; > > diff --git a/qom/object.c b/qom/object.c > index a8c3065..0b4e402 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -1590,11 +1590,6 @@ void object_property_add_uint64_ptr(Object *obj, > const char *name, > NULL, NULL, (void *)v, errp); > } > > -typedef struct { > -Object *target_obj; > -const char *target_name; > -} AliasProperty; > - > static void property_get_alias(Object *obj, struct Visitor *v, void *opaque, > const char *name, Error **errp) > { > @@ -1663,6 +1658,7 @@ void object_property_add_alias(Object *obj, const > char *name, > goto out; > } > op->resolve = property_resolve_alias; > +op->is_alias = true; > > out: > g_free(prop_type); > -- > 1.7.12.4 > > > > 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) > > > > That's a bit hackish, but perhaps it would be good enough for mst and > > Markus? > > > > Paolo > > Best regards, > -Gonglei
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
> -Original Message- > From: Paolo Bonzini [mailto:paolo.bonz...@gmail.com] On Behalf Of Paolo > Bonzini > Sent: Monday, September 22, 2014 8:07 PM > To: Gonglei (Arei); Michael S. Tsirkin > Cc: ag...@suse.de; Huangweidong (C); aligu...@amazon.com; Huangpeng > (Peter); qemu-devel@nongnu.org; stefa...@redhat.com; > lcapitul...@redhat.com > Subject: Re: [PATCH 0/3] Fix confused output for alias properties > > Il 22/09/2014 13:22, Gonglei (Arei) ha scritto: > > > 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. > What about this below way? Thanks! (I don't add alias-sepecific filed into ObjectProperty struct, just add a bool property. ) diff --git a/include/qom/object.h b/include/qom/object.h index 8a05a81..8b8ded5 100644 --- a/include/qom/object.h +++ b/include/qom/object.h @@ -334,6 +334,11 @@ typedef void (ObjectPropertyRelease)(Object *obj, const char *name, void *opaque); +typedef struct { +Object *target_obj; +const char *target_name; +} AliasProperty; + typedef struct ObjectProperty { gchar *name; @@ -344,6 +349,8 @@ typedef struct ObjectProperty ObjectPropertyRelease *release; void *opaque; +bool is_alias; + QTAILQ_ENTRY(ObjectProperty) node; } ObjectProperty; diff --git a/qom/object.c b/qom/object.c index a8c3065..0b4e402 100644 --- a/qom/object.c +++ b/qom/object.c @@ -1590,11 +1590,6 @@ void object_property_add_uint64_ptr(Object *obj, const char *name, NULL, NULL, (void *)v, errp); } -typedef struct { -Object *target_obj; -const char *target_name; -} AliasProperty; - static void property_get_alias(Object *obj, struct Visitor *v, void *opaque, const char *name, Error **errp) { @@ -1663,6 +1658,7 @@ void object_property_add_alias(Object *obj, const char *name, goto out; } op->resolve = property_resolve_alias; +op->is_alias = true; out: g_free(prop_type); -- 1.7.12.4 > 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) > > That's a bit hackish, but perhaps it would be good enough for mst and > Markus? > > Paolo Best regards, -Gonglei
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
Il 22/09/2014 13:43, Markus Armbruster ha scritto: > Paolo Bonzini writes: > >> Il 22/09/2014 10:34, Michael S. Tsirkin ha scritto: >>> Basically this patch brings back historical HMP behaviour. >>> As far as I could tell, it wasn't changed intentionally. >> >> It was changed intentionally. Or rather, the change was known at the >> time Stefan made it. > > Commit hash? There are three commits involved. The first is commit f4eb32b (qmp: show QOM properties in device-list-properties, 2014-05-20). It was done in preparation for the change of virtio-blk-pci.drive from qdev property to QOM property, to avoid breaking device-list-properties. The actual change took some more time to review, so it went in one month later. Commit caffdac (virtio-blk: use aliases instead of duplicate qdev properties, 2014-06-18) is what changed virtio-blk-pci.drive from qdev property to QOM property. This changed the type from 'drive' to 'str' in device-list-properties. (Note that this patch fixed an actual bug, it was not just for the sake of cleanup). I cannot find any reference to the change; perhaps it was discussed only on IRC. However, I'm quite certain that I knew about it. Now one thing did slip through; commit caffdac actually dropped virtio-blk-pci.drive from -device virtio-blk-pci,help. Either I misremembered that the first commit fixed command-line help too, or I just assumed that -device foo,help was implemented on top of qmp_device_list_properties. Which wasn't the case, so the third commit (9ef52358, qdev-monitor: include QOM properties in -device FOO, help output, 2014-07-09) did the right thing and brought back virtio-blk-pci.drive into the help message. Now, the cover latter for that patch finally has a hint that the change was known. http://thread.gmane.org/gmane.comp.emulators.qemu/285819 has this text: We simply need to update -device FOO,help code to use both qdev and QOM properties. Note that types change because a 'drive' qdev type is actually a 'str' QOM type. We're moving more and more to QOM properties where the final type for this property would be 'link' or similar. It is only in the cover letter and thus not part of any commit message. > I haven't got this part of the code present in my mind at the moment, > but I'm willing to take your assertion of a layering violation for > granted. Is this violation necessary for fixing the regression, or is > it just an artifact of this particular fix? I guess we could always add some ad-hoc mechanism for device-list-properties to get to the "drive" string, and smuggle it as a QOM feature. Then, aliases would just forward that mechanism to the aliased property, which would just work. Of course, with every new feature we would most likely have yet another unfinished transition. In the lack of a clear user complaint (or even of a clear indication that human users ever used -device foo,help...) the tempation to pass the buck is strong. Paolo
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
Il 22/09/2014 14:34, Michael S. Tsirkin ha scritto: > On Mon, Sep 22, 2014 at 02:06:38PM +0200, Paolo Bonzini wrote: >> Il 22/09/2014 13:22, Gonglei (Arei) ha scritto: 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) >> 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 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
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
Paolo Bonzini writes: > Il 22/09/2014 13:22, Gonglei (Arei) ha scritto: >> > 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) > > That's a bit hackish, but perhaps it would be good enough for mst and > Markus? Works for me. A step towards better self-documentation of properties.
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
On Mon, Sep 22, 2014 at 02:06:38PM +0200, Paolo Bonzini wrote: > Il 22/09/2014 13:22, Gonglei (Arei) ha scritto: > > > 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) > That's a bit hackish, but perhaps it would be good enough for mst and > Markus? > > Paolo I would just drop "str" and replace it with drive. I seems to convey zero information. Description should be something more informative, e.g. (ID of a drive to use as a backend) 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." -- MST
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
Il 22/09/2014 13:22, Gonglei (Arei) ha scritto: > > 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) That's a bit hackish, but perhaps it would be good enough for mst and Markus? Paolo
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
Paolo Bonzini writes: > Il 22/09/2014 10:34, Michael S. Tsirkin ha scritto: >> Basically this patch brings back historical HMP behaviour. >> As far as I could tell, it wasn't changed intentionally. > > It was changed intentionally. Or rather, the change was known at the > time Stefan made it. Commit hash? >> So how about applying this first and then making more >> changes on top if required? > > No. > > There is no reason to add alias-specific fields to ObjectProperty, and > the implementation is not even complete because you can alias property X > of object A to property Y of object B. I think there are two questions here. The first one is whether to accept the regression and move on. The second one is whether this patch is a satisfactory fix for the regression. I'm inclined to answer the first question in the negative. We discussed the relative usefulness of the help information before and after the regression, as well as after ripping out legacy_name entirely. We agree that help has always been somewhat cryptic. I'd certainly support a patch that replaces it wholesale by something better. I'm opposed, however, to degrading it further, if we can help it. The fact that such a degradation has slipped in already can't change my opinion. I view it as regression. Evidence of a deliberate, informed decision to regress could change it. That's why I ask for the commit hash. The "if we can help it" condition ties the first to the second question. I haven't got this part of the code present in my mind at the moment, but I'm willing to take your assertion of a layering violation for granted. Is this violation necessary for fixing the regression, or is it just an artifact of this particular fix?
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
> From: Paolo Bonzini [mailto:paolo.bonz...@gmail.com] On Behalf Of Paolo > Bonzini > Sent: Monday, September 22, 2014 6:04 PM > To: Gonglei (Arei); Michael S. Tsirkin > Cc: Huangweidong (C); aligu...@amazon.com; qemu-devel@nongnu.org; > ag...@suse.de; stefa...@redhat.com; Huangpeng (Peter); > lcapitul...@redhat.com > Subject: Re: [PATCH 0/3] Fix confused output for alias properties > > Il 22/09/2014 11:33, Gonglei (Arei) ha scritto: > > Yes, I knew this issue which object A's property name may > > is not the same with object B, so I had posted v2 to fix it. > > 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! Best regards, -Gonglei
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
Il 22/09/2014 11:33, Gonglei (Arei) ha scritto: > Yes, I knew this issue which object A's property name may > is not the same with object B, so I had posted v2 to fix it. This doesn't change the fact that ObjectProperty is a generic struct, and adding alias-specific fields there is wrong. Paolo
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
> From: Paolo Bonzini [mailto:paolo.bonz...@gmail.com] On Behalf Of Paolo > Bonzini > Sent: Monday, September 22, 2014 5:15 PM > To: Michael S. Tsirkin; Gonglei (Arei) > Cc: ag...@suse.de; Huangweidong (C); aligu...@amazon.com; Huangpeng > (Peter); qemu-devel@nongnu.org; stefa...@redhat.com; > lcapitul...@redhat.com > Subject: Re: [PATCH 0/3] Fix confused output for alias properties > > Il 22/09/2014 10:34, Michael S. Tsirkin ha scritto: > > Basically this patch brings back historical HMP behaviour. > > As far as I could tell, it wasn't changed intentionally. > > It was changed intentionally. Or rather, the change was known at the > time Stefan made it. > > > So how about applying this first and then making more > > changes on top if required? > > No. > > There is no reason to add alias-specific fields to ObjectProperty, and > the implementation is not even complete because you can alias property X > of object A to property Y of object B. > Yes, I knew this issue which object A's property name may is not the same with object B, so I had posted v2 to fix it. Best regards, -Gonglei
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
"Michael S. Tsirkin" writes: > On Mon, Sep 15, 2014 at 10:44:36PM +0800, arei.gong...@huawei.com wrote: >> From: Gonglei >> >> At present, people have no way to know they should >> have a specific format for alias properties. >> >> Example: >> >> before output: >> >> virtio-blk-pci.physical_block_size=uint16 >> virtio-blk-pci.logical_block_size=uint16 >> virtio-blk-pci.drive=str >> >> after output applied this patch series: >> >> virtio-blk-pci.physical_block_size=blocksize >> virtio-blk-pci.logical_block_size=blocksize >> virtio-blk-pci.drive=drive >> >> Gonglei (3): >> qom: add error handler for object alias property >> qom: add target object poniter for alias property in ObjectProperty >> qmp: print real legacy_name for alias property >> >> include/qom/object.h | 3 +++ >> qmp.c| 68 >> >> qom/object.c | 10 +++- >> 3 files changed, 59 insertions(+), 22 deletions(-) > > Basically this patch brings back historical HMP behaviour. > As far as I could tell, it wasn't changed intentionally. > So how about applying this first and then making more > changes on top if required? Makes sense to me.
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
Il 22/09/2014 10:34, Michael S. Tsirkin ha scritto: > Basically this patch brings back historical HMP behaviour. > As far as I could tell, it wasn't changed intentionally. It was changed intentionally. Or rather, the change was known at the time Stefan made it. > So how about applying this first and then making more > changes on top if required? No. There is no reason to add alias-specific fields to ObjectProperty, and the implementation is not even complete because you can alias property X of object A to property Y of object B. Paolo
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
> From: Michael S. Tsirkin [mailto:m...@redhat.com] > Sent: Monday, September 22, 2014 4:34 PM > Subject: Re: [PATCH 0/3] Fix confused output for alias properties > > On Mon, Sep 15, 2014 at 10:44:36PM +0800, arei.gong...@huawei.com wrote: > > From: Gonglei > > > > At present, people have no way to know they should > > have a specific format for alias properties. > > > > Example: > > > > before output: > > > > virtio-blk-pci.physical_block_size=uint16 > > virtio-blk-pci.logical_block_size=uint16 > > virtio-blk-pci.drive=str > > > > after output applied this patch series: > > > > virtio-blk-pci.physical_block_size=blocksize > > virtio-blk-pci.logical_block_size=blocksize > > virtio-blk-pci.drive=drive > > > > Gonglei (3): > > qom: add error handler for object alias property > > qom: add target object poniter for alias property in ObjectProperty > > qmp: print real legacy_name for alias property > > > > include/qom/object.h | 3 +++ > > qmp.c| 68 > > > qom/object.c | 10 +++- > > 3 files changed, 59 insertions(+), 22 deletions(-) > > Basically this patch brings back historical HMP behaviour. > As far as I could tell, it wasn't changed intentionally. > So how about applying this first and then making more > changes on top if required? > I agree with you because the below virtio patch series is somewhat of relying on this. (Please apply v2 if possible) [PATCH v2 0/9] virtio: fix virtio child recount in transports What's your opinion, Paolo? Thanks! Best regards, -Gonglei
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
On Mon, Sep 15, 2014 at 10:44:36PM +0800, arei.gong...@huawei.com wrote: > From: Gonglei > > At present, people have no way to know they should > have a specific format for alias properties. > > Example: > > before output: > > virtio-blk-pci.physical_block_size=uint16 > virtio-blk-pci.logical_block_size=uint16 > virtio-blk-pci.drive=str > > after output applied this patch series: > > virtio-blk-pci.physical_block_size=blocksize > virtio-blk-pci.logical_block_size=blocksize > virtio-blk-pci.drive=drive > > Gonglei (3): > qom: add error handler for object alias property > qom: add target object poniter for alias property in ObjectProperty > qmp: print real legacy_name for alias property > > include/qom/object.h | 3 +++ > qmp.c| 68 > > qom/object.c | 10 +++- > 3 files changed, 59 insertions(+), 22 deletions(-) Basically this patch brings back historical HMP behaviour. As far as I could tell, it wasn't changed intentionally. So how about applying this first and then making more changes on top if required? > -- > 1.7.12.4 >
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
Hi, Paolo > -Original Message- > From: Gonglei (Arei) > Sent: Wednesday, September 17, 2014 10:32 AM > To: 'Paolo Bonzini'; Markus Armbruster > Cc: Huangweidong (C); aligu...@amazon.com; Michael S. Tsirkin; > ag...@suse.de; qemu-devel@nongnu.org; stefa...@redhat.com; Huangpeng > (Peter); lcapitul...@redhat.com > Subject: RE: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties > > > From: Paolo Bonzini [mailto:pbonz...@redhat.com] > > Sent: Tuesday, September 16, 2014 10:37 PM > > Subject: Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias > > properties > > > > Il 16/09/2014 16:32, Markus Armbruster ha scritto: > > > Paolo Bonzini writes: > > > > > >> Il 16/09/2014 11:16, Markus Armbruster ha scritto: > > >>>> I think both "str" and "link" actually are a small > > degradation > > >>>> compared to "drive", and this is why I kept the legacy_name. But > > overall I > > >>>> think it's not really worth the layering violation that patches 2 and > > >>>> 3 are; > > >>>> and it's definitely not stable material. > > >>> > > >>> "str" is clearly a degradation for me. I breaks usage like > > >>> > > >>> for i in `qemu -device help 2>&1 | sed -n 's/^name > "\([^"]*\)".*/\1/p'` > > >>> do qemu -device $i,help 2>&1 > > >>> done | grep =drive > > >>> > > >>> Finds all block device models. I've done such things many times. > > >> > > >> Just replace "grep =drive" with "fgrep .drive". Similarly: > > >> > > >> =chr -> .chardev (bonus: matches the command line option) > > >> =netdev -> .netdev > > >> =vlan-> .vlan > > >> =macaddr -> .mac > > > > > > Unlike =drive, this isn't guaranteed to work. > > > > If it doesn't, when we've been consistent so far, it's a bug. > > > > >> It is, but we're suprisingly consistent in the naming of such > > >> special-typed properties. So it's actually a good thing that > > >> legacy_name provides redundant information. > > > > > > Given our overall consistency track record: yes, it's surprising, and no, > > > I'm not comfortable relying on us being consistent :) > > > > The point of stuff like QOM is exactly to force us to be consistent. > > No, it doesn't always work. But patches 2 and 3 really are a layering > > violation. I have no idea how to bring back "drive" without introducing > > a layering violation somewhere, but this one is really too much... > > > Sorry, I can't understand the layering violation on PATCH2 and PATCH3. > AliasProperty struct is QOM object and ObjectProperty is also QOM object, > I just move AliasProperty struct from Object.c to Object.h meanwhile add > a point reference in ObjectProperty struct for AliasProperty. Does this is a > layering violation? > Ping...? Thanks ! Best regards, -Gonglei > Hope for your more detailed information about this, thanks in advance! :) > > Best regards, > -Gonglei
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
On 09/16/2014 11:54 PM, Markus Armbruster wrote: > Eric Blake writes: > >> On 09/16/2014 12:31 PM, Paolo Bonzini wrote: >> Change legacy_name to point to a detailed human-readable description of the type? E.g. "Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF"? >>> >>> If libvirt can cope with >>> >>> e1000.mac=str (Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF) >> >> Isn't this output only available under -help? Libvirt only cares about >> QMP listings of devices and their properties, so improving the human >> interface should have no negative impact to the machine interface. > > Libvirt still has code parsing this, see virQEMUCapsExtractDeviceStr(). > Whether it's actually used with recent QEMU I don't know. That code is only used for qemu < 1.5 (when -help parsing is all we can use) - but those versions of qemu are not going to change -help output, and new versions of qemu use only QMP, so you are free to change it with no impact to libvirt. > > -device FOO,help merely formats qmp_device_list_properties()'s value for > human consumption. So, to make a description string available for help, > we also have to put it into the function's value. If we don't want it > in QMP, we need to filter it out in the command handler. Right now, > qmp_device_list_properties() *is* the command handler. > > Aside: why isn't the QMP command named query-device-list-properties? We > suck at consistency! > > -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
"Michael S. Tsirkin" writes: > On Tue, Sep 16, 2014 at 10:01:19PM +0300, Michael S. Tsirkin wrote: >> On Tue, Sep 16, 2014 at 08:31:08PM +0200, Paolo Bonzini wrote: >> > Il 16/09/2014 18:56, Michael S. Tsirkin ha scritto: >> > > On Tue, Sep 16, 2014 at 06:27:51PM +0200, Paolo Bonzini wrote: >> > >> Il 16/09/2014 18:26, Michael S. Tsirkin ha scritto: >> > >>> Right so types should be explicit. >> > >>> If an arbitrary string isn't allowed, this should be documented. >> > >>> It's not great as is: what's the format for macaddr? AA:BB:CC:DD:EE:FF? >> > >>> aa:bb:cc:dd:ee:ff? aabbccddeeff? 0xaabbccddeeff? >> > >>> But just saying "string" is going in the wrong direction imho. >> > >> >> > >> That's the purpose of documentation (docs/qdev-device-use.txt), >> > > >> > > That's not user documentation, that's developer documentation, >> > > isn't it? >> > >> > It's user documentation. It's not distributed because we suck at >> > documentation. >> > >> > >> and even >> > >> then is better done with examples. I don't think doing it in -device >> > >> foo,help (which I'm not even sure is particularly helpful. >> > > >> > > -device foo,help isn't helpful at all because it does not >> > > tell people what does each option do. >> > > But it really should be fixed. >> > >> > Exactly. >> > >> > >> I'm sympathetic towards fixing the drive->str change, but I have no idea >> > >> how to do it. >> > > >> > > Change legacy_name to point to a detailed human-readable >> > > description of the type? >> > > E.g. "Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF"? >> > >> > If libvirt can cope with >> > >> > e1000.mac=str (Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF) >> > >> > that would work for me. >> >> Shouldn't "str" be "string" in HMP? Probably. What about QMP? JSON calls the type "string". Possible justification for "str": QMP makes up its own type system, losely based on JSON's, with its own terminology. I'm not sure "=T" adds value over the description. Certainly not in the example above. >> Eblake - type is ignored right? Does this mean anything to the right of >> = is ignored? As far as I can tell from the libvirt sources: yes. >> > > We really really should add description to all properties, too. >> > >> > This is a huge job. We have hundreds of properties. >> > >> > Paolo >> >> Right. If we don't start we won't get there though, will we? >> > > And, we'll keep adding undocumented ones. Agree with all three points. We should get our act together on property documentation.
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
Eric Blake writes: > On 09/16/2014 12:31 PM, Paolo Bonzini wrote: > >>> Change legacy_name to point to a detailed human-readable >>> description of the type? >>> E.g. "Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF"? >> >> If libvirt can cope with >> >> e1000.mac=str (Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF) > > Isn't this output only available under -help? Libvirt only cares about > QMP listings of devices and their properties, so improving the human > interface should have no negative impact to the machine interface. Libvirt still has code parsing this, see virQEMUCapsExtractDeviceStr(). Whether it's actually used with recent QEMU I don't know. -device FOO,help merely formats qmp_device_list_properties()'s value for human consumption. So, to make a description string available for help, we also have to put it into the function's value. If we don't want it in QMP, we need to filter it out in the command handler. Right now, qmp_device_list_properties() *is* the command handler. Aside: why isn't the QMP command named query-device-list-properties? We suck at consistency!
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
> From: Paolo Bonzini [mailto:pbonz...@redhat.com] > Sent: Tuesday, September 16, 2014 10:37 PM > Subject: Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties > > Il 16/09/2014 16:32, Markus Armbruster ha scritto: > > Paolo Bonzini writes: > > > >> Il 16/09/2014 11:16, Markus Armbruster ha scritto: > >>>> I think both "str" and "link" actually are a small > degradation > >>>> compared to "drive", and this is why I kept the legacy_name. But > overall I > >>>> think it's not really worth the layering violation that patches 2 and 3 > >>>> are; > >>>> and it's definitely not stable material. > >>> > >>> "str" is clearly a degradation for me. I breaks usage like > >>> > >>> for i in `qemu -device help 2>&1 | sed -n 's/^name > >>> "\([^"]*\)".*/\1/p'` > >>> do qemu -device $i,help 2>&1 > >>> done | grep =drive > >>> > >>> Finds all block device models. I've done such things many times. > >> > >> Just replace "grep =drive" with "fgrep .drive". Similarly: > >> > >> =chr -> .chardev (bonus: matches the command line option) > >> =netdev -> .netdev > >> =vlan-> .vlan > >> =macaddr -> .mac > > > > Unlike =drive, this isn't guaranteed to work. > > If it doesn't, when we've been consistent so far, it's a bug. > > >> It is, but we're suprisingly consistent in the naming of such > >> special-typed properties. So it's actually a good thing that > >> legacy_name provides redundant information. > > > > Given our overall consistency track record: yes, it's surprising, and no, > > I'm not comfortable relying on us being consistent :) > > The point of stuff like QOM is exactly to force us to be consistent. > No, it doesn't always work. But patches 2 and 3 really are a layering > violation. I have no idea how to bring back "drive" without introducing > a layering violation somewhere, but this one is really too much... > Sorry, I can't understand the layering violation on PATCH2 and PATCH3. AliasProperty struct is QOM object and ObjectProperty is also QOM object, I just move AliasProperty struct from Object.c to Object.h meanwhile add a point reference in ObjectProperty struct for AliasProperty. Does this is a layering violation? Hope for your more detailed information about this, thanks in advance! :) Best regards, -Gonglei
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
On 09/16/2014 12:31 PM, Paolo Bonzini wrote: >> Change legacy_name to point to a detailed human-readable >> description of the type? >> E.g. "Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF"? > > If libvirt can cope with > > e1000.mac=str (Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF) Isn't this output only available under -help? Libvirt only cares about QMP listings of devices and their properties, so improving the human interface should have no negative impact to the machine interface. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
On Tue, Sep 16, 2014 at 10:01:19PM +0300, Michael S. Tsirkin wrote: > On Tue, Sep 16, 2014 at 08:31:08PM +0200, Paolo Bonzini wrote: > > Il 16/09/2014 18:56, Michael S. Tsirkin ha scritto: > > > On Tue, Sep 16, 2014 at 06:27:51PM +0200, Paolo Bonzini wrote: > > >> Il 16/09/2014 18:26, Michael S. Tsirkin ha scritto: > > >>> Right so types should be explicit. > > >>> If an arbitrary string isn't allowed, this should be documented. > > >>> It's not great as is: what's the format for macaddr? AA:BB:CC:DD:EE:FF? > > >>> aa:bb:cc:dd:ee:ff? aabbccddeeff? 0xaabbccddeeff? > > >>> But just saying "string" is going in the wrong direction imho. > > >> > > >> That's the purpose of documentation (docs/qdev-device-use.txt), > > > > > > That's not user documentation, that's developer documentation, > > > isn't it? > > > > It's user documentation. It's not distributed because we suck at > > documentation. > > > > >> and even > > >> then is better done with examples. I don't think doing it in -device > > >> foo,help (which I'm not even sure is particularly helpful. > > > > > > -device foo,help isn't helpful at all because it does not > > > tell people what does each option do. > > > But it really should be fixed. > > > > Exactly. > > > > >> I'm sympathetic towards fixing the drive->str change, but I have no idea > > >> how to do it. > > > > > > Change legacy_name to point to a detailed human-readable > > > description of the type? > > > E.g. "Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF"? > > > > If libvirt can cope with > > > > e1000.mac=str (Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF) > > > > that would work for me. > > Shouldn't "str" be "string" in HMP? > > Eblake - type is ignored right? Does this mean anything to the right of > = is ignored? > > > > We really really should add description to all properties, too. > > > > This is a huge job. We have hundreds of properties. > > > > Paolo > > Right. If we don't start we won't get there though, will we? > And, we'll keep adding undocumented ones.
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
On Tue, Sep 16, 2014 at 08:31:08PM +0200, Paolo Bonzini wrote: > Il 16/09/2014 18:56, Michael S. Tsirkin ha scritto: > > On Tue, Sep 16, 2014 at 06:27:51PM +0200, Paolo Bonzini wrote: > >> Il 16/09/2014 18:26, Michael S. Tsirkin ha scritto: > >>> Right so types should be explicit. > >>> If an arbitrary string isn't allowed, this should be documented. > >>> It's not great as is: what's the format for macaddr? AA:BB:CC:DD:EE:FF? > >>> aa:bb:cc:dd:ee:ff? aabbccddeeff? 0xaabbccddeeff? > >>> But just saying "string" is going in the wrong direction imho. > >> > >> That's the purpose of documentation (docs/qdev-device-use.txt), > > > > That's not user documentation, that's developer documentation, > > isn't it? > > It's user documentation. It's not distributed because we suck at > documentation. > > >> and even > >> then is better done with examples. I don't think doing it in -device > >> foo,help (which I'm not even sure is particularly helpful. > > > > -device foo,help isn't helpful at all because it does not > > tell people what does each option do. > > But it really should be fixed. > > Exactly. > > >> I'm sympathetic towards fixing the drive->str change, but I have no idea > >> how to do it. > > > > Change legacy_name to point to a detailed human-readable > > description of the type? > > E.g. "Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF"? > > If libvirt can cope with > > e1000.mac=str (Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF) > > that would work for me. Shouldn't "str" be "string" in HMP? Eblake - type is ignored right? Does this mean anything to the right of = is ignored? > > We really really should add description to all properties, too. > > This is a huge job. We have hundreds of properties. > > Paolo Right. If we don't start we won't get there though, will we?
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
Il 16/09/2014 18:56, Michael S. Tsirkin ha scritto: > On Tue, Sep 16, 2014 at 06:27:51PM +0200, Paolo Bonzini wrote: >> Il 16/09/2014 18:26, Michael S. Tsirkin ha scritto: >>> Right so types should be explicit. >>> If an arbitrary string isn't allowed, this should be documented. >>> It's not great as is: what's the format for macaddr? AA:BB:CC:DD:EE:FF? >>> aa:bb:cc:dd:ee:ff? aabbccddeeff? 0xaabbccddeeff? >>> But just saying "string" is going in the wrong direction imho. >> >> That's the purpose of documentation (docs/qdev-device-use.txt), > > That's not user documentation, that's developer documentation, > isn't it? It's user documentation. It's not distributed because we suck at documentation. >> and even >> then is better done with examples. I don't think doing it in -device >> foo,help (which I'm not even sure is particularly helpful. > > -device foo,help isn't helpful at all because it does not > tell people what does each option do. > But it really should be fixed. Exactly. >> I'm sympathetic towards fixing the drive->str change, but I have no idea >> how to do it. > > Change legacy_name to point to a detailed human-readable > description of the type? > E.g. "Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF"? If libvirt can cope with e1000.mac=str (Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF) that would work for me. > We really really should add description to all properties, too. This is a huge job. We have hundreds of properties. Paolo
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
On Tue, Sep 16, 2014 at 06:27:51PM +0200, Paolo Bonzini wrote: > Il 16/09/2014 18:26, Michael S. Tsirkin ha scritto: > > Right so types should be explicit. > > If an arbitrary string isn't allowed, this should be documented. > > It's not great as is: what's the format for macaddr? AA:BB:CC:DD:EE:FF? > > aa:bb:cc:dd:ee:ff? aabbccddeeff? 0xaabbccddeeff? > > But just saying "string" is going in the wrong direction imho. > > That's the purpose of documentation (docs/qdev-device-use.txt), That's not user documentation, that's developer documentation, isn't it? > and even > then is better done with examples. I don't think doing it in -device > foo,help (which I'm not even sure is particularly helpful. -device foo,help isn't helpful at all because it does not tell people what does each option do. But it really should be fixed. > "ip link help" doesn't document the format of a MAC address, either. Networking on linux is notoriously hard to configure. Hence the rise of wrapper tools such as network manager to hide all the details from users. > I'm sympathetic towards fixing the drive->str change, but I have no idea > how to do it. > > Paolo Change legacy_name to point to a detailed human-readable description of the type? E.g. "Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF"? We really really should add description to all properties, too. -- MST
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
On Tue, Sep 16, 2014 at 12:45:43PM +0200, Paolo Bonzini wrote: > Il 16/09/2014 12:37, Michael S. Tsirkin ha scritto: > > str really should be for free-form strings. > > It makes as much sense to call it as string > > Yes, that's why I said drive->str is a degradation. The question is, > how important is that degradation? > > > as it is to call an integer a string because you type > > a string of characters to specify it. > > I disagree. This is true for the command line, and for historical > reasons it is also true for device-add, but the newer QOM commands are > type-safe. You cannot pass an integer as a JSON string '123' to qom-set > or object-add, for example. > > Paolo Right so types should be explicit. If an arbitrary string isn't allowed, this should be documented. It's not great as is: what's the format for macaddr? AA:BB:CC:DD:EE:FF? aa:bb:cc:dd:ee:ff? aabbccddeeff? 0xaabbccddeeff? But just saying "string" is going in the wrong direction imho. -- MST
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
Il 16/09/2014 18:26, Michael S. Tsirkin ha scritto: > Right so types should be explicit. > If an arbitrary string isn't allowed, this should be documented. > It's not great as is: what's the format for macaddr? AA:BB:CC:DD:EE:FF? > aa:bb:cc:dd:ee:ff? aabbccddeeff? 0xaabbccddeeff? > But just saying "string" is going in the wrong direction imho. That's the purpose of documentation (docs/qdev-device-use.txt), and even then is better done with examples. I don't think doing it in -device foo,help (which I'm not even sure is particularly helpful. "ip link help" doesn't document the format of a MAC address, either. I'm sympathetic towards fixing the drive->str change, but I have no idea how to do it. Paolo
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
Paolo Bonzini writes: > Il 16/09/2014 11:16, Markus Armbruster ha scritto: >>> I think both "str" and "link" actually are a small >>> degradation >>> compared to "drive", and this is why I kept the legacy_name. But overall I >>> think it's not really worth the layering violation that patches 2 and 3 are; >>> and it's definitely not stable material. >> >> "str" is clearly a degradation for me. I breaks usage like >> >> for i in `qemu -device help 2>&1 | sed -n 's/^name "\([^"]*\)".*/\1/p'` >> do qemu -device $i,help 2>&1 >> done | grep =drive >> >> Finds all block device models. I've done such things many times. > > Just replace "grep =drive" with "fgrep .drive". Similarly: > > =chr -> .chardev (bonus: matches the command line option) > =netdev -> .netdev > =vlan-> .vlan > =macaddr -> .mac Unlike =drive, this isn't guaranteed to work. > We probably agree that having "=drive" work sometimes, but not always, > is the worst of both worlds. Still doesn't make it stable material, IMO. I'd consider it for stable. It's a regression. Pretty harmless, but regression all the same. A pretty harmless regression may be preferable to a hairy patch. Tradeoff, should be evaluated by the stable maintainer. >> Agree on the uselessness of "on/off". >> >> Agree on the uselessness of "blocksize" without a definition of the >> term. >> >> "chr" and "netdev" are like "drive", and replacing them by "str" is a >> degradation in my book. > > It is, but we're suprisingly consistent in the naming of such > special-typed properties. So it's actually a good thing that > legacy_name provides redundant information. Given our overall consistency track record: yes, it's surprising, and no, I'm not comfortable relying on us being consistent :) >> Help for enum-valued properties in the form of "prop=ENUM-NAME" is not >> really helpful without a definition of ENUM-NAME. It's still useful for >> finding devices with this kind of property. > > Yes, but here you wouldn't have 'str', you would have the corresponding > QAPI enum name. This would be an improvement, though a minor one. I'd be fine with selectively dropping those legacy_name that are actually less helpful than name.
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
Il 16/09/2014 16:32, Markus Armbruster ha scritto: > Paolo Bonzini writes: > >> Il 16/09/2014 11:16, Markus Armbruster ha scritto: I think both "str" and "link" actually are a small degradation compared to "drive", and this is why I kept the legacy_name. But overall I think it's not really worth the layering violation that patches 2 and 3 are; and it's definitely not stable material. >>> >>> "str" is clearly a degradation for me. I breaks usage like >>> >>> for i in `qemu -device help 2>&1 | sed -n 's/^name "\([^"]*\)".*/\1/p'` >>> do qemu -device $i,help 2>&1 >>> done | grep =drive >>> >>> Finds all block device models. I've done such things many times. >> >> Just replace "grep =drive" with "fgrep .drive". Similarly: >> >> =chr -> .chardev (bonus: matches the command line option) >> =netdev -> .netdev >> =vlan-> .vlan >> =macaddr -> .mac > > Unlike =drive, this isn't guaranteed to work. If it doesn't, when we've been consistent so far, it's a bug. >> It is, but we're suprisingly consistent in the naming of such >> special-typed properties. So it's actually a good thing that >> legacy_name provides redundant information. > > Given our overall consistency track record: yes, it's surprising, and no, > I'm not comfortable relying on us being consistent :) The point of stuff like QOM is exactly to force us to be consistent. No, it doesn't always work. But patches 2 and 3 really are a layering violation. I have no idea how to bring back "drive" without introducing a layering violation somewhere, but this one is really too much... Paolo
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
Hi, > From: Paolo Bonzini [mailto:pbonz...@redhat.com] > Sent: Tuesday, September 16, 2014 6:46 PM > Subject: Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties > > Il 16/09/2014 12:37, Michael S. Tsirkin ha scritto: > > str really should be for free-form strings. > > It makes as much sense to call it as string > > Yes, that's why I said drive->str is a degradation. The question is, > how important is that degradation? > After reading your conversation, I'm confused and have no idea for PATCH 2 and PATCH 3. Would you give me corresponding comments? Thanks! Best regards, -Gonglei > > as it is to call an integer a string because you type > > a string of characters to specify it. > > I disagree. This is true for the command line, and for historical > reasons it is also true for device-add, but the newer QOM commands are > type-safe. You cannot pass an integer as a JSON string '123' to qom-set > or object-add, for example. >
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
On Tue, Sep 16, 2014 at 11:34:07AM +0200, Paolo Bonzini wrote: > Il 16/09/2014 11:16, Markus Armbruster ha scritto: > >> I think both "str" and "link" actually are a small > >> degradation > >> compared to "drive", and this is why I kept the legacy_name. But overall I > >> think it's not really worth the layering violation that patches 2 and 3 > >> are; > >> and it's definitely not stable material. > > > > "str" is clearly a degradation for me. I breaks usage like > > > > for i in `qemu -device help 2>&1 | sed -n 's/^name "\([^"]*\)".*/\1/p'` > > do qemu -device $i,help 2>&1 > > done | grep =drive > > > > Finds all block device models. I've done such things many times. > > Just replace "grep =drive" with "fgrep .drive". Similarly: > > =chr -> .chardev (bonus: matches the command line option) > =netdev -> .netdev > =vlan-> .vlan > =macaddr -> .mac > > We probably agree that having "=drive" work sometimes, but not always, > is the worst of both worlds. Still doesn't make it stable material, IMO. > > > Agree on the uselessness of "on/off". > > > > Agree on the uselessness of "blocksize" without a definition of the > > term. > > > > "chr" and "netdev" are like "drive", and replacing them by "str" is a > > degradation in my book. > > It is, but we're suprisingly consistent in the naming of such > special-typed properties. So it's actually a good thing that > legacy_name provides redundant information. str really should be for free-form strings. It makes as much sense to call it as string as it is to call an integer a string because you type a string of characters to specify it. > > Help for enum-valued properties in the form of "prop=ENUM-NAME" is not > > really helpful without a definition of ENUM-NAME. It's still useful for > > finding devices with this kind of property. > > Yes, but here you wouldn't have 'str', you would have the corresponding > QAPI enum name. This would be an improvement, though a minor one. > > Paolo
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
Il 16/09/2014 12:37, Michael S. Tsirkin ha scritto: > str really should be for free-form strings. > It makes as much sense to call it as string Yes, that's why I said drive->str is a degradation. The question is, how important is that degradation? > as it is to call an integer a string because you type > a string of characters to specify it. I disagree. This is true for the command line, and for historical reasons it is also true for device-add, but the newer QOM commands are type-safe. You cannot pass an integer as a JSON string '123' to qom-set or object-add, for example. Paolo
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
Il 16/09/2014 11:16, Markus Armbruster ha scritto: >> I think both "str" and "link" actually are a small degradation >> compared to "drive", and this is why I kept the legacy_name. But overall I >> think it's not really worth the layering violation that patches 2 and 3 are; >> and it's definitely not stable material. > > "str" is clearly a degradation for me. I breaks usage like > > for i in `qemu -device help 2>&1 | sed -n 's/^name "\([^"]*\)".*/\1/p'` > do qemu -device $i,help 2>&1 > done | grep =drive > > Finds all block device models. I've done such things many times. Just replace "grep =drive" with "fgrep .drive". Similarly: =chr -> .chardev (bonus: matches the command line option) =netdev -> .netdev =vlan-> .vlan =macaddr -> .mac We probably agree that having "=drive" work sometimes, but not always, is the worst of both worlds. Still doesn't make it stable material, IMO. > Agree on the uselessness of "on/off". > > Agree on the uselessness of "blocksize" without a definition of the > term. > > "chr" and "netdev" are like "drive", and replacing them by "str" is a > degradation in my book. It is, but we're suprisingly consistent in the naming of such special-typed properties. So it's actually a good thing that legacy_name provides redundant information. > Help for enum-valued properties in the form of "prop=ENUM-NAME" is not > really helpful without a definition of ENUM-NAME. It's still useful for > finding devices with this kind of property. Yes, but here you wouldn't have 'str', you would have the corresponding QAPI enum name. This would be an improvement, though a minor one. Paolo
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
Paolo Bonzini writes: > Il 16/09/2014 09:21, Markus Armbruster ha scritto: >> The rebase onto QOM renamed name to legacy_name, to free name for use as >> QOM type name (commit cafe5bd). > > Also, the QOM type name has strict rules: > > - either it is a QAPI type (primitive, enum or struct) > > - or it is link > > - or it is child > > The qdev type names had no rules. We had uint8, hex8, on/off, drive, etc. > >> Human users do, however. I'd object to a degradation of -device >> FOO,help. Changing it is fine, but it should remain at least as helpful >> as it is now. > > The question is if it is really a degradation. > > Example 1: > > virtio-blk-pci.physical_block_size=blocksize > virtio-blk-pci.logical_block_size=blocksize > > vs. > > virtio-blk-pci.physical_block_size=uint16 > virtio-blk-pci.logical_block_size=uint16 > > What is a "blocksize"? It is a power of two between 512 and 32768, but > how does the user know? If the value is too small or invalid, the > error message is particularly helpful for QEMU standards: > > qemu-system-x86_64: -device > virtio-blk-pci,physical_block_size=128,drive=hd: > Property .physical_block_size doesn't take value 128 (minimum: > 512, maximum: > 32768) > > qemu-system-x86_64: -device > virtio-blk-pci,physical_block_size=1023,drive=hd: > Property .physical_block_size doesn't take value '1023', it's not > a power of 2 > > I think uint16 is actually more informative than "blocksize". Neither is particularly user-friendly, but I grant you uint16 is less bad than blocksize in absence of definitions for these terms. > Example 2: > > virtio-blk-pci.drive=drive > > vs. > > virtio-blk-pci.drive=str > > The fact that it points to a -drive is already guessable (for anyone who > knows the relationship between -drive and -device) from the name of the > property. > > $ qemu-system-x86_64 -drive if=none,file=$HOME/test2.img,id=hd > -device virtio-blk-pci > qemu-system-x86_64: -device virtio-blk-pci: drive property not set > qemu-system-x86_64: -device virtio-blk-pci: Device initialization failed. > qemu-system-x86_64: -device virtio-blk-pci: Device > virtio-blk-pci' could not be > initialized > > $ qemu-system-x86_64 -drive if=none,file=$HOME/test2.img,id=hd > -device virtio-blk-pci,drive=ff > qemu-system-x86_64: -device virtio-blk-pci,drive=ff: Property > 'virtio-blk-device.drive' can't find value 'ff' > > If we QOMified BlockBackend, BTW, it would show up as > > virtio-blk-pci.drive=link > > I think both "str" and "link" actually are a small degradation > compared to "drive", and this is why I kept the legacy_name. But overall I > think it's not really worth the layering violation that patches 2 and 3 are; > and it's definitely not stable material. "str" is clearly a degradation for me. I breaks usage like for i in `qemu -device help 2>&1 | sed -n 's/^name "\([^"]*\)".*/\1/p'` do qemu -device $i,help 2>&1 done | grep =drive Finds all block device models. I've done such things many times. Whether "link" is a degradation or an improvement is debatable. > I'd rather drop the legacy_name at all. Here are the legacy_names currently > in use: > > hw/core/qdev-properties-system.c:.legacy_name = "drive", > hw/core/qdev-properties-system.c:.legacy_name = "chr", > hw/core/qdev-properties-system.c:.legacy_name = "netdev", > hw/core/qdev-properties-system.c:.legacy_name = "vlan", > hw/core/qdev-properties.c:.legacy_name = "on/off", > hw/core/qdev-properties.c:.legacy_name = "macaddr", > hw/core/qdev-properties.c:.legacy_name = "bios-chs-trans", > hw/core/qdev-properties.c:.legacy_name = "pci-devfn", > hw/core/qdev-properties.c:.legacy_name = "blocksize", > hw/core/qdev-properties.c:.legacy_name = "pci-host-devaddr", You missed target-ppc/translate_init.c:8047:.legacy_name = "powerpc-server-compat", which is another enum. Aside: it should really use the infrastructure for enums. > > vlan is just a glorified int, not an id like the others. chr should be > named chardev. blocksize, I already covered above. bios-chs-trans is > an enum (QAPI name BiosAtaTranslation) and is useless. on/off vs. > bool is just bikeshedding. macaddr is obviously a string, whose format > is clear from the property name. > > pci-host-devaddr and pci-devfn are the only ones that do not have an > obvious property name (respectively "host" and "addr"). Agree on the uselessness of "on/off". Agree on the uselessness of "blocksize" without a definition of the term. "chr" and "netdev" are like "drive", and replacing them by "str" is a degradation in my book. Help for enum-valued properties in the form of "prop=ENUM-NAME" is not really helpful without a definition of ENUM-NAME. It's still useful for finding devices with this kind of property. Same for structured strings like macaddr, pci-devfn, pci-host-devaddr. legacy_nam
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
Il 15/09/2014 19:49, Michael S. Tsirkin ha scritto: > On Mon, Sep 15, 2014 at 10:44:36PM +0800, arei.gong...@huawei.com wrote: >> From: Gonglei >> >> At present, people have no way to know they should >> have a specific format for alias properties. >> >> Example: >> >> before output: >> >> virtio-blk-pci.physical_block_size=uint16 >> virtio-blk-pci.logical_block_size=uint16 >> virtio-blk-pci.drive=str >> >> after output applied this patch series: >> >> virtio-blk-pci.physical_block_size=blocksize >> virtio-blk-pci.logical_block_size=blocksize >> virtio-blk-pci.drive=drive >> >> Gonglei (3): >> qom: add error handler for object alias property >> qom: add target object poniter for alias property in ObjectProperty >> qmp: print real legacy_name for alias property >> >> include/qom/object.h | 3 +++ >> qmp.c| 68 >> >> qom/object.c | 10 +++- >> 3 files changed, 59 insertions(+), 22 deletions(-) > > fixes regression in 2.1 so 2.1.2 material? Given that there's no known breakage, I don't think so. I'd rather get rid completely of the legacy_name. Paolo
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
Il 16/09/2014 09:21, Markus Armbruster ha scritto: > The rebase onto QOM renamed name to legacy_name, to free name for use as > QOM type name (commit cafe5bd). Also, the QOM type name has strict rules: - either it is a QAPI type (primitive, enum or struct) - or it is link - or it is child The qdev type names had no rules. We had uint8, hex8, on/off, drive, etc. > Human users do, however. I'd object to a degradation of -device > FOO,help. Changing it is fine, but it should remain at least as helpful > as it is now. The question is if it is really a degradation. Example 1: virtio-blk-pci.physical_block_size=blocksize virtio-blk-pci.logical_block_size=blocksize vs. virtio-blk-pci.physical_block_size=uint16 virtio-blk-pci.logical_block_size=uint16 What is a "blocksize"? It is a power of two between 512 and 32768, but how does the user know? If the value is too small or invalid, the error message is particularly helpful for QEMU standards: qemu-system-x86_64: -device virtio-blk-pci,physical_block_size=128,drive=hd: Property .physical_block_size doesn't take value 128 (minimum: 512, maximum: 32768) qemu-system-x86_64: -device virtio-blk-pci,physical_block_size=1023,drive=hd: Property .physical_block_size doesn't take value '1023', it's not a power of 2 I think uint16 is actually more informative than "blocksize". Example 2: virtio-blk-pci.drive=drive vs. virtio-blk-pci.drive=str The fact that it points to a -drive is already guessable (for anyone who knows the relationship between -drive and -device) from the name of the property. $ qemu-system-x86_64 -drive if=none,file=$HOME/test2.img,id=hd -device virtio-blk-pci qemu-system-x86_64: -device virtio-blk-pci: drive property not set qemu-system-x86_64: -device virtio-blk-pci: Device initialization failed. qemu-system-x86_64: -device virtio-blk-pci: Device 'virtio-blk-pci' could not be initialized $ qemu-system-x86_64 -drive if=none,file=$HOME/test2.img,id=hd -device virtio-blk-pci,drive=ff qemu-system-x86_64: -device virtio-blk-pci,drive=ff: Property 'virtio-blk-device.drive' can't find value 'ff' If we QOMified BlockBackend, BTW, it would show up as virtio-blk-pci.drive=link I think both "str" and "link" actually are a small degradation compared to "drive", and this is why I kept the legacy_name. But overall I think it's not really worth the layering violation that patches 2 and 3 are; and it's definitely not stable material. I'd rather drop the legacy_name at all. Here are the legacy_names currently in use: hw/core/qdev-properties-system.c:.legacy_name = "drive", hw/core/qdev-properties-system.c:.legacy_name = "chr", hw/core/qdev-properties-system.c:.legacy_name = "netdev", hw/core/qdev-properties-system.c:.legacy_name = "vlan", hw/core/qdev-properties.c:.legacy_name = "on/off", hw/core/qdev-properties.c:.legacy_name = "macaddr", hw/core/qdev-properties.c:.legacy_name = "bios-chs-trans", hw/core/qdev-properties.c:.legacy_name = "pci-devfn", hw/core/qdev-properties.c:.legacy_name = "blocksize", hw/core/qdev-properties.c:.legacy_name = "pci-host-devaddr", vlan is just a glorified int, not an id like the others. chr should be named chardev. blocksize, I already covered above. bios-chs-trans is an enum (QAPI name BiosAtaTranslation) and is useless. on/off vs. bool is just bikeshedding. macaddr is obviously a string, whose format is clear from the property name. pci-host-devaddr and pci-devfn are the only ones that do not have an obvious property name (respectively "host" and "addr"). Paolo
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
> From: Markus Armbruster [mailto:arm...@redhat.com] > Sent: Tuesday, September 16, 2014 3:21 PM > To: Michael S. Tsirkin > Subject: Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties > > "Michael S. Tsirkin" writes: > > > On Mon, Sep 15, 2014 at 05:57:51PM +0200, Paolo Bonzini wrote: > >> Il 15/09/2014 16:44, arei.gong...@huawei.com ha scritto: > >> > From: Gonglei > >> > > >> > At present, people have no way to know they should > >> > have a specific format for alias properties. > >> > > >> > Example: > >> > > >> > before output: > >> > > >> > virtio-blk-pci.physical_block_size=uint16 > >> > virtio-blk-pci.logical_block_size=uint16 > >> > virtio-blk-pci.drive=str > >> > > >> > after output applied this patch series: > >> > > >> > virtio-blk-pci.physical_block_size=blocksize > >> > virtio-blk-pci.logical_block_size=blocksize > >> > virtio-blk-pci.drive=drive > >> > >> Is there actually anyone that is relying on the legacy_names of > >> properties? I would be inclined to drop them altogether... > >> > >> Paolo > > > > What makes these legacy? > > Rebasing qdev on top of QOM produced a bit of cruft. > > PropertyInfo defines a kind of property. It used to look like this: > > struct PropertyInfo { > const char *name; > size_t size; > enum PropertyType type; > int (*parse)(DeviceState *dev, Property *prop, const char *str); > int (*print)(DeviceState *dev, Property *prop, char *dest, size_t > len); > }; > > Lets generic code parse, unparse and copy properties. > > Commit 922910c added the -device FOO,? feature, using member name to > give a hint on possible property values. > > The rebase onto QOM renamed name to legacy_name, to free name for use as > QOM type name (commit cafe5bd). > > Current code uses legacy_name as follows: > > * QMP command device-list-properties > > Returns a list of DevicePropertyInfo, whose member "type" is > PropertyInfo's legacy_name if set, else its name. > > * -device FOO,help > > Prints FOO.NAME=TYPE, where NAME is the property's name (not to be > confused with PropertyInfo's name), and TYPE is PropertyInfo's legacy > name if set, else its name. > > -device FOO,? is an outmoded way to say -device FOO,help. No plans to > drop support for it. > > The second is actually built on top of the first. > > > How do you expect users to find out about the properties? > >>qemu -device virtio-blk-pci.? > > is not the correct way anymore? > > As far as I can tell, libvirt uses both device-list-properties and > "-device,?". It doesn't appear to use the returned "type". > > Human users do, however. I'd object to a degradation of -device > FOO,help. Changing it is fine, but it should remain at least as helpful > as it is now. > > > How the time flies, it seems only yesterday this was the > > new way, -help was the old one ... > > Everything must change, so that everything can stay the same. What detailed explanation it is! Thanks. :) Best regards, -Gonglei
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
"Michael S. Tsirkin" writes: > On Mon, Sep 15, 2014 at 05:57:51PM +0200, Paolo Bonzini wrote: >> Il 15/09/2014 16:44, arei.gong...@huawei.com ha scritto: >> > From: Gonglei >> > >> > At present, people have no way to know they should >> > have a specific format for alias properties. >> > >> > Example: >> > >> > before output: >> > >> > virtio-blk-pci.physical_block_size=uint16 >> > virtio-blk-pci.logical_block_size=uint16 >> > virtio-blk-pci.drive=str >> > >> > after output applied this patch series: >> > >> > virtio-blk-pci.physical_block_size=blocksize >> > virtio-blk-pci.logical_block_size=blocksize >> > virtio-blk-pci.drive=drive >> >> Is there actually anyone that is relying on the legacy_names of >> properties? I would be inclined to drop them altogether... >> >> Paolo > > What makes these legacy? Rebasing qdev on top of QOM produced a bit of cruft. PropertyInfo defines a kind of property. It used to look like this: struct PropertyInfo { const char *name; size_t size; enum PropertyType type; int (*parse)(DeviceState *dev, Property *prop, const char *str); int (*print)(DeviceState *dev, Property *prop, char *dest, size_t len); }; Lets generic code parse, unparse and copy properties. Commit 922910c added the -device FOO,? feature, using member name to give a hint on possible property values. The rebase onto QOM renamed name to legacy_name, to free name for use as QOM type name (commit cafe5bd). Current code uses legacy_name as follows: * QMP command device-list-properties Returns a list of DevicePropertyInfo, whose member "type" is PropertyInfo's legacy_name if set, else its name. * -device FOO,help Prints FOO.NAME=TYPE, where NAME is the property's name (not to be confused with PropertyInfo's name), and TYPE is PropertyInfo's legacy name if set, else its name. -device FOO,? is an outmoded way to say -device FOO,help. No plans to drop support for it. The second is actually built on top of the first. > How do you expect users to find out about the properties? >>qemu -device virtio-blk-pci.? > is not the correct way anymore? As far as I can tell, libvirt uses both device-list-properties and "-device,?". It doesn't appear to use the returned "type". Human users do, however. I'd object to a degradation of -device FOO,help. Changing it is fine, but it should remain at least as helpful as it is now. > How the time flies, it seems only yesterday this was the > new way, -help was the old one ... Everything must change, so that everything can stay the same.
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
> From: Michael S. Tsirkin [mailto:m...@redhat.com] > Subject: Re: [PATCH 0/3] Fix confused output for alias properties > > On Mon, Sep 15, 2014 at 10:44:36PM +0800, arei.gong...@huawei.com wrote: > > From: Gonglei > > > > At present, people have no way to know they should > > have a specific format for alias properties. > > > > Example: > > > > before output: > > > > virtio-blk-pci.physical_block_size=uint16 > > virtio-blk-pci.logical_block_size=uint16 > > virtio-blk-pci.drive=str > > > > after output applied this patch series: > > > > virtio-blk-pci.physical_block_size=blocksize > > virtio-blk-pci.logical_block_size=blocksize > > virtio-blk-pci.drive=drive > > > > Gonglei (3): > > qom: add error handler for object alias property > > qom: add target object poniter for alias property in ObjectProperty > > qmp: print real legacy_name for alias property > > > > include/qom/object.h | 3 +++ > > qmp.c| 68 > > > qom/object.c | 10 +++- > > 3 files changed, 59 insertions(+), 22 deletions(-) > > fixes regression in 2.1 so 2.1.2 material? > Yes. I think it should be. :) Best regards, -Gonglei
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
On Mon, Sep 15, 2014 at 10:44:36PM +0800, arei.gong...@huawei.com wrote: > From: Gonglei > > At present, people have no way to know they should > have a specific format for alias properties. > > Example: > > before output: > > virtio-blk-pci.physical_block_size=uint16 > virtio-blk-pci.logical_block_size=uint16 > virtio-blk-pci.drive=str > > after output applied this patch series: > > virtio-blk-pci.physical_block_size=blocksize > virtio-blk-pci.logical_block_size=blocksize > virtio-blk-pci.drive=drive > > Gonglei (3): > qom: add error handler for object alias property > qom: add target object poniter for alias property in ObjectProperty > qmp: print real legacy_name for alias property > > include/qom/object.h | 3 +++ > qmp.c| 68 > > qom/object.c | 10 +++- > 3 files changed, 59 insertions(+), 22 deletions(-) OK virtio patches depend on this one, so I guess I'll have to merge this in my tree ... Andreas can you ACK that please? > -- > 1.7.12.4 >
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
On Mon, Sep 15, 2014 at 10:44:36PM +0800, arei.gong...@huawei.com wrote: > From: Gonglei > > At present, people have no way to know they should > have a specific format for alias properties. > > Example: > > before output: > > virtio-blk-pci.physical_block_size=uint16 > virtio-blk-pci.logical_block_size=uint16 > virtio-blk-pci.drive=str > > after output applied this patch series: > > virtio-blk-pci.physical_block_size=blocksize > virtio-blk-pci.logical_block_size=blocksize > virtio-blk-pci.drive=drive > > Gonglei (3): > qom: add error handler for object alias property > qom: add target object poniter for alias property in ObjectProperty > qmp: print real legacy_name for alias property > > include/qom/object.h | 3 +++ > qmp.c| 68 > > qom/object.c | 10 +++- > 3 files changed, 59 insertions(+), 22 deletions(-) fixes regression in 2.1 so 2.1.2 material? > -- > 1.7.12.4 >
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
On Mon, Sep 15, 2014 at 05:57:51PM +0200, Paolo Bonzini wrote: > Il 15/09/2014 16:44, arei.gong...@huawei.com ha scritto: > > From: Gonglei > > > > At present, people have no way to know they should > > have a specific format for alias properties. > > > > Example: > > > > before output: > > > > virtio-blk-pci.physical_block_size=uint16 > > virtio-blk-pci.logical_block_size=uint16 > > virtio-blk-pci.drive=str > > > > after output applied this patch series: > > > > virtio-blk-pci.physical_block_size=blocksize > > virtio-blk-pci.logical_block_size=blocksize > > virtio-blk-pci.drive=drive > > Is there actually anyone that is relying on the legacy_names of > properties? I would be inclined to drop them altogether... > > Paolo What makes these legacy? How do you expect users to find out about the properties? >qemu -device virtio-blk-pci.? is not the correct way anymore? How the time flies, it seems only yesterday this was the new way, -help was the old one ... -- MST
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
On 09/15/2014 09:57 AM, Paolo Bonzini wrote: > Il 15/09/2014 16:44, arei.gong...@huawei.com ha scritto: >> From: Gonglei >> >> At present, people have no way to know they should >> have a specific format for alias properties. >> >> Example: >> >> before output: >> >> virtio-blk-pci.physical_block_size=uint16 >> virtio-blk-pci.logical_block_size=uint16 >> virtio-blk-pci.drive=str >> >> after output applied this patch series: >> >> virtio-blk-pci.physical_block_size=blocksize >> virtio-blk-pci.logical_block_size=blocksize >> virtio-blk-pci.drive=drive > > Is there actually anyone that is relying on the legacy_names of > properties? I would be inclined to drop them altogether... Libvirt does not appear to be relying on legacy names, but that's not conclusive evidence in favor of removing... -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
Il 15/09/2014 16:44, arei.gong...@huawei.com ha scritto: > From: Gonglei > > At present, people have no way to know they should > have a specific format for alias properties. > > Example: > > before output: > > virtio-blk-pci.physical_block_size=uint16 > virtio-blk-pci.logical_block_size=uint16 > virtio-blk-pci.drive=str > > after output applied this patch series: > > virtio-blk-pci.physical_block_size=blocksize > virtio-blk-pci.logical_block_size=blocksize > virtio-blk-pci.drive=drive Is there actually anyone that is relying on the legacy_names of properties? I would be inclined to drop them altogether... Paolo
[Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
From: Gonglei At present, people have no way to know they should have a specific format for alias properties. Example: before output: virtio-blk-pci.physical_block_size=uint16 virtio-blk-pci.logical_block_size=uint16 virtio-blk-pci.drive=str after output applied this patch series: virtio-blk-pci.physical_block_size=blocksize virtio-blk-pci.logical_block_size=blocksize virtio-blk-pci.drive=drive Gonglei (3): qom: add error handler for object alias property qom: add target object poniter for alias property in ObjectProperty qmp: print real legacy_name for alias property include/qom/object.h | 3 +++ qmp.c| 68 qom/object.c | 10 +++- 3 files changed, 59 insertions(+), 22 deletions(-) -- 1.7.12.4