qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v3 4/7] qom: add object_new_propv / object_new_p


From: Daniel P. Berrange
Subject: Re: [Qemu-devel] [PATCH v3 4/7] qom: add object_new_propv / object_new_proplist constructors
Date: Tue, 12 May 2015 17:49:55 +0100
User-agent: Mutt/1.5.23 (2014-03-12)

On Fri, May 08, 2015 at 07:10:49PM +0200, Andreas Färber wrote:
> Hi Daniel/Paolo,
> 
> Am 01.05.2015 um 12:30 schrieb Daniel P. Berrange:
> > It is reasonably common to want to create an object, set a
> > number of properties, register it in the hierarchy and then
> > mark it as complete (if a user creatable type). This requires
> > quite a lot of error prone, verbose, boilerplate code to achieve.
> > 
> > The object_new_propv / object_new_proplist constructors will
> > simplify this task by performing all required steps in one go,
> > accepting the property names/values as variadic args.
> 
> With this I disagree. I can see the virtue of adding properties in one
> go via some handy varargs function. But,
> 
> 1) The function does something different from what its name implies to
> me. It does not create a prop or proplist - instead of adding them it
> sets existing ones. Suggest object_new_with_props()?

Sure, with_props() looks fine.

> 2) You seem to mix up *v and non-v functions. v is with va_list usually,
> compare tests/libqtest.h.

Ok, I didn't see that qemu had a convention on that, so will change
to match.

> 3) Object construction is a tricky thing to get right. Anthony chose to
> be stricter than C++ and not let object_new() fail, one of the reasons
> we have the distinct realize step. Can we keep the two separate? qdev
> with all its convenience helpers didn't mix those either.
> I.e., use object_new() without Error** followed by object_set_props() or
> anything with Error**. That tells you if there's an Error* you need to
> unref the object. Otherwise it's in an unknown state.

I don't really think that forcing the callers to call new + set_props
separately is really makng it more reliable - in fact the contrary - it
means that the callers have more complex boilerplate code which they all
have to tediously duplicate in exactly the same way. With the single
object_new_with_props call, you know that if it returns NULL then it
failed and you have no cleanup that you need todo which is about as
reliable as it gets.

That said, I can see the value in having a standalone object_set_props()
method as a general feature. So I will add that, and simply make the
object_new_with_props method call object_new + object_set_props + 

> 4) What's the use case for this? I'm concerned about encouraging people
> to hardcode properties like this, when doing it in C can let the
> compiler detect any mismatches.

I use it in the VNC server when I convert it to use generic TLS encryption
code over to use the QCryptoTLSCreds object - it reduced a 100+ line
method into just two calls to object_new_propv. See vnc_display_create_creds()
in this RFC patch:

  https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02062.html


Then, I've got a bunch of unit tests related to that series which are
using it, again to reduce the amount of code it takes to create and
set props on this TLS creds object.

> > 
> > Usage would be:
> > 
> >    Error *err = NULL;
> >    Object *obj;
> >    obj = object_new_propv(TYPE_MEMORY_BACKEND_FILE,
> >                           "/objects",
> 
> This is not an Object*. ;) I like it better as it's implemented below,
> but cf. above for mixing this Error**-ing operation with object_new().

Yep, that's a docs mistake.

