* [Qemu-devel] [PATCH 1/9] qom/user-creatable: add a few helper macros
2018-09-12 12:55 [Qemu-devel] [PATCH 0/9] hostmem-ram: use whole path for region name with >= 3.1 Marc-André Lureau
@ 2018-09-12 12:55 ` Marc-André Lureau
2018-10-22 14:33 ` Igor Mammedov
2018-09-12 12:55 ` [Qemu-devel] [PATCH 2/9] accel: register global_props like machine globals Marc-André Lureau
` (8 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Marc-André Lureau @ 2018-09-12 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum, dgilbert,
Richard Henderson, Andreas Färber, Igor Mammedov,
Michael S. Tsirkin, Artyom Tarasenko, Mark Cave-Ayland,
Marc-André Lureau
Improve a bit code readability.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
include/qom/object_interfaces.h | 4 ++++
qom/object.c | 4 ++--
qom/object_interfaces.c | 9 +++------
3 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index 4d513fb329..46b0861457 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -9,9 +9,13 @@
#define USER_CREATABLE_CLASS(klass) \
OBJECT_CLASS_CHECK(UserCreatableClass, (klass), \
TYPE_USER_CREATABLE)
+#define IS_USER_CREATABLE_CLASS(klass) \
+ object_class_dynamic_cast(OBJECT_CLASS(oc), TYPE_USER_CREATABLE)
#define USER_CREATABLE_GET_CLASS(obj) \
OBJECT_GET_CLASS(UserCreatableClass, (obj), \
TYPE_USER_CREATABLE)
+#define IS_USER_CREATABLE(obj) \
+ object_dynamic_cast(OBJECT(obj), TYPE_USER_CREATABLE)
#define USER_CREATABLE(obj) \
INTERFACE_CHECK(UserCreatable, (obj), \
TYPE_USER_CREATABLE)
diff --git a/qom/object.c b/qom/object.c
index 75d1d48944..0703e8e4ff 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -424,7 +424,7 @@ void object_initialize_childv(Object *parentobj, const char *propname,
goto out;
}
- if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
+ if (IS_USER_CREATABLE(obj)) {
user_creatable_complete(obj, &local_err);
if (local_err) {
object_unparent(obj);
@@ -605,7 +605,7 @@ Object *object_new_with_propv(const char *typename,
goto error;
}
- if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
+ if (IS_USER_CREATABLE(obj)) {
user_creatable_complete(obj, &local_err);
if (local_err) {
object_unparent(obj);
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 72b97a8bed..e3084bc04a 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -10,18 +10,15 @@
void user_creatable_complete(Object *obj, Error **errp)
{
-
UserCreatableClass *ucc;
- UserCreatable *uc =
- (UserCreatable *)object_dynamic_cast(obj, TYPE_USER_CREATABLE);
- if (!uc) {
+ if (!IS_USER_CREATABLE(obj)) {
return;
}
- ucc = USER_CREATABLE_GET_CLASS(uc);
+ ucc = USER_CREATABLE_GET_CLASS(obj);
if (ucc->complete) {
- ucc->complete(uc, errp);
+ ucc->complete(USER_CREATABLE(obj), errp);
}
}
--
2.19.0.rc1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 1/9] qom/user-creatable: add a few helper macros
2018-09-12 12:55 ` [Qemu-devel] [PATCH 1/9] qom/user-creatable: add a few helper macros Marc-André Lureau
@ 2018-10-22 14:33 ` Igor Mammedov
2018-10-26 15:13 ` Eduardo Habkost
0 siblings, 1 reply; 29+ messages in thread
From: Igor Mammedov @ 2018-10-22 14:33 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum,
dgilbert, Richard Henderson, Andreas Färber,
Michael S. Tsirkin, Artyom Tarasenko, Mark Cave-Ayland
On Wed, 12 Sep 2018 16:55:23 +0400
Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> Improve a bit code readability.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> include/qom/object_interfaces.h | 4 ++++
> qom/object.c | 4 ++--
> qom/object_interfaces.c | 9 +++------
> 3 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
> index 4d513fb329..46b0861457 100644
> --- a/include/qom/object_interfaces.h
> +++ b/include/qom/object_interfaces.h
> @@ -9,9 +9,13 @@
> #define USER_CREATABLE_CLASS(klass) \
> OBJECT_CLASS_CHECK(UserCreatableClass, (klass), \
> TYPE_USER_CREATABLE)
> +#define IS_USER_CREATABLE_CLASS(klass) \
> + object_class_dynamic_cast(OBJECT_CLASS(oc), TYPE_USER_CREATABLE)
> #define USER_CREATABLE_GET_CLASS(obj) \
> OBJECT_GET_CLASS(UserCreatableClass, (obj), \
> TYPE_USER_CREATABLE)
> +#define IS_USER_CREATABLE(obj) \
> + object_dynamic_cast(OBJECT(obj), TYPE_USER_CREATABLE)
> #define USER_CREATABLE(obj) \
> INTERFACE_CHECK(UserCreatable, (obj), \
> TYPE_USER_CREATABLE)
> diff --git a/qom/object.c b/qom/object.c
> index 75d1d48944..0703e8e4ff 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -424,7 +424,7 @@ void object_initialize_childv(Object *parentobj, const char *propname,
> goto out;
> }
>
> - if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> + if (IS_USER_CREATABLE(obj)) {
> user_creatable_complete(obj, &local_err);
> if (local_err) {
> object_unparent(obj);
> @@ -605,7 +605,7 @@ Object *object_new_with_propv(const char *typename,
> goto error;
> }
>
> - if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> + if (IS_USER_CREATABLE(obj)) {
> user_creatable_complete(obj, &local_err);
> if (local_err) {
> object_unparent(obj);
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index 72b97a8bed..e3084bc04a 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -10,18 +10,15 @@
>
> void user_creatable_complete(Object *obj, Error **errp)
> {
> -
> UserCreatableClass *ucc;
> - UserCreatable *uc =
> - (UserCreatable *)object_dynamic_cast(obj, TYPE_USER_CREATABLE);
>
> - if (!uc) {
> + if (!IS_USER_CREATABLE(obj)) {
> return;
> }
>
> - ucc = USER_CREATABLE_GET_CLASS(uc);
> + ucc = USER_CREATABLE_GET_CLASS(obj);
> if (ucc->complete) {
> - ucc->complete(uc, errp);
> + ucc->complete(USER_CREATABLE(obj), errp);
^^^
even though function becomes more concise,
this will call expensive dynamic cast 2nd time (IS_USER_CREATABLE was the 1st and discarded)
so I'm not sure is a good idea to regress startup time for readability.
> }
> }
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 1/9] qom/user-creatable: add a few helper macros
2018-10-22 14:33 ` Igor Mammedov
@ 2018-10-26 15:13 ` Eduardo Habkost
2018-10-29 9:56 ` Igor Mammedov
0 siblings, 1 reply; 29+ messages in thread
From: Eduardo Habkost @ 2018-10-26 15:13 UTC (permalink / raw)
To: Igor Mammedov
Cc: Marc-André Lureau, qemu-devel, Paolo Bonzini,
Marcel Apfelbaum, dgilbert, Richard Henderson,
Andreas Färber, Michael S. Tsirkin, Artyom Tarasenko,
Mark Cave-Ayland
On Mon, Oct 22, 2018 at 03:33:30PM +0100, Igor Mammedov wrote:
> On Wed, 12 Sep 2018 16:55:23 +0400
> Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>
> > Improve a bit code readability.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > include/qom/object_interfaces.h | 4 ++++
> > qom/object.c | 4 ++--
> > qom/object_interfaces.c | 9 +++------
> > 3 files changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
> > index 4d513fb329..46b0861457 100644
> > --- a/include/qom/object_interfaces.h
> > +++ b/include/qom/object_interfaces.h
> > @@ -9,9 +9,13 @@
> > #define USER_CREATABLE_CLASS(klass) \
> > OBJECT_CLASS_CHECK(UserCreatableClass, (klass), \
> > TYPE_USER_CREATABLE)
> > +#define IS_USER_CREATABLE_CLASS(klass) \
> > + object_class_dynamic_cast(OBJECT_CLASS(oc), TYPE_USER_CREATABLE)
> > #define USER_CREATABLE_GET_CLASS(obj) \
> > OBJECT_GET_CLASS(UserCreatableClass, (obj), \
> > TYPE_USER_CREATABLE)
> > +#define IS_USER_CREATABLE(obj) \
> > + object_dynamic_cast(OBJECT(obj), TYPE_USER_CREATABLE)
> > #define USER_CREATABLE(obj) \
> > INTERFACE_CHECK(UserCreatable, (obj), \
> > TYPE_USER_CREATABLE)
> > diff --git a/qom/object.c b/qom/object.c
> > index 75d1d48944..0703e8e4ff 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -424,7 +424,7 @@ void object_initialize_childv(Object *parentobj, const char *propname,
> > goto out;
> > }
> >
> > - if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> > + if (IS_USER_CREATABLE(obj)) {
> > user_creatable_complete(obj, &local_err);
> > if (local_err) {
> > object_unparent(obj);
> > @@ -605,7 +605,7 @@ Object *object_new_with_propv(const char *typename,
> > goto error;
> > }
> >
> > - if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> > + if (IS_USER_CREATABLE(obj)) {
> > user_creatable_complete(obj, &local_err);
> > if (local_err) {
> > object_unparent(obj);
> > diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> > index 72b97a8bed..e3084bc04a 100644
> > --- a/qom/object_interfaces.c
> > +++ b/qom/object_interfaces.c
> > @@ -10,18 +10,15 @@
> >
> > void user_creatable_complete(Object *obj, Error **errp)
> > {
> > -
> > UserCreatableClass *ucc;
> > - UserCreatable *uc =
> > - (UserCreatable *)object_dynamic_cast(obj, TYPE_USER_CREATABLE);
> >
> > - if (!uc) {
> > + if (!IS_USER_CREATABLE(obj)) {
> > return;
> > }
> >
> > - ucc = USER_CREATABLE_GET_CLASS(uc);
> > + ucc = USER_CREATABLE_GET_CLASS(obj);
> > if (ucc->complete) {
> > - ucc->complete(uc, errp);
> > + ucc->complete(USER_CREATABLE(obj), errp);
> ^^^
> even though function becomes more concise,
> this will call expensive dynamic cast 2nd time (IS_USER_CREATABLE was the 1st and discarded)
> so I'm not sure is a good idea to regress startup time for readability.
(INTERFACE_CHECK is a nop if CONFIG_QOM_CAST_DEBUG is not
enabled, so I don't understand how it would regress startup time.
--
Eduardo
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 1/9] qom/user-creatable: add a few helper macros
2018-10-26 15:13 ` Eduardo Habkost
@ 2018-10-29 9:56 ` Igor Mammedov
2018-10-30 1:37 ` Eduardo Habkost
0 siblings, 1 reply; 29+ messages in thread
From: Igor Mammedov @ 2018-10-29 9:56 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Marc-André Lureau, qemu-devel, Paolo Bonzini,
Marcel Apfelbaum, dgilbert, Richard Henderson,
Andreas Färber, Michael S. Tsirkin, Artyom Tarasenko,
Mark Cave-Ayland
On Fri, 26 Oct 2018 12:13:21 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Mon, Oct 22, 2018 at 03:33:30PM +0100, Igor Mammedov wrote:
> > On Wed, 12 Sep 2018 16:55:23 +0400
> > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> >
> > > Improve a bit code readability.
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > > include/qom/object_interfaces.h | 4 ++++
> > > qom/object.c | 4 ++--
> > > qom/object_interfaces.c | 9 +++------
> > > 3 files changed, 9 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
> > > index 4d513fb329..46b0861457 100644
> > > --- a/include/qom/object_interfaces.h
> > > +++ b/include/qom/object_interfaces.h
> > > @@ -9,9 +9,13 @@
> > > #define USER_CREATABLE_CLASS(klass) \
> > > OBJECT_CLASS_CHECK(UserCreatableClass, (klass), \
> > > TYPE_USER_CREATABLE)
> > > +#define IS_USER_CREATABLE_CLASS(klass) \
> > > + object_class_dynamic_cast(OBJECT_CLASS(oc), TYPE_USER_CREATABLE)
> > > #define USER_CREATABLE_GET_CLASS(obj) \
> > > OBJECT_GET_CLASS(UserCreatableClass, (obj), \
> > > TYPE_USER_CREATABLE)
> > > +#define IS_USER_CREATABLE(obj) \
> > > + object_dynamic_cast(OBJECT(obj), TYPE_USER_CREATABLE)
> > > #define USER_CREATABLE(obj) \
> > > INTERFACE_CHECK(UserCreatable, (obj), \
> > > TYPE_USER_CREATABLE)
> > > diff --git a/qom/object.c b/qom/object.c
> > > index 75d1d48944..0703e8e4ff 100644
> > > --- a/qom/object.c
> > > +++ b/qom/object.c
> > > @@ -424,7 +424,7 @@ void object_initialize_childv(Object *parentobj, const char *propname,
> > > goto out;
> > > }
> > >
> > > - if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> > > + if (IS_USER_CREATABLE(obj)) {
> > > user_creatable_complete(obj, &local_err);
> > > if (local_err) {
> > > object_unparent(obj);
> > > @@ -605,7 +605,7 @@ Object *object_new_with_propv(const char *typename,
> > > goto error;
> > > }
> > >
> > > - if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> > > + if (IS_USER_CREATABLE(obj)) {
> > > user_creatable_complete(obj, &local_err);
> > > if (local_err) {
> > > object_unparent(obj);
> > > diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> > > index 72b97a8bed..e3084bc04a 100644
> > > --- a/qom/object_interfaces.c
> > > +++ b/qom/object_interfaces.c
> > > @@ -10,18 +10,15 @@
> > >
> > > void user_creatable_complete(Object *obj, Error **errp)
> > > {
> > > -
> > > UserCreatableClass *ucc;
> > > - UserCreatable *uc =
> > > - (UserCreatable *)object_dynamic_cast(obj, TYPE_USER_CREATABLE);
> > >
> > > - if (!uc) {
> > > + if (!IS_USER_CREATABLE(obj)) {
> > > return;
> > > }
> > >
> > > - ucc = USER_CREATABLE_GET_CLASS(uc);
> > > + ucc = USER_CREATABLE_GET_CLASS(obj);
> > > if (ucc->complete) {
> > > - ucc->complete(uc, errp);
> > > + ucc->complete(USER_CREATABLE(obj), errp);
> > ^^^
> > even though function becomes more concise,
> > this will call expensive dynamic cast 2nd time (IS_USER_CREATABLE was the 1st and discarded)
> > so I'm not sure is a good idea to regress startup time for readability.
>
> (INTERFACE_CHECK is a nop if CONFIG_QOM_CAST_DEBUG is not
> enabled, so I don't understand how it would regress startup time.
Isn't it enabled by default though?
Maybe we should flip default to disabled then cast should be ok and enable it
when generic '--debug' is enabled.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 1/9] qom/user-creatable: add a few helper macros
2018-10-29 9:56 ` Igor Mammedov
@ 2018-10-30 1:37 ` Eduardo Habkost
2018-10-30 9:26 ` Marc-André Lureau
0 siblings, 1 reply; 29+ messages in thread
From: Eduardo Habkost @ 2018-10-30 1:37 UTC (permalink / raw)
To: Igor Mammedov
Cc: Marc-André Lureau, qemu-devel, Paolo Bonzini,
Marcel Apfelbaum, dgilbert, Richard Henderson,
Andreas Färber, Michael S. Tsirkin, Artyom Tarasenko,
Mark Cave-Ayland
On Mon, Oct 29, 2018 at 10:56:57AM +0100, Igor Mammedov wrote:
> On Fri, 26 Oct 2018 12:13:21 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > On Mon, Oct 22, 2018 at 03:33:30PM +0100, Igor Mammedov wrote:
> > > On Wed, 12 Sep 2018 16:55:23 +0400
> > > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> > >
> > > > Improve a bit code readability.
> > > >
> > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > ---
> > > > include/qom/object_interfaces.h | 4 ++++
> > > > qom/object.c | 4 ++--
> > > > qom/object_interfaces.c | 9 +++------
> > > > 3 files changed, 9 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
> > > > index 4d513fb329..46b0861457 100644
> > > > --- a/include/qom/object_interfaces.h
> > > > +++ b/include/qom/object_interfaces.h
> > > > @@ -9,9 +9,13 @@
> > > > #define USER_CREATABLE_CLASS(klass) \
> > > > OBJECT_CLASS_CHECK(UserCreatableClass, (klass), \
> > > > TYPE_USER_CREATABLE)
> > > > +#define IS_USER_CREATABLE_CLASS(klass) \
> > > > + object_class_dynamic_cast(OBJECT_CLASS(oc), TYPE_USER_CREATABLE)
> > > > #define USER_CREATABLE_GET_CLASS(obj) \
> > > > OBJECT_GET_CLASS(UserCreatableClass, (obj), \
> > > > TYPE_USER_CREATABLE)
> > > > +#define IS_USER_CREATABLE(obj) \
> > > > + object_dynamic_cast(OBJECT(obj), TYPE_USER_CREATABLE)
> > > > #define USER_CREATABLE(obj) \
> > > > INTERFACE_CHECK(UserCreatable, (obj), \
> > > > TYPE_USER_CREATABLE)
> > > > diff --git a/qom/object.c b/qom/object.c
> > > > index 75d1d48944..0703e8e4ff 100644
> > > > --- a/qom/object.c
> > > > +++ b/qom/object.c
> > > > @@ -424,7 +424,7 @@ void object_initialize_childv(Object *parentobj, const char *propname,
> > > > goto out;
> > > > }
> > > >
> > > > - if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> > > > + if (IS_USER_CREATABLE(obj)) {
> > > > user_creatable_complete(obj, &local_err);
> > > > if (local_err) {
> > > > object_unparent(obj);
> > > > @@ -605,7 +605,7 @@ Object *object_new_with_propv(const char *typename,
> > > > goto error;
> > > > }
> > > >
> > > > - if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> > > > + if (IS_USER_CREATABLE(obj)) {
> > > > user_creatable_complete(obj, &local_err);
> > > > if (local_err) {
> > > > object_unparent(obj);
> > > > diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> > > > index 72b97a8bed..e3084bc04a 100644
> > > > --- a/qom/object_interfaces.c
> > > > +++ b/qom/object_interfaces.c
> > > > @@ -10,18 +10,15 @@
> > > >
> > > > void user_creatable_complete(Object *obj, Error **errp)
> > > > {
> > > > -
> > > > UserCreatableClass *ucc;
> > > > - UserCreatable *uc =
> > > > - (UserCreatable *)object_dynamic_cast(obj, TYPE_USER_CREATABLE);
> > > >
> > > > - if (!uc) {
> > > > + if (!IS_USER_CREATABLE(obj)) {
> > > > return;
> > > > }
> > > >
> > > > - ucc = USER_CREATABLE_GET_CLASS(uc);
> > > > + ucc = USER_CREATABLE_GET_CLASS(obj);
> > > > if (ucc->complete) {
> > > > - ucc->complete(uc, errp);
> > > > + ucc->complete(USER_CREATABLE(obj), errp);
> > > ^^^
> > > even though function becomes more concise,
> > > this will call expensive dynamic cast 2nd time (IS_USER_CREATABLE was the 1st and discarded)
> > > so I'm not sure is a good idea to regress startup time for readability.
> >
> > (INTERFACE_CHECK is a nop if CONFIG_QOM_CAST_DEBUG is not
> > enabled, so I don't understand how it would regress startup time.
> Isn't it enabled by default though?
> Maybe we should flip default to disabled then cast should be ok and enable it
> when generic '--debug' is enabled.
This is called only once for each -object option. Even if QOM
debugging is enabled by default I don't see why it wouldn't be OK
to call object_dynamic_cast_assert() here.
--
Eduardo
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 1/9] qom/user-creatable: add a few helper macros
2018-10-30 1:37 ` Eduardo Habkost
@ 2018-10-30 9:26 ` Marc-André Lureau
2018-10-30 14:22 ` Igor Mammedov
0 siblings, 1 reply; 29+ messages in thread
From: Marc-André Lureau @ 2018-10-30 9:26 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Igor Mammedov, qemu-devel, Bonzini, Paolo, Marcel Apfelbaum,
Dr. David Alan Gilbert, Richard Henderson, Andreas Färber,
Michael S . Tsirkin, atar4qemu, mark.cave-ayland
Hi
On Tue, Oct 30, 2018 at 5:37 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> On Mon, Oct 29, 2018 at 10:56:57AM +0100, Igor Mammedov wrote:
> > On Fri, 26 Oct 2018 12:13:21 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> > > On Mon, Oct 22, 2018 at 03:33:30PM +0100, Igor Mammedov wrote:
> > > > On Wed, 12 Sep 2018 16:55:23 +0400
> > > > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> > > >
> > > > > Improve a bit code readability.
> > > > >
> > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > ---
> > > > > include/qom/object_interfaces.h | 4 ++++
> > > > > qom/object.c | 4 ++--
> > > > > qom/object_interfaces.c | 9 +++------
> > > > > 3 files changed, 9 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
> > > > > index 4d513fb329..46b0861457 100644
> > > > > --- a/include/qom/object_interfaces.h
> > > > > +++ b/include/qom/object_interfaces.h
> > > > > @@ -9,9 +9,13 @@
> > > > > #define USER_CREATABLE_CLASS(klass) \
> > > > > OBJECT_CLASS_CHECK(UserCreatableClass, (klass), \
> > > > > TYPE_USER_CREATABLE)
> > > > > +#define IS_USER_CREATABLE_CLASS(klass) \
> > > > > + object_class_dynamic_cast(OBJECT_CLASS(oc), TYPE_USER_CREATABLE)
> > > > > #define USER_CREATABLE_GET_CLASS(obj) \
> > > > > OBJECT_GET_CLASS(UserCreatableClass, (obj), \
> > > > > TYPE_USER_CREATABLE)
> > > > > +#define IS_USER_CREATABLE(obj) \
> > > > > + object_dynamic_cast(OBJECT(obj), TYPE_USER_CREATABLE)
> > > > > #define USER_CREATABLE(obj) \
> > > > > INTERFACE_CHECK(UserCreatable, (obj), \
> > > > > TYPE_USER_CREATABLE)
> > > > > diff --git a/qom/object.c b/qom/object.c
> > > > > index 75d1d48944..0703e8e4ff 100644
> > > > > --- a/qom/object.c
> > > > > +++ b/qom/object.c
> > > > > @@ -424,7 +424,7 @@ void object_initialize_childv(Object *parentobj, const char *propname,
> > > > > goto out;
> > > > > }
> > > > >
> > > > > - if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> > > > > + if (IS_USER_CREATABLE(obj)) {
> > > > > user_creatable_complete(obj, &local_err);
> > > > > if (local_err) {
> > > > > object_unparent(obj);
> > > > > @@ -605,7 +605,7 @@ Object *object_new_with_propv(const char *typename,
> > > > > goto error;
> > > > > }
> > > > >
> > > > > - if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> > > > > + if (IS_USER_CREATABLE(obj)) {
> > > > > user_creatable_complete(obj, &local_err);
> > > > > if (local_err) {
> > > > > object_unparent(obj);
> > > > > diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> > > > > index 72b97a8bed..e3084bc04a 100644
> > > > > --- a/qom/object_interfaces.c
> > > > > +++ b/qom/object_interfaces.c
> > > > > @@ -10,18 +10,15 @@
> > > > >
> > > > > void user_creatable_complete(Object *obj, Error **errp)
> > > > > {
> > > > > -
> > > > > UserCreatableClass *ucc;
> > > > > - UserCreatable *uc =
> > > > > - (UserCreatable *)object_dynamic_cast(obj, TYPE_USER_CREATABLE);
> > > > >
> > > > > - if (!uc) {
> > > > > + if (!IS_USER_CREATABLE(obj)) {
> > > > > return;
> > > > > }
> > > > >
> > > > > - ucc = USER_CREATABLE_GET_CLASS(uc);
> > > > > + ucc = USER_CREATABLE_GET_CLASS(obj);
> > > > > if (ucc->complete) {
> > > > > - ucc->complete(uc, errp);
> > > > > + ucc->complete(USER_CREATABLE(obj), errp);
> > > > ^^^
> > > > even though function becomes more concise,
> > > > this will call expensive dynamic cast 2nd time (IS_USER_CREATABLE was the 1st and discarded)
> > > > so I'm not sure is a good idea to regress startup time for readability.
I hope it's not measurable, unless we create billions of objects. Do
you want some figures?
> > >
> > > (INTERFACE_CHECK is a nop if CONFIG_QOM_CAST_DEBUG is not
> > > enabled, so I don't understand how it would regress startup time.
> > Isn't it enabled by default though?
> > Maybe we should flip default to disabled then cast should be ok and enable it
> > when generic '--debug' is enabled.
>
> This is called only once for each -object option. Even if QOM
> debugging is enabled by default I don't see why it wouldn't be OK
> to call object_dynamic_cast_assert() here.
Yes, imho it's very marginal. Thus I would value more readability.
Alternatively, we can consider replacing
ucc->complete(USER_CREATABLE(obj), errp); by
ucc->complete((UserCreatable*)obj, errp); to get back to the same
number of dynamic_cast calls.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 1/9] qom/user-creatable: add a few helper macros
2018-10-30 9:26 ` Marc-André Lureau
@ 2018-10-30 14:22 ` Igor Mammedov
2018-10-30 23:07 ` Eduardo Habkost
0 siblings, 1 reply; 29+ messages in thread
From: Igor Mammedov @ 2018-10-30 14:22 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Eduardo Habkost, qemu-devel, Bonzini, Paolo, Marcel Apfelbaum,
Dr. David Alan Gilbert, Richard Henderson, Andreas Färber,
Michael S . Tsirkin, atar4qemu, mark.cave-ayland
On Tue, 30 Oct 2018 13:26:40 +0400
Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> Hi
>
> On Tue, Oct 30, 2018 at 5:37 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> > On Mon, Oct 29, 2018 at 10:56:57AM +0100, Igor Mammedov wrote:
> > > On Fri, 26 Oct 2018 12:13:21 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >
> > > > On Mon, Oct 22, 2018 at 03:33:30PM +0100, Igor Mammedov wrote:
> > > > > On Wed, 12 Sep 2018 16:55:23 +0400
> > > > > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> > > > >
> > > > > > Improve a bit code readability.
> > > > > >
> > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > ---
> > > > > > include/qom/object_interfaces.h | 4 ++++
> > > > > > qom/object.c | 4 ++--
> > > > > > qom/object_interfaces.c | 9 +++------
> > > > > > 3 files changed, 9 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
> > > > > > index 4d513fb329..46b0861457 100644
> > > > > > --- a/include/qom/object_interfaces.h
> > > > > > +++ b/include/qom/object_interfaces.h
> > > > > > @@ -9,9 +9,13 @@
> > > > > > #define USER_CREATABLE_CLASS(klass) \
> > > > > > OBJECT_CLASS_CHECK(UserCreatableClass, (klass), \
> > > > > > TYPE_USER_CREATABLE)
> > > > > > +#define IS_USER_CREATABLE_CLASS(klass) \
> > > > > > + object_class_dynamic_cast(OBJECT_CLASS(oc), TYPE_USER_CREATABLE)
> > > > > > #define USER_CREATABLE_GET_CLASS(obj) \
> > > > > > OBJECT_GET_CLASS(UserCreatableClass, (obj), \
> > > > > > TYPE_USER_CREATABLE)
> > > > > > +#define IS_USER_CREATABLE(obj) \
> > > > > > + object_dynamic_cast(OBJECT(obj), TYPE_USER_CREATABLE)
> > > > > > #define USER_CREATABLE(obj) \
> > > > > > INTERFACE_CHECK(UserCreatable, (obj), \
> > > > > > TYPE_USER_CREATABLE)
> > > > > > diff --git a/qom/object.c b/qom/object.c
> > > > > > index 75d1d48944..0703e8e4ff 100644
> > > > > > --- a/qom/object.c
> > > > > > +++ b/qom/object.c
> > > > > > @@ -424,7 +424,7 @@ void object_initialize_childv(Object *parentobj, const char *propname,
> > > > > > goto out;
> > > > > > }
> > > > > >
> > > > > > - if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> > > > > > + if (IS_USER_CREATABLE(obj)) {
> > > > > > user_creatable_complete(obj, &local_err);
> > > > > > if (local_err) {
> > > > > > object_unparent(obj);
> > > > > > @@ -605,7 +605,7 @@ Object *object_new_with_propv(const char *typename,
> > > > > > goto error;
> > > > > > }
> > > > > >
> > > > > > - if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> > > > > > + if (IS_USER_CREATABLE(obj)) {
> > > > > > user_creatable_complete(obj, &local_err);
> > > > > > if (local_err) {
> > > > > > object_unparent(obj);
> > > > > > diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> > > > > > index 72b97a8bed..e3084bc04a 100644
> > > > > > --- a/qom/object_interfaces.c
> > > > > > +++ b/qom/object_interfaces.c
> > > > > > @@ -10,18 +10,15 @@
> > > > > >
> > > > > > void user_creatable_complete(Object *obj, Error **errp)
> > > > > > {
> > > > > > -
> > > > > > UserCreatableClass *ucc;
> > > > > > - UserCreatable *uc =
> > > > > > - (UserCreatable *)object_dynamic_cast(obj, TYPE_USER_CREATABLE);
> > > > > >
> > > > > > - if (!uc) {
> > > > > > + if (!IS_USER_CREATABLE(obj)) {
> > > > > > return;
> > > > > > }
> > > > > >
> > > > > > - ucc = USER_CREATABLE_GET_CLASS(uc);
> > > > > > + ucc = USER_CREATABLE_GET_CLASS(obj);
> > > > > > if (ucc->complete) {
> > > > > > - ucc->complete(uc, errp);
> > > > > > + ucc->complete(USER_CREATABLE(obj), errp);
> > > > > ^^^
> > > > > even though function becomes more concise,
> > > > > this will call expensive dynamic cast 2nd time (IS_USER_CREATABLE was the 1st and discarded)
> > > > > so I'm not sure is a good idea to regress startup time for readability.
>
> I hope it's not measurable, unless we create billions of objects. Do
> you want some figures?
I recall penalty was big enough for QEMU, that we added qom debug option,
in case of -object, the cost could be multiplied by hundreds
> > > > (INTERFACE_CHECK is a nop if CONFIG_QOM_CAST_DEBUG is not
> > > > enabled, so I don't understand how it would regress startup time.
> > > Isn't it enabled by default though?
> > > Maybe we should flip default to disabled then cast should be ok and enable it
> > > when generic '--debug' is enabled.
> >
> > This is called only once for each -object option. Even if QOM
> > debugging is enabled by default I don't see why it wouldn't be OK
> > to call object_dynamic_cast_assert() here.
>
> Yes, imho it's very marginal. Thus I would value more readability.
Probably not marginal for folks who are trying to reduce every
tiny bit of start up time.
Considering readablity change here doesn't really make a difference,
I'd pick performance. i.e. do cast and then reuse result later.
> Alternatively, we can consider replacing
> ucc->complete(USER_CREATABLE(obj), errp); by
> ucc->complete((UserCreatable*)obj, errp); to get back to the same
> number of dynamic_cast calls.
Unsafe casts (foo*) typically frowned upon but it should be safe
since one gets here after IS_USER_CREATABLE check, so I don't really
care in this case. But I'd drop user_creatable_complete() hunk,
so that original cast result is being reused.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 1/9] qom/user-creatable: add a few helper macros
2018-10-30 14:22 ` Igor Mammedov
@ 2018-10-30 23:07 ` Eduardo Habkost
2018-11-01 12:16 ` Igor Mammedov
0 siblings, 1 reply; 29+ messages in thread
From: Eduardo Habkost @ 2018-10-30 23:07 UTC (permalink / raw)
To: Igor Mammedov
Cc: Marc-André Lureau, qemu-devel, Bonzini, Paolo,
Marcel Apfelbaum, Dr. David Alan Gilbert, Richard Henderson,
Andreas Färber, Michael S . Tsirkin, atar4qemu,
mark.cave-ayland
On Tue, Oct 30, 2018 at 03:22:43PM +0100, Igor Mammedov wrote:
> On Tue, 30 Oct 2018 13:26:40 +0400
> Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>
> > Hi
> >
> > On Tue, Oct 30, 2018 at 5:37 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >
> > > On Mon, Oct 29, 2018 at 10:56:57AM +0100, Igor Mammedov wrote:
> > > > On Fri, 26 Oct 2018 12:13:21 -0300
> > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > >
> > > > > On Mon, Oct 22, 2018 at 03:33:30PM +0100, Igor Mammedov wrote:
> > > > > > On Wed, 12 Sep 2018 16:55:23 +0400
> > > > > > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> > > > > >
> > > > > > > Improve a bit code readability.
> > > > > > >
> > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > > ---
> > > > > > > include/qom/object_interfaces.h | 4 ++++
> > > > > > > qom/object.c | 4 ++--
> > > > > > > qom/object_interfaces.c | 9 +++------
> > > > > > > 3 files changed, 9 insertions(+), 8 deletions(-)
> > > > > > >
> > > > > > > diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
> > > > > > > index 4d513fb329..46b0861457 100644
> > > > > > > --- a/include/qom/object_interfaces.h
> > > > > > > +++ b/include/qom/object_interfaces.h
> > > > > > > @@ -9,9 +9,13 @@
> > > > > > > #define USER_CREATABLE_CLASS(klass) \
> > > > > > > OBJECT_CLASS_CHECK(UserCreatableClass, (klass), \
> > > > > > > TYPE_USER_CREATABLE)
> > > > > > > +#define IS_USER_CREATABLE_CLASS(klass) \
> > > > > > > + object_class_dynamic_cast(OBJECT_CLASS(oc), TYPE_USER_CREATABLE)
> > > > > > > #define USER_CREATABLE_GET_CLASS(obj) \
> > > > > > > OBJECT_GET_CLASS(UserCreatableClass, (obj), \
> > > > > > > TYPE_USER_CREATABLE)
> > > > > > > +#define IS_USER_CREATABLE(obj) \
> > > > > > > + object_dynamic_cast(OBJECT(obj), TYPE_USER_CREATABLE)
> > > > > > > #define USER_CREATABLE(obj) \
> > > > > > > INTERFACE_CHECK(UserCreatable, (obj), \
> > > > > > > TYPE_USER_CREATABLE)
> > > > > > > diff --git a/qom/object.c b/qom/object.c
> > > > > > > index 75d1d48944..0703e8e4ff 100644
> > > > > > > --- a/qom/object.c
> > > > > > > +++ b/qom/object.c
> > > > > > > @@ -424,7 +424,7 @@ void object_initialize_childv(Object *parentobj, const char *propname,
> > > > > > > goto out;
> > > > > > > }
> > > > > > >
> > > > > > > - if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> > > > > > > + if (IS_USER_CREATABLE(obj)) {
> > > > > > > user_creatable_complete(obj, &local_err);
> > > > > > > if (local_err) {
> > > > > > > object_unparent(obj);
> > > > > > > @@ -605,7 +605,7 @@ Object *object_new_with_propv(const char *typename,
> > > > > > > goto error;
> > > > > > > }
> > > > > > >
> > > > > > > - if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> > > > > > > + if (IS_USER_CREATABLE(obj)) {
> > > > > > > user_creatable_complete(obj, &local_err);
> > > > > > > if (local_err) {
> > > > > > > object_unparent(obj);
> > > > > > > diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> > > > > > > index 72b97a8bed..e3084bc04a 100644
> > > > > > > --- a/qom/object_interfaces.c
> > > > > > > +++ b/qom/object_interfaces.c
> > > > > > > @@ -10,18 +10,15 @@
> > > > > > >
> > > > > > > void user_creatable_complete(Object *obj, Error **errp)
> > > > > > > {
> > > > > > > -
> > > > > > > UserCreatableClass *ucc;
> > > > > > > - UserCreatable *uc =
> > > > > > > - (UserCreatable *)object_dynamic_cast(obj, TYPE_USER_CREATABLE);
> > > > > > >
> > > > > > > - if (!uc) {
> > > > > > > + if (!IS_USER_CREATABLE(obj)) {
> > > > > > > return;
> > > > > > > }
> > > > > > >
> > > > > > > - ucc = USER_CREATABLE_GET_CLASS(uc);
> > > > > > > + ucc = USER_CREATABLE_GET_CLASS(obj);
> > > > > > > if (ucc->complete) {
> > > > > > > - ucc->complete(uc, errp);
> > > > > > > + ucc->complete(USER_CREATABLE(obj), errp);
> > > > > > ^^^
> > > > > > even though function becomes more concise,
> > > > > > this will call expensive dynamic cast 2nd time (IS_USER_CREATABLE was the 1st and discarded)
> > > > > > so I'm not sure is a good idea to regress startup time for readability.
> >
> > I hope it's not measurable, unless we create billions of objects. Do
> > you want some figures?
> I recall penalty was big enough for QEMU, that we added qom debug option,
> in case of -object, the cost could be multiplied by hundreds
This sounds like premature optimization. Did you measure how
many nanoseconds we're saving per -object option?
> > > > > (INTERFACE_CHECK is a nop if CONFIG_QOM_CAST_DEBUG is not
> > > > > enabled, so I don't understand how it would regress startup time.
> > > > Isn't it enabled by default though?
> > > > Maybe we should flip default to disabled then cast should be ok and enable it
> > > > when generic '--debug' is enabled.
> > >
> > > This is called only once for each -object option. Even if QOM
> > > debugging is enabled by default I don't see why it wouldn't be OK
> > > to call object_dynamic_cast_assert() here.
> >
> > Yes, imho it's very marginal. Thus I would value more readability.
> Probably not marginal for folks who are trying to reduce every
> tiny bit of start up time.
These people should be disabling QOM debugging, then.
>
> Considering readablity change here doesn't really make a difference,
> I'd pick performance. i.e. do cast and then reuse result later.
>
> > Alternatively, we can consider replacing
> > ucc->complete(USER_CREATABLE(obj), errp); by
> > ucc->complete((UserCreatable*)obj, errp); to get back to the same
> > number of dynamic_cast calls.
> Unsafe casts (foo*) typically frowned upon but it should be safe
> since one gets here after IS_USER_CREATABLE check, so I don't really
> care in this case. But I'd drop user_creatable_complete() hunk,
> so that original cast result is being reused.
Please don't do that. Readability, safety, and consistency are
more important. For the few users where QOM debugging has a
measurable performance impact, we already have a mechanism to
disable it.
--
Eduardo
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 1/9] qom/user-creatable: add a few helper macros
2018-10-30 23:07 ` Eduardo Habkost
@ 2018-11-01 12:16 ` Igor Mammedov
2018-11-01 15:02 ` Eduardo Habkost
0 siblings, 1 reply; 29+ messages in thread
From: Igor Mammedov @ 2018-11-01 12:16 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Marc-André Lureau, qemu-devel, Bonzini, Paolo,
Marcel Apfelbaum, Dr. David Alan Gilbert, Richard Henderson,
Andreas Färber, Michael S . Tsirkin, atar4qemu,
mark.cave-ayland
On Tue, 30 Oct 2018 20:07:17 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Tue, Oct 30, 2018 at 03:22:43PM +0100, Igor Mammedov wrote:
> > On Tue, 30 Oct 2018 13:26:40 +0400
> > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> >
> > > Hi
> > >
> > > On Tue, Oct 30, 2018 at 5:37 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > >
> > > > On Mon, Oct 29, 2018 at 10:56:57AM +0100, Igor Mammedov wrote:
> > > > > On Fri, 26 Oct 2018 12:13:21 -0300
> > > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > >
> > > > > > On Mon, Oct 22, 2018 at 03:33:30PM +0100, Igor Mammedov wrote:
> > > > > > > On Wed, 12 Sep 2018 16:55:23 +0400
> > > > > > > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> > > > > > >
> > > > > > > > Improve a bit code readability.
> > > > > > > >
> > > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > > > ---
> > > > > > > > include/qom/object_interfaces.h | 4 ++++
> > > > > > > > qom/object.c | 4 ++--
> > > > > > > > qom/object_interfaces.c | 9 +++------
> > > > > > > > 3 files changed, 9 insertions(+), 8 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
> > > > > > > > index 4d513fb329..46b0861457 100644
> > > > > > > > --- a/include/qom/object_interfaces.h
> > > > > > > > +++ b/include/qom/object_interfaces.h
> > > > > > > > @@ -9,9 +9,13 @@
> > > > > > > > #define USER_CREATABLE_CLASS(klass) \
> > > > > > > > OBJECT_CLASS_CHECK(UserCreatableClass, (klass), \
> > > > > > > > TYPE_USER_CREATABLE)
> > > > > > > > +#define IS_USER_CREATABLE_CLASS(klass) \
> > > > > > > > + object_class_dynamic_cast(OBJECT_CLASS(oc), TYPE_USER_CREATABLE)
> > > > > > > > #define USER_CREATABLE_GET_CLASS(obj) \
> > > > > > > > OBJECT_GET_CLASS(UserCreatableClass, (obj), \
> > > > > > > > TYPE_USER_CREATABLE)
> > > > > > > > +#define IS_USER_CREATABLE(obj) \
> > > > > > > > + object_dynamic_cast(OBJECT(obj), TYPE_USER_CREATABLE)
> > > > > > > > #define USER_CREATABLE(obj) \
> > > > > > > > INTERFACE_CHECK(UserCreatable, (obj), \
> > > > > > > > TYPE_USER_CREATABLE)
> > > > > > > > diff --git a/qom/object.c b/qom/object.c
> > > > > > > > index 75d1d48944..0703e8e4ff 100644
> > > > > > > > --- a/qom/object.c
> > > > > > > > +++ b/qom/object.c
> > > > > > > > @@ -424,7 +424,7 @@ void object_initialize_childv(Object *parentobj, const char *propname,
> > > > > > > > goto out;
> > > > > > > > }
> > > > > > > >
> > > > > > > > - if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> > > > > > > > + if (IS_USER_CREATABLE(obj)) {
> > > > > > > > user_creatable_complete(obj, &local_err);
> > > > > > > > if (local_err) {
> > > > > > > > object_unparent(obj);
> > > > > > > > @@ -605,7 +605,7 @@ Object *object_new_with_propv(const char *typename,
> > > > > > > > goto error;
> > > > > > > > }
> > > > > > > >
> > > > > > > > - if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> > > > > > > > + if (IS_USER_CREATABLE(obj)) {
> > > > > > > > user_creatable_complete(obj, &local_err);
> > > > > > > > if (local_err) {
> > > > > > > > object_unparent(obj);
> > > > > > > > diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> > > > > > > > index 72b97a8bed..e3084bc04a 100644
> > > > > > > > --- a/qom/object_interfaces.c
> > > > > > > > +++ b/qom/object_interfaces.c
> > > > > > > > @@ -10,18 +10,15 @@
> > > > > > > >
> > > > > > > > void user_creatable_complete(Object *obj, Error **errp)
> > > > > > > > {
> > > > > > > > -
> > > > > > > > UserCreatableClass *ucc;
> > > > > > > > - UserCreatable *uc =
> > > > > > > > - (UserCreatable *)object_dynamic_cast(obj, TYPE_USER_CREATABLE);
> > > > > > > >
> > > > > > > > - if (!uc) {
> > > > > > > > + if (!IS_USER_CREATABLE(obj)) {
> > > > > > > > return;
> > > > > > > > }
> > > > > > > >
> > > > > > > > - ucc = USER_CREATABLE_GET_CLASS(uc);
> > > > > > > > + ucc = USER_CREATABLE_GET_CLASS(obj);
> > > > > > > > if (ucc->complete) {
> > > > > > > > - ucc->complete(uc, errp);
> > > > > > > > + ucc->complete(USER_CREATABLE(obj), errp);
> > > > > > > ^^^
> > > > > > > even though function becomes more concise,
> > > > > > > this will call expensive dynamic cast 2nd time (IS_USER_CREATABLE was the 1st and discarded)
> > > > > > > so I'm not sure is a good idea to regress startup time for readability.
> > >
> > > I hope it's not measurable, unless we create billions of objects. Do
> > > you want some figures?
> > I recall penalty was big enough for QEMU, that we added qom debug option,
> > in case of -object, the cost could be multiplied by hundreds
>
> This sounds like premature optimization. Did you measure how
> many nanoseconds we're saving per -object option?
it's not optimization, it's about not making code worse and using
resources reasonably object_dynamic_cast() with IS_USER_CREATABLE().
I'd replace object_dynamic_cast() with IS_FOO() in places where
we just test but do not use the result for a better readability.
But is we need the result later I'd keep object_dynamic_cast(),
it's not much worse but allows us to reuse the result of cast.
> > > > > > (INTERFACE_CHECK is a nop if CONFIG_QOM_CAST_DEBUG is not
> > > > > > enabled, so I don't understand how it would regress startup time.
> > > > > Isn't it enabled by default though?
> > > > > Maybe we should flip default to disabled then cast should be ok and enable it
> > > > > when generic '--debug' is enabled.
> > > >
> > > > This is called only once for each -object option. Even if QOM
> > > > debugging is enabled by default I don't see why it wouldn't be OK
> > > > to call object_dynamic_cast_assert() here.
> > >
> > > Yes, imho it's very marginal. Thus I would value more readability.
> > Probably not marginal for folks who are trying to reduce every
> > tiny bit of start up time.
>
> These people should be disabling QOM debugging, then.
>
> >
> > Considering readablity change here doesn't really make a difference,
> > I'd pick performance. i.e. do cast and then reuse result later.
> >
> > > Alternatively, we can consider replacing
> > > ucc->complete(USER_CREATABLE(obj), errp); by
> > > ucc->complete((UserCreatable*)obj, errp); to get back to the same
> > > number of dynamic_cast calls.
> > Unsafe casts (foo*) typically frowned upon but it should be safe
> > since one gets here after IS_USER_CREATABLE check, so I don't really
> > care in this case. But I'd drop user_creatable_complete() hunk,
> > so that original cast result is being reused.
>
> Please don't do that. Readability, safety, and consistency are
> more important. For the few users where QOM debugging has a
> measurable performance impact, we already have a mechanism to
> disable it.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 1/9] qom/user-creatable: add a few helper macros
2018-11-01 12:16 ` Igor Mammedov
@ 2018-11-01 15:02 ` Eduardo Habkost
2018-11-01 15:46 ` Igor Mammedov
0 siblings, 1 reply; 29+ messages in thread
From: Eduardo Habkost @ 2018-11-01 15:02 UTC (permalink / raw)
To: Igor Mammedov
Cc: Marc-André Lureau, qemu-devel, Bonzini, Paolo,
Marcel Apfelbaum, Dr. David Alan Gilbert, Richard Henderson,
Andreas Färber, Michael S . Tsirkin, atar4qemu,
mark.cave-ayland
On Thu, Nov 01, 2018 at 01:16:37PM +0100, Igor Mammedov wrote:
> On Tue, 30 Oct 2018 20:07:17 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > On Tue, Oct 30, 2018 at 03:22:43PM +0100, Igor Mammedov wrote:
> > > On Tue, 30 Oct 2018 13:26:40 +0400
> > > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> > >
> > > > Hi
> > > >
> > > > On Tue, Oct 30, 2018 at 5:37 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > >
> > > > > On Mon, Oct 29, 2018 at 10:56:57AM +0100, Igor Mammedov wrote:
> > > > > > On Fri, 26 Oct 2018 12:13:21 -0300
> > > > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > > >
> > > > > > > On Mon, Oct 22, 2018 at 03:33:30PM +0100, Igor Mammedov wrote:
> > > > > > > > On Wed, 12 Sep 2018 16:55:23 +0400
> > > > > > > > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
[...]
> > > > > > > > > void user_creatable_complete(Object *obj, Error **errp)
> > > > > > > > > {
> > > > > > > > > -
> > > > > > > > > UserCreatableClass *ucc;
> > > > > > > > > - UserCreatable *uc =
> > > > > > > > > - (UserCreatable *)object_dynamic_cast(obj, TYPE_USER_CREATABLE);
> > > > > > > > >
> > > > > > > > > - if (!uc) {
> > > > > > > > > + if (!IS_USER_CREATABLE(obj)) {
> > > > > > > > > return;
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > - ucc = USER_CREATABLE_GET_CLASS(uc);
> > > > > > > > > + ucc = USER_CREATABLE_GET_CLASS(obj);
> > > > > > > > > if (ucc->complete) {
> > > > > > > > > - ucc->complete(uc, errp);
> > > > > > > > > + ucc->complete(USER_CREATABLE(obj), errp);
> > > > > > > > ^^^
> > > > > > > > even though function becomes more concise,
> > > > > > > > this will call expensive dynamic cast 2nd time (IS_USER_CREATABLE was the 1st and discarded)
> > > > > > > > so I'm not sure is a good idea to regress startup time for readability.
> > > >
> > > > I hope it's not measurable, unless we create billions of objects. Do
> > > > you want some figures?
> > > I recall penalty was big enough for QEMU, that we added qom debug option,
> > > in case of -object, the cost could be multiplied by hundreds
> >
> > This sounds like premature optimization. Did you measure how
> > many nanoseconds we're saving per -object option?
> it's not optimization, it's about not making code worse and using
> resources reasonably object_dynamic_cast() with IS_USER_CREATABLE().
How exactly is the code worse?
>
> I'd replace object_dynamic_cast() with IS_FOO() in places where
> we just test but do not use the result for a better readability.
> But is we need the result later I'd keep object_dynamic_cast(),
> it's not much worse but allows us to reuse the result of cast.
Readability is orders of magnitude more important to me.
This is not readable:
(UserCreatable *)object_dynamic_cast(obj, TYPE_USER_CREATABLE)
This is readable:
USER_CREATABLE(obj)
If the overhead of the extra object_dynamic_cast() call is too
much, it's already possible to disable QOM cast debugging.
> [...]
--
Eduardo
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 1/9] qom/user-creatable: add a few helper macros
2018-11-01 15:02 ` Eduardo Habkost
@ 2018-11-01 15:46 ` Igor Mammedov
0 siblings, 0 replies; 29+ messages in thread
From: Igor Mammedov @ 2018-11-01 15:46 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Marc-André Lureau, qemu-devel, Bonzini, Paolo,
Marcel Apfelbaum, Dr. David Alan Gilbert, Richard Henderson,
Andreas Färber, Michael S . Tsirkin, atar4qemu,
mark.cave-ayland
On Thu, 1 Nov 2018 12:02:03 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Thu, Nov 01, 2018 at 01:16:37PM +0100, Igor Mammedov wrote:
> > On Tue, 30 Oct 2018 20:07:17 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> > > On Tue, Oct 30, 2018 at 03:22:43PM +0100, Igor Mammedov wrote:
> > > > On Tue, 30 Oct 2018 13:26:40 +0400
> > > > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> > > >
> > > > > Hi
> > > > >
> > > > > On Tue, Oct 30, 2018 at 5:37 AM Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > > >
> > > > > > On Mon, Oct 29, 2018 at 10:56:57AM +0100, Igor Mammedov wrote:
> > > > > > > On Fri, 26 Oct 2018 12:13:21 -0300
> > > > > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > > > >
> > > > > > > > On Mon, Oct 22, 2018 at 03:33:30PM +0100, Igor Mammedov wrote:
> > > > > > > > > On Wed, 12 Sep 2018 16:55:23 +0400
> > > > > > > > > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> [...]
> > > > > > > > > > void user_creatable_complete(Object *obj, Error **errp)
> > > > > > > > > > {
> > > > > > > > > > -
> > > > > > > > > > UserCreatableClass *ucc;
> > > > > > > > > > - UserCreatable *uc =
> > > > > > > > > > - (UserCreatable *)object_dynamic_cast(obj, TYPE_USER_CREATABLE);
> > > > > > > > > >
> > > > > > > > > > - if (!uc) {
> > > > > > > > > > + if (!IS_USER_CREATABLE(obj)) {
> > > > > > > > > > return;
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > - ucc = USER_CREATABLE_GET_CLASS(uc);
> > > > > > > > > > + ucc = USER_CREATABLE_GET_CLASS(obj);
> > > > > > > > > > if (ucc->complete) {
> > > > > > > > > > - ucc->complete(uc, errp);
> > > > > > > > > > + ucc->complete(USER_CREATABLE(obj), errp);
> > > > > > > > > ^^^
> > > > > > > > > even though function becomes more concise,
> > > > > > > > > this will call expensive dynamic cast 2nd time (IS_USER_CREATABLE was the 1st and discarded)
> > > > > > > > > so I'm not sure is a good idea to regress startup time for readability.
> > > > >
> > > > > I hope it's not measurable, unless we create billions of objects. Do
> > > > > you want some figures?
> > > > I recall penalty was big enough for QEMU, that we added qom debug option,
> > > > in case of -object, the cost could be multiplied by hundreds
> > >
> > > This sounds like premature optimization. Did you measure how
> > > many nanoseconds we're saving per -object option?
> > it's not optimization, it's about not making code worse and using
> > resources reasonably object_dynamic_cast() with IS_USER_CREATABLE().
>
> How exactly is the code worse?
making it slower /doesn't matter how much/ when there is better way
to do it.
Anyways the point is moot as Mark rewrote it in another way (better)
so this question won't arise.
> >
> > I'd replace object_dynamic_cast() with IS_FOO() in places where
> > we just test but do not use the result for a better readability.
> > But is we need the result later I'd keep object_dynamic_cast(),
> > it's not much worse but allows us to reuse the result of cast.
>
> Readability is orders of magnitude more important to me.
>
> This is not readable:
> (UserCreatable *)object_dynamic_cast(obj, TYPE_USER_CREATABLE)
>
> This is readable:
> USER_CREATABLE(obj)
>
> If the overhead of the extra object_dynamic_cast() call is too
> much, it's already possible to disable QOM cast debugging.
I'm typically on the side of simplifying code and readability
but in this case I have to disagree, out of context ^^^ it might
look fine but with full context
uc = (UserCreatable *)object_dynamic_cast(obj, TYPE_USER_CREATABLE)
if(uc)
call(uc)
vs
if(IS_USER_CREATABLE(obj))
// discard above hidden cast result and do it again
call(USER_CREATABLE(obj))
the former is not horrible enough to warrant doing senseless discard.
So lets agree to disagree :)
>
> > [...]
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 2/9] accel: register global_props like machine globals
2018-09-12 12:55 [Qemu-devel] [PATCH 0/9] hostmem-ram: use whole path for region name with >= 3.1 Marc-André Lureau
2018-09-12 12:55 ` [Qemu-devel] [PATCH 1/9] qom/user-creatable: add a few helper macros Marc-André Lureau
@ 2018-09-12 12:55 ` Marc-André Lureau
2018-10-22 14:47 ` Igor Mammedov
2018-09-12 12:55 ` [Qemu-devel] [PATCH 3/9] qdev: move qdev_prop_register_global_list() to tests Marc-André Lureau
` (7 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Marc-André Lureau @ 2018-09-12 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum, dgilbert,
Richard Henderson, Andreas Färber, Igor Mammedov,
Michael S. Tsirkin, Artyom Tarasenko, Mark Cave-Ayland,
Marc-André Lureau
global_props is only used for Xen xen_compat_props. It's a static
array of GlobalProperty, like machine globals in SET_MACHINE_COMPAT().
Let's register the globals the same way, without extra copy allocation.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
include/hw/qdev-properties.h | 29 -----------------------------
accel/accel.c | 9 ++++++++-
hw/core/qdev-properties.c | 21 ---------------------
3 files changed, 8 insertions(+), 51 deletions(-)
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 4f60cc88f3..a95f4a73eb 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -255,35 +255,6 @@ void qdev_prop_set_globals(DeviceState *dev);
void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
Property *prop, const char *value);
-/**
- * register_compat_prop:
- *
- * Register internal (not user-provided) global property, changing the
- * default value of a given property in a device type. This can be used
- * for enabling machine-type compatibility or for enabling
- * accelerator-specific defaults in devices.
- *
- * The property values set using this function must be always valid and
- * never report setter errors, as the property will have
- * GlobalProperty::errp set to &error_abort.
- *
- * User-provided global properties should override internal global
- * properties, so callers of this function should ensure that it is
- * called before user-provided global properties are registered.
- *
- * @driver: Device type to be affected
- * @property: Property whose default value is going to be changed
- * @value: New default value for the property
- */
-void register_compat_prop(const char *driver, const char *property,
- const char *value);
-/*
- * register_compat_props_array(): using register_compat_prop(), which
- * only registers internal global properties (which has lower priority
- * than user-provided global properties)
- */
-void register_compat_props_array(GlobalProperty *prop);
-
/**
* qdev_property_add_static:
* @dev: Device to add the property to.
diff --git a/accel/accel.c b/accel/accel.c
index 966b2d8f53..3da26eb90f 100644
--- a/accel/accel.c
+++ b/accel/accel.c
@@ -34,6 +34,7 @@
#include "qom/object.h"
#include "qemu/error-report.h"
#include "qemu/option.h"
+#include "qapi/error.h"
static const TypeInfo accel_type = {
.name = TYPE_ACCEL,
@@ -121,7 +122,13 @@ void configure_accelerator(MachineState *ms)
void accel_register_compat_props(AccelState *accel)
{
AccelClass *class = ACCEL_GET_CLASS(accel);
- register_compat_props_array(class->global_props);
+ GlobalProperty *prop = class->global_props;
+
+ for (; prop && prop->driver; prop++) {
+ /* Any compat_props must never cause error */
+ prop->errp = &error_abort;
+ qdev_prop_register_global(prop);
+ }
}
void accel_setup_post(MachineState *ms)
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 35072dec1e..ab61d502fd 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1180,27 +1180,6 @@ void qdev_prop_register_global(GlobalProperty *prop)
global_props = g_list_append(global_props, prop);
}
-void register_compat_prop(const char *driver,
- const char *property,
- const char *value)
-{
- GlobalProperty *p = g_new0(GlobalProperty, 1);
-
- /* Any compat_props must never cause error */
- p->errp = &error_abort;
- p->driver = driver;
- p->property = property;
- p->value = value;
- qdev_prop_register_global(p);
-}
-
-void register_compat_props_array(GlobalProperty *prop)
-{
- for (; prop && prop->driver; prop++) {
- register_compat_prop(prop->driver, prop->property, prop->value);
- }
-}
-
void qdev_prop_register_global_list(GlobalProperty *props)
{
int i;
--
2.19.0.rc1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 2/9] accel: register global_props like machine globals
2018-09-12 12:55 ` [Qemu-devel] [PATCH 2/9] accel: register global_props like machine globals Marc-André Lureau
@ 2018-10-22 14:47 ` Igor Mammedov
0 siblings, 0 replies; 29+ messages in thread
From: Igor Mammedov @ 2018-10-22 14:47 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, sstabellini, Eduardo Habkost, xen-devel,
Richard Henderson
On Wed, 12 Sep 2018 16:55:24 +0400
Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> global_props is only used for Xen xen_compat_props. It's a static
minor nit:
should be AccelClass::global_props
> array of GlobalProperty, like machine globals in SET_MACHINE_COMPAT().
> Let's register the globals the same way, without extra copy allocation.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
otherwise looks good to me, CCing xen folks since it concerns them.
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
> include/hw/qdev-properties.h | 29 -----------------------------
> accel/accel.c | 9 ++++++++-
> hw/core/qdev-properties.c | 21 ---------------------
> 3 files changed, 8 insertions(+), 51 deletions(-)
>
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 4f60cc88f3..a95f4a73eb 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -255,35 +255,6 @@ void qdev_prop_set_globals(DeviceState *dev);
> void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
> Property *prop, const char *value);
>
> -/**
> - * register_compat_prop:
> - *
> - * Register internal (not user-provided) global property, changing the
> - * default value of a given property in a device type. This can be used
> - * for enabling machine-type compatibility or for enabling
> - * accelerator-specific defaults in devices.
> - *
> - * The property values set using this function must be always valid and
> - * never report setter errors, as the property will have
> - * GlobalProperty::errp set to &error_abort.
> - *
> - * User-provided global properties should override internal global
> - * properties, so callers of this function should ensure that it is
> - * called before user-provided global properties are registered.
> - *
> - * @driver: Device type to be affected
> - * @property: Property whose default value is going to be changed
> - * @value: New default value for the property
> - */
> -void register_compat_prop(const char *driver, const char *property,
> - const char *value);
> -/*
> - * register_compat_props_array(): using register_compat_prop(), which
> - * only registers internal global properties (which has lower priority
> - * than user-provided global properties)
> - */
> -void register_compat_props_array(GlobalProperty *prop);
> -
> /**
> * qdev_property_add_static:
> * @dev: Device to add the property to.
> diff --git a/accel/accel.c b/accel/accel.c
> index 966b2d8f53..3da26eb90f 100644
> --- a/accel/accel.c
> +++ b/accel/accel.c
> @@ -34,6 +34,7 @@
> #include "qom/object.h"
> #include "qemu/error-report.h"
> #include "qemu/option.h"
> +#include "qapi/error.h"
>
> static const TypeInfo accel_type = {
> .name = TYPE_ACCEL,
> @@ -121,7 +122,13 @@ void configure_accelerator(MachineState *ms)
> void accel_register_compat_props(AccelState *accel)
> {
> AccelClass *class = ACCEL_GET_CLASS(accel);
> - register_compat_props_array(class->global_props);
> + GlobalProperty *prop = class->global_props;
> +
> + for (; prop && prop->driver; prop++) {
> + /* Any compat_props must never cause error */
> + prop->errp = &error_abort;
> + qdev_prop_register_global(prop);
> + }
> }
>
> void accel_setup_post(MachineState *ms)
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 35072dec1e..ab61d502fd 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1180,27 +1180,6 @@ void qdev_prop_register_global(GlobalProperty *prop)
> global_props = g_list_append(global_props, prop);
> }
>
> -void register_compat_prop(const char *driver,
> - const char *property,
> - const char *value)
> -{
> - GlobalProperty *p = g_new0(GlobalProperty, 1);
> -
> - /* Any compat_props must never cause error */
> - p->errp = &error_abort;
> - p->driver = driver;
> - p->property = property;
> - p->value = value;
> - qdev_prop_register_global(p);
> -}
> -
> -void register_compat_props_array(GlobalProperty *prop)
> -{
> - for (; prop && prop->driver; prop++) {
> - register_compat_prop(prop->driver, prop->property, prop->value);
> - }
> -}
> -
> void qdev_prop_register_global_list(GlobalProperty *props)
> {
> int i;
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 3/9] qdev: move qdev_prop_register_global_list() to tests
2018-09-12 12:55 [Qemu-devel] [PATCH 0/9] hostmem-ram: use whole path for region name with >= 3.1 Marc-André Lureau
2018-09-12 12:55 ` [Qemu-devel] [PATCH 1/9] qom/user-creatable: add a few helper macros Marc-André Lureau
2018-09-12 12:55 ` [Qemu-devel] [PATCH 2/9] accel: register global_props like machine globals Marc-André Lureau
@ 2018-09-12 12:55 ` Marc-André Lureau
2018-10-22 14:51 ` Igor Mammedov
2018-09-12 12:55 ` [Qemu-devel] [PATCH 4/9] qom/globals: move qdev globals to qom Marc-André Lureau
` (6 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Marc-André Lureau @ 2018-09-12 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum, dgilbert,
Richard Henderson, Andreas Färber, Igor Mammedov,
Michael S. Tsirkin, Artyom Tarasenko, Mark Cave-Ayland,
Marc-André Lureau
The function is only used by a test, move it there.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
include/hw/qdev-properties.h | 1 -
hw/core/qdev-properties.c | 9 ---------
tests/test-qdev-global-props.c | 18 ++++++++++++++----
3 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index a95f4a73eb..3ab9cd2eb6 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -249,7 +249,6 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value);
void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
void qdev_prop_register_global(GlobalProperty *prop);
-void qdev_prop_register_global_list(GlobalProperty *props);
int qdev_prop_check_globals(void);
void qdev_prop_set_globals(DeviceState *dev);
void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index ab61d502fd..bd84c4ea4c 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1180,15 +1180,6 @@ void qdev_prop_register_global(GlobalProperty *prop)
global_props = g_list_append(global_props, prop);
}
-void qdev_prop_register_global_list(GlobalProperty *props)
-{
- int i;
-
- for (i = 0; props[i].driver != NULL; i++) {
- qdev_prop_register_global(props+i);
- }
-}
-
int qdev_prop_check_globals(void)
{
GList *l;
diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
index d81b0862d5..d8596e6637 100644
--- a/tests/test-qdev-global-props.c
+++ b/tests/test-qdev-global-props.c
@@ -89,6 +89,16 @@ static void test_static_prop(void)
g_test_trap_assert_stdout("");
}
+static void register_global_list(GlobalProperty *props)
+{
+ int i;
+
+ for (i = 0; props[i].driver != NULL; i++) {
+ qdev_prop_register_global(props + i);
+ }
+}
+
+
/* Test setting of static property using global properties */
static void test_static_globalprop_subprocess(void)
{
@@ -98,7 +108,7 @@ static void test_static_globalprop_subprocess(void)
{}
};
- qdev_prop_register_global_list(props);
+ register_global_list(props);
mt = STATIC_TYPE(object_new(TYPE_STATIC_PROPS));
qdev_init_nofail(DEVICE(mt));
@@ -216,7 +226,7 @@ static void test_dynamic_globalprop_subprocess(void)
};
int all_used;
- qdev_prop_register_global_list(props);
+ register_global_list(props);
mt = DYNAMIC_TYPE(object_new(TYPE_DYNAMIC_PROPS));
qdev_init_nofail(DEVICE(mt));
@@ -261,7 +271,7 @@ static void test_dynamic_globalprop_nouser_subprocess(void)
};
int all_used;
- qdev_prop_register_global_list(props);
+ register_global_list(props);
mt = DYNAMIC_TYPE(object_new(TYPE_DYNAMIC_PROPS));
qdev_init_nofail(DEVICE(mt));
@@ -299,7 +309,7 @@ static void test_subclass_global_props(void)
{}
};
- qdev_prop_register_global_list(props);
+ register_global_list(props);
mt = STATIC_TYPE(object_new(TYPE_SUBCLASS));
qdev_init_nofail(DEVICE(mt));
--
2.19.0.rc1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 3/9] qdev: move qdev_prop_register_global_list() to tests
2018-09-12 12:55 ` [Qemu-devel] [PATCH 3/9] qdev: move qdev_prop_register_global_list() to tests Marc-André Lureau
@ 2018-10-22 14:51 ` Igor Mammedov
0 siblings, 0 replies; 29+ messages in thread
From: Igor Mammedov @ 2018-10-22 14:51 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum,
dgilbert, Richard Henderson, Andreas Färber,
Michael S. Tsirkin, Artyom Tarasenko, Mark Cave-Ayland
On Wed, 12 Sep 2018 16:55:25 +0400
Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> The function is only used by a test, move it there.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> include/hw/qdev-properties.h | 1 -
> hw/core/qdev-properties.c | 9 ---------
> tests/test-qdev-global-props.c | 18 ++++++++++++++----
> 3 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index a95f4a73eb..3ab9cd2eb6 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -249,7 +249,6 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value);
> void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
>
> void qdev_prop_register_global(GlobalProperty *prop);
> -void qdev_prop_register_global_list(GlobalProperty *props);
> int qdev_prop_check_globals(void);
> void qdev_prop_set_globals(DeviceState *dev);
> void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index ab61d502fd..bd84c4ea4c 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1180,15 +1180,6 @@ void qdev_prop_register_global(GlobalProperty *prop)
> global_props = g_list_append(global_props, prop);
> }
>
> -void qdev_prop_register_global_list(GlobalProperty *props)
> -{
> - int i;
> -
> - for (i = 0; props[i].driver != NULL; i++) {
> - qdev_prop_register_global(props+i);
> - }
> -}
> -
> int qdev_prop_check_globals(void)
> {
> GList *l;
> diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
> index d81b0862d5..d8596e6637 100644
> --- a/tests/test-qdev-global-props.c
> +++ b/tests/test-qdev-global-props.c
> @@ -89,6 +89,16 @@ static void test_static_prop(void)
> g_test_trap_assert_stdout("");
> }
>
> +static void register_global_list(GlobalProperty *props)
naming looks too ambiguous, I'd keep original name or
maybe use something more specific, like 'register_global_properties'
> +{
> + int i;
> +
> + for (i = 0; props[i].driver != NULL; i++) {
> + qdev_prop_register_global(props + i);
> + }
> +}
> +
> +
> /* Test setting of static property using global properties */
> static void test_static_globalprop_subprocess(void)
> {
> @@ -98,7 +108,7 @@ static void test_static_globalprop_subprocess(void)
> {}
> };
>
> - qdev_prop_register_global_list(props);
> + register_global_list(props);
>
> mt = STATIC_TYPE(object_new(TYPE_STATIC_PROPS));
> qdev_init_nofail(DEVICE(mt));
> @@ -216,7 +226,7 @@ static void test_dynamic_globalprop_subprocess(void)
> };
> int all_used;
>
> - qdev_prop_register_global_list(props);
> + register_global_list(props);
>
> mt = DYNAMIC_TYPE(object_new(TYPE_DYNAMIC_PROPS));
> qdev_init_nofail(DEVICE(mt));
> @@ -261,7 +271,7 @@ static void test_dynamic_globalprop_nouser_subprocess(void)
> };
> int all_used;
>
> - qdev_prop_register_global_list(props);
> + register_global_list(props);
>
> mt = DYNAMIC_TYPE(object_new(TYPE_DYNAMIC_PROPS));
> qdev_init_nofail(DEVICE(mt));
> @@ -299,7 +309,7 @@ static void test_subclass_global_props(void)
> {}
> };
>
> - qdev_prop_register_global_list(props);
> + register_global_list(props);
>
> mt = STATIC_TYPE(object_new(TYPE_SUBCLASS));
> qdev_init_nofail(DEVICE(mt));
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 4/9] qom/globals: move qdev globals to qom
2018-09-12 12:55 [Qemu-devel] [PATCH 0/9] hostmem-ram: use whole path for region name with >= 3.1 Marc-André Lureau
` (2 preceding siblings ...)
2018-09-12 12:55 ` [Qemu-devel] [PATCH 3/9] qdev: move qdev_prop_register_global_list() to tests Marc-André Lureau
@ 2018-09-12 12:55 ` Marc-André Lureau
2018-09-12 12:55 ` [Qemu-devel] [PATCH 5/9] qom/globals: generalize object_property_set_globals() Marc-André Lureau
` (5 subsequent siblings)
9 siblings, 0 replies; 29+ messages in thread
From: Marc-André Lureau @ 2018-09-12 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum, dgilbert,
Richard Henderson, Andreas Färber, Igor Mammedov,
Michael S. Tsirkin, Artyom Tarasenko, Mark Cave-Ayland,
Marc-André Lureau
Move the global properties handling to QOM. For now, this is just a
simple moving & renaming and the globals are only applied to
TYPE_DEVICE. The following patches will generalize the global handling
to other object types.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
include/hw/qdev-core.h | 20 ---------
include/hw/qdev-properties.h | 4 +-
include/qom/globals.h | 32 +++++++++++++++
accel/accel.c | 2 +-
hw/core/machine.c | 2 +-
hw/core/qdev-properties.c | 67 ------------------------------
hw/core/qdev.c | 2 +-
qom/cpu.c | 2 +-
qom/globals.c | 75 ++++++++++++++++++++++++++++++++++
target/i386/cpu.c | 2 +-
target/sparc/cpu.c | 2 +-
tests/test-qdev-global-props.c | 6 +--
vl.c | 8 ++--
qom/Makefile.objs | 2 +-
14 files changed, 122 insertions(+), 104 deletions(-)
create mode 100644 include/qom/globals.h
create mode 100644 qom/globals.c
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index f1fd0f8736..9e4695d03f 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -246,26 +246,6 @@ struct PropertyInfo {
ObjectPropertyRelease *release;
};
-/**
- * GlobalProperty:
- * @user_provided: Set to true if property comes from user-provided config
- * (command-line or config file).
- * @used: Set to true if property was used when initializing a device.
- * @errp: Error destination, used like first argument of error_setg()
- * in case property setting fails later. If @errp is NULL, we
- * print warnings instead of ignoring errors silently. For
- * hotplugged devices, errp is always ignored and warnings are
- * printed instead.
- */
-typedef struct GlobalProperty {
- const char *driver;
- const char *property;
- const char *value;
- bool user_provided;
- bool used;
- Error **errp;
-} GlobalProperty;
-
/*** Board API. This should go away once we have a machine config file. ***/
DeviceState *qdev_create(BusState *bus, const char *name);
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 3ab9cd2eb6..c8b727f081 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -4,6 +4,7 @@
#include "qapi/qapi-types-block.h"
#include "qapi/qapi-types-misc.h"
#include "hw/qdev-core.h"
+#include "qom/globals.h"
/*** qdev-properties.c ***/
@@ -248,9 +249,6 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value);
/* FIXME: Remove opaque pointer properties. */
void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
-void qdev_prop_register_global(GlobalProperty *prop);
-int qdev_prop_check_globals(void);
-void qdev_prop_set_globals(DeviceState *dev);
void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
Property *prop, const char *value);
diff --git a/include/qom/globals.h b/include/qom/globals.h
new file mode 100644
index 0000000000..28a0390233
--- /dev/null
+++ b/include/qom/globals.h
@@ -0,0 +1,32 @@
+#ifndef QOM_GLOBALS
+#define QOM_GLOBALS
+
+#include "qom/object.h"
+
+/**
+ * GlobalProperty:
+ * @user_provided: Set to true if property comes from user-provided config
+ * (command-line or config file).
+ * @used: Set to true if property was used when initializing a device.
+ * @errp: Error destination, used like first argument of error_setg()
+ * in case property setting fails later. If @errp is NULL, we
+ * print warnings instead of ignoring errors silently. For
+ * hotplugged devices, errp is always ignored and warnings are
+ * printed instead.
+ */
+typedef struct GlobalProperty {
+ const char *driver;
+ const char *property;
+ const char *value;
+ bool user_provided;
+ bool used;
+ Error **errp;
+} GlobalProperty;
+
+void object_property_register_global(GlobalProperty *prop);
+
+void object_property_set_globals(Object *obj);
+
+int object_property_check_globals(void);
+
+#endif
diff --git a/accel/accel.c b/accel/accel.c
index 3da26eb90f..6916e0d63d 100644
--- a/accel/accel.c
+++ b/accel/accel.c
@@ -127,7 +127,7 @@ void accel_register_compat_props(AccelState *accel)
for (; prop && prop->driver; prop++) {
/* Any compat_props must never cause error */
prop->errp = &error_abort;
- qdev_prop_register_global(prop);
+ object_property_register_global(prop);
}
}
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 6b68e1218f..5f532939e4 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -855,7 +855,7 @@ void machine_register_compat_props(MachineState *machine)
p = g_array_index(mc->compat_props, GlobalProperty *, i);
/* Machine compat_props must never cause errors: */
p->errp = &error_abort;
- qdev_prop_register_global(p);
+ object_property_register_global(p);
}
}
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index bd84c4ea4c..4605ca9b78 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1173,73 +1173,6 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value)
*ptr = value;
}
-static GList *global_props;
-
-void qdev_prop_register_global(GlobalProperty *prop)
-{
- global_props = g_list_append(global_props, prop);
-}
-
-int qdev_prop_check_globals(void)
-{
- GList *l;
- int ret = 0;
-
- for (l = global_props; l; l = l->next) {
- GlobalProperty *prop = l->data;
- ObjectClass *oc;
- DeviceClass *dc;
- if (prop->used) {
- continue;
- }
- if (!prop->user_provided) {
- continue;
- }
- oc = object_class_by_name(prop->driver);
- oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
- if (!oc) {
- warn_report("global %s.%s has invalid class name",
- prop->driver, prop->property);
- ret = 1;
- continue;
- }
- dc = DEVICE_CLASS(oc);
- if (!dc->hotpluggable && !prop->used) {
- warn_report("global %s.%s=%s not used",
- prop->driver, prop->property, prop->value);
- ret = 1;
- continue;
- }
- }
- return ret;
-}
-
-void qdev_prop_set_globals(DeviceState *dev)
-{
- GList *l;
-
- for (l = global_props; l; l = l->next) {
- GlobalProperty *prop = l->data;
- Error *err = NULL;
-
- if (object_dynamic_cast(OBJECT(dev), prop->driver) == NULL) {
- continue;
- }
- prop->used = true;
- object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
- if (err != NULL) {
- error_prepend(&err, "can't apply global %s.%s=%s: ",
- prop->driver, prop->property, prop->value);
- if (!dev->hotplugged && prop->errp) {
- error_propagate(prop->errp, err);
- } else {
- assert(prop->user_provided);
- warn_report_err(err);
- }
- }
- }
-}
-
/* --- 64bit unsigned int 'size' type --- */
static void get_size(Object *obj, Visitor *v, const char *name, void *opaque,
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 529b82de18..473060b551 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -976,7 +976,7 @@ static void device_initfn(Object *obj)
static void device_post_init(Object *obj)
{
- qdev_prop_set_globals(DEVICE(obj));
+ object_property_set_globals(obj);
}
/* Unlink device from bus and free the structure. */
diff --git a/qom/cpu.c b/qom/cpu.c
index 92599f3541..aaabe67648 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -314,7 +314,7 @@ static void cpu_common_parse_features(const char *typename, char *features,
prop->property = g_strdup(featurestr);
prop->value = g_strdup(val);
prop->errp = &error_fatal;
- qdev_prop_register_global(prop);
+ object_property_register_global(prop);
} else {
error_setg(errp, "Expected key=value format, found %s.",
featurestr);
diff --git a/qom/globals.c b/qom/globals.c
new file mode 100644
index 0000000000..587f4a1b5c
--- /dev/null
+++ b/qom/globals.c
@@ -0,0 +1,75 @@
+#include "qemu/osdep.h"
+
+#include "hw/qdev.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "qom/globals.h"
+#include "qom/object_interfaces.h"
+
+static GList *global_props;
+
+void object_property_register_global(GlobalProperty *prop)
+{
+ global_props = g_list_append(global_props, prop);
+}
+
+void object_property_set_globals(Object *obj)
+{
+ DeviceState *dev = DEVICE(obj);
+ GList *l;
+
+ for (l = global_props; l; l = l->next) {
+ GlobalProperty *prop = l->data;
+ Error *err = NULL;
+
+ if (object_dynamic_cast(OBJECT(dev), prop->driver) == NULL) {
+ continue;
+ }
+ prop->used = true;
+ object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
+ if (err != NULL) {
+ error_prepend(&err, "can't apply global %s.%s=%s: ",
+ prop->driver, prop->property, prop->value);
+ if (!dev->hotplugged && prop->errp) {
+ error_propagate(prop->errp, err);
+ } else {
+ assert(prop->user_provided);
+ warn_report_err(err);
+ }
+ }
+ }
+}
+
+int object_property_check_globals(void)
+{
+ GList *l;
+ int ret = 0;
+
+ for (l = global_props; l; l = l->next) {
+ GlobalProperty *prop = l->data;
+ ObjectClass *oc;
+ DeviceClass *dc;
+ if (prop->used) {
+ continue;
+ }
+ if (!prop->user_provided) {
+ continue;
+ }
+ oc = object_class_by_name(prop->driver);
+ oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
+ if (!oc) {
+ warn_report("global %s.%s has invalid class name",
+ prop->driver, prop->property);
+ ret = 1;
+ continue;
+ }
+ dc = DEVICE_CLASS(oc);
+ if (!dc->hotpluggable && !prop->used) {
+ warn_report("global %s.%s=%s not used",
+ prop->driver, prop->property, prop->value);
+ ret = 1;
+ continue;
+ }
+ }
+ return ret;
+}
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index f24295e6e4..ead1f67805 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3414,7 +3414,7 @@ static void x86_cpu_parse_featurestr(const char *typename, char *features,
prop->property = g_strdup(name);
prop->value = g_strdup(val);
prop->errp = &error_fatal;
- qdev_prop_register_global(prop);
+ object_property_register_global(prop);
}
if (ambiguous) {
diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index 0f090ece54..3fde246d0d 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -112,7 +112,7 @@ cpu_add_feat_as_prop(const char *typename, const char *name, const char *val)
prop->property = g_strdup(name);
prop->value = g_strdup(val);
prop->errp = &error_fatal;
- qdev_prop_register_global(prop);
+ object_property_register_global(prop);
}
/* Parse "+feature,-feature,feature=foo" CPU feature string */
diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
index d8596e6637..9dffdf336d 100644
--- a/tests/test-qdev-global-props.c
+++ b/tests/test-qdev-global-props.c
@@ -94,7 +94,7 @@ static void register_global_list(GlobalProperty *props)
int i;
for (i = 0; props[i].driver != NULL; i++) {
- qdev_prop_register_global(props + i);
+ object_property_register_global(props + i);
}
}
@@ -233,7 +233,7 @@ static void test_dynamic_globalprop_subprocess(void)
g_assert_cmpuint(mt->prop1, ==, 101);
g_assert_cmpuint(mt->prop2, ==, 102);
- all_used = qdev_prop_check_globals();
+ all_used = object_property_check_globals();
g_assert_cmpuint(all_used, ==, 1);
g_assert(props[0].used);
g_assert(props[1].used);
@@ -278,7 +278,7 @@ static void test_dynamic_globalprop_nouser_subprocess(void)
g_assert_cmpuint(mt->prop1, ==, 101);
g_assert_cmpuint(mt->prop2, ==, 102);
- all_used = qdev_prop_check_globals();
+ all_used = object_property_check_globals();
g_assert_cmpuint(all_used, ==, 0);
g_assert(props[0].used);
g_assert(props[1].used);
diff --git a/vl.c b/vl.c
index 5ba06adf78..03b8bda0ac 100644
--- a/vl.c
+++ b/vl.c
@@ -903,7 +903,7 @@ static void configure_rtc(QemuOpts *opts)
.value = "slew",
};
- qdev_prop_register_global(&slew_lost_ticks);
+ object_property_register_global(&slew_lost_ticks);
} else if (!strcmp(value, "none")) {
/* discard is default */
} else {
@@ -2872,7 +2872,7 @@ static int global_init_func(void *opaque, QemuOpts *opts, Error **errp)
g->value = qemu_opt_get(opts, "value");
g->user_provided = true;
g->errp = &error_fatal;
- qdev_prop_register_global(g);
+ object_property_register_global(g);
return 0;
}
@@ -3564,7 +3564,7 @@ int main(int argc, char **argv, char **envp)
.value = "slew",
};
- qdev_prop_register_global(&slew_lost_ticks);
+ object_property_register_global(&slew_lost_ticks);
warn_report("This option is deprecated, "
"use '-rtc driftfix=slew' instead.");
break;
@@ -4623,7 +4623,7 @@ int main(int argc, char **argv, char **envp)
}
}
- qdev_prop_check_globals();
+ object_property_check_globals();
if (vmstate_dump_file) {
/* dump and exit */
dump_vmstate_json_to_file(vmstate_dump_file);
diff --git a/qom/Makefile.objs b/qom/Makefile.objs
index 516349eec3..723851ff28 100644
--- a/qom/Makefile.objs
+++ b/qom/Makefile.objs
@@ -1,4 +1,4 @@
qom-obj-y = object.o container.o qom-qobject.o
-qom-obj-y += object_interfaces.o
+qom-obj-y += object_interfaces.o globals.o
common-obj-y = cpu.o
--
2.19.0.rc1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [Qemu-devel] [PATCH 5/9] qom/globals: generalize object_property_set_globals()
2018-09-12 12:55 [Qemu-devel] [PATCH 0/9] hostmem-ram: use whole path for region name with >= 3.1 Marc-André Lureau
` (3 preceding siblings ...)
2018-09-12 12:55 ` [Qemu-devel] [PATCH 4/9] qom/globals: move qdev globals to qom Marc-André Lureau
@ 2018-09-12 12:55 ` Marc-André Lureau
2018-10-29 13:11 ` Igor Mammedov
2018-09-12 12:55 ` [Qemu-devel] [PATCH 6/9] qom/object: set globals when initializing object Marc-André Lureau
` (4 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Marc-André Lureau @ 2018-09-12 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum, dgilbert,
Richard Henderson, Andreas Färber, Igor Mammedov,
Michael S. Tsirkin, Artyom Tarasenko, Mark Cave-Ayland,
Marc-André Lureau
Handle calls of object_property_set_globals() with any object type,
but only apply globals to TYPE_DEVICE & TYPE_USER_CREATABLE.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
qom/globals.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/qom/globals.c b/qom/globals.c
index 587f4a1b5c..8664baebe0 100644
--- a/qom/globals.c
+++ b/qom/globals.c
@@ -15,22 +15,28 @@ void object_property_register_global(GlobalProperty *prop)
void object_property_set_globals(Object *obj)
{
- DeviceState *dev = DEVICE(obj);
GList *l;
+ DeviceState *dev = (DeviceState *)object_dynamic_cast(obj, TYPE_DEVICE);
+
+ if (!dev && !IS_USER_CREATABLE(obj)) {
+ /* only TYPE_DEVICE and TYPE_USER_CREATABLE support globals */
+ return;
+ }
for (l = global_props; l; l = l->next) {
GlobalProperty *prop = l->data;
Error *err = NULL;
- if (object_dynamic_cast(OBJECT(dev), prop->driver) == NULL) {
+ if (object_dynamic_cast(obj, prop->driver) == NULL) {
continue;
}
prop->used = true;
- object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
+ object_property_parse(obj, prop->value, prop->property, &err);
if (err != NULL) {
error_prepend(&err, "can't apply global %s.%s=%s: ",
prop->driver, prop->property, prop->value);
- if (!dev->hotplugged && prop->errp) {
+
+ if (dev && !dev->hotplugged && prop->errp) {
error_propagate(prop->errp, err);
} else {
assert(prop->user_provided);
@@ -56,15 +62,15 @@ int object_property_check_globals(void)
continue;
}
oc = object_class_by_name(prop->driver);
- oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
- if (!oc) {
+ dc = (DeviceClass *)object_class_dynamic_cast(oc, TYPE_DEVICE);
+ if (!IS_USER_CREATABLE_CLASS(oc) && !dc) {
warn_report("global %s.%s has invalid class name",
prop->driver, prop->property);
ret = 1;
continue;
}
- dc = DEVICE_CLASS(oc);
- if (!dc->hotpluggable && !prop->used) {
+
+ if (dc && !dc->hotpluggable) {
warn_report("global %s.%s=%s not used",
prop->driver, prop->property, prop->value);
ret = 1;
--
2.19.0.rc1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 5/9] qom/globals: generalize object_property_set_globals()
2018-09-12 12:55 ` [Qemu-devel] [PATCH 5/9] qom/globals: generalize object_property_set_globals() Marc-André Lureau
@ 2018-10-29 13:11 ` Igor Mammedov
2018-10-30 12:16 ` Marc-André Lureau
0 siblings, 1 reply; 29+ messages in thread
From: Igor Mammedov @ 2018-10-29 13:11 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Eduardo Habkost, Michael S. Tsirkin, Mark Cave-Ayland,
dgilbert, Paolo Bonzini, Andreas Färber, Artyom Tarasenko,
Richard Henderson
On Wed, 12 Sep 2018 16:55:27 +0400
Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> Handle calls of object_property_set_globals() with any object type,
> but only apply globals to TYPE_DEVICE & TYPE_USER_CREATABLE.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> qom/globals.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/qom/globals.c b/qom/globals.c
> index 587f4a1b5c..8664baebe0 100644
> --- a/qom/globals.c
> +++ b/qom/globals.c
> @@ -15,22 +15,28 @@ void object_property_register_global(GlobalProperty *prop)
>
> void object_property_set_globals(Object *obj)
> {
> - DeviceState *dev = DEVICE(obj);
> GList *l;
> + DeviceState *dev = (DeviceState *)object_dynamic_cast(obj, TYPE_DEVICE);
> +
> + if (!dev && !IS_USER_CREATABLE(obj)) {
> + /* only TYPE_DEVICE and TYPE_USER_CREATABLE support globals */
> + return;
> + }
more dynamic casts but now imposed on every create object :(
Maybe we should add ObjectClass::check/set_globals hook?
It would be cheap to check and only objects we intend to work with
it would be affected. On top of that hooks could be different so
that device/user_creatable specifics won't be in generic code
(like it's implemented here).
>
> for (l = global_props; l; l = l->next) {
> GlobalProperty *prop = l->data;
> Error *err = NULL;
>
> - if (object_dynamic_cast(OBJECT(dev), prop->driver) == NULL) {
> + if (object_dynamic_cast(obj, prop->driver) == NULL) {
> continue;
> }
> prop->used = true;
> - object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
> + object_property_parse(obj, prop->value, prop->property, &err);
> if (err != NULL) {
> error_prepend(&err, "can't apply global %s.%s=%s: ",
> prop->driver, prop->property, prop->value);
> - if (!dev->hotplugged && prop->errp) {
> +
> + if (dev && !dev->hotplugged && prop->errp) {
> error_propagate(prop->errp, err);
> } else {
> assert(prop->user_provided);
> @@ -56,15 +62,15 @@ int object_property_check_globals(void)
> continue;
> }
> oc = object_class_by_name(prop->driver);
> - oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
> - if (!oc) {
> + dc = (DeviceClass *)object_class_dynamic_cast(oc, TYPE_DEVICE);
> + if (!IS_USER_CREATABLE_CLASS(oc) && !dc) {
> warn_report("global %s.%s has invalid class name",
> prop->driver, prop->property);
> ret = 1;
> continue;
> }
> - dc = DEVICE_CLASS(oc);
> - if (!dc->hotpluggable && !prop->used) {
> +
> + if (dc && !dc->hotpluggable) {
> warn_report("global %s.%s=%s not used",
> prop->driver, prop->property, prop->value);
> ret = 1;
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 5/9] qom/globals: generalize object_property_set_globals()
2018-10-29 13:11 ` Igor Mammedov
@ 2018-10-30 12:16 ` Marc-André Lureau
2018-10-30 14:05 ` Igor Mammedov
0 siblings, 1 reply; 29+ messages in thread
From: Marc-André Lureau @ 2018-10-30 12:16 UTC (permalink / raw)
To: Igor Mammedov
Cc: qemu-devel, Eduardo Habkost, Michael S . Tsirkin,
mark.cave-ayland, Dr. David Alan Gilbert, Bonzini, Paolo,
Andreas Färber, atar4qemu, Richard Henderson
Hi
On Mon, Oct 29, 2018 at 5:11 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Wed, 12 Sep 2018 16:55:27 +0400
> Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>
> > Handle calls of object_property_set_globals() with any object type,
> > but only apply globals to TYPE_DEVICE & TYPE_USER_CREATABLE.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > qom/globals.c | 22 ++++++++++++++--------
> > 1 file changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/qom/globals.c b/qom/globals.c
> > index 587f4a1b5c..8664baebe0 100644
> > --- a/qom/globals.c
> > +++ b/qom/globals.c
> > @@ -15,22 +15,28 @@ void object_property_register_global(GlobalProperty *prop)
> >
> > void object_property_set_globals(Object *obj)
> > {
> > - DeviceState *dev = DEVICE(obj);
> > GList *l;
> > + DeviceState *dev = (DeviceState *)object_dynamic_cast(obj, TYPE_DEVICE);
> > +
> > + if (!dev && !IS_USER_CREATABLE(obj)) {
> > + /* only TYPE_DEVICE and TYPE_USER_CREATABLE support globals */
> > + return;
> > + }
> more dynamic casts but now imposed on every create object :(
>
> Maybe we should add ObjectClass::check/set_globals hook?
> It would be cheap to check and only objects we intend to work with
> it would be affected. On top of that hooks could be different so
> that device/user_creatable specifics won't be in generic code
> (like it's implemented here).
I don't think adding a few casts during object creation will impact
negatively in a measurable way.
However I'll add an additional patch to the series to implement what I
think you suggested.
thanks
>
> >
> > for (l = global_props; l; l = l->next) {
> > GlobalProperty *prop = l->data;
> > Error *err = NULL;
> >
> > - if (object_dynamic_cast(OBJECT(dev), prop->driver) == NULL) {
> > + if (object_dynamic_cast(obj, prop->driver) == NULL) {
> > continue;
> > }
> > prop->used = true;
> > - object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
> > + object_property_parse(obj, prop->value, prop->property, &err);
> > if (err != NULL) {
> > error_prepend(&err, "can't apply global %s.%s=%s: ",
> > prop->driver, prop->property, prop->value);
> > - if (!dev->hotplugged && prop->errp) {
> > +
> > + if (dev && !dev->hotplugged && prop->errp) {
> > error_propagate(prop->errp, err);
> > } else {
> > assert(prop->user_provided);
> > @@ -56,15 +62,15 @@ int object_property_check_globals(void)
> > continue;
> > }
> > oc = object_class_by_name(prop->driver);
> > - oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
> > - if (!oc) {
> > + dc = (DeviceClass *)object_class_dynamic_cast(oc, TYPE_DEVICE);
> > + if (!IS_USER_CREATABLE_CLASS(oc) && !dc) {
> > warn_report("global %s.%s has invalid class name",
> > prop->driver, prop->property);
> > ret = 1;
> > continue;
> > }
> > - dc = DEVICE_CLASS(oc);
> > - if (!dc->hotpluggable && !prop->used) {
> > +
> > + if (dc && !dc->hotpluggable) {
> > warn_report("global %s.%s=%s not used",
> > prop->driver, prop->property, prop->value);
> > ret = 1;
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 5/9] qom/globals: generalize object_property_set_globals()
2018-10-30 12:16 ` Marc-André Lureau
@ 2018-10-30 14:05 ` Igor Mammedov
0 siblings, 0 replies; 29+ messages in thread
From: Igor Mammedov @ 2018-10-30 14:05 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Eduardo Habkost, Michael S . Tsirkin,
mark.cave-ayland, Dr. David Alan Gilbert, Bonzini, Paolo,
Andreas Färber, atar4qemu, Richard Henderson
On Tue, 30 Oct 2018 16:16:43 +0400
Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> Hi
>
> On Mon, Oct 29, 2018 at 5:11 PM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Wed, 12 Sep 2018 16:55:27 +0400
> > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> >
> > > Handle calls of object_property_set_globals() with any object type,
> > > but only apply globals to TYPE_DEVICE & TYPE_USER_CREATABLE.
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > > qom/globals.c | 22 ++++++++++++++--------
> > > 1 file changed, 14 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/qom/globals.c b/qom/globals.c
> > > index 587f4a1b5c..8664baebe0 100644
> > > --- a/qom/globals.c
> > > +++ b/qom/globals.c
> > > @@ -15,22 +15,28 @@ void object_property_register_global(GlobalProperty *prop)
> > >
> > > void object_property_set_globals(Object *obj)
> > > {
> > > - DeviceState *dev = DEVICE(obj);
> > > GList *l;
> > > + DeviceState *dev = (DeviceState *)object_dynamic_cast(obj, TYPE_DEVICE);
> > > +
> > > + if (!dev && !IS_USER_CREATABLE(obj)) {
> > > + /* only TYPE_DEVICE and TYPE_USER_CREATABLE support globals */
> > > + return;
> > > + }
> > more dynamic casts but now imposed on every create object :(
> >
> > Maybe we should add ObjectClass::check/set_globals hook?
> > It would be cheap to check and only objects we intend to work with
> > it would be affected. On top of that hooks could be different so
> > that device/user_creatable specifics won't be in generic code
> > (like it's implemented here).
>
> I don't think adding a few casts during object creation will impact
> negatively in a measurable way.
When it's applied to every object created as opposed to -object CLI option,
it will add up. For example 512 x 2 (cpus + apic) ~ 1000 x cast_cost,
for pc-dimm 255 x 3 (backend + memory_region + pc-dimm) ~ 800 x cast_cost,
We can have more backends of other types, basically everything QOM will
incur tiny extra cost.
I'm just thinking about worst case scenario.
> However I'll add an additional patch to the series to implement what I
> think you suggested.
thanks,
for this patch also important to keep Device vs UserCreatable
parts separate instead of dumping evrything into one pit.
>
> thanks
> >
> > >
> > > for (l = global_props; l; l = l->next) {
> > > GlobalProperty *prop = l->data;
> > > Error *err = NULL;
> > >
> > > - if (object_dynamic_cast(OBJECT(dev), prop->driver) == NULL) {
> > > + if (object_dynamic_cast(obj, prop->driver) == NULL) {
> > > continue;
> > > }
> > > prop->used = true;
> > > - object_property_parse(OBJECT(dev), prop->value, prop->property, &err);
> > > + object_property_parse(obj, prop->value, prop->property, &err);
> > > if (err != NULL) {
> > > error_prepend(&err, "can't apply global %s.%s=%s: ",
> > > prop->driver, prop->property, prop->value);
> > > - if (!dev->hotplugged && prop->errp) {
> > > +
> > > + if (dev && !dev->hotplugged && prop->errp) {
> > > error_propagate(prop->errp, err);
> > > } else {
> > > assert(prop->user_provided);
> > > @@ -56,15 +62,15 @@ int object_property_check_globals(void)
> > > continue;
> > > }
> > > oc = object_class_by_name(prop->driver);
> > > - oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
> > > - if (!oc) {
> > > + dc = (DeviceClass *)object_class_dynamic_cast(oc, TYPE_DEVICE);
> > > + if (!IS_USER_CREATABLE_CLASS(oc) && !dc) {
> > > warn_report("global %s.%s has invalid class name",
> > > prop->driver, prop->property);
> > > ret = 1;
> > > continue;
> > > }
> > > - dc = DEVICE_CLASS(oc);
> > > - if (!dc->hotpluggable && !prop->used) {
> > > +
> > > + if (dc && !dc->hotpluggable) {
> > > warn_report("global %s.%s=%s not used",
> > > prop->driver, prop->property, prop->value);
> > > ret = 1;
> >
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 6/9] qom/object: set globals when initializing object
2018-09-12 12:55 [Qemu-devel] [PATCH 0/9] hostmem-ram: use whole path for region name with >= 3.1 Marc-André Lureau
` (4 preceding siblings ...)
2018-09-12 12:55 ` [Qemu-devel] [PATCH 5/9] qom/globals: generalize object_property_set_globals() Marc-André Lureau
@ 2018-09-12 12:55 ` Marc-André Lureau
2018-10-29 12:20 ` Igor Mammedov
2018-09-12 12:55 ` [Qemu-devel] [PATCH 7/9] tests: add user-creatable test to test-qdev-global-props Marc-André Lureau
` (3 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Marc-André Lureau @ 2018-09-12 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum, dgilbert,
Richard Henderson, Andreas Färber, Igor Mammedov,
Michael S. Tsirkin, Artyom Tarasenko, Mark Cave-Ayland,
Marc-André Lureau
Set globals for all objects, although only TYPE_DEVICE &
TYPE_USER_CREATABLE can have globals for now.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
hw/core/qdev.c | 6 ------
qom/object.c | 2 ++
2 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 473060b551..28c6c8d7c9 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -974,11 +974,6 @@ static void device_initfn(Object *obj)
QLIST_INIT(&dev->gpios);
}
-static void device_post_init(Object *obj)
-{
- object_property_set_globals(obj);
-}
-
/* Unlink device from bus and free the structure. */
static void device_finalize(Object *obj)
{
@@ -1103,7 +1098,6 @@ static const TypeInfo device_type_info = {
.parent = TYPE_OBJECT,
.instance_size = sizeof(DeviceState),
.instance_init = device_initfn,
- .instance_post_init = device_post_init,
.instance_finalize = device_finalize,
.class_base_init = device_class_base_init,
.class_init = device_class_init,
diff --git a/qom/object.c b/qom/object.c
index 0703e8e4ff..025ad0e191 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -12,6 +12,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
+#include "qom/globals.h"
#include "qom/object.h"
#include "qom/object_interfaces.h"
#include "qemu/cutils.h"
@@ -382,6 +383,7 @@ static void object_initialize_with_type(void *data, size_t size, TypeImpl *type)
obj->properties = g_hash_table_new_full(g_str_hash, g_str_equal,
NULL, object_property_free);
object_init_with_type(obj, type);
+ object_property_set_globals(obj);
object_post_init_with_type(obj, type);
}
--
2.19.0.rc1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 6/9] qom/object: set globals when initializing object
2018-09-12 12:55 ` [Qemu-devel] [PATCH 6/9] qom/object: set globals when initializing object Marc-André Lureau
@ 2018-10-29 12:20 ` Igor Mammedov
0 siblings, 0 replies; 29+ messages in thread
From: Igor Mammedov @ 2018-10-29 12:20 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum,
dgilbert, Richard Henderson, Andreas Färber,
Michael S. Tsirkin, Artyom Tarasenko, Mark Cave-Ayland
On Wed, 12 Sep 2018 16:55:28 +0400
Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> Set globals for all objects, although only TYPE_DEVICE &
> TYPE_USER_CREATABLE can have globals for now.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> hw/core/qdev.c | 6 ------
> qom/object.c | 2 ++
> 2 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 473060b551..28c6c8d7c9 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -974,11 +974,6 @@ static void device_initfn(Object *obj)
> QLIST_INIT(&dev->gpios);
> }
>
> -static void device_post_init(Object *obj)
> -{
> - object_property_set_globals(obj);
> -}
> -
> /* Unlink device from bus and free the structure. */
> static void device_finalize(Object *obj)
> {
> @@ -1103,7 +1098,6 @@ static const TypeInfo device_type_info = {
> .parent = TYPE_OBJECT,
> .instance_size = sizeof(DeviceState),
> .instance_init = device_initfn,
> - .instance_post_init = device_post_init,
> .instance_finalize = device_finalize,
> .class_base_init = device_class_base_init,
> .class_init = device_class_init,
> diff --git a/qom/object.c b/qom/object.c
> index 0703e8e4ff..025ad0e191 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -12,6 +12,7 @@
>
> #include "qemu/osdep.h"
> #include "qapi/error.h"
> +#include "qom/globals.h"
> #include "qom/object.h"
> #include "qom/object_interfaces.h"
> #include "qemu/cutils.h"
> @@ -382,6 +383,7 @@ static void object_initialize_with_type(void *data, size_t size, TypeImpl *type)
> obj->properties = g_hash_table_new_full(g_str_hash, g_str_equal,
> NULL, object_property_free);
> object_init_with_type(obj, type);
> + object_property_set_globals(obj);
> object_post_init_with_type(obj, type);
this would somewhat inverse post_init call chain,
if there weren't any other instance_post_init() users that would be fine
but it potentially can break arm_cpu_post_init() since globals would be
set before the later finishes creating per instance properties.
arm cpu feature setting code is spread across initfn/post_init/realize
and it's unreadable mess.
Maybe we should get rid of instance_post_init() altogether and explicitly
call arm_cpu_post_init() from each initfn().
> }
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 7/9] tests: add user-creatable test to test-qdev-global-props
2018-09-12 12:55 [Qemu-devel] [PATCH 0/9] hostmem-ram: use whole path for region name with >= 3.1 Marc-André Lureau
` (5 preceding siblings ...)
2018-09-12 12:55 ` [Qemu-devel] [PATCH 6/9] qom/object: set globals when initializing object Marc-André Lureau
@ 2018-09-12 12:55 ` Marc-André Lureau
2018-09-12 12:55 ` [Qemu-devel] [PATCH 8/9] hw/i386: add pc-i440fx-3.1 & pc-q35-3.1 Marc-André Lureau
` (2 subsequent siblings)
9 siblings, 0 replies; 29+ messages in thread
From: Marc-André Lureau @ 2018-09-12 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum, dgilbert,
Richard Henderson, Andreas Färber, Igor Mammedov,
Michael S. Tsirkin, Artyom Tarasenko, Mark Cave-Ayland,
Marc-André Lureau
Add a TYPE_USER_CREATABLE object global property check.
Rename the test, since it is no longer QDev-specific.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
...dev-global-props.c => test-global-props.c} | 128 ++++++++++++++++--
tests/Makefile.include | 4 +-
2 files changed, 117 insertions(+), 15 deletions(-)
rename tests/{test-qdev-global-props.c => test-global-props.c} (74%)
diff --git a/tests/test-qdev-global-props.c b/tests/test-global-props.c
similarity index 74%
rename from tests/test-qdev-global-props.c
rename to tests/test-global-props.c
index 9dffdf336d..1c725245bc 100644
--- a/tests/test-qdev-global-props.c
+++ b/tests/test-global-props.c
@@ -27,7 +27,100 @@
#include "hw/qdev.h"
#include "qom/object.h"
#include "qapi/visitor.h"
+#include "qom/object_interfaces.h"
+#define TYPE_DUMMY "qemu-dummy"
+
+typedef struct DummyObject DummyObject;
+typedef struct DummyObjectClass DummyObjectClass;
+
+#define DUMMY_OBJECT(obj) \
+ OBJECT_CHECK(DummyObject, (obj), TYPE_DUMMY)
+
+struct DummyObject {
+ Object parent_obj;
+
+ char *sv;
+};
+
+struct DummyObjectClass {
+ ObjectClass parent_class;
+};
+
+static void dummy_set_sv(Object *obj,
+ const char *value,
+ Error **errp)
+{
+ DummyObject *dobj = DUMMY_OBJECT(obj);
+
+ g_free(dobj->sv);
+ dobj->sv = g_strdup(value);
+}
+
+static char *dummy_get_sv(Object *obj,
+ Error **errp)
+{
+ DummyObject *dobj = DUMMY_OBJECT(obj);
+
+ return g_strdup(dobj->sv);
+}
+
+
+
+static void dummy_class_init(ObjectClass *cls, void *data)
+{
+ object_class_property_add_str(cls, "sv",
+ dummy_get_sv,
+ dummy_set_sv,
+ NULL);
+}
+
+
+static void dummy_finalize(Object *obj)
+{
+ DummyObject *dobj = DUMMY_OBJECT(obj);
+
+ g_free(dobj->sv);
+}
+
+
+static const TypeInfo dummy_info = {
+ .name = TYPE_DUMMY,
+ .parent = TYPE_OBJECT,
+ .instance_size = sizeof(DummyObject),
+ .instance_finalize = dummy_finalize,
+ .class_size = sizeof(DummyObjectClass),
+ .class_init = dummy_class_init,
+ .interfaces = (InterfaceInfo[]) {
+ { TYPE_USER_CREATABLE },
+ { }
+ }
+};
+
+static void test_global_props_uc_subprocess(void)
+{
+ DummyObject *d;
+ static GlobalProperty gp = {
+ TYPE_DUMMY, "sv", "foobar",
+ };
+
+ d = DUMMY_OBJECT(object_new(TYPE_DUMMY));
+ g_assert_null(d->sv);
+ object_unref(OBJECT(d));
+
+ object_property_register_global(&gp);
+ d = DUMMY_OBJECT(object_new(TYPE_DUMMY));
+ g_assert_cmpstr(d->sv, ==, "foobar");
+ object_unref(OBJECT(d));
+}
+
+static void test_global_props_uc(void)
+{
+ g_test_trap_subprocess("/global-props/usercreatable/subprocess", 0, 0);
+ g_test_trap_assert_passed();
+ g_test_trap_assert_stderr("");
+ g_test_trap_assert_stdout("");
+}
#define TYPE_STATIC_PROPS "static_prop_type"
#define STATIC_TYPE(obj) \
@@ -83,7 +176,8 @@ static void test_static_prop_subprocess(void)
static void test_static_prop(void)
{
- g_test_trap_subprocess("/qdev/properties/static/default/subprocess", 0, 0);
+ g_test_trap_subprocess("/global-props/qdev/static/default/subprocess",
+ 0, 0);
g_test_trap_assert_passed();
g_test_trap_assert_stderr("");
g_test_trap_assert_stdout("");
@@ -119,7 +213,7 @@ static void test_static_globalprop_subprocess(void)
static void test_static_globalprop(void)
{
- g_test_trap_subprocess("/qdev/properties/static/global/subprocess", 0, 0);
+ g_test_trap_subprocess("/global-props/qdev/static/global/subprocess", 0, 0);
g_test_trap_assert_passed();
g_test_trap_assert_stderr("");
g_test_trap_assert_stdout("");
@@ -245,7 +339,8 @@ static void test_dynamic_globalprop_subprocess(void)
static void test_dynamic_globalprop(void)
{
- g_test_trap_subprocess("/qdev/properties/dynamic/global/subprocess", 0, 0);
+ g_test_trap_subprocess("/global-props/qdev/dynamic/global/subprocess",
+ 0, 0);
g_test_trap_assert_passed();
g_test_trap_assert_stderr_unmatched("*prop1*");
g_test_trap_assert_stderr_unmatched("*prop2*");
@@ -290,7 +385,8 @@ static void test_dynamic_globalprop_nouser_subprocess(void)
static void test_dynamic_globalprop_nouser(void)
{
- g_test_trap_subprocess("/qdev/properties/dynamic/global/nouser/subprocess", 0, 0);
+ g_test_trap_subprocess("/global-props/qdev"
+ "/dynamic/global/nouser/subprocess", 0, 0);
g_test_trap_assert_passed();
g_test_trap_assert_stderr("");
g_test_trap_assert_stdout("");
@@ -323,6 +419,7 @@ int main(int argc, char **argv)
g_test_init(&argc, &argv, NULL);
module_call_init(MODULE_INIT_QOM);
+ type_register_static(&dummy_info);
type_register_static(&static_prop_type);
type_register_static(&subclass_type);
type_register_static(&dynamic_prop_type);
@@ -330,27 +427,32 @@ int main(int argc, char **argv)
type_register_static(&nohotplug_type);
type_register_static(&nondevice_type);
- g_test_add_func("/qdev/properties/static/default/subprocess",
+ g_test_add_func("/global-props/usercreatable/subprocess",
+ test_global_props_uc_subprocess);
+ g_test_add_func("/global-props/usercreatable",
+ test_global_props_uc);
+
+ g_test_add_func("/global-props/qdev/static/default/subprocess",
test_static_prop_subprocess);
- g_test_add_func("/qdev/properties/static/default",
+ g_test_add_func("/global-props/qdev/static/default",
test_static_prop);
- g_test_add_func("/qdev/properties/static/global/subprocess",
+ g_test_add_func("/global-props/qdev/static/global/subprocess",
test_static_globalprop_subprocess);
- g_test_add_func("/qdev/properties/static/global",
+ g_test_add_func("/global-props/qdev/static/global",
test_static_globalprop);
- g_test_add_func("/qdev/properties/dynamic/global/subprocess",
+ g_test_add_func("/global-props/qdev/dynamic/global/subprocess",
test_dynamic_globalprop_subprocess);
- g_test_add_func("/qdev/properties/dynamic/global",
+ g_test_add_func("/global-props/qdev/dynamic/global",
test_dynamic_globalprop);
- g_test_add_func("/qdev/properties/dynamic/global/nouser/subprocess",
+ g_test_add_func("/global-props/qdev/dynamic/global/nouser/subprocess",
test_dynamic_globalprop_nouser_subprocess);
- g_test_add_func("/qdev/properties/dynamic/global/nouser",
+ g_test_add_func("/global-props/qdev/dynamic/global/nouser",
test_dynamic_globalprop_nouser);
- g_test_add_func("/qdev/properties/global/subclass",
+ g_test_add_func("/global-props/qdev/global/subclass",
test_subclass_global_props);
g_test_run();
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 87c81d1dcc..080b218fd3 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -128,7 +128,7 @@ check-unit-y += tests/test-qht-par$(EXESUF)
gcov-files-test-qht-par-y = util/qht.c
check-unit-y += tests/test-bitops$(EXESUF)
check-unit-y += tests/test-bitcnt$(EXESUF)
-check-unit-y += tests/test-qdev-global-props$(EXESUF)
+check-unit-y += tests/test-global-props$(EXESUF)
check-unit-y += tests/check-qom-interface$(EXESUF)
gcov-files-check-qom-interface-y = qom/object.c
check-unit-y += tests/check-qom-proplist$(EXESUF)
@@ -657,7 +657,7 @@ tests/qht-bench$(EXESUF): tests/qht-bench.o $(test-util-obj-y)
tests/test-bufferiszero$(EXESUF): tests/test-bufferiszero.o $(test-util-obj-y)
tests/atomic_add-bench$(EXESUF): tests/atomic_add-bench.o $(test-util-obj-y)
-tests/test-qdev-global-props$(EXESUF): tests/test-qdev-global-props.o \
+tests/test-global-props$(EXESUF): tests/test-global-props.o \
hw/core/qdev.o hw/core/qdev-properties.o hw/core/hotplug.o\
hw/core/bus.o \
hw/core/irq.o \
--
2.19.0.rc1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [Qemu-devel] [PATCH 8/9] hw/i386: add pc-i440fx-3.1 & pc-q35-3.1
2018-09-12 12:55 [Qemu-devel] [PATCH 0/9] hostmem-ram: use whole path for region name with >= 3.1 Marc-André Lureau
` (6 preceding siblings ...)
2018-09-12 12:55 ` [Qemu-devel] [PATCH 7/9] tests: add user-creatable test to test-qdev-global-props Marc-André Lureau
@ 2018-09-12 12:55 ` Marc-André Lureau
2018-09-12 12:55 ` [Qemu-devel] [PATCH 9/9] hostmem-ram: use whole path for memory region name with >= 3.1 Marc-André Lureau
2018-10-02 10:24 ` [Qemu-devel] [PATCH 0/9] hostmem-ram: use whole path for " Marc-André Lureau
9 siblings, 0 replies; 29+ messages in thread
From: Marc-André Lureau @ 2018-09-12 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum, dgilbert,
Richard Henderson, Andreas Färber, Igor Mammedov,
Michael S. Tsirkin, Artyom Tarasenko, Mark Cave-Ayland,
Marc-André Lureau
The following patch is going to add compatiblity parameters.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
include/hw/i386/pc.h | 5 ++++-
hw/i386/pc_piix.c | 15 ++++++++++++---
hw/i386/pc_q35.c | 13 +++++++++++--
3 files changed, 27 insertions(+), 6 deletions(-)
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 6894f37df1..09b0365a8e 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -294,7 +294,10 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
int e820_get_num_entries(void);
bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
-#define PC_COMPAT_2_12 \
+#define PC_COMPAT_3_0 \
+ HW_COMPAT_3_0
+
+#define PC_COMPAT_2_12 \
HW_COMPAT_2_12 \
{\
.driver = TYPE_X86_CPU,\
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index dc09466b3e..7092d6d13f 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -428,21 +428,30 @@ static void pc_i440fx_machine_options(MachineClass *m)
machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
}
-static void pc_i440fx_3_0_machine_options(MachineClass *m)
+static void pc_i440fx_3_1_machine_options(MachineClass *m)
{
pc_i440fx_machine_options(m);
m->alias = "pc";
m->is_default = 1;
}
+DEFINE_I440FX_MACHINE(v3_1, "pc-i440fx-3.1", NULL,
+ pc_i440fx_3_1_machine_options);
+
+static void pc_i440fx_3_0_machine_options(MachineClass *m)
+{
+ pc_i440fx_3_1_machine_options(m);
+ m->is_default = 0;
+ m->alias = NULL;
+ SET_MACHINE_COMPAT(m, PC_COMPAT_3_0);
+}
+
DEFINE_I440FX_MACHINE(v3_0, "pc-i440fx-3.0", NULL,
pc_i440fx_3_0_machine_options);
static void pc_i440fx_2_12_machine_options(MachineClass *m)
{
pc_i440fx_3_0_machine_options(m);
- m->is_default = 0;
- m->alias = NULL;
SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
}
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 532241e3f8..4702bb13c4 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -311,19 +311,28 @@ static void pc_q35_machine_options(MachineClass *m)
m->max_cpus = 288;
}
-static void pc_q35_3_0_machine_options(MachineClass *m)
+static void pc_q35_3_1_machine_options(MachineClass *m)
{
pc_q35_machine_options(m);
m->alias = "q35";
}
+DEFINE_Q35_MACHINE(v3_1, "pc-q35-3.1", NULL,
+ pc_q35_3_1_machine_options);
+
+static void pc_q35_3_0_machine_options(MachineClass *m)
+{
+ pc_q35_3_1_machine_options(m);
+ m->alias = NULL;
+ SET_MACHINE_COMPAT(m, PC_COMPAT_3_0);
+}
+
DEFINE_Q35_MACHINE(v3_0, "pc-q35-3.0", NULL,
pc_q35_3_0_machine_options);
static void pc_q35_2_12_machine_options(MachineClass *m)
{
pc_q35_3_0_machine_options(m);
- m->alias = NULL;
SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
}
--
2.19.0.rc1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [Qemu-devel] [PATCH 9/9] hostmem-ram: use whole path for memory region name with >= 3.1
2018-09-12 12:55 [Qemu-devel] [PATCH 0/9] hostmem-ram: use whole path for region name with >= 3.1 Marc-André Lureau
` (7 preceding siblings ...)
2018-09-12 12:55 ` [Qemu-devel] [PATCH 8/9] hw/i386: add pc-i440fx-3.1 & pc-q35-3.1 Marc-André Lureau
@ 2018-09-12 12:55 ` Marc-André Lureau
2018-09-13 10:19 ` Dr. David Alan Gilbert
2018-10-29 15:16 ` Igor Mammedov
2018-10-02 10:24 ` [Qemu-devel] [PATCH 0/9] hostmem-ram: use whole path for " Marc-André Lureau
9 siblings, 2 replies; 29+ messages in thread
From: Marc-André Lureau @ 2018-09-12 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum, dgilbert,
Richard Henderson, Andreas Färber, Igor Mammedov,
Michael S. Tsirkin, Artyom Tarasenko, Mark Cave-Ayland,
Marc-André Lureau
hostmem-file and hostmem-memfd use the whole object path for the
memory region name, but hostname-ram uses only the path component (the
basename):
qemu -m 1024 -object memory-backend-ram,id=mem,size=1G -numa node,memdev=mem -monitor stdio
(qemu) info ramblock
Block Name PSize Offset Used Total
mem 4 KiB 0x0000000000000000 0x0000000040000000 0x0000000040000000
qemu -m 1024 -object memory-backend-file,id=mem,size=1G,mem-path=/tmp/foo -numa node,memdev=mem -monitor stdio
(qemu) info ramblock
Block Name PSize Offset Used Total
/objects/mem 4 KiB 0x0000000000000000 0x0000000040000000 0x0000000040000000
qemu -m 1024 -object memory-backend-memfd,id=mem,size=1G -numa node,memdev=mem -monitor stdio
(qemu) info ramblock
Block Name PSize Offset Used Total
/objects/mem 4 KiB 0x0000000000000000 0x0000000040000000 0x0000000040000000
Use the whole path name with >= 3.1. Having a consistent naming allow
to migrate to different hostmem backends.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
include/hw/compat.h | 6 +++++-
backends/hostmem-ram.c | 47 ++++++++++++++++++++++++++++++++++++++----
2 files changed, 48 insertions(+), 5 deletions(-)
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 6f4d5fc647..8ce7a7057b 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -2,7 +2,11 @@
#define HW_COMPAT_H
#define HW_COMPAT_3_0 \
- /* empty */
+ {\
+ .driver = "memory-backend-ram",\
+ .property = "x-component-name",\
+ .value = "true",\
+ },
#define HW_COMPAT_2_12 \
{\
diff --git a/backends/hostmem-ram.c b/backends/hostmem-ram.c
index 7ddd08d370..a9eb99cf1b 100644
--- a/backends/hostmem-ram.c
+++ b/backends/hostmem-ram.c
@@ -16,21 +16,56 @@
#define TYPE_MEMORY_BACKEND_RAM "memory-backend-ram"
+typedef struct RamMemoryBackend {
+ HostMemoryBackend parent;
+
+ bool component_name;
+} RamMemoryBackend;
+
+#define RAM_BACKEND(obj) \
+ OBJECT_CHECK(RamMemoryBackend, (obj), TYPE_MEMORY_BACKEND_RAM)
+
+static char *
+ram_backend_get_name(RamMemoryBackend *self)
+{
+ /* < 3.1 use the component as memory region name */
+ if (self->component_name) {
+ return object_get_canonical_path_component(OBJECT(self));
+ }
+
+ return object_get_canonical_path(OBJECT(self));
+}
static void
ram_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
{
- char *path;
+ char *name;
if (!backend->size) {
error_setg(errp, "can't create backend with size 0");
return;
}
- path = object_get_canonical_path_component(OBJECT(backend));
- memory_region_init_ram_shared_nomigrate(&backend->mr, OBJECT(backend), path,
+ name = ram_backend_get_name(RAM_BACKEND(backend));
+ memory_region_init_ram_shared_nomigrate(&backend->mr, OBJECT(backend), name,
backend->size, backend->share, errp);
- g_free(path);
+ g_free(name);
+}
+
+static bool
+ram_backend_get_component_name(Object *obj, Error **errp)
+{
+ RamMemoryBackend *self = RAM_BACKEND(obj);
+
+ return self->component_name;
+}
+
+static void
+ram_backend_set_component_name(Object *obj, bool value, Error **errp)
+{
+ RamMemoryBackend *self = RAM_BACKEND(obj);
+
+ self->component_name = value;
}
static void
@@ -39,11 +74,15 @@ ram_backend_class_init(ObjectClass *oc, void *data)
HostMemoryBackendClass *bc = MEMORY_BACKEND_CLASS(oc);
bc->alloc = ram_backend_memory_alloc;
+ object_class_property_add_bool(oc, "x-component-name",
+ ram_backend_get_component_name,
+ ram_backend_set_component_name, &error_abort);
}
static const TypeInfo ram_backend_info = {
.name = TYPE_MEMORY_BACKEND_RAM,
.parent = TYPE_MEMORY_BACKEND,
+ .instance_size = sizeof(RamMemoryBackend),
.class_init = ram_backend_class_init,
};
--
2.19.0.rc1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 9/9] hostmem-ram: use whole path for memory region name with >= 3.1
2018-09-12 12:55 ` [Qemu-devel] [PATCH 9/9] hostmem-ram: use whole path for memory region name with >= 3.1 Marc-André Lureau
@ 2018-09-13 10:19 ` Dr. David Alan Gilbert
2018-10-29 15:16 ` Igor Mammedov
1 sibling, 0 replies; 29+ messages in thread
From: Dr. David Alan Gilbert @ 2018-09-13 10:19 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum,
Richard Henderson, Andreas Färber, Igor Mammedov,
Michael S. Tsirkin, Artyom Tarasenko, Mark Cave-Ayland
* Marc-André Lureau (marcandre.lureau@redhat.com) wrote:
> hostmem-file and hostmem-memfd use the whole object path for the
> memory region name, but hostname-ram uses only the path component (the
> basename):
>
> qemu -m 1024 -object memory-backend-ram,id=mem,size=1G -numa node,memdev=mem -monitor stdio
> (qemu) info ramblock
> Block Name PSize Offset Used Total
> mem 4 KiB 0x0000000000000000 0x0000000040000000 0x0000000040000000
>
> qemu -m 1024 -object memory-backend-file,id=mem,size=1G,mem-path=/tmp/foo -numa node,memdev=mem -monitor stdio
> (qemu) info ramblock
> Block Name PSize Offset Used Total
> /objects/mem 4 KiB 0x0000000000000000 0x0000000040000000 0x0000000040000000
>
> qemu -m 1024 -object memory-backend-memfd,id=mem,size=1G -numa node,memdev=mem -monitor stdio
> (qemu) info ramblock
> Block Name PSize Offset Used Total
> /objects/mem 4 KiB 0x0000000000000000 0x0000000040000000 0x0000000040000000
>
> Use the whole path name with >= 3.1. Having a consistent naming allow
> to migrate to different hostmem backends.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> include/hw/compat.h | 6 +++++-
> backends/hostmem-ram.c | 47 ++++++++++++++++++++++++++++++++++++++----
> 2 files changed, 48 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 6f4d5fc647..8ce7a7057b 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -2,7 +2,11 @@
> #define HW_COMPAT_H
>
> #define HW_COMPAT_3_0 \
> - /* empty */
> + {\
> + .driver = "memory-backend-ram",\
> + .property = "x-component-name",\
> + .value = "true",\
> + },
I don't think this needs to be an 'x-' - I think we originally used
those just for eXperimental things.
However, that's a minor nit, so:
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> #define HW_COMPAT_2_12 \
> {\
> diff --git a/backends/hostmem-ram.c b/backends/hostmem-ram.c
> index 7ddd08d370..a9eb99cf1b 100644
> --- a/backends/hostmem-ram.c
> +++ b/backends/hostmem-ram.c
> @@ -16,21 +16,56 @@
>
> #define TYPE_MEMORY_BACKEND_RAM "memory-backend-ram"
>
> +typedef struct RamMemoryBackend {
> + HostMemoryBackend parent;
> +
> + bool component_name;
> +} RamMemoryBackend;
> +
> +#define RAM_BACKEND(obj) \
> + OBJECT_CHECK(RamMemoryBackend, (obj), TYPE_MEMORY_BACKEND_RAM)
> +
> +static char *
> +ram_backend_get_name(RamMemoryBackend *self)
> +{
> + /* < 3.1 use the component as memory region name */
> + if (self->component_name) {
> + return object_get_canonical_path_component(OBJECT(self));
> + }
> +
> + return object_get_canonical_path(OBJECT(self));
> +}
>
> static void
> ram_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
> {
> - char *path;
> + char *name;
>
> if (!backend->size) {
> error_setg(errp, "can't create backend with size 0");
> return;
> }
>
> - path = object_get_canonical_path_component(OBJECT(backend));
> - memory_region_init_ram_shared_nomigrate(&backend->mr, OBJECT(backend), path,
> + name = ram_backend_get_name(RAM_BACKEND(backend));
> + memory_region_init_ram_shared_nomigrate(&backend->mr, OBJECT(backend), name,
> backend->size, backend->share, errp);
> - g_free(path);
> + g_free(name);
> +}
> +
> +static bool
> +ram_backend_get_component_name(Object *obj, Error **errp)
> +{
> + RamMemoryBackend *self = RAM_BACKEND(obj);
> +
> + return self->component_name;
> +}
> +
> +static void
> +ram_backend_set_component_name(Object *obj, bool value, Error **errp)
> +{
> + RamMemoryBackend *self = RAM_BACKEND(obj);
> +
> + self->component_name = value;
> }
>
> static void
> @@ -39,11 +74,15 @@ ram_backend_class_init(ObjectClass *oc, void *data)
> HostMemoryBackendClass *bc = MEMORY_BACKEND_CLASS(oc);
>
> bc->alloc = ram_backend_memory_alloc;
> + object_class_property_add_bool(oc, "x-component-name",
> + ram_backend_get_component_name,
> + ram_backend_set_component_name, &error_abort);
> }
>
> static const TypeInfo ram_backend_info = {
> .name = TYPE_MEMORY_BACKEND_RAM,
> .parent = TYPE_MEMORY_BACKEND,
> + .instance_size = sizeof(RamMemoryBackend),
> .class_init = ram_backend_class_init,
> };
>
> --
> 2.19.0.rc1
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 9/9] hostmem-ram: use whole path for memory region name with >= 3.1
2018-09-12 12:55 ` [Qemu-devel] [PATCH 9/9] hostmem-ram: use whole path for memory region name with >= 3.1 Marc-André Lureau
2018-09-13 10:19 ` Dr. David Alan Gilbert
@ 2018-10-29 15:16 ` Igor Mammedov
1 sibling, 0 replies; 29+ messages in thread
From: Igor Mammedov @ 2018-10-29 15:16 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum,
dgilbert, Richard Henderson, Andreas Färber,
Michael S. Tsirkin, Artyom Tarasenko, Mark Cave-Ayland
On Wed, 12 Sep 2018 16:55:31 +0400
Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> hostmem-file and hostmem-memfd use the whole object path
maybe add something along the lines:
consisting from user supplied 'id' and ...
> for the
> memory region name, but hostname-ram uses only the path component (the
> basename):
so path component would mean a concrete thing.
>
> qemu -m 1024 -object memory-backend-ram,id=mem,size=1G -numa node,memdev=mem -monitor stdio
> (qemu) info ramblock
> Block Name PSize Offset Used Total
> mem 4 KiB 0x0000000000000000 0x0000000040000000 0x0000000040000000
>
> qemu -m 1024 -object memory-backend-file,id=mem,size=1G,mem-path=/tmp/foo -numa node,memdev=mem -monitor stdio
> (qemu) info ramblock
> Block Name PSize Offset Used Total
> /objects/mem 4 KiB 0x0000000000000000 0x0000000040000000 0x0000000040000000
>
> qemu -m 1024 -object memory-backend-memfd,id=mem,size=1G -numa node,memdev=mem -monitor stdio
> (qemu) info ramblock
> Block Name PSize Offset Used Total
> /objects/mem 4 KiB 0x0000000000000000 0x0000000040000000 0x0000000040000000
>
> Use the whole path name with >= 3.1. Having a consistent naming allow
> to migrate to different hostmem backends.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> include/hw/compat.h | 6 +++++-
> backends/hostmem-ram.c | 47 ++++++++++++++++++++++++++++++++++++++----
> 2 files changed, 48 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 6f4d5fc647..8ce7a7057b 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -2,7 +2,11 @@
> #define HW_COMPAT_H
>
> #define HW_COMPAT_3_0 \
> - /* empty */
> + {\
> + .driver = "memory-backend-ram",\
> + .property = "x-component-name",\
> + .value = "true",\
> + },
>
> #define HW_COMPAT_2_12 \
> {\
> diff --git a/backends/hostmem-ram.c b/backends/hostmem-ram.c
> index 7ddd08d370..a9eb99cf1b 100644
> --- a/backends/hostmem-ram.c
> +++ b/backends/hostmem-ram.c
> @@ -16,21 +16,56 @@
>
> #define TYPE_MEMORY_BACKEND_RAM "memory-backend-ram"
>
> +typedef struct RamMemoryBackend {
> + HostMemoryBackend parent;
> +
> + bool component_name;
> +} RamMemoryBackend;
> +
> +#define RAM_BACKEND(obj) \
> + OBJECT_CHECK(RamMemoryBackend, (obj), TYPE_MEMORY_BACKEND_RAM)
> +
> +static char *
> +ram_backend_get_name(RamMemoryBackend *self)
> +{
> + /* < 3.1 use the component as memory region name */
I'd drop comment here, it belongs more to HW_COMPAT_...
> + if (self->component_name) {
> + return object_get_canonical_path_component(OBJECT(self));
> + }
> +
> + return object_get_canonical_path(OBJECT(self));
> +}
>
> static void
> ram_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
> {
> - char *path;
> + char *name;
>
> if (!backend->size) {
> error_setg(errp, "can't create backend with size 0");
> return;
> }
>
> - path = object_get_canonical_path_component(OBJECT(backend));
> - memory_region_init_ram_shared_nomigrate(&backend->mr, OBJECT(backend), path,
> + name = ram_backend_get_name(RAM_BACKEND(backend));
> + memory_region_init_ram_shared_nomigrate(&backend->mr, OBJECT(backend), name,
> backend->size, backend->share, errp);
> - g_free(path);
> + g_free(name);
> +}
> +
> +static bool
> +ram_backend_get_component_name(Object *obj, Error **errp)
> +{
> + RamMemoryBackend *self = RAM_BACKEND(obj);
> +
> + return self->component_name;
> +}
> +
> +static void
> +ram_backend_set_component_name(Object *obj, bool value, Error **errp)
> +{
> + RamMemoryBackend *self = RAM_BACKEND(obj);
> +
> + self->component_name = value;
> }
>
> static void
> @@ -39,11 +74,15 @@ ram_backend_class_init(ObjectClass *oc, void *data)
> HostMemoryBackendClass *bc = MEMORY_BACKEND_CLASS(oc);
>
> bc->alloc = ram_backend_memory_alloc;
> + object_class_property_add_bool(oc, "x-component-name",
If we would convert initial RAM to backend/frontend model,
we would actually need to keep current ramblock naming scheme for
ram_backend and even do this /i.e. non path variant/ for other
backends to keep migration stream compatible.
How about adding this property to generic memory-backend and
naming it something like 'x-use-canonical-path-for-ramblock-id'
I wonder if there is a point to use path for ramblock at all,
considering that it is single name space for all backends anyways.
Maybe by default we should just use 'id' everywhere?
> + ram_backend_get_component_name,
> + ram_backend_set_component_name, &error_abort);
+ object_class_property_set_description(...)
> }
>
> static const TypeInfo ram_backend_info = {
> .name = TYPE_MEMORY_BACKEND_RAM,
> .parent = TYPE_MEMORY_BACKEND,
> + .instance_size = sizeof(RamMemoryBackend),
> .class_init = ram_backend_class_init,
> };
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 0/9] hostmem-ram: use whole path for region name with >= 3.1
2018-09-12 12:55 [Qemu-devel] [PATCH 0/9] hostmem-ram: use whole path for region name with >= 3.1 Marc-André Lureau
` (8 preceding siblings ...)
2018-09-12 12:55 ` [Qemu-devel] [PATCH 9/9] hostmem-ram: use whole path for memory region name with >= 3.1 Marc-André Lureau
@ 2018-10-02 10:24 ` Marc-André Lureau
9 siblings, 0 replies; 29+ messages in thread
From: Marc-André Lureau @ 2018-10-02 10:24 UTC (permalink / raw)
To: QEMU
Cc: Eduardo Habkost, Michael S. Tsirkin, mark.cave-ayland,
Dr. David Alan Gilbert, Igor Mammedov, Paolo Bonzini,
Andreas Färber, atar4qemu, Richard Henderson
Hi
On Wed, Sep 12, 2018 at 4:57 PM Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Hi,
>
> The migration code expects the RAM block name to match between the
> source and destination. However the backend type may change, so it is
> possible (so far) to migrate from a hostmem-file to a hostmem-memfd
> for example.
>
> However, hostmem backends use inconsistent RAM naming, which can cause
> a name mismatch. hostmem-file and hostmem-memfd use the whole object
> path as memory name, while -ram use only the "component" (or basename)
> of the path.
>
> This series adds global/compat property support for objects others
> than TYPE_DEVICE (only TYPE_USER_CREATABLE for now), and a new compat
> property "x-component-name" to hostmem-ram. Machines >= 3.1 will use
> the whole path for the -ram memory name (like -file and -memfd).
The series now has a minor conflict in vl.c. I can resend if necessary.
Anyone to review the qdev/qom changes?
thanks
>
> Marc-André Lureau (9):
> qom/user-creatable: add a few helper macros
> accel: register global_props like machine globals
> qdev: move qdev_prop_register_global_list() to tests
> qom/globals: move qdev globals to qom
> qom/globals: generalize object_property_set_globals()
> qom/object: set globals when initializing object
> tests: add user-creatable test to test-qdev-global-props
> hw/i386: add pc-i440fx-3.1 & pc-q35-3.1
> hostmem-ram: use whole path for memory region name with >= 3.1
>
> include/hw/compat.h | 6 +-
> include/hw/i386/pc.h | 5 +-
> include/hw/qdev-core.h | 20 ---
> include/hw/qdev-properties.h | 34 +---
> include/qom/globals.h | 32 ++++
> include/qom/object_interfaces.h | 4 +
> accel/accel.c | 9 +-
> backends/hostmem-ram.c | 47 +++++-
> hw/core/machine.c | 2 +-
> hw/core/qdev-properties.c | 97 -----------
> hw/core/qdev.c | 6 -
> hw/i386/pc_piix.c | 15 +-
> hw/i386/pc_q35.c | 13 +-
> qom/cpu.c | 2 +-
> qom/globals.c | 81 ++++++++++
> qom/object.c | 6 +-
> qom/object_interfaces.c | 9 +-
> target/i386/cpu.c | 2 +-
> target/sparc/cpu.c | 2 +-
> ...dev-global-props.c => test-global-props.c} | 150 +++++++++++++++---
> vl.c | 8 +-
> qom/Makefile.objs | 2 +-
> tests/Makefile.include | 4 +-
> 23 files changed, 350 insertions(+), 206 deletions(-)
> create mode 100644 include/qom/globals.h
> create mode 100644 qom/globals.c
> rename tests/{test-qdev-global-props.c => test-global-props.c} (71%)
>
> --
> 2.19.0.rc1
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 29+ messages in thread