Re: [libvirt] [PATCH 03/14] secret: Make some virSecretObj* functions static

On 04/25/2017 08:23 AM, Pavel Hrdina wrote: > On Tue, Apr 25, 2017 at 07:55:33AM -0400, John Ferlan wrote: >> >> >> On 04/25/2017 04:12 AM, Pavel Hrdina wrote: >>> On Mon, Apr 24, 2017 at 02:00:12PM -0400, John Ferlan wrote: >>>> Make various virSecretObjList*Locked functions static and make >>>> virSecretObjNew static since they're only called within virtsecretobj.c >>>> >>>> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >>>> --- >>>> src/conf/virsecretobj.c | 33 +++++++-------------------------- >>>> src/conf/virsecretobj.h | 18 ------------------ >>>> 2 files changed, 7 insertions(+), 44 deletions(-) >>>> >>>> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c >>>> index cc18459..064e66c 100644 >>>> --- a/src/conf/virsecretobj.c >>>> +++ b/src/conf/virsecretobj.c >>>> @@ -86,7 +86,7 @@ virSecretObjOnceInit(void) >>>> >>>> VIR_ONCE_GLOBAL_INIT(virSecretObj) >>>> >>>> -virSecretObjPtr >>>> +static virSecretObjPtr >>>> virSecretObjNew(void) >>>> { >>>> virSecretObjPtr secret; >>>> @@ -158,16 +158,7 @@ virSecretObjListDispose(void *obj) >>>> } >>>> >>>> >>>> -/** >>>> - * virSecretObjFindByUUIDLocked: >>>> - * @secrets: list of secret objects >>>> - * @uuid: secret uuid to find >>>> - * >>>> - * This functions requires @secrets to be locked already! >>>> - * >>>> - * Returns: not locked, but ref'd secret object. >>>> - */ >>> >>> I don't think that we need to remove the documentation, it is also useful for >>> static function. >>> >> >> Not that it matters, but it's essentially the same as the non-Locked >> version except for the piece noted below which I also adjusted to >> combine them. I guess I'm following other advice received in other >> reviews to reduce the extraneous comments. Since the Locked version is >> no longer accessible to anything other than a local consumer that's why >> I removed it. >> >> Of course if you see the patches I have in my branch - it may not matter >> because the eventual goal is to have FindByUUID and FindByUsage >> essentially match and thus be combined into FindByObject type function >> where the FindBy is based on a parameter telling the function to look in >> a primary hash table or a secondary hash table. >> >> Long term the goal is to have ObjListPtr be a pointer to an Object that >> keeps two hash tables - one a primary (UUID for secrets) and one an >> optional secondary (Usage for secrets). An object can be in both hash >> tables (see how the domain code does this) and lookup goes entirely >> through the virHash* functions rather than potentially slow linked list >> traversal. >> >> Consider some recent issues with "large numbers" of objects that are in >> a forward linked list. When there's 10-50 elements perhaps the lookup >> times aren't so bad, but if there's 200, 500, 1000 elements in the list, >> then it becomes exponentially slower for every lookup that's at the end >> of the list. For some things like "node device" - those could be the >> ones that are referenced more frequently too. >> >> If we alter all those drivers to use hash tables lookups and the bulk of >> the code is in the form of common/shared object, then not only is lookup >> quicker, but we don't have multiple different mechanisms to do the same >> thing and we share a lot more code. > > I know about those patches sent as RFC and that you want to rewrite the > object lists to share a common code, that's definitely good idea and I'll > support it, currently it's a mess. However posting a patch with changes > that may or may not be used by some future patches that aren't part of the > current patch series, it's hard to justify such patch. > Understood - still I think my frame of reference when writing was more towards the first paragraph above. I modified into a non-static function and replicating the comments felt superfluous based on the number of times I get dinged for over commenting... >> Unfortunately the journey to get there is going to be a bit painful with >> the volume of patches, but the end result is a lot of common code and >> common objects. >> >> >>>> -virSecretObjPtr >>>> +static virSecretObjPtr >>>> virSecretObjListFindByUUIDLocked(virSecretObjListPtr secrets, >>>> const unsigned char *uuid) >>>> { >>>> @@ -187,7 +178,7 @@ virSecretObjListFindByUUIDLocked(virSecretObjListPtr secrets, >>>> * This function locks @secrets and finds the secret object which >>>> * corresponds to @uuid. >>>> * >>>> - * Returns: locked and ref'd secret object. >>>> + * Returns: locked and ref'd secret object on success, NULL on failure. >>> >>> Unrelated change. >>> >> >> wellll... it's the combination of the two >> >> I can restore the *Locked comments, but they will disappear later on >> perhaps making differences look awful. >> >> Let me know either way... > > If they eventually disappear then there is no point of modifying them at all. > > This patch says that it makes some functions static, so all other changes are > just unrelated to this patch. The documentation comments can just disappear > together with the function. Would it have made a difference if the commit message indicated removing superfluous comments ;-)

I've restored the comments... Once I've worked through the rest of the patches I'll post a V2 starting with this patch since it's affect patch 10 John -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list