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

[Pavel Hrdina]

On Thu, Jun 22, 2017 at 10:02:30AM -0400, John Ferlan wrote:

Let's move the discussion [1] into correct place.

> Still, I must be missing something. Why is it wrong to create a new
> object that would have a specific use? virObjectLockable was created at
> some point in history and then used as a way to provide locking for a
> virObject that only had a @ref. Someone could still create a virObject
> as well and just get @refs. With the new objects based on those two
> objects you get a few more features that allow for add, search, lookup,
> remove.  But you call that wrong, which is confusing to me.

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170725/6808d45e/attachment-0001.sig>