On Tue, Apr 25, 2017 at 09:01:11AM -0400, John Ferlan wrote:
>
>
> 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 ;-)
You know what would happen :), I would ask you to split the commit because
it would be to different changes in one commit.
> 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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
