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

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.

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/ :|