10:34 a.m.
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.