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