v3: https://www.redhat.com/archives/libvir-list/2017-June/msg00916.html

Changes since v3 - honestly it's been too long to remember exactly
what changes have taken place. This series provide the util/virobject
changes and the implementation for nodedev, secret, interface, and
network drivers/vir*obj's.  The nwfilter is awaiting more upstream
review and work and some more Storage Pool/Volume patches will be
posted in conjunction with these. These changes also use the rwlocks
Michal recently added to virobject as the means to provide locks
for the LookupHash table(s).

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",
I think that's the whole purpose of it. After some soul searching I feel
using "name" or "uuid" is too restrictive and lends more towards the shim
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.

FWIW:
Patches 1, 12, and 16 could be easily separated out, but since I was
working in the area - they were added here as well.

John Ferlan (17):
  util: Use VIR_ERROR instead of VIR_WARN
  util: Introduce virObjectLookupKeys
  util: Introduce virObjectLookupHash
  util: Introduce virObjectLookupKeys*Active API's
  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}

 src/conf/virinterfaceobj.c  | 301 ++++++++++----------
 src/conf/virnetworkobj.c    | 293 ++++++--------------
 src/conf/virnetworkobj.h    |   5 +-
 src/conf/virnodedeviceobj.c | 286 ++++++-------------
 src/conf/virsecretobj.c     | 263 +++++-------------
 src/libvirt_private.syms    |  15 +
 src/test/test_driver.c      |  55 +---
 src/util/virobject.c        | 658 +++++++++++++++++++++++++++++++++++++++++++-
 src/util/virobject.h        | 119 ++++++++
 tests/networkxml2conftest.c |   4 +-
 10 files changed, 1190 insertions(+), 809 deletions(-)

-- 
2.9.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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}
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 08/21/2017 09:06 AM, Peter Krempa wrote:
> 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.

I suspected that you'd object and that's fine.

Oddly it's OK for virhash code to generically call something a "key" and
have a default search mechanism that uses a void * STREQ comparison, but
yet when applied to virobject code used to combine the needs for the
various driver/vir*obj modules to create, add, search, list, remove, and
delete for a what amounts to be common code between each of them using
hash table(s) to manage "uuid" or "name" based objects, then it's not OK.

Ironically my first/initial pass at this took the path of a single
src/conf/virpoolobj.c module, but it was suggested to use virobject
instead to abstract things. When that's done - it's back to well I don't
like that approach because it's too generic, so you should use a common
module in order to make all those abstractions along with quite a bit
more specific code in order to handle (known) differences between how
each of the driver/vir*obj's needs to reference things.

FWIW: Initial posting... Patch 2 is where the virobject enhancement was
suggested.

https://www.redhat.com/archives/libvir-list/2017-February/msg00519.html

> 
> 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.
> 

How is it hard to use/remember?  I would think it's much harder to use
and remember 8 different ways to do things as opposed to one, but maybe
I'm the only one looking to simplify/unify.

If you really look at the code - you'll see for the most part it's a
wrapper around various virhash functions with specific needs for
driver/vir*obj that includes the refcnt, locking, and smaller subset of
driver/vir*obj specific code to "manage" the details of/for a vir*DefPtr.

It works for nodedev, secret, network, and interface already as seen in
later patches in the series.

And again, usage of "key1/key2" vs. "uuid/name" is because:

Nodedev uses "name" as it's key
Secret uses "uuid" as it's key
Network uses "uuid" and (with these patches) "name" as keys
Interface uses "name" as it's key
Nwfilter uses "name" as it's key
Storage Pool uses "uuid" and "name" as keys
Storage Volume uses "name" as it's key
Domains use "uuid" and "name" as keys.

So, is that really that hard to remember? Do you really have a need to
remember? Why do you feel it's so important that the object knows it's
managing a table of UUIDs or a table of names? The driver/vir*obj
implementation essentially wants at least 1 way to store things and
possibly 2 so that lookup by a second key is quicker (lookup-by-keyid
vs. search all for specific name). Yes, they are one or the other
currently. In the long run, it's just a name.