> 
> >                           "hostmem0",
> >                           &err,
> >                           "share", "yes",
> >                           "mem-path", "/dev/shm/somefile",
> >                           "prealloc", "yes",
> >                           "size", "1048576",
> >                           NULL);
> > 
> > Note all property values are passed in string form and will
> > be parsed into their required data types.
> > 
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> >  include/qom/object.h       |  67 ++++++++++++++++
> >  qom/object.c               |  66 ++++++++++++++++
> >  tests/.gitignore           |   1 +
> >  tests/Makefile             |   5 +-
> >  tests/check-qom-proplist.c | 190 
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 328 insertions(+), 1 deletion(-)
> >  create mode 100644 tests/check-qom-proplist.c
> > 
> > diff --git a/include/qom/object.h b/include/qom/object.h
> > index d2d7748..15ac314 100644
> > --- a/include/qom/object.h
> > +++ b/include/qom/object.h
> > @@ -607,6 +607,73 @@ Object *object_new(const char *typename);
> >  Object *object_new_with_type(Type type);
> >  
> >  /**
> > + * object_new_propv:
> > + * @typename:  The name of the type of the object to instantiate.
> > + * @parent: the parent object
> > + * @id: The unique ID of the object
> > + * @errp: pointer to error object
> > + * @...: list of property names and values
> > + *
> > + * This function with initialize a new object using heap allocated memory.
> 
> Grammar. ("will"?)
> 
> > + * The returned object has a reference count of 1, and will be freed when
> > + * the last reference is dropped.
> > + *
> > + * The @id parameter will be used when registering the object as a
> > + * child of @parent in the objects hierarchy.
> 
> s/objects hierarchy/composition tree/
> 
> > + *
> > + * The variadic parameters are a list of pairs of (propname, propvalue)
> > + * strings. The propname of NULL indicates the end of the property
> 
> %NULL
> 
> > + * list. If the object implements the user creatable interface, the
> > + * object will be marked complete once all the properties have been
> > + * processed.
> > + *
> > + *   Error *err = NULL;
> > + *   Object *obj;
> > + *
> > + *   obj = object_new_propv(TYPE_MEMORY_BACKEND_FILE,
> > + *                          container_get(object_get_root(), "/objects")
> 
> If this is used in multiple places, please introduce a helper like I did
> for /machine. The reason being avoiding hardcoded string paths.

Sure, will do that.

> 
> > + *                          "hostmem0",
> > + *                          &err,
> > + *                          "share", "yes",
> > + *                          "mem-path", "/dev/shm/somefile",
> > + *                          "prealloc", "yes",
> > + *                          "size", "1048576",
> > + *                          NULL);
> > + *
> > + *   if (!obj) {
> > + *     g_printerr("Cannot create memory backend: %s\n",
> > + *                error_get_pretty(err));
> > + *   }
> 
> Please see in the top of the file for examples how to enclose sample code.

Ok.

> 
> > + *
> > + * The returned object will have one stable reference maintained
> > + * for as long as it is present in the object hierarchy.
> > + *
> > + * Returns: The newly allocated, instantiated & initialized object.
> > + */
> > +Object *object_new_propv(const char *typename,
> > +                         Object *parent,
> > +                         const char *id,
> > +                         Error **errp,
> > +                         ...)
> > +    __attribute__((sentinel));
> 
> First time I see this in QEMU - is it safe to use unconditionally?
> (clang, older gcc, etc.)

I'm actually not sure - will look into this.

