[libvirt] [PATCH RFC 2/9] conf: Introduce virPoolObj and virPoolTableObj and PoolObj API's

Wed Mar 1 10:33:36 UTC 2017

On Wed, Mar 01, 2017 at 10:06:20AM +0000, Daniel P. Berrange wrote:
>On Wed, Mar 01, 2017 at 10:56:16AM +0100, Martin Kletzander wrote:
>> On Wed, Mar 01, 2017 at 09:42:37AM +0000, Daniel P. Berrange wrote:
>> > On Wed, Mar 01, 2017 at 10:27:32AM +0100, Martin Kletzander wrote:
>> > > On Wed, Mar 01, 2017 at 09:06:13AM +0000, Daniel P. Berrange wrote:
>> > > > On Wed, Mar 01, 2017 at 09:55:03AM +0100, Martin Kletzander wrote:
>> > > > > On Tue, Feb 28, 2017 at 04:39:52PM +0000, Daniel P. Berrange wrote:
>> > > > > > FWIW, the virObject framework as it exists today was just the bare
>> > > > > > minimum we needed in order to get a basic inheritance system up and
>> > > > > > running in libvirt. I rather expected that we would extend it in the
>> > > > > > future to add further concepts, inspired/borrowed from glib (which
>> > > > > > is what stimulated my creation of virObject in the first place). In
>> > > > > > particular I could see
>> > > > > >
>> > > > > > - Interface support - the virObjectListable concept sounds like the
>> > > > > >   kind of thing that would be suited to an interface, rather than a
>> > > > > >   subclass, as it lets different objects support the API contract
>> > > > > >   without forcing a common ancestor which might not be appropriate
>> > > > > >
>> > > > >
>> > > > > This is really interesting and bunch of ideas *how* this would be
>> > > > > implemented pop into my mind.  I like the overall idea of using more
>> > > > > virObject goodness and expanding it so that it can to more.
>> > > > >
>> > > > > I thing I get why interfaces are more suitable in some cases.  Having
>> > > > > 'lockable'  and 'poolable' interfaces, for example, make it possible to
>> > > > > have classes that are lockable, poolable, both, or none of those things
>> > > > > without having three intermediate classes to derive from.
>> > > > >
>> > > > > I do not understand one tiny thing, though.  Why is interface good for
>> > > > > this one particular occasion when every poolable object will always need
>> > > > > to be lockable?  I think in this particular case having
>> > > > > virObjectPoolable derive from virObjectLockable is pretty reasonable.
>> > > >
>> > > > I didn't think about that specific case too much. Considering the
>> > > > integration with the access control code, however, the access control
>> > > > driver needs to obtain 1 or more identifying attributes from an object
>> > > > to serve as an "identity" against which access rules are written.
>> > > > Rather than having the access control code know about specific objects,
>> > > > you could invent a "virObjectIdentity" struct which contains a generic
>> > > > list of key+value pairs and then define an virOobjectIdentifier
>> > > > interface which contained a single method that returned a virObjectIdentity
>> > > > instance.  This would decouple the access control code from the specific
>> > > > config classes we use.
>> > > >
>> > >
>> > > Os, so it's about the ACL code.  That makes sense, yes.
>> > >
>> > > > > One more thing.  Can we do something about the static type-safety of
>> > > > > virObject APIs in the future?  The only idea I could come up with is
>> > > > > optional GCC plugin tailored to our usage, but that seems cumbersome.
>> > > >
>> > > > Any particular parts you are thinking about ?
>> > > >
>> > >
>> > > I'm talking about the fact that if you want to have a function (say
>> > > virObjectLock() for example) for a class, it must accept void * if it's
>> > > supposed to be called on an object of a subclass as well.  We are
>> > > checking whether it's the right class and if that function can be called
>> > > on that object, yada yada, but that is done in runtime and not
>> > > compile-time.  My question was whether there are any ideas how ti make
>> > > it more safe during compile-time.
>> >
>> > It doesn't have to accept void *. That is just a convenience to avoid need
>> > for type casting - I'd suggest such usage of 'void *' be only done for the
>> > top most classes virObject & virObjectLockable. Once we get more specialized
>> > we should require explicit casts, which will give us a greater degree of
>> > compile time checking - of course it doesn't prevent people doing incorrect
>> > casts, but it does at least force the programmer to think about what object
>> > they are passing in.
>> >
>> > Also note, when I say "use casts", I don't mean use a plain C language
>> > cast - I mean use something that actually validates the object type
>> > dynamically.
>> >
>> > In GLib (and QEMU's object model), for each class you define, you add
>> > a macro for doing casts to objects of that class.
>> >
>> > eg if you had a  virObjectPoolable, you would define a macro
>> >
>> >  #define  VIR_OBJECT_POOLABLE(obj) \
>> >        VIR_OBJECT_CHECK(obj, virObjectPoolableClass)
>> >
>> > and the VIR_OBJECT_CHECK macro would call virObjectIsClass() and
>> > return NULL if it fails. Every API against the object would have
>> > to have a check for NULL present so you could blindly do
>> >
>> >     virObjectPoolableSomeMethod(VIR_OBJECT_POOLABLE(dom))
>> >
>> > With GLib & QENMU, VIR_OBJECT_CHECK would in fact assert() if the
>> > wrong class was passed, avoidig the need for NULL checks in every
>> > method, but we avoid assert in libvirt.
>> >
>>
>> As I understand it, that's still done during runtime.  And since every
>> single call to virObjectPoolable* would require you to call it with the
>> macro, it's not that different to putting the functionality of the macro
>> inside that function.
>
>It catches the case where you do
>
>   virObjectPoolableSomeMethod(wibble)
>
>and wibble is some unrelated type virWibble.  If virObjectPoolableSomeMethod
>took 'void *' you'd get no warning at compile time. Forcing the explicit cast
>using VIR_OBJECT_POOLABLE encourages users to think about the type they are
>passing in, so lets some, but certinly not all, bugs be avoided at compile
>time. You do still need the runtime type check too, to avoid crash at
>runtime if the user intentionally did a bad cast.
>
>You could get a stronger compile time check if you added one macro for
>each parent class of an object. ie if virObjectPoolable, inherited from
>virObjectLockable you could define
>
> #define VIR_OBJECT_POOLABLE_AS_OBJECT_LOCKABLE(obj) \
>        return &(obj->parent)
>
> #define VIR_OBJECT_POOLABLE_AS_OBJECT(obj) \
>        return &(obj->parent->parent)
>
>this becomes somewhat tedious if you have deep hierarchies, but then we
>don't have that problem in libvirt, so perhaps its reasonable
>

Yeah, but that's really not very readable if you have more calls like
that.  But in that case you can just have a function for each class,
e.g.:

virObjectRef
virObjectLockableRef
virObjectPoolableRef
...

Which then defeats the point of inheritance.  Also none of the
approaches will work with _really_ opaque pointers (which I believe we
use at least references on).  I mean opaque from the POV of the function
calling virObjectRef.

>Regards,
>Daniel
>--
>|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
>|: http://libvirt.org              -o-             http://virt-manager.org :|
>|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170301/f5f9db16/attachment-0001.sig>