The driver/vir*obj.c code can choose which to use and manages it's own
mapping. One need only look at the argument in the create/new to know
which is which. But, it doesn't matter once you get to the Find/Search
code as if there are two tables an object has to be in both tables. So,
when using a Lookup function on some string both tables can be searched
in order to perform lookup. When it comes to the ForEach or Search type
functions, only one table needs to be searched in order to run through
all the elements. So rather than need to make the (generic) object code
need to check if one table or the other exists, just make at least one
table required and define that to be the first table. It's just a design
decision.

>> 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.
> 

As if I haven't already elaborated the purpose and final goal. It's been
stated multiple times and I think it's very obvious.

The goal is to make things generic enough to be used by various
driver/vir*obj modules in order to abstract away or combine alike code.
If that's truly not desired that's fine. I think it's far better to have
less cut/copy-n-paste code, but not everyone has that same viewpoint.

Rather than take a myopic viewpoint of a domain or storage or network -
once you consider that each of those subsystems essentially does the
same thing - creates an object, stores that object, allows searches on
that object, and eventually deletes that object.  The details of the
object is still handle-able by specific code, but there's much with that
object that can and should be generically handled.

The "reason" I went down this rabbit hole was some patches posted by
Virtuozzo which essentially copied *a lot* from the storage driver in
order to make another storage driver specific for their needs. I felt
rather than cut-copy-n-paste that we should be able to "combine" a lot
of code. As I dug into the code I found a mish-mash of object management
throughout the drivers. Mostly forward linked lists, but each set of
code just different enough to make things "difficult" as it relates to
being able to support/maintain code over time. So rather than have
multiple ways to do things, take a common approach so that when you're
looking through code you don't have to know the differences between 8
piles of code that all accomplished the same goal, but each in its own
unique way.

Other than NWFilter and Storage Pool/Vols other code is all using hash
tables. The NWFitler patches are onlist, but recursive locking is
proving to be a PITA. I have patches to convert storage pools to use
hash tables ready to post - I was just running it through avocado
testing first but found that test harness is a bit broken. Patches to
convert storage vols to use a hash table would be in a followup set. So
from a code convergence viewpoint - at least mostly everything is fairly
common now, which is good. Still, the domain code while using hash
tables doesn't exactly conform to refcnt logic very well (yet). I've
been waiting to finish everything else before tackling that.

>> 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]
> 

The active could be removed if it was that objectionable. Still let's
face it - it's being written to serve a purpose within the libvirt code
and it's not some "generic" for general consumption. The 'active'
concept is prevalent in many driver/vir*obj's.

>> 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.

Like I noted above - ironically I took that path originally, but once
encouraged to consider augmenting virObject - I found that to be a
better solution. I think the shim idea is going to run into some
problems, but I'll be happy to look at code someone else writes that
accomplishes that. As I see it there are 4 driver/vir*obj that are ready
for such a convergence. I have a suspicion that much of what's done
could use what I've done; however, I think there will need to be
additional code and switches to handle certain tasks that are more
easily handled by what I've done. I'll also be very curious to see what
happens when the domain code is considered. Suffice to say IMO there's
some AFU'd code there especially as it relates to refcnt and locks.

> 
> The wrapper object you are adding here doesn't seem to add any value,

That's your opinion.

> since you can't really remove the name or UUID value from the internal
> objects, so that still would be duplicated.

Not sure what you mean here.  Which internal object?

Oh and here's the magic word "internal" - that all this is - an internal
only mechanism to abstract the way to add, search, remove objects from a
table.  And like other subsystems we can define (to a degree) what the
data and APIs would be needed to manage those objects.


John

> 
> [...]
> 
>> 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}

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Aug 21, 2017 at 11:27:17 -0400, John Ferlan wrote:
> On 08/21/2017 09:06 AM, Peter Krempa wrote:
> > On Fri, Aug 18, 2017 at 17:50:24 -0400, John Ferlan wrote:

[...]

> Oddly it's OK for virhash code to generically call something a "key" and
> have a default search mechanism that uses a void * STREQ comparison, but
> yet when applied to virobject code used to combine the needs for the

virHash code also stores anything you give it and returns it back. In
your approach the code is not generic enough to take a virObject or a
void *.

