Re: [libvirt] [PATCH v3 00/16] virObject adjustments for common object

On Thu, Jun 22, 2017 at 10:02:30AM -0400, John Ferlan wrote: Let's move the discussion [1] into correct place. What's wrong, or better wording, what I don't like personally about this approach is that in order to have features like add, search, lookup, remove it would require that the object is derived from virObjectLookupKeys. The only thing it adds to the simple virObject is two variables @uuid and @name. We don't need the virObjectLookupKeys object at all. For example, current add/remove API is: ==== virObjectLookupHashNew ==== virObjectLookupHashNew(virClassPtr klass, int tableElemsStart); could be virObjectListNew(int tableSize, bool useName, bool useUUID); or typedef enum { VIR_OBJECT_LIST_TABLE_NAME = (1 << 0), VIR_OBJECT_LIST_TABLE_UUID = (1 << 1), } virObjectListTableType; virObjectListNew(int tableSize, unsigned int flags); I don't see why the virObjectLookupHashNew() has to have the @klass parameter and the @tableElemsStart is not correct, it's not the number of elements, it's has table size. ==== virObjectLookupHash(Add|Remove) ==== virObjectLookupHashAdd(void *tableobj, virObjectLookupKeysPtr obj); virObjectLookupHashRemove(void *tableobj, virObjectLookupKeysPtr obj); The only difference without the virObjectLookupKeys object would be that the API will take two more parameters to give it the @uuid and @name. virObjectListAdd(virObjectListPtr listObj, void *obj, const char *name, const char *uuid); virObjectListRemove(virObjectListPtr listObj, const char *name, const char *uuid); or virObjectListRemoveByName(virObjectListPtr listObj, const char *name); virObjectListRemoveByUUID(virObjectListPtr listObj, const char *uuid); Yes, at first it may looks worse, but on the other hand it gives you better flexibility in the way that the object added/removed from the list doesn't even have to have the exact same variables and it will work with every object that is derived from virObject. ==== virObjectLookupHashFind ==== virObjectLookupHashFind(void *tableobj, bool useUUID, const char *key); could be split into two APIs: virObjectListFindByName(virObjectListPtr listObj, const char *name); virObjectListFindByUUID(virObjectListPtr listObj, const char *UUID); Which in my opinion is way better than having a single API with a boolean parameter that switches what type of key it is. ==== virObjectLookupHashForEach ==== virObjectLookupHashForEach(void *tableobj, bool useUUID, virHashIterator callback, virObjectLookupHashForEachDataPtr data); could be virObjectListForEach(virObjectListPtr listObj, virHashIterator callback, void *callbackData); There are two things that I don't like. There is no need to have the @useUUID because both tables should contain the same objects if both are used so this can be hidden from the user and the API will simply use one of the available tables. The second issue is that this API forces you to use some predefined data structure and you cannot create your own specifically tailored for the task that you are about to do. The @useUUID argument also applies to virObjectLookupHashSearch(). The usage of the virObjectList* APIs could be something like this: virObjectListPtr virInterfaceObjListNew(void) { return virObjectListNew(10, VIR_OBJECT_LIST_TABLE_NAME); } static virInterfaceObjPtr virInterfaceObjListFindByName(virObjectListPtr interfaces, const char *name) { return virObjectListFindByName(interfaces, name); } Pavel