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

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

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(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list