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

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.