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

[John Ferlan]

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 at redhat.com>
>>>> ---
>>>>  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