> various driver/vir*obj modules to create, add, search, list, remove, and
> delete for a what amounts to be common code between each of them using
> hash table(s) to manage "uuid" or "name" based objects, then it's not OK.

No, not entirely. As I've said, if you are going to add a "hash table on
steroids", it's fine if you call it anonymously.

What is not okay, that you are going to use the new object type and make
all (domain, storage, ...) objects inherit from it. I strongly dislike
this. Object members need to be clearly named, otherwise it will end up
only in confusion.

You certainly can add some internal data structure, which can be an
object, to hold the data which allow this to work. But making domain
objects inherit from it is wrong.

> Ironically my first/initial pass at this took the path of a single
> src/conf/virpoolobj.c module, but it was suggested to use virobject
> instead to abstract things. When that's done - it's back to well I don't
> like that approach because it's too generic, so you should use a common
> module in order to make all those abstractions along with quite a bit
> more specific code in order to handle (known) differences between how
> each of the driver/vir*obj's needs to reference things.

Your first implementation was neither generic nor specific. It contained
generic parts (anonymized name and UUID) and then specific parts, where
you'd have to pass in VM/storage/nw objects. It couldn't really be used
for anything else.

And this is mostly the same. You have a hybrid, where you need the
stored objects to inherit from the object you've created, and then
introduce fields which make it specific for this only use case "active".

With this you are basically attempting to move an implementation detail
of the approach to map multiple keys to a virObject of some type to the
object properties themselves, which feels wrong. Especially since the
properties you are trying to extract can't be named uniformly for all
possible objects inheriting from such object.

> FWIW: Initial posting... Patch 2 is where the virobject enhancement was
> suggested.
> 
> https://www.redhat.com/archives/libvir-list/2017-February/msg00519.html
> 
> > 
> > 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.
> > 
> 
> How is it hard to use/remember?  I would think it's much harder to use
> and remember 8 different ways to do things as opposed to one, but maybe
> I'm the only one looking to simplify/unify.

If 'key1' is a name of the VM but 'uuid' for a secret, then it's
confusing and hard to remember. Confusing is worse than having different
implementation. If you are going to argue that 'key1' should ever be
interpreted in the context of the lookup functions, then you made it an
implementation detail of the lookup functions thus not requiring
everything to inherit from it.

> If you really look at the code - you'll see for the most part it's a
> wrapper around various virhash functions with specific needs for
> driver/vir*obj that includes the refcnt, locking, and smaller subset of
> driver/vir*obj specific code to "manage" the details of/for a vir*DefPtr.

Yes. That's what I object to. If

> 
> It works for nodedev, secret, network, and interface already as seen in
> later patches in the series.
> 
> And again, usage of "key1/key2" vs. "uuid/name" is because:
> 
> Nodedev uses "name" as it's key
> Secret uses "uuid" as it's key
> Network uses "uuid" and (with these patches) "name" as keys
> Interface uses "name" as it's key
> Nwfilter uses "name" as it's key
> Storage Pool uses "uuid" and "name" as keys
> Storage Volume uses "name" as it's key
> Domains use "uuid" and "name" as keys.

Yes they are different. And that's my point. If you are going to make an
object, which will agregate those properties, you need to be able to
name them. If you can't, don't try


> So, is that really that hard to remember? Do you really have a need to

Yes. Also duplicated. You still have the name in vm->def or pool->def.
It bugs me that we store the name in 'def' rather the object itself, but
I don't feel like trying to change it. (I tried and gave up few years
ago.)

> remember? Why do you feel it's so important that the object knows it's
> managing a table of UUIDs or a table of names? The driver/vir*obj

No that's fine. As long as you don't force us to inherit from it. If you
inherit from it, the names need to be meaningful. If you treat the
object as an impl. Detail then they don't.

