From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41983) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YsDQp-0008He-Hz for qemu-devel@nongnu.org; Tue, 12 May 2015 12:53:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YsDQk-0007Fm-O3 for qemu-devel@nongnu.org; Tue, 12 May 2015 12:53:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46498) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YsDQk-0007Dy-Dd for qemu-devel@nongnu.org; Tue, 12 May 2015 12:53:18 -0400 Date: Tue, 12 May 2015 17:53:11 +0100 From: "Daniel P. Berrange" Message-ID: <20150512165310.GQ18200@redhat.com> References: <1430476206-26034-1-git-send-email-berrange@redhat.com> <1430476206-26034-7-git-send-email-berrange@redhat.com> <554CF626.5000009@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <554CF626.5000009@suse.de> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 6/7] qom: add a object_property_add_enum helper method Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?utf-8?Q?F=C3=A4rber?= Cc: Paolo Bonzini , qemu-devel@nongnu.org On Fri, May 08, 2015 at 07:45:10PM +0200, Andreas F=C3=A4rber wrote: > Am 01.05.2015 um 12:30 schrieb Daniel P. Berrange: >=20 > Looks good in general. Some minor nits below, and one limitation > possibly worth mentioning in the second paragraph of the commit message= : > It assumes a 1:1 mapping. I do guess most ones are, but I remember some > CPUID bits having different names for the same values, for instance. Worst case, in that edge case, we can simply not use this stricter enum property type - just carry on with existing custom property > > diff --git a/include/qom/object.h b/include/qom/object.h > > index bf76f7a..f6a2a9d 100644 > > --- a/include/qom/object.h > > +++ b/include/qom/object.h > > @@ -1271,6 +1271,25 @@ void object_property_add_bool(Object *obj, con= st char *name, > > Error **errp); > > =20 > > /** > > + * object_property_add_enum: > > + * @obj: the object to add a property to > > + * @name: the name of the property > > + * @typename: the name of the enum data type > > + * @get: the getter or NULL if the property is write-only. >=20 > %NULL >=20 > > + * @set: the setter or NULL if the property is read-only > > + * @errp: if an error occurs, a pointer to an area to store the erro= r > > + * > > + * Add a enum property using getters/setters. This function will ad= d a > > + * property of type 'enum'. >=20 > This is slightly ambiguous, as I understand it the type we're actually > using is the one in @typename, not "enum"? Yeah, you are right - this doc mistake is left over from a previous version before Paolo asked me to add the @typename parameter. > > diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c > > index 9f16cdb..de142e3 100644 > > --- a/tests/check-qom-proplist.c > > +++ b/tests/check-qom-proplist.c > > @@ -32,10 +32,28 @@ typedef struct DummyObjectClass DummyObjectClass; > > #define DUMMY_OBJECT(obj) \ > > OBJECT_CHECK(DummyObject, (obj), TYPE_DUMMY) > > =20 > > +typedef enum DummyAnimal DummyAnimal; > > + > > +enum DummyAnimal { > > + DUMMY_FROG, > > + DUMMY_ALLIGATOR, > > + DUMMY_PLATYPUS, > > + > > + DUMMY_LAST, > > +}; > > + > > +static const char *const dummyanimalmap[DUMMY_LAST + 1] =3D { >=20 > dummy_animal_map would be slightly easier to read. Sure > > +static void test_dummy_badenum(void) > > +{ > > + Error *err =3D NULL; > > + Object *parent =3D container_get(object_get_root(), > > + "/objects"); > > + DummyObject *dobj =3D DUMMY_OBJECT( > > + object_new_propv(TYPE_DUMMY, > > + parent, > > + "dummy0", > > + &err, > > + "bv", "yes", > > + "sv", "Hiss hiss hiss", > > + "av", "yeti", > > + NULL)); > > + > > + g_assert(dobj =3D=3D NULL); >=20 > Superfluous. >=20 > > + g_assert(err !=3D NULL); > > + g_assert(g_str_equal(error_get_pretty(err), > > + "Invalid parameter 'yeti'")); >=20 > Same question as in previous test: alternatives? Yep, will check Regards, Daniel --=20 |: http://berrange.com -o- http://www.flickr.com/photos/dberrange= / :| |: http://libvirt.org -o- http://virt-manager.or= g :| |: http://autobuild.org -o- http://search.cpan.org/~danberr= / :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vn= c :|