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.
> 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.
Pavel
>
>
> John
>
> >> */
> >> virSecretObjPtr
> >> virSecretObjListFindByUUID(virSecretObjListPtr secrets,
> >> @@ -228,17 +219,7 @@ virSecretObjSearchName(const void *payload,
> >> }
> >>
> >>
> >> -/**
> >> - * virSecretObjFindByUsageLocked:
> >> - * @secrets: list of secret objects
> >> - * @usageType: secret usageType to find
> >> - * @usageID: secret usage string
> >> - *
> >> - * This functions requires @secrets to be locked already!
> >> - *
> >> - * Returns: not locked, but ref'd secret object.
> >> - */
> >
> > Same here, we can keep the documentation.
> >
> >> -virSecretObjPtr
> >> +static virSecretObjPtr
> >> virSecretObjListFindByUsageLocked(virSecretObjListPtr secrets,
> >> int usageType,
> >> const char *usageID)
> >> @@ -263,7 +244,7 @@ virSecretObjListFindByUsageLocked(virSecretObjListPtr secrets,
> >> * This function locks @secrets and finds the secret object which
> >> * corresponds to @usageID of @usageType.
> >> *
> >> - * Returns: locked and ref'd secret object.
> >> + * Returns: locked and ref'd secret object on success, NULL on failure.
> >
> > Unrelated change.
> >
> >> */
> >> virSecretObjPtr
> >> virSecretObjListFindByUsage(virSecretObjListPtr secrets,
> >> @@ -320,9 +301,9 @@ virSecretObjListRemove(virSecretObjListPtr secrets,
> >> *
> >> * This functions requires @secrets to be locked already!
> >> *
> >> - * Returns pointer to secret or NULL if failure to add
> >> + * Returns: locked secret or NULL if failure to add
> >
> > Unrelated change.
> >
> >> */
> >> -virSecretObjPtr
> >> +static virSecretObjPtr
> >> virSecretObjListAddLocked(virSecretObjListPtr secrets,
> >> virSecretDefPtr def,
> >> const char *configDir,
> >> diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h
> >> index b26061a..9638b69 100644
> >> --- a/src/conf/virsecretobj.h
> >> +++ b/src/conf/virsecretobj.h
> >> @@ -29,9 +29,6 @@
> >> typedef struct _virSecretObj virSecretObj;
> >> typedef virSecretObj *virSecretObjPtr;
> >>
> >> -virSecretObjPtr
> >> -virSecretObjNew(void);
> >> -
> >> void
> >> virSecretObjEndAPI(virSecretObjPtr *secret);
> >>
> >> @@ -42,19 +39,10 @@ virSecretObjListPtr
> >> virSecretObjListNew(void);
> >>
> >> virSecretObjPtr
> >> -virSecretObjListFindByUUIDLocked(virSecretObjListPtr secrets,
> >> - const unsigned char *uuid);
> >> -
> >> -virSecretObjPtr
> >> virSecretObjListFindByUUID(virSecretObjListPtr secrets,
> >> const unsigned char *uuid);
> >>
> >> virSecretObjPtr
> >> -virSecretObjListFindByUsageLocked(virSecretObjListPtr secrets,
> >> - int usageType,
> >> - const char *usageID);
> >> -
> >> -virSecretObjPtr
> >> virSecretObjListFindByUsage(virSecretObjListPtr secrets,
> >> int usageType,
> >> const char *usageID);
> >> @@ -64,12 +52,6 @@ virSecretObjListRemove(virSecretObjListPtr secrets,
> >> virSecretObjPtr secret);
> >>
> >> virSecretObjPtr
> >> -virSecretObjListAddLocked(virSecretObjListPtr secrets,
> >> - virSecretDefPtr def,
> >> - const char *configDir,
> >> - virSecretDefPtr *oldDef);
> >> -
> >> -virSecretObjPtr
> >> virSecretObjListAdd(virSecretObjListPtr secrets,
> >> virSecretDefPtr def,
> >> const char *configDir,
> >> --
> >> 2.9.3
> >>
> >> --
> >> libvir-list mailing list
> >> libvir-list@xxxxxxxxxx
> >> https://www.redhat.com/mailman/listinfo/libvir-list
>
> --
> 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
