On Tue, Feb 28, 2017 at 04:39:52PM +0000, Daniel P. Berrange wrote:
On Tue, Feb 28, 2017 at 04:52:31PM +0100, Pavel Hrdina wrote:On Tue, Feb 28, 2017 at 04:15:42PM +0100, Michal Privoznik wrote: > On 02/11/2017 05:29 PM, John Ferlan wrote: > > +struct _virPoolDef { > > + char *uuid; > > + char *name; > > +}; > > + > > +struct _virPoolObj { > > + virObjectLockable parent; > > + > > + /* copy of the def->name and def->uuid (if available) used for lookups > > + * without needing to know the object type */ > > + virPoolDefPtr pooldef; > > + > > + /* Boolean states that can be managed by consumer */ > > + bool active; /* object is active or not */ > > + bool beingRemoved; /* object being prepared for removal */ > > + bool autostart; /* object is autostarted */ > > + bool persistent; /* object definition is persistent */ > > + bool updated; /* object config has been updated in some way */ > > + > > + /* Boolean states managed by virPoolObj */ > > + bool removing; /* true when object has been removed from table(s) */ > > + > > + /* 'def' is the current config definition. > > + * 'newDef' is the next boot configuration. > > + * The 'type' value will describe which _vir*Def struct describes > > + * The 'privateData' will be private object data for each pool obj > > + * and specific to the 'type' of object being managed. */ > > + void *def; > > + void *newDef; > > + virFreeCallback defFreeFunc; > > + > > + /* Private data to be managed by the specific object using > > + * vir{$OBJ}ObjPrivate{Get|Set}{$FIELD} type API's. Each of those > > + * calling the virPoolObjGetPrivateData in order to access this field */ > > + void *privateData; > > + void (*privateDataFreeFunc)(void *); > > +}; > > So we are creating a new object to be put into hash table. How does this > work with the actual object then? I mean, take patch 6/9 for instance. > virSecretObj is actual object. Now, when dealing with virPoolObjList, > it's just virPoolObj which is locked and not the actual virSecretObj. > I'm worried that this could cause some trouble. > Maybe virSecret is not the best example, because it doesn't usually get > unlocked & locked again throughout virSecret APIs. Maybe virDomain would > be a better example. > > A-ha! after going through all the patches, this new virPoolObj is a > merge of all the vir*Obj (virSecretObj, virNetworkObj, virNodeDeviceObj, > ...). So the issue I'm describing above will never occur. > > I wonder whether we can change this code so that we don't have to change > all those objects to virPoolObj. I mean, our virObject supports morphism > therefore we should be able to have APIs that are able to work over > different classes derived from virObject. For instance: > > virSecretObjPtr sec; > virNodeDeviceObjPtr node; > > virObjectListEndAPI(&sec); > virObjectListEndAPI(&node); > > > The same way we can call: > > virObjectLock(sec); > virObjectLock((node); > > currently. This would help us to ensure type safeness when passing args > to functions. > > One approach that comes to my mind right now as I'm writing these lines > (read as "I haven't thought it through properly yet") is to create a new > type of virObject. Currently we have virObject and virObjectLockable. > What if we would have virObjectListable which would be derived from > virObjectLockable. All those driver objects (virSecretObj for instance) > would be derived from virObjectListable then. And in virObjectListable > you could hold all those booleans you need. This would have the benefit > of not dropping virSecretObj and thus rewriting half of libvirt. I agree that the virPoolObj is not the best way to go. This is similar to what I had in mind while reading this series. We should definitely preserve all those object as they are. I had a similar approach in my mind, it would basically follow how the virDomainObjList is done, however the name would be generic (virObjectList, virObjectListable or virObjectTable) with general APIs for searching by using a function pointer, inserting, removing, etc. And each object type would implement specific stuff. This would nicely follow how the real object-inheritance is done in class oriented languages. I definitely like the idea of having unified approach how to handle all lists that we use in our code and this would give you the tool how to do it. The virPoolObj kind of force you to use it.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. 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.
- Signal support - a generalization of our current event reporting system to integrate directly with the object system
- Job control support - similarly to the signal support, we could have
generalized job control for any object. This
could be an Interface for Lockable objects.
- Weak references - this is a concept which is useful with dealing with event callback registration. GLib uses this concept to dynamically kill off signal handlers when an object is disposed. This avoids problem we currently have where event handlers keep an object alive as long as they are registered, so you leak if you forget to unregister everything In any case, I agree with the idea that it would be desirable to try and model these pool concepts more rexplicitly in the object system, so that we can maintain type safety instead of turning lots of things into void* opaque pointers. 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@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