> > +
> > +/**
> > + * object_new_proplist:
> > + * @typename:  The name of the type of the object to instantiate.
> > + * @parent: the parent object
> > + * @id: The unique ID of the object
> > + * @errp: pointer to error object
> > + * @vargs: list of property names and values
> > + *
> > + * See object_new_propv for documentation.
> 
> Needs to be object_new_propv() for referencing.

Ok

> 
> > + */
> > +Object *object_new_proplist(const char *typename,
> > +                            Object *parent,
> > +                            const char *id,
> > +                            Error **errp,
> > +                            va_list vargs);
> > +
> > +/**
> >   * object_initialize_with_type:
> >   * @data: A pointer to the memory to be used for the object.
> >   * @size: The maximum size available at @data for the object.
> > diff --git a/qom/object.c b/qom/object.c
> > index b8dff43..2115542 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -11,6 +11,7 @@
> >   */
> >  
> >  #include "qom/object.h"
> > +#include "qom/object_interfaces.h"
> >  #include "qemu-common.h"
> >  #include "qapi/visitor.h"
> >  #include "qapi-visit.h"
> > @@ -439,6 +440,71 @@ Object *object_new(const char *typename)
> >      return object_new_with_type(ti);
> >  }
> >  
> > +Object *object_new_propv(const char *typename,
> > +                         Object *parent,
> > +                         const char *id,
> > +                         Error **errp,
> > +                         ...)
> > +{
> > +    va_list vargs;
> > +    Object *obj;
> > +
> > +    va_start(vargs, errp);
> > +    obj = object_new_proplist(typename, parent, id, errp, vargs);
> > +    va_end(vargs);
> > +
> > +    return obj;
> > +}
> > +
> > +Object *object_new_proplist(const char *typename,
> > +                            Object *parent,
> > +                            const char *id,
> > +                            Error **errp,
> > +                            va_list vargs)
> > +{
> > +    Object *obj;
> > +    const char *propname;
> > +
> > +    obj = object_new(typename);
> > +
> > +    if (object_class_is_abstract(object_get_class(obj))) {
> 
> This check seems too late. If the type is abstract, object_new() will
> have aborted.

Ah, I didn't realize that it did that.

> 
> > +        error_setg(errp, "object type '%s' is abstract", typename);
> > +        goto error;
> > +    }
> > +
> > +    propname = va_arg(vargs, char *);
> > +    while (propname != NULL) {
> > +        const char *value = va_arg(vargs, char *);
> > +
> > +        g_assert(value != NULL);
> > +        object_property_parse(obj, value, propname, errp);
> > +        if (*errp) {
> 
> This pattern is wrong. You need a local Error *err = NULL;, otherwise
> you may be deferencing NULL.
> 
> > +            goto error;
> > +        }
> > +        propname = va_arg(vargs, char *);
> > +    }
> > +
> > +    object_property_add_child(parent, id, obj, errp);
> > +    if (*errp) {
> > +        goto error;
> > +    }
> > +
> > +    if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> > +        user_creatable_complete(obj, errp);
> > +        if (*errp) {
> > +            object_unparent(obj);
> > +            goto error;
> > +        }
> > +    }
> > +
> > +    object_unref(OBJECT(obj));
> > +    return obj;
> > +
> > + error:
> 
> Intentionally indented?

> > diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
> > new file mode 100644
> > index 0000000..9f16cdb
> > --- /dev/null
> > +++ b/tests/check-qom-proplist.c


> > +#define TYPE_DUMMY "qemu:dummy"
> 
> Is colon considered valid in a type name?

Nothing complained, but I can trivially remove the colon.

> > +#define DUMMY_OBJECT(obj)                               \
> > +    OBJECT_CHECK(DummyObject, (obj), TYPE_DUMMY)
> > +
> > +struct DummyObject {
> > +    Object parent;
> 
> parent_obj for consistency please.
> 
> > +
> > +    bool bv;
> > +    char *sv;
> > +};
> > +
> > +struct DummyObjectClass {
> > +    ObjectClass parent;
> 
> parent_class for consistency. If you copied these, please indicate from
> where so that we can fix that.

This is just the naming convention used by GObject and I hadn't noticed
that QEMU had its own convention. I'll change these as you suggest.


> > +static void test_dummy_createv(void)
> > +{
> > +    Error *err = NULL;
> > +    Object *parent = container_get(object_get_root(),
> > +                                   "/objects");
> > +    DummyObject *dobj = DUMMY_OBJECT(
> > +        object_new_propv(TYPE_DUMMY,
> > +                         parent,
> > +                         "dummy0",
> > +                         &err,
> > +                         "bv", "yes",
> > +                         "sv", "Hiss hiss hiss",
> > +                         NULL));
> > +
> > +    g_assert(dobj != NULL);
> 
> I believe DUMMY_OBJECT() would assert in that case already. There is
> object_dynamic_cast() in case that is undesired.

Ah, good point.

> > +    g_assert(err == NULL);
> > +    g_assert(g_str_equal(dobj->sv, "Hiss hiss hiss"));
> 
> Isn't there a GTester string comparison function for this that outputs
> both strings in case of a mismatch?

Possibly, I'll look into that.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|



reply via email to

[Prev in Thread] Current Thread [Next in Thread]