> implementation essentially wants at least 1 way to store things and
> possibly 2 so that lookup by a second key is quicker (lookup-by-keyid
> vs. search all for specific name). Yes, they are one or the other
> currently. In the long run, it's just a name.
> 
> The driver/vir*obj.c code can choose which to use and manages it's own
> mapping. One need only look at the argument in the create/new to know
> which is which. But, it doesn't matter once you get to the Find/Search
> code as if there are two tables an object has to be in both tables. So,
> when using a Lookup function on some string both tables can be searched
> in order to perform lookup. When it comes to the ForEach or Search type
> functions, only one table needs to be searched in order to run through
> all the elements. So rather than need to make the (generic) object code
> need to check if one table or the other exists, just make at least one
> table required and define that to be the first table. It's just a design
> decision.
> 
> >> 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.
> > 
> 
> As if I haven't already elaborated the purpose and final goal. It's been
> stated multiple times and I think it's very obvious.
> 
> The goal is to make things generic enough to be used by various
> driver/vir*obj modules in order to abstract away or combine alike code.
> If that's truly not desired that's fine. I think it's far better to have
> less cut/copy-n-paste code, but not everyone has that same viewpoint.
> 
> Rather than take a myopic viewpoint of a domain or storage or network -
> once you consider that each of those subsystems essentially does the
> same thing - creates an object, stores that object, allows searches on
> that object, and eventually deletes that object.  The details of the
> object is still handle-able by specific code, but there's much with that
> object that can and should be generically handled.
> 
> The "reason" I went down this rabbit hole was some patches posted by
> Virtuozzo which essentially copied *a lot* from the storage driver in
> order to make another storage driver specific for their needs. I felt
> rather than cut-copy-n-paste that we should be able to "combine" a lot
> of code. As I dug into the code I found a mish-mash of object management
> throughout the drivers. Mostly forward linked lists, but each set of
> code just different enough to make things "difficult" as it relates to
> being able to support/maintain code over time. So rather than have
> multiple ways to do things, take a common approach so that when you're
> looking through code you don't have to know the differences between 8
> piles of code that all accomplished the same goal, but each in its own
> unique way.

Fair enough. So you want a better way to collect objects in lists.
That's a very worthwile goal.

[... trimmed the rest, since most points would repeat ... ]

> > 
> > The wrapper object you are adding here doesn't seem to add any value,
> 
> That's your opinion.

Well, you techincally asked for it by sending it for review. It would be
more worhtwhile to refute it rather than dismiss it.

I might have not responded to everything, but this message was rather
long. Feel free to reiterate points which you feel I've missed.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 08/22/2017 11:34 AM, Peter Krempa wrote:
> On Mon, Aug 21, 2017 at 11:27:17 -0400, John Ferlan wrote:
>> On 08/21/2017 09:06 AM, Peter Krempa wrote:
>>> On Fri, Aug 18, 2017 at 17:50:24 -0400, John Ferlan wrote:
> 
> [...]
> 
>> Oddly it's OK for virhash code to generically call something a "key" and
>> have a default search mechanism that uses a void * STREQ comparison, but
>> yet when applied to virobject code used to combine the needs for the
> 
> virHash code also stores anything you give it and returns it back. In
> your approach the code is not generic enough to take a virObject or a
> void *.
> 
>> various driver/vir*obj modules to create, add, search, list, remove, and
>> delete for a what amounts to be common code between each of them using
>> hash table(s) to manage "uuid" or "name" based objects, then it's not OK.
> 
> No, not entirely. As I've said, if you are going to add a "hash table on
> steroids", it's fine if you call it anonymously.
> 
> What is not okay, that you are going to use the new object type and make
> all (domain, storage, ...) objects inherit from it. I strongly dislike
> this. Object members need to be clearly named, otherwise it will end up
> only in confusion.
> 

There's a difference between dislike and technically incorrect.

In my mind - if {k|K}ey1 was uuid and {k|K}ey2 was name and code was
altered to handle whether one exists or not, then there'd still be
issues with the approach. The effort expended to make those adjustments
and then have some other issue may not be worth it IMO.

Still I'd like to find some common ground using objects, but I have a
feeling that's just not going to be possible.

If LookupKeys were removed and LookupHash changed to rename objsKey1 and
objsKey2 to objsUUID and objsName, would that provide a path forward or
are you so against any of this that it's pointless to even attempt?

Trying to figure a way to constructively move forward.

