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

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

- Signal support - a generalization of our current event reporting system to integrate directly with the object system

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