Make various virSecretObjList*Locked functions static and make
virSecretObjNew static since they're only called within virtsecretobj.c
Signed-off-by: John Ferlan <jferlan@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.
- */
-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.
*/
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.
- */
-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.
*/
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
*/
-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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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.
> -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.
> */
> 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@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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.
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...
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@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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.
> 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@redhat.com
> >> https://www.redhat.com/mailman/listinfo/libvir-list
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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 ;-)
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@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.