> You certainly can add some internal data structure, which can be an
> object, to hold the data which allow this to work. But making domain
> objects inherit from it is wrong.
> 
>> Ironically my first/initial pass at this took the path of a single
>> src/conf/virpoolobj.c module, but it was suggested to use virobject
>> instead to abstract things. When that's done - it's back to well I don't
>> like that approach because it's too generic, so you should use a common
>> module in order to make all those abstractions along with quite a bit
>> more specific code in order to handle (known) differences between how
>> each of the driver/vir*obj's needs to reference things.
> 
> Your first implementation was neither generic nor specific. It contained
> generic parts (anonymized name and UUID) and then specific parts, where
> you'd have to pass in VM/storage/nw objects. It couldn't really be used
> for anything else.
> 

The first one changed all the vir*Obj[List] to be virPoolObj[Table]. It
worked for the existing driver/vir*objs implementations. It was to a
essentially a proof of concept.

> And this is mostly the same. You have a hybrid, where you need the
> stored objects to inherit from the object you've created, and then
> introduce fields which make it specific for this only use case "active".
> 

I can remove active - it's not that important.

Using fields from the creator is mainly a shortcut. Removing them means
adjusting the Add/Remove API's a bit to take parameters.

Yes, it's a hybrid with new objects that have specific usages. I'm not
expecting other areas to use the objects like virObjectLockable is
"reused" in many places. If a new driver/vir*obj is created, then using
these objects I figure would make life easier.

> With this you are basically attempting to move an implementation detail
> of the approach to map multiple keys to a virObject of some type to the
> object properties themselves, which feels wrong. Especially since the
> properties you are trying to extract can't be named uniformly for all
> possible objects inheriting from such object.
> 

And the alternative solution is to create a common vircommonobjlist.c
module that will essentially do something similar. There would be quite
a bit of shimmying going on though in order to FindEach and Search
ByUUID or ByName. I haven't put too much thought into that model though
since I focused on the initial review comments that pointed in the
direction of objects. Just because the object is "special" or "directly"
purposed, doesn't invalidate it's usefulness.

>> FWIW: Initial posting... Patch 2 is where the virobject enhancement was
>> suggested.
>>
>> https://www.redhat.com/archives/libvir-list/2017-February/msg00519.html
>>
>>>
>>> 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.
>>>
>>
>> How is it hard to use/remember?  I would think it's much harder to use
>> and remember 8 different ways to do things as opposed to one, but maybe
>> I'm the only one looking to simplify/unify.
> 
> If 'key1' is a name of the VM but 'uuid' for a secret, then it's
> confusing and hard to remember. Confusing is worse than having different

I don't see it as confusing... What I do find confusing is the multiple
layers of domain XML post parse processing and validation via callbacks.
I find it easy to map name to key1, but trying to figure out and
understand all the rules of processing boggles my mind. Then there's the
hidden private data structures on top of that!

> implementation. If you are going to argue that 'key1' should ever be
> interpreted in the context of the lookup functions, then you made it an
> implementation detail of the lookup functions thus not requiring
> everything to inherit from it.
> 

So you prefer multiple cut-copy-n-paste functions that when one goes to
make a change to say which lock mechanism is used, then one must go
modify multiple files making multiple of the same changes. Essentially
what Michal would be doing to make rwlocks used for all the hash/list
processing instead of modifying one place.

Likewise when an issue pops up and is fixed in one place, then it would
(theoretically at least) need to be fixed in all places. Of course we
all know that's not how it usually happens unless a code reviewer asks,
wouldn't this also be a problem elsewhere or if by chance the person
making the changes actually does the same change across all drivers that
would have the same issue.

>> If you really look at the code - you'll see for the most part it's a
>> wrapper around various virhash functions with specific needs for
>> driver/vir*obj that includes the refcnt, locking, and smaller subset of
>> driver/vir*obj specific code to "manage" the details of/for a vir*DefPtr.
> 
> Yes. That's what I object to. If
> 

But this is where I figured the greatest gain is at least w/r/t code
reuse without needlessly adding switches to account for specific data.

