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

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.