[Qemu-devel] [PATCH] qom: add object_property_is_set

[Qemu-devel] [PATCH] qom: add object_property_is_set

[Marcel Apfelbaum]

Sometimes is not enough to get property's value,
but it is needed to know if the value was actually set.

This is especially useful when querying bool properties
and having different defaults on different scenarios.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 include/qom/object.h | 11 +++++++++++
 qom/object.c         | 12 ++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index e0ff212..9257bb1 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -319,6 +319,7 @@ typedef struct ObjectProperty
 {
     gchar *name;
     gchar *type;
+    bool is_set;
     ObjectPropertyAccessor *get;
     ObjectPropertyAccessor *set;
     ObjectPropertyRelease *release;
@@ -931,6 +932,16 @@ void object_property_set(Object *obj, struct Visitor *v, const char *name,
                          Error **errp);
 
 /**
+ * object_property_is_set:
+ * @obj: the object
+ * @name: the name of the property
+ * @errp: returns an error if this function fails
+ *
+ * Returns: true if object's property is set, false otherwise.
+ */
+bool object_property_is_set(Object *obj, const char *name,
+                            Error **errp);
+/**
  * object_property_parse:
  * @obj: the object
  * @string: the string that will be used to parse the property value.
diff --git a/qom/object.c b/qom/object.c
index 62e7e41..229a601 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -817,9 +817,21 @@ void object_property_set(Object *obj, Visitor *v, const char *name,
         error_set(errp, QERR_PERMISSION_DENIED);
     } else {
         prop->set(obj, v, prop->opaque, name, errp);
+        prop->is_set = true;
     }
 }
 
+bool object_property_is_set(Object *obj, const char *name,
+                            Error **errp)
+{
+    ObjectProperty *prop = object_property_find(obj, name, errp);
+    if (prop == NULL) {
+        return false;
+    }
+
+    return prop->is_set;
+}
+
 void object_property_set_str(Object *obj, const char *value,
                              const char *name, Error **errp)
 {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] qom: add object_property_is_set

[Paolo Bonzini]

Il 18/02/2014 17:46, Marcel Apfelbaum ha scritto:
> Sometimes is not enough to get property's value,
> but it is needed to know if the value was actually set.
>
> This is especially useful when querying bool properties
> and having different defaults on different scenarios.

I think this needs to go together with the use, so that I can understand 
what exactly you need.

Is it because the bool property is "bool *" and thus cannot be set to 
(for example) -1 to indicate the default value?

Paolo

Re: [Qemu-devel] [PATCH] qom: add object_property_is_set

[Marcel Apfelbaum]

On Tue, 2014-02-18 at 17:51 +0100, Paolo Bonzini wrote:
> Il 18/02/2014 17:46, Marcel Apfelbaum ha scritto:
> > Sometimes is not enough to get property's value,
> > but it is needed to know if the value was actually set.
> >
> > This is especially useful when querying bool properties
> > and having different defaults on different scenarios.
> 
> I think this needs to go together with the use, so that I can understand 
> what exactly you need.
It is used to replace qemu_opt_get_bool that provides a
parameter for a default value. In this case we need to
differentiate "no value" from "false."

I could send it with QemuMachine QOMify feature,
but this will be a fairly large series and I *really* want
to minimize it as much as I can, this is why I release small,
stand-alone patches, that fixes some issue (see [Qemu-devel]
[PATCH] qapi: output visitor crashes qemu if it encounters a NULL value),
or adds a fairly useful feature like this one.

I understand the downside of adding a feature without using it,
but in the same time I want to get maintainers feedback and
make my series as small as possible, bigger ones are harder to submit.

Thanks for the review!
Marcel

> 
> Is it because the bool property is "bool *" and thus cannot be set to 
> (for example) -1 to indicate the default value?
Exactly, but I suppose it can match every property that all its value range is valid.
> 
> Paolo

Re: [Qemu-devel] [PATCH] qom: add object_property_is_set

[Paolo Bonzini]

Il 18/02/2014 18:11, Marcel Apfelbaum ha scritto:
> It is used to replace qemu_opt_get_bool that provides a
> parameter for a default value. In this case we need to
> differentiate "no value" from "false."

But what would the getter return in that case?  If possible, it's better 
to initialize to the default value in an instance_init method.

> I could send it with QemuMachine QOMify feature,
> but this will be a fairly large series and I *really* want
> to minimize it as much as I can, this is why I release small,
> stand-alone patches, that fixes some issue (see [Qemu-devel]
> [PATCH] qapi: output visitor crashes qemu if it encounters a NULL value),
> or adds a fairly useful feature like this one.

It depends, features are not necessarily useful without users.

Paolo

Re: [Qemu-devel] [PATCH] qom: add object_property_is_set

[Andreas Färber]

Am 18.02.2014 18:26, schrieb Paolo Bonzini:
> Il 18/02/2014 18:11, Marcel Apfelbaum ha scritto:
>> It is used to replace qemu_opt_get_bool that provides a
>> parameter for a default value. In this case we need to
>> differentiate "no value" from "false."
> 
> But what would the getter return in that case?  If possible, it's better
> to initialize to the default value in an instance_init method.

Another issue I see is that it's currently a valid use case to use a
setter in instance_init to set default values. Doing so would flag the
property as set with this patch.

To me it sounded like a concept similar to component-oriented IDEs where
non-default values are shown in bold. We'd need a QMP API for that
however, and we'd need to reset properties in instance_post_init to
unset then (globals would be considered unset in that case).

Another aspect is that dynamic properties are slightly awkward, if we
think of setting the rtc, which then advances and reads back differently.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] qom: add object_property_is_set

[Marcel Apfelbaum]

On Tue, 2014-02-18 at 18:26 +0100, Paolo Bonzini wrote:
> Il 18/02/2014 18:11, Marcel Apfelbaum ha scritto:
> > It is used to replace qemu_opt_get_bool that provides a
> > parameter for a default value. In this case we need to
> > differentiate "no value" from "false."
> 
> But what would the getter return in that case?  If possible, it's better 
> to initialize to the default value in an instance_init method.
I thought about it, but what if from different places you want different
defaults? Meaning, give me prop value, but if not set choose x (or y)

> 
> > I could send it with QemuMachine QOMify feature,
> > but this will be a fairly large series and I *really* want
> > to minimize it as much as I can, this is why I release small,
> > stand-alone patches, that fixes some issue (see [Qemu-devel]
> > [PATCH] qapi: output visitor crashes qemu if it encounters a NULL value),
> > or adds a fairly useful feature like this one.
> 
> It depends, features are not necessarily useful without users.
Agreed, I will make this as part of my series.

Thanks,
Marcel

> 
> Paolo

Re: [Qemu-devel] [PATCH] qom: add object_property_is_set

[Marcel Apfelbaum]

On Tue, 2014-02-18 at 18:49 +0100, Andreas Färber wrote:
> Am 18.02.2014 18:26, schrieb Paolo Bonzini:
> > Il 18/02/2014 18:11, Marcel Apfelbaum ha scritto:
> >> It is used to replace qemu_opt_get_bool that provides a
> >> parameter for a default value. In this case we need to
> >> differentiate "no value" from "false."
> > 
> > But what would the getter return in that case?  If possible, it's better
> > to initialize to the default value in an instance_init method.
> 
> Another issue I see is that it's currently a valid use case to use a
> setter in instance_init to set default values. Doing so would flag the
> property as set with this patch.
Hi Andreas, thank you for the review!

As I replayed to Paolo, this does not contradict the patch's aim.
Meaning, if you have a default for all cases, is ok to init it and make
it "set". The interesting case is if you have 2 places: one that you want
the value to be "x" if is not set, and other place that you need "y" if
is not set.

> 
> To me it sounded like a concept similar to component-oriented IDEs where
> non-default values are shown in bold. We'd need a QMP API for that
> however, and we'd need to reset properties in instance_post_init to
> unset then (globals would be considered unset in that case).
I thought about it, but it seemed to me over-engineering *for my case*,
where all I need to know if the user supplied the value or not,
not need to "unset" it.

> 
> Another aspect is that dynamic properties are slightly awkward, if we
> think of setting the rtc, which then advances and reads back differently.
Sadly I am not familiar with the "rtc", but as I explained before,
I don't need the "repeatedly set/unset" use-case.

Thanks,
Marcel
   
> 
> Regards,
> Andreas
>