[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v4 00/17] virObject adjustments for common object



On Fri, Aug 18, 2017 at 17:50:24 -0400, John Ferlan wrote:
> v3: https://www.redhat.com/archives/libvir-list/2017-June/msg00916.html

[...]

> Although I understand it's not the preference of one reviewer, I've
> kept with the virObject model. If that still doesn't pass muster and
> someone else wants to create some other mechanism to combine the existing
> drivers in a more sane manner, then have at it. This is the model
> I've chosen. I personally don't see the value in just a shim API.
> 
> This set of patches moves away from using a strict "uuid/name" designation
> in favor of using "key1" and "key2". While some may find that "too generic",

Yes. I'm here to complain about this. As I've pointed out some time ago,
abstracting stuff, which you can't name properly does not seem to make
sense if you are in the end attaching it to something non-generic.

This makes such helpers hard to use and hard to remember which
fields serves which purpose in the given use case. This also then
triggers custom wrappers for every single usage area where you put names
to those fields.

> I think that's the whole purpose of it. After some soul searching I feel

Could you elaborate please on the purpose and final goal of this. I
was't able to piece together what you are trying to achieve. If you want
to unify the code that sections of libvirt use to hold lists of objects
I don't think you need a custom sub-type for them.

> using "name" or "uuid" is too restrictive and lends more towards the shim

It has to be restrictive if you want to couple it to specific objects
like VMs and such. It would be too restrictive only if you are going to
add a generic implementation of a container holding objects which can be
referenced by generic keys.

You are trying to add a hybrid. Something which is generic enough to
allow anonymous naming, but specific enough that it has to have an
"active" field. [1]

> API model. Besides for some consumers they don't have a uuid (nodedev,
> interface, and nwfilter). In the long run it doesn't matter whether it's
> a uuid, name, or whatever as long as it's a character string.

I don't really see the point in trying to abstract the contents of a
"object list". I see value in having a object/set of APIs which would
basically allow multiple keys for an entry in a hash table (for any
possible implementation of this). All of such can be done on a virObject
and you don't need any custom wrapper object which holds certain
anonymous properties.

The wrapper object you are adding here doesn't seem to add any value,
since you can't really remove the name or UUID value from the internal
objects, so that still would be duplicated.

[...]

> John Ferlan (17):
>   util: Use VIR_ERROR instead of VIR_WARN
>   util: Introduce virObjectLookupKeys
>   util: Introduce virObjectLookupHash
>   util: Introduce virObjectLookupKeys*Active API's

[1]

>   util: Introduce virObjectLookupHash{Add|Remove}
>   util: Introduce virObjectLookupHashFind[Locked]
>   util: Introduce virObjectLookupHashForEach
>   util: Introduce virObjectLookupHashSearch[Locked]
>   nodedev: Use virObjectLookup{Keys|Hash}
>   secret: Use virObjectLookup{Keys|Hash}
>   util: Introduce virObjectLookupHashClone
>   Revert "interface: Consume @def in virInterfaceObjNew"
>   interface: Use virObjectLookup{Keys|Hash}
>   test: Clean up test driver Interface interactions
>   util: Introduce virObjectLookupHashPrune
>   network: Fix virNetworkObjBridgeInUse return type
>   network: Use virObjectLookup{Keys|Hash}

Attachment: signature.asc
Description: PGP signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]