>>
>> It works for nodedev, secret, network, and interface already as seen in
>> later patches in the series.
>>
>> And again, usage of "key1/key2" vs. "uuid/name" is because:
>>
>> Nodedev uses "name" as it's key
>> Secret uses "uuid" as it's key
>> Network uses "uuid" and (with these patches) "name" as keys
>> Interface uses "name" as it's key
>> Nwfilter uses "name" as it's key
>> Storage Pool uses "uuid" and "name" as keys
>> Storage Volume uses "name" as it's key
>> Domains use "uuid" and "name" as keys.
> 
> Yes they are different. And that's my point. If you are going to make an
> object, which will agregate those properties, you need to be able to
> name them. If you can't, don't try
> 

A rose by any other name is still a rose.

> 
>> So, is that really that hard to remember? Do you really have a need to
> 
> Yes. Also duplicated. You still have the name in vm->def or pool->def.
> It bugs me that we store the name in 'def' rather the object itself, but
> I don't feel like trying to change it. (I tried and gave up few years
> ago.)
> 

Hmmm... so I create an object that stores the name and/or uuid and
that's a problem? Or did I misinterpret what you're trying to say.

>> remember? Why do you feel it's so important that the object knows it's
>> managing a table of UUIDs or a table of names? The driver/vir*obj
> 
> No that's fine. As long as you don't force us to inherit from it. If you
> inherit from it, the names need to be meaningful. If you treat the
> object as an impl. Detail then they don't.
> 
>> implementation essentially wants at least 1 way to store things and
>> possibly 2 so that lookup by a second key is quicker (lookup-by-keyid
>> vs. search all for specific name). Yes, they are one or the other
>> currently. In the long run, it's just a name.
>>
>> The driver/vir*obj.c code can choose which to use and manages it's own
>> mapping. One need only look at the argument in the create/new to know
>> which is which. But, it doesn't matter once you get to the Find/Search
>> code as if there are two tables an object has to be in both tables. So,
>> when using a Lookup function on some string both tables can be searched
>> in order to perform lookup. When it comes to the ForEach or Search type
>> functions, only one table needs to be searched in order to run through
>> all the elements. So rather than need to make the (generic) object code
>> need to check if one table or the other exists, just make at least one
>> table required and define that to be the first table. It's just a design
>> decision.
>>
>>>> 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.
>>>
>>
>> As if I haven't already elaborated the purpose and final goal. It's been
>> stated multiple times and I think it's very obvious.
>>
>> The goal is to make things generic enough to be used by various
>> driver/vir*obj modules in order to abstract away or combine alike code.
>> If that's truly not desired that's fine. I think it's far better to have
>> less cut/copy-n-paste code, but not everyone has that same viewpoint.
>>
>> Rather than take a myopic viewpoint of a domain or storage or network -
>> once you consider that each of those subsystems essentially does the
>> same thing - creates an object, stores that object, allows searches on
>> that object, and eventually deletes that object.  The details of the
>> object is still handle-able by specific code, but there's much with that
>> object that can and should be generically handled.
>>
>> The "reason" I went down this rabbit hole was some patches posted by
>> Virtuozzo which essentially copied *a lot* from the storage driver in
>> order to make another storage driver specific for their needs. I felt
>> rather than cut-copy-n-paste that we should be able to "combine" a lot
>> of code. As I dug into the code I found a mish-mash of object management
>> throughout the drivers. Mostly forward linked lists, but each set of
>> code just different enough to make things "difficult" as it relates to
>> being able to support/maintain code over time. So rather than have
>> multiple ways to do things, take a common approach so that when you're
>> looking through code you don't have to know the differences between 8
>> piles of code that all accomplished the same goal, but each in its own
>> unique way.
> 
> Fair enough. So you want a better way to collect objects in lists.
> That's a very worthwile goal.
> 
> [... trimmed the rest, since most points would repeat ... ]
> 
>>>
>>> The wrapper object you are adding here doesn't seem to add any value,
>>
>> That's your opinion.
> 
> Well, you techincally asked for it by sending it for review. It would be
> more worhtwhile to refute it rather than dismiss it.

I'm merely pointing out my opinion of the value of the code to oppose
your opinion. There is value since it's a worthwhile goal. This
mechanism is one way to achieve that.

John

> 
> I might have not responded to everything, but this message was rather
> long. Feel free to reiterate points which you feel I've missed.
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list