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