8:01 a.m.
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>
>>> ---
>>> 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