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

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

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

On Mon, Apr 24, 2017 at 02:00:14PM -0400, John Ferlan wrote:

When processing a virSecretPtr use 'secret' as a variable name.

When processing a virSecretObjPtr use 'obj' as a variable name.

When processing a virSecretDefPtr use 'def' as a variable name, unless a distinction needs to be made with a 'newdef' such as virSecretObjListAddLocked (which also used the VIR_STEAL_PTR macro for the configFile and base64File).

NB: Also made a slight algorithm adjustment for virSecretObjListRemove to check if the passed obj was NULL rather than having the caller virSecretLoad need to check prior to calling.

I can see the motivation for this change, it's easier to follow the code if the naming is consistent, however this patch adds one extra step when tracing history of the code via git blame. All the changes except the variable rename should be in separate patch as they are unrelated to the rename. Pavel

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 275 +++++++++++++++++++++++---------------------- src/conf/virsecretobj.h | 26 ++--- src/secret/secret_driver.c | 139 ++++++++++++----------- 3 files changed, 226 insertions(+), 214 deletions(-)

diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 348feb3..42f36c8 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -89,29 +89,29 @@ VIR_ONCE_GLOBAL_INIT(virSecretObj) static virSecretObjPtr virSecretObjNew(void) { - virSecretObjPtr secret; + virSecretObjPtr obj;

if (virSecretObjInitialize() < 0) return NULL;

- if (!(secret = virObjectLockableNew(virSecretObjClass))) + if (!(obj = virObjectLockableNew(virSecretObjClass))) return NULL;

- virObjectLock(secret); + virObjectLock(obj);

- return secret; + return obj; }

void -virSecretObjEndAPI(virSecretObjPtr *secret) +virSecretObjEndAPI(virSecretObjPtr *obj) { - if (!*secret) + if (!*obj) return;

- virObjectUnlock(*secret); - virObjectUnref(*secret); - *secret = NULL; + virObjectUnlock(*obj); + virObjectUnref(*obj); + *obj = NULL; }

@@ -136,18 +136,18 @@ virSecretObjListNew(void)

static void -virSecretObjDispose(void *obj) +virSecretObjDispose(void *opaque) { - virSecretObjPtr secret = obj; + virSecretObjPtr obj = opaque;

- virSecretDefFree(secret->def); - if (secret->value) { + virSecretDefFree(obj->def); + if (obj->value) { /* Wipe before free to ensure we don't leave a secret on the heap */ - memset(secret->value, 0, secret->value_size); - VIR_FREE(secret->value); + memset(obj->value, 0, obj->value_size); + VIR_FREE(obj->value); } - VIR_FREE(secret->configFile); - VIR_FREE(secret->base64File); + VIR_FREE(obj->configFile); + VIR_FREE(obj->base64File); }

@@ -186,14 +186,14 @@ virSecretObjPtr virSecretObjListFindByUUID(virSecretObjListPtr secrets, const unsigned char *uuid) { - virSecretObjPtr ret; + virSecretObjPtr obj;

virObjectLock(secrets); - ret = virSecretObjListFindByUUIDLocked(secrets, uuid); + obj = virSecretObjListFindByUUIDLocked(secrets, uuid); virObjectUnlock(secrets); - if (ret) - virObjectLock(ret); - return ret; + if (obj) + virObjectLock(obj); + return obj; }

@@ -202,21 +202,21 @@ virSecretObjSearchName(const void *payload, const void *name ATTRIBUTE_UNUSED, const void *opaque) { - virSecretObjPtr secret = (virSecretObjPtr) payload; + virSecretObjPtr obj = (virSecretObjPtr) payload; struct virSecretSearchData *data = (struct virSecretSearchData *) opaque; int found = 0;

- virObjectLock(secret); + virObjectLock(obj);

- if (secret->def->usage_type != data->usageType) + if (obj->def->usage_type != data->usageType) goto cleanup;

if (data->usageType != VIR_SECRET_USAGE_TYPE_NONE && - STREQ(secret->def->usage_id, data->usageID)) + STREQ(obj->def->usage_id, data->usageID)) found = 1;

cleanup: - virObjectUnlock(secret); + virObjectUnlock(obj); return found; }

@@ -226,14 +226,14 @@ virSecretObjListFindByUsageLocked(virSecretObjListPtr secrets, int usageType, const char *usageID) { - virSecretObjPtr ret = NULL; + virSecretObjPtr obj = NULL; struct virSecretSearchData data = { .usageType = usageType, .usageID = usageID };

- ret = virHashSearch(secrets->objs, virSecretObjSearchName, &data); - if (ret) - virObjectRef(ret); - return ret; + obj = virHashSearch(secrets->objs, virSecretObjSearchName, &data); + if (obj) + virObjectRef(obj); + return obj; }

@@ -253,14 +253,14 @@ virSecretObjListFindByUsage(virSecretObjListPtr secrets, int usageType, const char *usageID) { - virSecretObjPtr ret; + virSecretObjPtr obj;

virObjectLock(secrets); - ret = virSecretObjListFindByUsageLocked(secrets, usageType, usageID); + obj = virSecretObjListFindByUsageLocked(secrets, usageType, usageID); virObjectUnlock(secrets); - if (ret) - virObjectLock(ret); - return ret; + if (obj) + virObjectLock(obj); + return obj; }

@@ -275,19 +275,22 @@ virSecretObjListFindByUsage(virSecretObjListPtr secrets, */ void virSecretObjListRemove(virSecretObjListPtr secrets, - virSecretObjPtr secret) + virSecretObjPtr obj) { char uuidstr[VIR_UUID_STRING_BUFLEN];

- virUUIDFormat(secret->def->uuid, uuidstr); - virObjectRef(secret); - virObjectUnlock(secret); + if (!obj) + return; + + virUUIDFormat(obj->def->uuid, uuidstr); + virObjectRef(obj); + virObjectUnlock(obj);

virObjectLock(secrets); - virObjectLock(secret); + virObjectLock(obj); virHashRemoveEntry(secrets->objs, uuidstr); - virObjectUnlock(secret); - virObjectUnref(secret); + virObjectUnlock(obj); + virObjectUnref(obj); virObjectUnlock(secrets); }

@@ -295,11 +298,11 @@ virSecretObjListRemove(virSecretObjListPtr secrets, /* * virSecretObjListAddLocked: * @secrets: list of secret objects - * @def: new secret definition + * @newdef: new secret definition * @configDir: directory to place secret config files * @oldDef: Former secret def (e.g. a reload path perhaps) * - * Add the new def to the secret obj table hash + * Add the new @newdef to the secret obj table hash * * This functions requires @secrets to be locked already! * @@ -307,11 +310,11 @@ virSecretObjListRemove(virSecretObjListPtr secrets, */ static virSecretObjPtr virSecretObjListAddLocked(virSecretObjListPtr secrets, - virSecretDefPtr def, + virSecretDefPtr newdef, const char *configDir, virSecretDefPtr *oldDef) { - virSecretObjPtr secret; + virSecretObjPtr obj; virSecretObjPtr ret = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; char *configFile = NULL, *base64File = NULL; @@ -320,71 +323,69 @@ virSecretObjListAddLocked(virSecretObjListPtr secrets, *oldDef = NULL;

/* Is there a secret already matching this UUID */ - if ((secret = virSecretObjListFindByUUIDLocked(secrets, def->uuid))) { - virObjectLock(secret); + if ((obj = virSecretObjListFindByUUIDLocked(secrets, newdef->uuid))) { + virObjectLock(obj);

- if (STRNEQ_NULLABLE(secret->def->usage_id, def->usage_id)) { - virUUIDFormat(secret->def->uuid, uuidstr); + if (STRNEQ_NULLABLE(obj->def->usage_id, newdef->usage_id)) { + virUUIDFormat(obj->def->uuid, uuidstr); virReportError(VIR_ERR_INTERNAL_ERROR, _("a secret with UUID %s is already defined for " "use with %s"), - uuidstr, secret->def->usage_id); + uuidstr, obj->def->usage_id); goto cleanup; }

- if (secret->def->isprivate && !def->isprivate) { + if (obj->def->isprivate && !newdef->isprivate) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot change private flag on existing secret")); goto cleanup; }

if (oldDef) - *oldDef = secret->def; + *oldDef = obj->def; else - virSecretDefFree(secret->def); - secret->def = def; + virSecretDefFree(obj->def); + obj->def = newdef; } else { /* No existing secret with same UUID, * try look for matching usage instead */ - if ((secret = virSecretObjListFindByUsageLocked(secrets, - def->usage_type, - def->usage_id))) { - virObjectLock(secret); - virUUIDFormat(secret->def->uuid, uuidstr); + if ((obj = virSecretObjListFindByUsageLocked(secrets, + newdef->usage_type, + newdef->usage_id))) { + virObjectLock(obj); + virUUIDFormat(obj->def->uuid, uuidstr); virReportError(VIR_ERR_INTERNAL_ERROR, _("a secret with UUID %s already defined for " "use with %s"), - uuidstr, def->usage_id); + uuidstr, newdef->usage_id); goto cleanup; }

/* Generate the possible configFile and base64File strings * using the configDir, uuidstr, and appropriate suffix */ - virUUIDFormat(def->uuid, uuidstr); + virUUIDFormat(newdef->uuid, uuidstr); if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) || !(base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) goto cleanup;

- if (!(secret = virSecretObjNew())) + if (!(obj = virSecretObjNew())) goto cleanup;

- if (virHashAddEntry(secrets->objs, uuidstr, secret) < 0) + if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) goto cleanup;

- secret->def = def; - secret->configFile = configFile; - secret->base64File = base64File; - configFile = NULL; - base64File = NULL; - virObjectRef(secret); + obj->def = newdef; + VIR_STEAL_PTR(obj->configFile, configFile); + VIR_STEAL_PTR(obj->base64File, base64File); + virObjectRef(obj); }

- ret = secret; - secret = NULL; + ret = obj; + obj = NULL;

cleanup: - virSecretObjEndAPI(&secret); + virSecretObjEndAPI(&obj); VIR_FREE(configFile); VIR_FREE(base64File); return ret; @@ -393,16 +394,16 @@ virSecretObjListAddLocked(virSecretObjListPtr secrets,

virSecretObjPtr virSecretObjListAdd(virSecretObjListPtr secrets, - virSecretDefPtr def, + virSecretDefPtr newdef, const char *configDir, virSecretDefPtr *oldDef) { - virSecretObjPtr ret; + virSecretObjPtr obj;

virObjectLock(secrets); - ret = virSecretObjListAddLocked(secrets, def, configDir, oldDef); + obj = virSecretObjListAddLocked(secrets, newdef, configDir, oldDef); virObjectUnlock(secrets); - return ret; + return obj; }

@@ -474,23 +475,23 @@ virSecretObjListNumOfSecrets(virSecretObjListPtr secrets,

#define MATCH(FLAG) (flags & (FLAG)) static bool -virSecretObjMatchFlags(virSecretObjPtr secret, +virSecretObjMatchFlags(virSecretObjPtr obj, unsigned int flags) { /* filter by whether it's ephemeral */ if (MATCH(VIR_CONNECT_LIST_SECRETS_FILTERS_EPHEMERAL) && !((MATCH(VIR_CONNECT_LIST_SECRETS_EPHEMERAL) && - secret->def->isephemeral) || + obj->def->isephemeral) || (MATCH(VIR_CONNECT_LIST_SECRETS_NO_EPHEMERAL) && - !secret->def->isephemeral))) + !obj->def->isephemeral))) return false;

/* filter by whether it's private */ if (MATCH(VIR_CONNECT_LIST_SECRETS_FILTERS_PRIVATE) && !((MATCH(VIR_CONNECT_LIST_SECRETS_PRIVATE) && - secret->def->isprivate) || + obj->def->isprivate) || (MATCH(VIR_CONNECT_LIST_SECRETS_NO_PRIVATE) && - !secret->def->isprivate))) + !obj->def->isprivate))) return false;

return true; @@ -621,12 +622,12 @@ virSecretObjListGetUUIDs(virSecretObjListPtr secrets,

int -virSecretObjDeleteConfig(virSecretObjPtr secret) +virSecretObjDeleteConfig(virSecretObjPtr obj) { - if (!secret->def->isephemeral && - unlink(secret->configFile) < 0 && errno != ENOENT) { + if (!obj->def->isephemeral && + unlink(obj->configFile) < 0 && errno != ENOENT) { virReportSystemError(errno, _("cannot unlink '%s'"), - secret->configFile); + obj->configFile); return -1; }

@@ -635,11 +636,11 @@ virSecretObjDeleteConfig(virSecretObjPtr secret)

void -virSecretObjDeleteData(virSecretObjPtr secret) +virSecretObjDeleteData(virSecretObjPtr obj) { /* The configFile will already be removed, so secret won't be * loaded again if this fails */ - (void)unlink(secret->base64File); + (void)unlink(obj->base64File); }

@@ -650,15 +651,15 @@ virSecretObjDeleteData(virSecretObjPtr secret) secret is defined, it is stored as base64 (with no formatting) in "$basename.base64". "$basename" is in both cases the base64-encoded UUID. */ int -virSecretObjSaveConfig(virSecretObjPtr secret) +virSecretObjSaveConfig(virSecretObjPtr obj) { char *xml = NULL; int ret = -1;

- if (!(xml = virSecretDefFormat(secret->def))) + if (!(xml = virSecretDefFormat(obj->def))) goto cleanup;

- if (virFileRewriteStr(secret->configFile, S_IRUSR | S_IWUSR, xml) < 0) + if (virFileRewriteStr(obj->configFile, S_IRUSR | S_IWUSR, xml) < 0) goto cleanup;

ret = 0; @@ -670,18 +671,18 @@ virSecretObjSaveConfig(virSecretObjPtr secret)

int -virSecretObjSaveData(virSecretObjPtr secret) +virSecretObjSaveData(virSecretObjPtr obj) { char *base64 = NULL; int ret = -1;

- if (!secret->value) + if (!obj->value) return 0;

- if (!(base64 = virStringEncodeBase64(secret->value, secret->value_size))) + if (!(base64 = virStringEncodeBase64(obj->value, obj->value_size))) goto cleanup;

- if (virFileRewriteStr(secret->base64File, S_IRUSR | S_IWUSR, base64) < 0) + if (virFileRewriteStr(obj->base64File, S_IRUSR | S_IWUSR, base64) < 0) goto cleanup;

ret = 0; @@ -693,36 +694,36 @@ virSecretObjSaveData(virSecretObjPtr secret)

virSecretDefPtr -virSecretObjGetDef(virSecretObjPtr secret) +virSecretObjGetDef(virSecretObjPtr obj) { - return secret->def; + return obj->def; }

void -virSecretObjSetDef(virSecretObjPtr secret, +virSecretObjSetDef(virSecretObjPtr obj, virSecretDefPtr def) { - secret->def = def; + obj->def = def; }

unsigned char * -virSecretObjGetValue(virSecretObjPtr secret) +virSecretObjGetValue(virSecretObjPtr obj) { unsigned char *ret = NULL;

- if (!secret->value) { + if (!obj->value) { char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(secret->def->uuid, uuidstr); + virUUIDFormat(obj->def->uuid, uuidstr); virReportError(VIR_ERR_NO_SECRET, _("secret '%s' does not have a value"), uuidstr); goto cleanup; }

- if (VIR_ALLOC_N(ret, secret->value_size) < 0) + if (VIR_ALLOC_N(ret, obj->value_size) < 0) goto cleanup; - memcpy(ret, secret->value, secret->value_size); + memcpy(ret, obj->value, obj->value_size);

cleanup: return ret; @@ -730,7 +731,7 @@ virSecretObjGetValue(virSecretObjPtr secret)

int -virSecretObjSetValue(virSecretObjPtr secret, +virSecretObjSetValue(virSecretObjPtr obj, const unsigned char *value, size_t value_size) { @@ -740,14 +741,14 @@ virSecretObjSetValue(virSecretObjPtr secret, if (VIR_ALLOC_N(new_value, value_size) < 0) return -1;

- old_value = secret->value; - old_value_size = secret->value_size; + old_value = obj->value; + old_value_size = obj->value_size;

memcpy(new_value, value, value_size); - secret->value = new_value; - secret->value_size = value_size; + obj->value = new_value; + obj->value_size = value_size;

- if (!secret->def->isephemeral && virSecretObjSaveData(secret) < 0) + if (!obj->def->isephemeral && virSecretObjSaveData(obj) < 0) goto error;

/* Saved successfully - drop old value */ @@ -760,8 +761,8 @@ virSecretObjSetValue(virSecretObjPtr secret,

error: /* Error - restore previous state and free new value */ - secret->value = old_value; - secret->value_size = old_value_size; + obj->value = old_value; + obj->value_size = old_value_size; memset(new_value, 0, value_size); VIR_FREE(new_value); return -1; @@ -769,17 +770,17 @@ virSecretObjSetValue(virSecretObjPtr secret,

size_t -virSecretObjGetValueSize(virSecretObjPtr secret) +virSecretObjGetValueSize(virSecretObjPtr obj) { - return secret->value_size; + return obj->value_size; }

void -virSecretObjSetValueSize(virSecretObjPtr secret, +virSecretObjSetValueSize(virSecretObjPtr obj, size_t value_size) { - secret->value_size = value_size; + obj->value_size = value_size; }

@@ -803,33 +804,33 @@ virSecretLoadValidateUUID(virSecretDefPtr def,

static int -virSecretLoadValue(virSecretObjPtr secret) +virSecretLoadValue(virSecretObjPtr obj) { int ret = -1, fd = -1; struct stat st; char *contents = NULL, *value = NULL; size_t value_size;

- if ((fd = open(secret->base64File, O_RDONLY)) == -1) { + if ((fd = open(obj->base64File, O_RDONLY)) == -1) { if (errno == ENOENT) { ret = 0; goto cleanup; } virReportSystemError(errno, _("cannot open '%s'"), - secret->base64File); + obj->base64File); goto cleanup; }

if (fstat(fd, &st) < 0) { virReportSystemError(errno, _("cannot stat '%s'"), - secret->base64File); + obj->base64File); goto cleanup; }

if ((size_t)st.st_size != st.st_size) { virReportError(VIR_ERR_INTERNAL_ERROR, _("'%s' file does not fit in memory"), - secret->base64File); + obj->base64File); goto cleanup; }

@@ -838,7 +839,7 @@ virSecretLoadValue(virSecretObjPtr secret)

if (saferead(fd, contents, st.st_size) != st.st_size) { virReportSystemError(errno, _("cannot read '%s'"), - secret->base64File); + obj->base64File); goto cleanup; }

@@ -847,15 +848,15 @@ virSecretLoadValue(virSecretObjPtr secret) if (!base64_decode_alloc(contents, st.st_size, &value, &value_size)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid base64 in '%s'"), - secret->base64File); + obj->base64File); goto cleanup; } if (value == NULL) goto cleanup;

- secret->value = (unsigned char *)value; + obj->value = (unsigned char *)value; value = NULL; - secret->value_size = value_size; + obj->value_size = value_size;

ret = 0;

@@ -880,7 +881,8 @@ virSecretLoad(virSecretObjListPtr secrets, const char *configDir) { virSecretDefPtr def = NULL; - virSecretObjPtr secret = NULL, ret = NULL; + virSecretObjPtr obj = NULL; + virSecretObjPtr ret = NULL;

if (!(def = virSecretDefParseFile(path))) goto cleanup; @@ -888,19 +890,18 @@ virSecretLoad(virSecretObjListPtr secrets, if (virSecretLoadValidateUUID(def, file) < 0) goto cleanup;

- if (!(secret = virSecretObjListAdd(secrets, def, configDir, NULL))) + if (!(obj = virSecretObjListAdd(secrets, def, configDir, NULL))) goto cleanup; def = NULL;

- if (virSecretLoadValue(secret) < 0) + if (virSecretLoadValue(obj) < 0) goto cleanup;

- ret = secret; - secret = NULL; + ret = obj; + obj = NULL;

cleanup: - if (secret) - virSecretObjListRemove(secrets, secret); + virSecretObjListRemove(secrets, obj); virSecretDefFree(def); return ret; } @@ -921,7 +922,7 @@ virSecretLoadAllConfigs(virSecretObjListPtr secrets, * loop (if any). It's better to keep the secrets we managed to find. */ while (virDirRead(dir, &de, NULL) > 0) { char *path; - virSecretObjPtr secret; + virSecretObjPtr obj;

if (!virFileHasSuffix(de->d_name, ".xml")) continue; @@ -929,7 +930,7 @@ virSecretLoadAllConfigs(virSecretObjListPtr secrets, if (!(path = virFileBuildPath(configDir, de->d_name, NULL))) continue;

- if (!(secret = virSecretLoad(secrets, de->d_name, path, configDir))) { + if (!(obj = virSecretLoad(secrets, de->d_name, path, configDir))) { VIR_ERROR(_("Error reading secret: %s"), virGetLastErrorMessage()); VIR_FREE(path); @@ -937,7 +938,7 @@ virSecretLoadAllConfigs(virSecretObjListPtr secrets, }

VIR_FREE(path); - virSecretObjEndAPI(&secret); + virSecretObjEndAPI(&obj); }

VIR_DIR_CLOSE(dir); diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h index 9638b69..8038faa 100644 --- a/src/conf/virsecretobj.h +++ b/src/conf/virsecretobj.h @@ -30,7 +30,7 @@ typedef struct _virSecretObj virSecretObj; typedef virSecretObj *virSecretObjPtr;

void -virSecretObjEndAPI(virSecretObjPtr *secret); +virSecretObjEndAPI(virSecretObjPtr *obj);

typedef struct _virSecretObjList virSecretObjList; typedef virSecretObjList *virSecretObjListPtr; @@ -49,11 +49,11 @@ virSecretObjListFindByUsage(virSecretObjListPtr secrets,

void virSecretObjListRemove(virSecretObjListPtr secrets, - virSecretObjPtr secret); + virSecretObjPtr obj);

virSecretObjPtr virSecretObjListAdd(virSecretObjListPtr secrets, - virSecretDefPtr def, + virSecretDefPtr newdef, const char *configDir, virSecretDefPtr *oldDef);

@@ -81,37 +81,37 @@ virSecretObjListGetUUIDs(virSecretObjListPtr secrets, virConnectPtr conn);

int -virSecretObjDeleteConfig(virSecretObjPtr secret); +virSecretObjDeleteConfig(virSecretObjPtr obj);

void -virSecretObjDeleteData(virSecretObjPtr secret); +virSecretObjDeleteData(virSecretObjPtr obj);

int -virSecretObjSaveConfig(virSecretObjPtr secret); +virSecretObjSaveConfig(virSecretObjPtr obj);

int -virSecretObjSaveData(virSecretObjPtr secret); +virSecretObjSaveData(virSecretObjPtr obj);

virSecretDefPtr -virSecretObjGetDef(virSecretObjPtr secret); +virSecretObjGetDef(virSecretObjPtr obj);

void -virSecretObjSetDef(virSecretObjPtr secret, +virSecretObjSetDef(virSecretObjPtr obj, virSecretDefPtr def);

unsigned char * -virSecretObjGetValue(virSecretObjPtr secret); +virSecretObjGetValue(virSecretObjPtr obj);

int -virSecretObjSetValue(virSecretObjPtr secret, +virSecretObjSetValue(virSecretObjPtr obj, const unsigned char *value, size_t value_size);

size_t -virSecretObjGetValueSize(virSecretObjPtr secret); +virSecretObjGetValueSize(virSecretObjPtr obj);

void -virSecretObjSetValueSize(virSecretObjPtr secret, +virSecretObjSetValueSize(virSecretObjPtr obj, size_t value_size);

int diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 2a371b6..cc050ff 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -72,6 +72,7 @@ secretDriverLock(void) virMutexLock(&driver->lock); }

+ static void secretDriverUnlock(void) { @@ -79,7 +80,6 @@ secretDriverUnlock(void) }

- static virSecretObjPtr secretObjFromSecret(virSecretPtr secret) { @@ -120,6 +120,7 @@ secretConnectNumOfSecrets(virConnectPtr conn) conn); }

+ static int secretConnectListSecrets(virConnectPtr conn, char **uuids, @@ -156,10 +157,10 @@ secretLookupByUUID(virConnectPtr conn, const unsigned char *uuid) { virSecretPtr ret = NULL; - virSecretObjPtr secret; + virSecretObjPtr obj; virSecretDefPtr def;

- if (!(secret = virSecretObjListFindByUUID(driver->secrets, uuid))) { + if (!(obj = virSecretObjListFindByUUID(driver->secrets, uuid))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(uuid, uuidstr); virReportError(VIR_ERR_NO_SECRET, @@ -167,7 +168,7 @@ secretLookupByUUID(virConnectPtr conn, goto cleanup; }

- def = virSecretObjGetDef(secret); + def = virSecretObjGetDef(obj); if (virSecretLookupByUUIDEnsureACL(conn, def) < 0) goto cleanup;

@@ -177,7 +178,7 @@ secretLookupByUUID(virConnectPtr conn, def->usage_id);

cleanup: - virSecretObjEndAPI(&secret); + virSecretObjEndAPI(&obj); return ret; }

@@ -188,17 +189,17 @@ secretLookupByUsage(virConnectPtr conn, const char *usageID) { virSecretPtr ret = NULL; - virSecretObjPtr secret; + virSecretObjPtr obj; virSecretDefPtr def;

- if (!(secret = virSecretObjListFindByUsage(driver->secrets, - usageType, usageID))) { + if (!(obj = virSecretObjListFindByUsage(driver->secrets, + usageType, usageID))) { virReportError(VIR_ERR_NO_SECRET, _("no secret with matching usage '%s'"), usageID); goto cleanup; }

- def = virSecretObjGetDef(secret); + def = virSecretObjGetDef(obj); if (virSecretLookupByUsageEnsureACL(conn, def) < 0) goto cleanup;

@@ -208,7 +209,7 @@ secretLookupByUsage(virConnectPtr conn, def->usage_id);

cleanup: - virSecretObjEndAPI(&secret); + virSecretObjEndAPI(&obj); return ret; }

@@ -219,129 +220,131 @@ secretDefineXML(virConnectPtr conn, unsigned int flags) { virSecretPtr ret = NULL; - virSecretObjPtr secret = NULL; + virSecretObjPtr obj = NULL; virSecretDefPtr backup = NULL; - virSecretDefPtr new_attrs; + virSecretDefPtr def; virObjectEventPtr event = NULL;

virCheckFlags(0, NULL);

- if (!(new_attrs = virSecretDefParseString(xml))) + if (!(def = virSecretDefParseString(xml))) return NULL;

- if (virSecretDefineXMLEnsureACL(conn, new_attrs) < 0) + if (virSecretDefineXMLEnsureACL(conn, def) < 0) goto cleanup;

- if (!(secret = virSecretObjListAdd(driver->secrets, new_attrs, - driver->configDir, &backup))) + if (!(obj = virSecretObjListAdd(driver->secrets, def, + driver->configDir, &backup))) goto cleanup;

- if (!new_attrs->isephemeral) { + if (!def->isephemeral) { if (secretEnsureDirectory() < 0) goto cleanup;

if (backup && backup->isephemeral) { - if (virSecretObjSaveData(secret) < 0) + if (virSecretObjSaveData(obj) < 0) goto restore_backup; }

- if (virSecretObjSaveConfig(secret) < 0) { + if (virSecretObjSaveConfig(obj) < 0) { if (backup && backup->isephemeral) { /* Undo the virSecretObjSaveData() above; ignore errors */ - virSecretObjDeleteData(secret); + virSecretObjDeleteData(obj); } goto restore_backup; } } else if (backup && !backup->isephemeral) { - if (virSecretObjDeleteConfig(secret) < 0) + if (virSecretObjDeleteConfig(obj) < 0) goto restore_backup;

- virSecretObjDeleteData(secret); + virSecretObjDeleteData(obj); } /* Saved successfully - drop old values */ virSecretDefFree(backup);

- event = virSecretEventLifecycleNew(new_attrs->uuid, - new_attrs->usage_type, - new_attrs->usage_id, + event = virSecretEventLifecycleNew(def->uuid, + def->usage_type, + def->usage_id, VIR_SECRET_EVENT_DEFINED, 0);

ret = virGetSecret(conn, - new_attrs->uuid, - new_attrs->usage_type, - new_attrs->usage_id); - new_attrs = NULL; + def->uuid, + def->usage_type, + def->usage_id); + def = NULL; goto cleanup;

restore_backup: /* If we have a backup, then secret was defined before, so just restore - * the backup. The current (new_attrs) will be handled below. + * the backup. The current def will be handled below. * Otherwise, this is a new secret, thus remove it. */ if (backup) - virSecretObjSetDef(secret, backup); + virSecretObjSetDef(obj, backup); else - virSecretObjListRemove(driver->secrets, secret); + virSecretObjListRemove(driver->secrets, obj);

cleanup: - virSecretDefFree(new_attrs); - virSecretObjEndAPI(&secret); + virSecretDefFree(def); + virSecretObjEndAPI(&obj); if (event) virObjectEventStateQueue(driver->secretEventState, event);

return ret; }

+ static char * -secretGetXMLDesc(virSecretPtr obj, +secretGetXMLDesc(virSecretPtr secret, unsigned int flags) { char *ret = NULL; - virSecretObjPtr secret; + virSecretObjPtr obj; virSecretDefPtr def;

virCheckFlags(0, NULL);

- if (!(secret = secretObjFromSecret(obj))) + if (!(obj = secretObjFromSecret(secret))) goto cleanup;

- def = virSecretObjGetDef(secret); - if (virSecretGetXMLDescEnsureACL(obj->conn, def) < 0) + def = virSecretObjGetDef(obj); + if (virSecretGetXMLDescEnsureACL(secret->conn, def) < 0) goto cleanup;

ret = virSecretDefFormat(def);

cleanup: - virSecretObjEndAPI(&secret); + virSecretObjEndAPI(&obj);

return ret; }

+ static int -secretSetValue(virSecretPtr obj, +secretSetValue(virSecretPtr secret, const unsigned char *value, size_t value_size, unsigned int flags) { int ret = -1; - virSecretObjPtr secret; + virSecretObjPtr obj; virSecretDefPtr def; virObjectEventPtr event = NULL;

virCheckFlags(0, -1);

- if (!(secret = secretObjFromSecret(obj))) + if (!(obj = secretObjFromSecret(secret))) goto cleanup;

- def = virSecretObjGetDef(secret); - if (virSecretSetValueEnsureACL(obj->conn, def) < 0) + def = virSecretObjGetDef(obj); + if (virSecretSetValueEnsureACL(secret->conn, def) < 0) goto cleanup;

if (secretEnsureDirectory() < 0) goto cleanup;

- if (virSecretObjSetValue(secret, value, value_size) < 0) + if (virSecretObjSetValue(obj, value, value_size) < 0) goto cleanup;

event = virSecretEventValueChangedNew(def->uuid, @@ -350,30 +353,31 @@ secretSetValue(virSecretPtr obj, ret = 0;

cleanup: - virSecretObjEndAPI(&secret); + virSecretObjEndAPI(&obj); if (event) virObjectEventStateQueue(driver->secretEventState, event);

return ret; }

+ static unsigned char * -secretGetValue(virSecretPtr obj, +secretGetValue(virSecretPtr secret, size_t *value_size, unsigned int flags, unsigned int internalFlags) { unsigned char *ret = NULL; - virSecretObjPtr secret; + virSecretObjPtr obj; virSecretDefPtr def;

virCheckFlags(0, NULL);

- if (!(secret = secretObjFromSecret(obj))) + if (!(obj = secretObjFromSecret(secret))) goto cleanup;

- def = virSecretObjGetDef(secret); - if (virSecretGetValueEnsureACL(obj->conn, def) < 0) + def = virSecretObjGetDef(obj); + if (virSecretGetValueEnsureACL(secret->conn, def) < 0) goto cleanup;

if ((internalFlags & VIR_SECRET_GET_VALUE_INTERNAL_CALL) == 0 && @@ -383,33 +387,34 @@ secretGetValue(virSecretPtr obj, goto cleanup; }

- if (!(ret = virSecretObjGetValue(secret))) + if (!(ret = virSecretObjGetValue(obj))) goto cleanup;

- *value_size = virSecretObjGetValueSize(secret); + *value_size = virSecretObjGetValueSize(obj);

cleanup: - virSecretObjEndAPI(&secret); + virSecretObjEndAPI(&obj);

return ret; }

+ static int -secretUndefine(virSecretPtr obj) +secretUndefine(virSecretPtr secret) { int ret = -1; - virSecretObjPtr secret; + virSecretObjPtr obj; virSecretDefPtr def; virObjectEventPtr event = NULL;

- if (!(secret = secretObjFromSecret(obj))) + if (!(obj = secretObjFromSecret(secret))) goto cleanup;

- def = virSecretObjGetDef(secret); - if (virSecretUndefineEnsureACL(obj->conn, def) < 0) + def = virSecretObjGetDef(obj); + if (virSecretUndefineEnsureACL(secret->conn, def) < 0) goto cleanup;

- if (virSecretObjDeleteConfig(secret) < 0) + if (virSecretObjDeleteConfig(obj) < 0) goto cleanup;

event = virSecretEventLifecycleNew(def->uuid, @@ -418,20 +423,21 @@ secretUndefine(virSecretPtr obj) VIR_SECRET_EVENT_UNDEFINED, 0);

- virSecretObjDeleteData(secret); + virSecretObjDeleteData(obj);

- virSecretObjListRemove(driver->secrets, secret); + virSecretObjListRemove(driver->secrets, obj);

ret = 0;

cleanup: - virSecretObjEndAPI(&secret); + virSecretObjEndAPI(&obj); if (event) virObjectEventStateQueue(driver->secretEventState, event);

return ret; }

+ static int secretStateCleanup(void) { @@ -452,6 +458,7 @@ secretStateCleanup(void) return 0; }

+ static int secretStateInitialize(bool privileged, virStateInhibitCallback callback ATTRIBUTE_UNUSED, @@ -497,6 +504,7 @@ secretStateInitialize(bool privileged, return -1; }

+ static int secretStateReload(void) { @@ -511,6 +519,7 @@ secretStateReload(void) return 0; }

+ static int secretConnectSecretEventRegisterAny(virConnectPtr conn, virSecretPtr secret, @@ -532,6 +541,7 @@ secretConnectSecretEventRegisterAny(virConnectPtr conn, return callbackID; }

+ static int secretConnectSecretEventDeregisterAny(virConnectPtr conn, int callbackID) @@ -576,6 +586,7 @@ static virStateDriver stateDriver = { .stateReload = secretStateReload, };

+ int secretRegister(void) { -- 2.9.3

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, Apr 25, 2017 at 07:57:44AM -0400, John Ferlan wrote:

On 04/25/2017 04:29 AM, Pavel Hrdina wrote:

...

On Mon, Apr 24, 2017 at 02:00:14PM -0400, John Ferlan wrote:

...

When processing a virSecretPtr use 'secret' as a variable name.

When processing a virSecretObjPtr use 'obj' as a variable name.

When processing a virSecretDefPtr use 'def' as a variable name, unless a distinction needs to be made with a 'newdef' such as virSecretObjListAddLocked (which also used the VIR_STEAL_PTR macro for the configFile and base64File).

NB: Also made a slight algorithm adjustment for virSecretObjListRemove to check if the passed obj was NULL rather than having the caller virSecretLoad need to check prior to calling.

I can see the motivation for this change, it's easier to follow the code if the naming is consistent, however this patch adds one extra step when tracing history of the code via git blame.

Yes and I'm certainly not the first one down this path! Code motion tends to be the biggest offender though. At least with git blame variable name changes don't cause one to jump into some other module to find the code.... My least favorite is when a module "disappears" or is renamed - those always cause heartache, but I always find that gitk will find whatever I need without having to know some magic git command.

I'm not against this change, I just think that it's not necessary. If there are no objections from others I guess that it can be ACKed.

...

All the changes except the variable rename should be in separate patch as they are unrelated to the rename.

I can split out the virSecretObjListRemove change into a separate patch since it's the only non name change patch.

I guess that it would be better to split it. Pavel

John

...

Pavel

...

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 275 +++++++++++++++++++++++---------------------- src/conf/virsecretobj.h | 26 ++--- src/secret/secret_driver.c | 139 ++++++++++++----------- 3 files changed, 226 insertions(+), 214 deletions(-)

diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 348feb3..42f36c8 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -89,29 +89,29 @@ VIR_ONCE_GLOBAL_INIT(virSecretObj) static virSecretObjPtr virSecretObjNew(void) { - virSecretObjPtr secret; + virSecretObjPtr obj;

if (virSecretObjInitialize() < 0) return NULL;

- if (!(secret = virObjectLockableNew(virSecretObjClass))) + if (!(obj = virObjectLockableNew(virSecretObjClass))) return NULL;

- virObjectLock(secret); + virObjectLock(obj);

- return secret; + return obj; }

void -virSecretObjEndAPI(virSecretObjPtr *secret) +virSecretObjEndAPI(virSecretObjPtr *obj) { - if (!*secret) + if (!*obj) return;

- virObjectUnlock(*secret); - virObjectUnref(*secret); - *secret = NULL; + virObjectUnlock(*obj); + virObjectUnref(*obj); + *obj = NULL; }

@@ -136,18 +136,18 @@ virSecretObjListNew(void)

static void -virSecretObjDispose(void *obj) +virSecretObjDispose(void *opaque) { - virSecretObjPtr secret = obj; + virSecretObjPtr obj = opaque;

- virSecretDefFree(secret->def); - if (secret->value) { + virSecretDefFree(obj->def); + if (obj->value) { /* Wipe before free to ensure we don't leave a secret on the heap */ - memset(secret->value, 0, secret->value_size); - VIR_FREE(secret->value); + memset(obj->value, 0, obj->value_size); + VIR_FREE(obj->value); } - VIR_FREE(secret->configFile); - VIR_FREE(secret->base64File); + VIR_FREE(obj->configFile); + VIR_FREE(obj->base64File); }

@@ -186,14 +186,14 @@ virSecretObjPtr virSecretObjListFindByUUID(virSecretObjListPtr secrets, const unsigned char *uuid) { - virSecretObjPtr ret; + virSecretObjPtr obj;

virObjectLock(secrets); - ret = virSecretObjListFindByUUIDLocked(secrets, uuid); + obj = virSecretObjListFindByUUIDLocked(secrets, uuid); virObjectUnlock(secrets); - if (ret) - virObjectLock(ret); - return ret; + if (obj) + virObjectLock(obj); + return obj; }

@@ -202,21 +202,21 @@ virSecretObjSearchName(const void *payload, const void *name ATTRIBUTE_UNUSED, const void *opaque) { - virSecretObjPtr secret = (virSecretObjPtr) payload; + virSecretObjPtr obj = (virSecretObjPtr) payload; struct virSecretSearchData *data = (struct virSecretSearchData *) opaque; int found = 0;

- virObjectLock(secret); + virObjectLock(obj);

- if (secret->def->usage_type != data->usageType) + if (obj->def->usage_type != data->usageType) goto cleanup;

if (data->usageType != VIR_SECRET_USAGE_TYPE_NONE && - STREQ(secret->def->usage_id, data->usageID)) + STREQ(obj->def->usage_id, data->usageID)) found = 1;

cleanup: - virObjectUnlock(secret); + virObjectUnlock(obj); return found; }

@@ -226,14 +226,14 @@ virSecretObjListFindByUsageLocked(virSecretObjListPtr secrets, int usageType, const char *usageID) { - virSecretObjPtr ret = NULL; + virSecretObjPtr obj = NULL; struct virSecretSearchData data = { .usageType = usageType, .usageID = usageID };

- ret = virHashSearch(secrets->objs, virSecretObjSearchName, &data); - if (ret) - virObjectRef(ret); - return ret; + obj = virHashSearch(secrets->objs, virSecretObjSearchName, &data); + if (obj) + virObjectRef(obj); + return obj; }

@@ -253,14 +253,14 @@ virSecretObjListFindByUsage(virSecretObjListPtr secrets, int usageType, const char *usageID) { - virSecretObjPtr ret; + virSecretObjPtr obj;

virObjectLock(secrets); - ret = virSecretObjListFindByUsageLocked(secrets, usageType, usageID); + obj = virSecretObjListFindByUsageLocked(secrets, usageType, usageID); virObjectUnlock(secrets); - if (ret) - virObjectLock(ret); - return ret; + if (obj) + virObjectLock(obj); + return obj; }

@@ -275,19 +275,22 @@ virSecretObjListFindByUsage(virSecretObjListPtr secrets, */ void virSecretObjListRemove(virSecretObjListPtr secrets, - virSecretObjPtr secret) + virSecretObjPtr obj) { char uuidstr[VIR_UUID_STRING_BUFLEN];

- virUUIDFormat(secret->def->uuid, uuidstr); - virObjectRef(secret); - virObjectUnlock(secret); + if (!obj) + return; + + virUUIDFormat(obj->def->uuid, uuidstr); + virObjectRef(obj); + virObjectUnlock(obj);

virObjectLock(secrets); - virObjectLock(secret); + virObjectLock(obj); virHashRemoveEntry(secrets->objs, uuidstr); - virObjectUnlock(secret); - virObjectUnref(secret); + virObjectUnlock(obj); + virObjectUnref(obj); virObjectUnlock(secrets); }

@@ -295,11 +298,11 @@ virSecretObjListRemove(virSecretObjListPtr secrets, /* * virSecretObjListAddLocked: * @secrets: list of secret objects - * @def: new secret definition + * @newdef: new secret definition * @configDir: directory to place secret config files * @oldDef: Former secret def (e.g. a reload path perhaps) * - * Add the new def to the secret obj table hash + * Add the new @newdef to the secret obj table hash * * This functions requires @secrets to be locked already! * @@ -307,11 +310,11 @@ virSecretObjListRemove(virSecretObjListPtr secrets, */ static virSecretObjPtr virSecretObjListAddLocked(virSecretObjListPtr secrets, - virSecretDefPtr def, + virSecretDefPtr newdef, const char *configDir, virSecretDefPtr *oldDef) { - virSecretObjPtr secret; + virSecretObjPtr obj; virSecretObjPtr ret = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; char *configFile = NULL, *base64File = NULL; @@ -320,71 +323,69 @@ virSecretObjListAddLocked(virSecretObjListPtr secrets, *oldDef = NULL;

/* Is there a secret already matching this UUID */ - if ((secret = virSecretObjListFindByUUIDLocked(secrets, def->uuid))) { - virObjectLock(secret); + if ((obj = virSecretObjListFindByUUIDLocked(secrets, newdef->uuid))) { + virObjectLock(obj);

- if (STRNEQ_NULLABLE(secret->def->usage_id, def->usage_id)) { - virUUIDFormat(secret->def->uuid, uuidstr); + if (STRNEQ_NULLABLE(obj->def->usage_id, newdef->usage_id)) { + virUUIDFormat(obj->def->uuid, uuidstr); virReportError(VIR_ERR_INTERNAL_ERROR, _("a secret with UUID %s is already defined for " "use with %s"), - uuidstr, secret->def->usage_id); + uuidstr, obj->def->usage_id); goto cleanup; }

- if (secret->def->isprivate && !def->isprivate) { + if (obj->def->isprivate && !newdef->isprivate) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot change private flag on existing secret")); goto cleanup; }

if (oldDef) - *oldDef = secret->def; + *oldDef = obj->def; else - virSecretDefFree(secret->def); - secret->def = def; + virSecretDefFree(obj->def); + obj->def = newdef; } else { /* No existing secret with same UUID, * try look for matching usage instead */ - if ((secret = virSecretObjListFindByUsageLocked(secrets, - def->usage_type, - def->usage_id))) { - virObjectLock(secret); - virUUIDFormat(secret->def->uuid, uuidstr); + if ((obj = virSecretObjListFindByUsageLocked(secrets, + newdef->usage_type, + newdef->usage_id))) { + virObjectLock(obj); + virUUIDFormat(obj->def->uuid, uuidstr); virReportError(VIR_ERR_INTERNAL_ERROR, _("a secret with UUID %s already defined for " "use with %s"), - uuidstr, def->usage_id); + uuidstr, newdef->usage_id); goto cleanup; }

/* Generate the possible configFile and base64File strings * using the configDir, uuidstr, and appropriate suffix */ - virUUIDFormat(def->uuid, uuidstr); + virUUIDFormat(newdef->uuid, uuidstr); if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) || !(base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) goto cleanup;

- if (!(secret = virSecretObjNew())) + if (!(obj = virSecretObjNew())) goto cleanup;

- if (virHashAddEntry(secrets->objs, uuidstr, secret) < 0) + if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) goto cleanup;

- secret->def = def; - secret->configFile = configFile; - secret->base64File = base64File; - configFile = NULL; - base64File = NULL; - virObjectRef(secret); + obj->def = newdef; + VIR_STEAL_PTR(obj->configFile, configFile); + VIR_STEAL_PTR(obj->base64File, base64File); + virObjectRef(obj); }

- ret = secret; - secret = NULL; + ret = obj; + obj = NULL;

cleanup: - virSecretObjEndAPI(&secret); + virSecretObjEndAPI(&obj); VIR_FREE(configFile); VIR_FREE(base64File); return ret; @@ -393,16 +394,16 @@ virSecretObjListAddLocked(virSecretObjListPtr secrets,

virSecretObjPtr virSecretObjListAdd(virSecretObjListPtr secrets, - virSecretDefPtr def, + virSecretDefPtr newdef, const char *configDir, virSecretDefPtr *oldDef) { - virSecretObjPtr ret; + virSecretObjPtr obj;

virObjectLock(secrets); - ret = virSecretObjListAddLocked(secrets, def, configDir, oldDef); + obj = virSecretObjListAddLocked(secrets, newdef, configDir, oldDef); virObjectUnlock(secrets); - return ret; + return obj; }

@@ -474,23 +475,23 @@ virSecretObjListNumOfSecrets(virSecretObjListPtr secrets,

#define MATCH(FLAG) (flags & (FLAG)) static bool -virSecretObjMatchFlags(virSecretObjPtr secret, +virSecretObjMatchFlags(virSecretObjPtr obj, unsigned int flags) { /* filter by whether it's ephemeral */ if (MATCH(VIR_CONNECT_LIST_SECRETS_FILTERS_EPHEMERAL) && !((MATCH(VIR_CONNECT_LIST_SECRETS_EPHEMERAL) && - secret->def->isephemeral) || + obj->def->isephemeral) || (MATCH(VIR_CONNECT_LIST_SECRETS_NO_EPHEMERAL) && - !secret->def->isephemeral))) + !obj->def->isephemeral))) return false;

/* filter by whether it's private */ if (MATCH(VIR_CONNECT_LIST_SECRETS_FILTERS_PRIVATE) && !((MATCH(VIR_CONNECT_LIST_SECRETS_PRIVATE) && - secret->def->isprivate) || + obj->def->isprivate) || (MATCH(VIR_CONNECT_LIST_SECRETS_NO_PRIVATE) && - !secret->def->isprivate))) + !obj->def->isprivate))) return false;

return true; @@ -621,12 +622,12 @@ virSecretObjListGetUUIDs(virSecretObjListPtr secrets,

int -virSecretObjDeleteConfig(virSecretObjPtr secret) +virSecretObjDeleteConfig(virSecretObjPtr obj) { - if (!secret->def->isephemeral && - unlink(secret->configFile) < 0 && errno != ENOENT) { + if (!obj->def->isephemeral && + unlink(obj->configFile) < 0 && errno != ENOENT) { virReportSystemError(errno, _("cannot unlink '%s'"), - secret->configFile); + obj->configFile); return -1; }

@@ -635,11 +636,11 @@ virSecretObjDeleteConfig(virSecretObjPtr secret)

void -virSecretObjDeleteData(virSecretObjPtr secret) +virSecretObjDeleteData(virSecretObjPtr obj) { /* The configFile will already be removed, so secret won't be * loaded again if this fails */ - (void)unlink(secret->base64File); + (void)unlink(obj->base64File); }

@@ -650,15 +651,15 @@ virSecretObjDeleteData(virSecretObjPtr secret) secret is defined, it is stored as base64 (with no formatting) in "$basename.base64". "$basename" is in both cases the base64-encoded UUID. */ int -virSecretObjSaveConfig(virSecretObjPtr secret) +virSecretObjSaveConfig(virSecretObjPtr obj) { char *xml = NULL; int ret = -1;

- if (!(xml = virSecretDefFormat(secret->def))) + if (!(xml = virSecretDefFormat(obj->def))) goto cleanup;

- if (virFileRewriteStr(secret->configFile, S_IRUSR | S_IWUSR, xml) < 0) + if (virFileRewriteStr(obj->configFile, S_IRUSR | S_IWUSR, xml) < 0) goto cleanup;

ret = 0; @@ -670,18 +671,18 @@ virSecretObjSaveConfig(virSecretObjPtr secret)

int -virSecretObjSaveData(virSecretObjPtr secret) +virSecretObjSaveData(virSecretObjPtr obj) { char *base64 = NULL; int ret = -1;

- if (!secret->value) + if (!obj->value) return 0;

- if (!(base64 = virStringEncodeBase64(secret->value, secret->value_size))) + if (!(base64 = virStringEncodeBase64(obj->value, obj->value_size))) goto cleanup;

- if (virFileRewriteStr(secret->base64File, S_IRUSR | S_IWUSR, base64) < 0) + if (virFileRewriteStr(obj->base64File, S_IRUSR | S_IWUSR, base64) < 0) goto cleanup;

ret = 0; @@ -693,36 +694,36 @@ virSecretObjSaveData(virSecretObjPtr secret)

virSecretDefPtr -virSecretObjGetDef(virSecretObjPtr secret) +virSecretObjGetDef(virSecretObjPtr obj) { - return secret->def; + return obj->def; }

void -virSecretObjSetDef(virSecretObjPtr secret, +virSecretObjSetDef(virSecretObjPtr obj, virSecretDefPtr def) { - secret->def = def; + obj->def = def; }

unsigned char * -virSecretObjGetValue(virSecretObjPtr secret) +virSecretObjGetValue(virSecretObjPtr obj) { unsigned char *ret = NULL;

- if (!secret->value) { + if (!obj->value) { char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(secret->def->uuid, uuidstr); + virUUIDFormat(obj->def->uuid, uuidstr); virReportError(VIR_ERR_NO_SECRET, _("secret '%s' does not have a value"), uuidstr); goto cleanup; }

- if (VIR_ALLOC_N(ret, secret->value_size) < 0) + if (VIR_ALLOC_N(ret, obj->value_size) < 0) goto cleanup; - memcpy(ret, secret->value, secret->value_size); + memcpy(ret, obj->value, obj->value_size);

cleanup: return ret; @@ -730,7 +731,7 @@ virSecretObjGetValue(virSecretObjPtr secret)

int -virSecretObjSetValue(virSecretObjPtr secret, +virSecretObjSetValue(virSecretObjPtr obj, const unsigned char *value, size_t value_size) { @@ -740,14 +741,14 @@ virSecretObjSetValue(virSecretObjPtr secret, if (VIR_ALLOC_N(new_value, value_size) < 0) return -1;

- old_value = secret->value; - old_value_size = secret->value_size; + old_value = obj->value; + old_value_size = obj->value_size;

memcpy(new_value, value, value_size); - secret->value = new_value; - secret->value_size = value_size; + obj->value = new_value; + obj->value_size = value_size;

- if (!secret->def->isephemeral && virSecretObjSaveData(secret) < 0) + if (!obj->def->isephemeral && virSecretObjSaveData(obj) < 0) goto error;

/* Saved successfully - drop old value */ @@ -760,8 +761,8 @@ virSecretObjSetValue(virSecretObjPtr secret,

error: /* Error - restore previous state and free new value */ - secret->value = old_value; - secret->value_size = old_value_size; + obj->value = old_value; + obj->value_size = old_value_size; memset(new_value, 0, value_size); VIR_FREE(new_value); return -1; @@ -769,17 +770,17 @@ virSecretObjSetValue(virSecretObjPtr secret,

size_t -virSecretObjGetValueSize(virSecretObjPtr secret) +virSecretObjGetValueSize(virSecretObjPtr obj) { - return secret->value_size; + return obj->value_size; }

void -virSecretObjSetValueSize(virSecretObjPtr secret, +virSecretObjSetValueSize(virSecretObjPtr obj, size_t value_size) { - secret->value_size = value_size; + obj->value_size = value_size; }

@@ -803,33 +804,33 @@ virSecretLoadValidateUUID(virSecretDefPtr def,

static int -virSecretLoadValue(virSecretObjPtr secret) +virSecretLoadValue(virSecretObjPtr obj) { int ret = -1, fd = -1; struct stat st; char *contents = NULL, *value = NULL; size_t value_size;

- if ((fd = open(secret->base64File, O_RDONLY)) == -1) { + if ((fd = open(obj->base64File, O_RDONLY)) == -1) { if (errno == ENOENT) { ret = 0; goto cleanup; } virReportSystemError(errno, _("cannot open '%s'"), - secret->base64File); + obj->base64File); goto cleanup; }

if (fstat(fd, &st) < 0) { virReportSystemError(errno, _("cannot stat '%s'"), - secret->base64File); + obj->base64File); goto cleanup; }

if ((size_t)st.st_size != st.st_size) { virReportError(VIR_ERR_INTERNAL_ERROR, _("'%s' file does not fit in memory"), - secret->base64File); + obj->base64File); goto cleanup; }

@@ -838,7 +839,7 @@ virSecretLoadValue(virSecretObjPtr secret)

if (saferead(fd, contents, st.st_size) != st.st_size) { virReportSystemError(errno, _("cannot read '%s'"), - secret->base64File); + obj->base64File); goto cleanup; }

@@ -847,15 +848,15 @@ virSecretLoadValue(virSecretObjPtr secret) if (!base64_decode_alloc(contents, st.st_size, &value, &value_size)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid base64 in '%s'"), - secret->base64File); + obj->base64File); goto cleanup; } if (value == NULL) goto cleanup;

- secret->value = (unsigned char *)value; + obj->value = (unsigned char *)value; value = NULL; - secret->value_size = value_size; + obj->value_size = value_size;

ret = 0;

@@ -880,7 +881,8 @@ virSecretLoad(virSecretObjListPtr secrets, const char *configDir) { virSecretDefPtr def = NULL; - virSecretObjPtr secret = NULL, ret = NULL; + virSecretObjPtr obj = NULL; + virSecretObjPtr ret = NULL;

if (!(def = virSecretDefParseFile(path))) goto cleanup; @@ -888,19 +890,18 @@ virSecretLoad(virSecretObjListPtr secrets, if (virSecretLoadValidateUUID(def, file) < 0) goto cleanup;

- if (!(secret = virSecretObjListAdd(secrets, def, configDir, NULL))) + if (!(obj = virSecretObjListAdd(secrets, def, configDir, NULL))) goto cleanup; def = NULL;

- if (virSecretLoadValue(secret) < 0) + if (virSecretLoadValue(obj) < 0) goto cleanup;

- ret = secret; - secret = NULL; + ret = obj; + obj = NULL;

cleanup: - if (secret) - virSecretObjListRemove(secrets, secret); + virSecretObjListRemove(secrets, obj); virSecretDefFree(def); return ret; } @@ -921,7 +922,7 @@ virSecretLoadAllConfigs(virSecretObjListPtr secrets, * loop (if any). It's better to keep the secrets we managed to find. */ while (virDirRead(dir, &de, NULL) > 0) { char *path; - virSecretObjPtr secret; + virSecretObjPtr obj;

if (!virFileHasSuffix(de->d_name, ".xml")) continue; @@ -929,7 +930,7 @@ virSecretLoadAllConfigs(virSecretObjListPtr secrets, if (!(path = virFileBuildPath(configDir, de->d_name, NULL))) continue;

- if (!(secret = virSecretLoad(secrets, de->d_name, path, configDir))) { + if (!(obj = virSecretLoad(secrets, de->d_name, path, configDir))) { VIR_ERROR(_("Error reading secret: %s"), virGetLastErrorMessage()); VIR_FREE(path); @@ -937,7 +938,7 @@ virSecretLoadAllConfigs(virSecretObjListPtr secrets, }

VIR_FREE(path); - virSecretObjEndAPI(&secret); + virSecretObjEndAPI(&obj); }

VIR_DIR_CLOSE(dir); diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h index 9638b69..8038faa 100644 --- a/src/conf/virsecretobj.h +++ b/src/conf/virsecretobj.h @@ -30,7 +30,7 @@ typedef struct _virSecretObj virSecretObj; typedef virSecretObj *virSecretObjPtr;

void -virSecretObjEndAPI(virSecretObjPtr *secret); +virSecretObjEndAPI(virSecretObjPtr *obj);

typedef struct _virSecretObjList virSecretObjList; typedef virSecretObjList *virSecretObjListPtr; @@ -49,11 +49,11 @@ virSecretObjListFindByUsage(virSecretObjListPtr secrets,

void virSecretObjListRemove(virSecretObjListPtr secrets, - virSecretObjPtr secret); + virSecretObjPtr obj);

virSecretObjPtr virSecretObjListAdd(virSecretObjListPtr secrets, - virSecretDefPtr def, + virSecretDefPtr newdef, const char *configDir, virSecretDefPtr *oldDef);

@@ -81,37 +81,37 @@ virSecretObjListGetUUIDs(virSecretObjListPtr secrets, virConnectPtr conn);

int -virSecretObjDeleteConfig(virSecretObjPtr secret); +virSecretObjDeleteConfig(virSecretObjPtr obj);

void -virSecretObjDeleteData(virSecretObjPtr secret); +virSecretObjDeleteData(virSecretObjPtr obj);

int -virSecretObjSaveConfig(virSecretObjPtr secret); +virSecretObjSaveConfig(virSecretObjPtr obj);

int -virSecretObjSaveData(virSecretObjPtr secret); +virSecretObjSaveData(virSecretObjPtr obj);

virSecretDefPtr -virSecretObjGetDef(virSecretObjPtr secret); +virSecretObjGetDef(virSecretObjPtr obj);

void -virSecretObjSetDef(virSecretObjPtr secret, +virSecretObjSetDef(virSecretObjPtr obj, virSecretDefPtr def);

unsigned char * -virSecretObjGetValue(virSecretObjPtr secret); +virSecretObjGetValue(virSecretObjPtr obj);

int -virSecretObjSetValue(virSecretObjPtr secret, +virSecretObjSetValue(virSecretObjPtr obj, const unsigned char *value, size_t value_size);

size_t -virSecretObjGetValueSize(virSecretObjPtr secret); +virSecretObjGetValueSize(virSecretObjPtr obj);

void -virSecretObjSetValueSize(virSecretObjPtr secret, +virSecretObjSetValueSize(virSecretObjPtr obj, size_t value_size);

int diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 2a371b6..cc050ff 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -72,6 +72,7 @@ secretDriverLock(void) virMutexLock(&driver->lock); }

+ static void secretDriverUnlock(void) { @@ -79,7 +80,6 @@ secretDriverUnlock(void) }

- static virSecretObjPtr secretObjFromSecret(virSecretPtr secret) { @@ -120,6 +120,7 @@ secretConnectNumOfSecrets(virConnectPtr conn) conn); }

+ static int secretConnectListSecrets(virConnectPtr conn, char **uuids, @@ -156,10 +157,10 @@ secretLookupByUUID(virConnectPtr conn, const unsigned char *uuid) { virSecretPtr ret = NULL; - virSecretObjPtr secret; + virSecretObjPtr obj; virSecretDefPtr def;

- if (!(secret = virSecretObjListFindByUUID(driver->secrets, uuid))) { + if (!(obj = virSecretObjListFindByUUID(driver->secrets, uuid))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(uuid, uuidstr); virReportError(VIR_ERR_NO_SECRET, @@ -167,7 +168,7 @@ secretLookupByUUID(virConnectPtr conn, goto cleanup; }

- def = virSecretObjGetDef(secret); + def = virSecretObjGetDef(obj); if (virSecretLookupByUUIDEnsureACL(conn, def) < 0) goto cleanup;

@@ -177,7 +178,7 @@ secretLookupByUUID(virConnectPtr conn, def->usage_id);

cleanup: - virSecretObjEndAPI(&secret); + virSecretObjEndAPI(&obj); return ret; }

@@ -188,17 +189,17 @@ secretLookupByUsage(virConnectPtr conn, const char *usageID) { virSecretPtr ret = NULL; - virSecretObjPtr secret; + virSecretObjPtr obj; virSecretDefPtr def;

- if (!(secret = virSecretObjListFindByUsage(driver->secrets, - usageType, usageID))) { + if (!(obj = virSecretObjListFindByUsage(driver->secrets, + usageType, usageID))) { virReportError(VIR_ERR_NO_SECRET, _("no secret with matching usage '%s'"), usageID); goto cleanup; }

- def = virSecretObjGetDef(secret); + def = virSecretObjGetDef(obj); if (virSecretLookupByUsageEnsureACL(conn, def) < 0) goto cleanup;

@@ -208,7 +209,7 @@ secretLookupByUsage(virConnectPtr conn, def->usage_id);

cleanup: - virSecretObjEndAPI(&secret); + virSecretObjEndAPI(&obj); return ret; }

@@ -219,129 +220,131 @@ secretDefineXML(virConnectPtr conn, unsigned int flags) { virSecretPtr ret = NULL; - virSecretObjPtr secret = NULL; + virSecretObjPtr obj = NULL; virSecretDefPtr backup = NULL; - virSecretDefPtr new_attrs; + virSecretDefPtr def; virObjectEventPtr event = NULL;

virCheckFlags(0, NULL);

- if (!(new_attrs = virSecretDefParseString(xml))) + if (!(def = virSecretDefParseString(xml))) return NULL;

- if (virSecretDefineXMLEnsureACL(conn, new_attrs) < 0) + if (virSecretDefineXMLEnsureACL(conn, def) < 0) goto cleanup;

- if (!(secret = virSecretObjListAdd(driver->secrets, new_attrs, - driver->configDir, &backup))) + if (!(obj = virSecretObjListAdd(driver->secrets, def, + driver->configDir, &backup))) goto cleanup;

- if (!new_attrs->isephemeral) { + if (!def->isephemeral) { if (secretEnsureDirectory() < 0) goto cleanup;

if (backup && backup->isephemeral) { - if (virSecretObjSaveData(secret) < 0) + if (virSecretObjSaveData(obj) < 0) goto restore_backup; }

- if (virSecretObjSaveConfig(secret) < 0) { + if (virSecretObjSaveConfig(obj) < 0) { if (backup && backup->isephemeral) { /* Undo the virSecretObjSaveData() above; ignore errors */ - virSecretObjDeleteData(secret); + virSecretObjDeleteData(obj); } goto restore_backup; } } else if (backup && !backup->isephemeral) { - if (virSecretObjDeleteConfig(secret) < 0) + if (virSecretObjDeleteConfig(obj) < 0) goto restore_backup;

- virSecretObjDeleteData(secret); + virSecretObjDeleteData(obj); } /* Saved successfully - drop old values */ virSecretDefFree(backup);

- event = virSecretEventLifecycleNew(new_attrs->uuid, - new_attrs->usage_type, - new_attrs->usage_id, + event = virSecretEventLifecycleNew(def->uuid, + def->usage_type, + def->usage_id, VIR_SECRET_EVENT_DEFINED, 0);

ret = virGetSecret(conn, - new_attrs->uuid, - new_attrs->usage_type, - new_attrs->usage_id); - new_attrs = NULL; + def->uuid, + def->usage_type, + def->usage_id); + def = NULL; goto cleanup;

restore_backup: /* If we have a backup, then secret was defined before, so just restore - * the backup. The current (new_attrs) will be handled below. + * the backup. The current def will be handled below. * Otherwise, this is a new secret, thus remove it. */ if (backup) - virSecretObjSetDef(secret, backup); + virSecretObjSetDef(obj, backup); else - virSecretObjListRemove(driver->secrets, secret); + virSecretObjListRemove(driver->secrets, obj);

cleanup: - virSecretDefFree(new_attrs); - virSecretObjEndAPI(&secret); + virSecretDefFree(def); + virSecretObjEndAPI(&obj); if (event) virObjectEventStateQueue(driver->secretEventState, event);

return ret; }

+ static char * -secretGetXMLDesc(virSecretPtr obj, +secretGetXMLDesc(virSecretPtr secret, unsigned int flags) { char *ret = NULL; - virSecretObjPtr secret; + virSecretObjPtr obj; virSecretDefPtr def;

virCheckFlags(0, NULL);

- if (!(secret = secretObjFromSecret(obj))) + if (!(obj = secretObjFromSecret(secret))) goto cleanup;

- def = virSecretObjGetDef(secret); - if (virSecretGetXMLDescEnsureACL(obj->conn, def) < 0) + def = virSecretObjGetDef(obj); + if (virSecretGetXMLDescEnsureACL(secret->conn, def) < 0) goto cleanup;

ret = virSecretDefFormat(def);

cleanup: - virSecretObjEndAPI(&secret); + virSecretObjEndAPI(&obj);

return ret; }

+ static int -secretSetValue(virSecretPtr obj, +secretSetValue(virSecretPtr secret, const unsigned char *value, size_t value_size, unsigned int flags) { int ret = -1; - virSecretObjPtr secret; + virSecretObjPtr obj; virSecretDefPtr def; virObjectEventPtr event = NULL;

virCheckFlags(0, -1);

- if (!(secret = secretObjFromSecret(obj))) + if (!(obj = secretObjFromSecret(secret))) goto cleanup;

- def = virSecretObjGetDef(secret); - if (virSecretSetValueEnsureACL(obj->conn, def) < 0) + def = virSecretObjGetDef(obj); + if (virSecretSetValueEnsureACL(secret->conn, def) < 0) goto cleanup;

if (secretEnsureDirectory() < 0) goto cleanup;

- if (virSecretObjSetValue(secret, value, value_size) < 0) + if (virSecretObjSetValue(obj, value, value_size) < 0) goto cleanup;

event = virSecretEventValueChangedNew(def->uuid, @@ -350,30 +353,31 @@ secretSetValue(virSecretPtr obj, ret = 0;

cleanup: - virSecretObjEndAPI(&secret); + virSecretObjEndAPI(&obj); if (event) virObjectEventStateQueue(driver->secretEventState, event);

return ret; }

+ static unsigned char * -secretGetValue(virSecretPtr obj, +secretGetValue(virSecretPtr secret, size_t *value_size, unsigned int flags, unsigned int internalFlags) { unsigned char *ret = NULL; - virSecretObjPtr secret; + virSecretObjPtr obj; virSecretDefPtr def;

virCheckFlags(0, NULL);

- if (!(secret = secretObjFromSecret(obj))) + if (!(obj = secretObjFromSecret(secret))) goto cleanup;

- def = virSecretObjGetDef(secret); - if (virSecretGetValueEnsureACL(obj->conn, def) < 0) + def = virSecretObjGetDef(obj); + if (virSecretGetValueEnsureACL(secret->conn, def) < 0) goto cleanup;

if ((internalFlags & VIR_SECRET_GET_VALUE_INTERNAL_CALL) == 0 && @@ -383,33 +387,34 @@ secretGetValue(virSecretPtr obj, goto cleanup; }

- if (!(ret = virSecretObjGetValue(secret))) + if (!(ret = virSecretObjGetValue(obj))) goto cleanup;

- *value_size = virSecretObjGetValueSize(secret); + *value_size = virSecretObjGetValueSize(obj);

cleanup: - virSecretObjEndAPI(&secret); + virSecretObjEndAPI(&obj);

return ret; }

+ static int -secretUndefine(virSecretPtr obj) +secretUndefine(virSecretPtr secret) { int ret = -1; - virSecretObjPtr secret; + virSecretObjPtr obj; virSecretDefPtr def; virObjectEventPtr event = NULL;

- if (!(secret = secretObjFromSecret(obj))) + if (!(obj = secretObjFromSecret(secret))) goto cleanup;

- def = virSecretObjGetDef(secret); - if (virSecretUndefineEnsureACL(obj->conn, def) < 0) + def = virSecretObjGetDef(obj); + if (virSecretUndefineEnsureACL(secret->conn, def) < 0) goto cleanup;

- if (virSecretObjDeleteConfig(secret) < 0) + if (virSecretObjDeleteConfig(obj) < 0) goto cleanup;

event = virSecretEventLifecycleNew(def->uuid, @@ -418,20 +423,21 @@ secretUndefine(virSecretPtr obj) VIR_SECRET_EVENT_UNDEFINED, 0);

- virSecretObjDeleteData(secret); + virSecretObjDeleteData(obj);

- virSecretObjListRemove(driver->secrets, secret); + virSecretObjListRemove(driver->secrets, obj);

ret = 0;

cleanup: - virSecretObjEndAPI(&secret); + virSecretObjEndAPI(&obj); if (event) virObjectEventStateQueue(driver->secretEventState, event);

return ret; }

+ static int secretStateCleanup(void) { @@ -452,6 +458,7 @@ secretStateCleanup(void) return 0; }

+ static int secretStateInitialize(bool privileged, virStateInhibitCallback callback ATTRIBUTE_UNUSED, @@ -497,6 +504,7 @@ secretStateInitialize(bool privileged, return -1; }

+ static int secretStateReload(void) { @@ -511,6 +519,7 @@ secretStateReload(void) return 0; }

+ static int secretConnectSecretEventRegisterAny(virConnectPtr conn, virSecretPtr secret, @@ -532,6 +541,7 @@ secretConnectSecretEventRegisterAny(virConnectPtr conn, return callbackID; }

+ static int secretConnectSecretEventDeregisterAny(virConnectPtr conn, int callbackID) @@ -576,6 +586,7 @@ static virStateDriver stateDriver = { .stateReload = secretStateReload, };

+ int secretRegister(void) { -- 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 Mon, Apr 24, 2017 at 02:00:15PM -0400, John Ferlan wrote:

Rather than dereferencing obj->def->X, create a local 'def' variable variable that will dereference the def and use directly.

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 69 +++++++++++++++++++++++++++++++------------------ 1 file changed, 44 insertions(+), 25 deletions(-)

diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 42f36c8..413955d 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -139,8 +139,9 @@ static void virSecretObjDispose(void *opaque) { virSecretObjPtr obj = opaque; + virSecretDefPtr def = obj->def;

- virSecretDefFree(obj->def); + virSecretDefFree(def);

Here it adds only a noise into the code, no actual improvement.

if (obj->value) { /* Wipe before free to ensure we don't leave a secret on the heap */ memset(obj->value, 0, obj->value_size); @@ -203,16 +204,18 @@ virSecretObjSearchName(const void *payload, const void *opaque) { virSecretObjPtr obj = (virSecretObjPtr) payload; + virSecretDefPtr def; struct virSecretSearchData *data = (struct virSecretSearchData *) opaque; int found = 0;

virObjectLock(obj); + def = obj->def;

- if (obj->def->usage_type != data->usageType) + if (def->usage_type != data->usageType) goto cleanup;

if (data->usageType != VIR_SECRET_USAGE_TYPE_NONE && - STREQ(obj->def->usage_id, data->usageID)) + STREQ(def->usage_id, data->usageID)) found = 1;

cleanup: @@ -278,11 +281,13 @@ virSecretObjListRemove(virSecretObjListPtr secrets, virSecretObjPtr obj) { char uuidstr[VIR_UUID_STRING_BUFLEN]; + virSecretDefPtr def;

if (!obj) return; + def = obj->def;

- virUUIDFormat(obj->def->uuid, uuidstr); + virUUIDFormat(def->uuid, uuidstr);

Same here,

virObjectRef(obj); virObjectUnlock(obj);

@@ -315,6 +320,7 @@ virSecretObjListAddLocked(virSecretObjListPtr secrets, virSecretDefPtr *oldDef) { virSecretObjPtr obj; + virSecretDefPtr def; virSecretObjPtr ret = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; char *configFile = NULL, *base64File = NULL; @@ -325,26 +331,27 @@ virSecretObjListAddLocked(virSecretObjListPtr secrets, /* Is there a secret already matching this UUID */ if ((obj = virSecretObjListFindByUUIDLocked(secrets, newdef->uuid))) { virObjectLock(obj); + def = obj->def;

- if (STRNEQ_NULLABLE(obj->def->usage_id, newdef->usage_id)) { - virUUIDFormat(obj->def->uuid, uuidstr); + if (STRNEQ_NULLABLE(def->usage_id, newdef->usage_id)) { + virUUIDFormat(def->uuid, uuidstr); virReportError(VIR_ERR_INTERNAL_ERROR, _("a secret with UUID %s is already defined for " "use with %s"), - uuidstr, obj->def->usage_id); + uuidstr, def->usage_id); goto cleanup; }

- if (obj->def->isprivate && !newdef->isprivate) { + if (def->isprivate && !newdef->isprivate) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot change private flag on existing secret")); goto cleanup; }

if (oldDef) - *oldDef = obj->def; + *oldDef = def; else - virSecretDefFree(obj->def); + virSecretDefFree(def); obj->def = newdef; } else { /* No existing secret with same UUID, @@ -353,7 +360,8 @@ virSecretObjListAddLocked(virSecretObjListPtr secrets, newdef->usage_type, newdef->usage_id))) { virObjectLock(obj); - virUUIDFormat(obj->def->uuid, uuidstr); + def = obj->def; + virUUIDFormat(def->uuid, uuidstr); virReportError(VIR_ERR_INTERNAL_ERROR, _("a secret with UUID %s already defined for " "use with %s"), @@ -424,6 +432,7 @@ virSecretObjListGetHelper(void *payload, { struct virSecretObjListGetHelperData *data = opaque; virSecretObjPtr obj = payload; + virSecretDefPtr def;

if (data->error) return 0; @@ -432,8 +441,9 @@ virSecretObjListGetHelper(void *payload, return 0;

virObjectLock(obj); + def = obj->def;

- if (data->filter && !data->filter(data->conn, obj->def)) + if (data->filter && !data->filter(data->conn, def)) goto cleanup;

if (data->uuids) { @@ -444,7 +454,7 @@ virSecretObjListGetHelper(void *payload, goto cleanup; }

- virUUIDFormat(obj->def->uuid, uuidstr); + virUUIDFormat(def->uuid, uuidstr); data->uuids[data->got] = uuidstr; }

@@ -478,20 +488,22 @@ static bool virSecretObjMatchFlags(virSecretObjPtr obj, unsigned int flags) { + virSecretDefPtr def = obj->def; + /* filter by whether it's ephemeral */ if (MATCH(VIR_CONNECT_LIST_SECRETS_FILTERS_EPHEMERAL) && !((MATCH(VIR_CONNECT_LIST_SECRETS_EPHEMERAL) && - obj->def->isephemeral) || + def->isephemeral) || (MATCH(VIR_CONNECT_LIST_SECRETS_NO_EPHEMERAL) && - !obj->def->isephemeral))) + !def->isephemeral))) return false;

/* filter by whether it's private */ if (MATCH(VIR_CONNECT_LIST_SECRETS_FILTERS_PRIVATE) && !((MATCH(VIR_CONNECT_LIST_SECRETS_PRIVATE) && - obj->def->isprivate) || + def->isprivate) || (MATCH(VIR_CONNECT_LIST_SECRETS_NO_PRIVATE) && - !obj->def->isprivate))) + !def->isprivate))) return false;

return true; @@ -515,14 +527,16 @@ virSecretObjListPopulate(void *payload, { struct virSecretObjListData *data = opaque; virSecretObjPtr obj = payload; + virSecretDefPtr def; virSecretPtr secret = NULL;

if (data->error) return 0;

virObjectLock(obj); + def = obj->def;

- if (data->filter && !data->filter(data->conn, obj->def)) + if (data->filter && !data->filter(data->conn, def)) goto cleanup;

if (!virSecretObjMatchFlags(obj, data->flags)) @@ -533,9 +547,9 @@ virSecretObjListPopulate(void *payload, goto cleanup; }

- if (!(secret = virGetSecret(data->conn, obj->def->uuid, - obj->def->usage_type, - obj->def->usage_id))) { + if (!(secret = virGetSecret(data->conn, def->uuid, + def->usage_type, + def->usage_id))) { data->error = true; goto cleanup; } @@ -624,7 +638,9 @@ virSecretObjListGetUUIDs(virSecretObjListPtr secrets, int virSecretObjDeleteConfig(virSecretObjPtr obj) { - if (!obj->def->isephemeral && + virSecretDefPtr def = obj->def; + + if (!def->isephemeral &&

here,

unlink(obj->configFile) < 0 && errno != ENOENT) { virReportSystemError(errno, _("cannot unlink '%s'"), obj->configFile); @@ -653,10 +669,11 @@ virSecretObjDeleteData(virSecretObjPtr obj) int virSecretObjSaveConfig(virSecretObjPtr obj) { + virSecretDefPtr def = obj->def; char *xml = NULL; int ret = -1;

- if (!(xml = virSecretDefFormat(obj->def))) + if (!(xml = virSecretDefFormat(def))) goto cleanup;

here,

if (virFileRewriteStr(obj->configFile, S_IRUSR | S_IWUSR, xml) < 0) @@ -711,11 +728,12 @@ virSecretObjSetDef(virSecretObjPtr obj, unsigned char * virSecretObjGetValue(virSecretObjPtr obj) { + virSecretDefPtr def = obj->def; unsigned char *ret = NULL;

if (!obj->value) { char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(obj->def->uuid, uuidstr); + virUUIDFormat(def->uuid, uuidstr);

here,

virReportError(VIR_ERR_NO_SECRET, _("secret '%s' does not have a value"), uuidstr); goto cleanup; @@ -735,6 +753,7 @@ virSecretObjSetValue(virSecretObjPtr obj, const unsigned char *value, size_t value_size) { + virSecretDefPtr def = obj->def; unsigned char *old_value, *new_value; size_t old_value_size;

@@ -748,7 +767,7 @@ virSecretObjSetValue(virSecretObjPtr obj, obj->value = new_value; obj->value_size = value_size;

- if (!obj->def->isephemeral && virSecretObjSaveData(obj) < 0) + if (!def->isephemeral && virSecretObjSaveData(obj) < 0) goto error;

and here. Pavel

On Tue, Apr 25, 2017 at 07:58:58AM -0400, John Ferlan wrote:

On 04/25/2017 04:36 AM, Pavel Hrdina wrote:

...

On Mon, Apr 24, 2017 at 02:00:15PM -0400, John Ferlan wrote:

...

Rather than dereferencing obj->def->X, create a local 'def' variable variable that will dereference the def and use directly.

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 69 +++++++++++++++++++++++++++++++------------------ 1 file changed, 44 insertions(+), 25 deletions(-)

diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 42f36c8..413955d 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -139,8 +139,9 @@ static void virSecretObjDispose(void *opaque) { virSecretObjPtr obj = opaque; + virSecretDefPtr def = obj->def;

- virSecretDefFree(obj->def); + virSecretDefFree(def);

Here it adds only a noise into the code, no actual improvement.

Well.... not entirely. Later on in patches that aren't posted yet the "obj->def" is going to be replaced by virObjectPoolableDefGetDef() and while yes, one could make that call inside the virSecretDefFree, it's also just as simple to make it outside that call. It's a rote exercise. Besides I find it "ugly" to read functionA(functionB()). Even worse if functionB now could return NULL or some value one then has to split apart the code anyway.

Like I replied to your replay, these change itself doesn't improve anything and there is no benefit from these changes further in the series. If you have some patches prepared in your branch that will benefit these changes it should be part of that series, not this one.

...

...

if (obj->value) { /* Wipe before free to ensure we don't leave a secret on the heap */ memset(obj->value, 0, obj->value_size); @@ -203,16 +204,18 @@ virSecretObjSearchName(const void *payload, const void *opaque) { virSecretObjPtr obj = (virSecretObjPtr) payload; + virSecretDefPtr def; struct virSecretSearchData *data = (struct virSecretSearchData *) opaque; int found = 0;

virObjectLock(obj); + def = obj->def;

- if (obj->def->usage_type != data->usageType) + if (def->usage_type != data->usageType) goto cleanup;

if (data->usageType != VIR_SECRET_USAGE_TYPE_NONE && - STREQ(obj->def->usage_id, data->usageID)) + STREQ(def->usage_id, data->usageID)) found = 1;

cleanup: @@ -278,11 +281,13 @@ virSecretObjListRemove(virSecretObjListPtr secrets, virSecretObjPtr obj) { char uuidstr[VIR_UUID_STRING_BUFLEN]; + virSecretDefPtr def;

if (!obj) return; + def = obj->def;

- virUUIDFormat(obj->def->uuid, uuidstr); + virUUIDFormat(def->uuid, uuidstr);

Same here,

No this is "obj->def->uuid" into "def->uuid"... Once "->def" becomes part of an "obj" it won't be visible and that would require more change later on to essentially perform the same action.

Once, but it's not a part of this series so without that patch it doesn't add any benefit to the current code. This probably applies to the remaining places. And getting def->uuid instead of obj->def->uuid doesn't seem like an improvement, it's used only once in this function. If there were more usages of def it would make sense to do this change. Pavel

...

...

virObjectRef(obj); virObjectUnlock(obj);

@@ -315,6 +320,7 @@ virSecretObjListAddLocked(virSecretObjListPtr secrets, virSecretDefPtr *oldDef) { virSecretObjPtr obj; + virSecretDefPtr def; virSecretObjPtr ret = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; char *configFile = NULL, *base64File = NULL; @@ -325,26 +331,27 @@ virSecretObjListAddLocked(virSecretObjListPtr secrets, /* Is there a secret already matching this UUID */ if ((obj = virSecretObjListFindByUUIDLocked(secrets, newdef->uuid))) { virObjectLock(obj); + def = obj->def;

- if (STRNEQ_NULLABLE(obj->def->usage_id, newdef->usage_id)) { - virUUIDFormat(obj->def->uuid, uuidstr); + if (STRNEQ_NULLABLE(def->usage_id, newdef->usage_id)) { + virUUIDFormat(def->uuid, uuidstr); virReportError(VIR_ERR_INTERNAL_ERROR, _("a secret with UUID %s is already defined for " "use with %s"), - uuidstr, obj->def->usage_id); + uuidstr, def->usage_id); goto cleanup; }

- if (obj->def->isprivate && !newdef->isprivate) { + if (def->isprivate && !newdef->isprivate) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot change private flag on existing secret")); goto cleanup; }

if (oldDef) - *oldDef = obj->def; + *oldDef = def; else - virSecretDefFree(obj->def); + virSecretDefFree(def); obj->def = newdef; } else { /* No existing secret with same UUID, @@ -353,7 +360,8 @@ virSecretObjListAddLocked(virSecretObjListPtr secrets, newdef->usage_type, newdef->usage_id))) { virObjectLock(obj); - virUUIDFormat(obj->def->uuid, uuidstr); + def = obj->def; + virUUIDFormat(def->uuid, uuidstr); virReportError(VIR_ERR_INTERNAL_ERROR, _("a secret with UUID %s already defined for " "use with %s"), @@ -424,6 +432,7 @@ virSecretObjListGetHelper(void *payload, { struct virSecretObjListGetHelperData *data = opaque; virSecretObjPtr obj = payload; + virSecretDefPtr def;

if (data->error) return 0; @@ -432,8 +441,9 @@ virSecretObjListGetHelper(void *payload, return 0;

virObjectLock(obj); + def = obj->def;

- if (data->filter && !data->filter(data->conn, obj->def)) + if (data->filter && !data->filter(data->conn, def)) goto cleanup;

if (data->uuids) { @@ -444,7 +454,7 @@ virSecretObjListGetHelper(void *payload, goto cleanup; }

- virUUIDFormat(obj->def->uuid, uuidstr); + virUUIDFormat(def->uuid, uuidstr); data->uuids[data->got] = uuidstr; }

@@ -478,20 +488,22 @@ static bool virSecretObjMatchFlags(virSecretObjPtr obj, unsigned int flags) { + virSecretDefPtr def = obj->def; + /* filter by whether it's ephemeral */ if (MATCH(VIR_CONNECT_LIST_SECRETS_FILTERS_EPHEMERAL) && !((MATCH(VIR_CONNECT_LIST_SECRETS_EPHEMERAL) && - obj->def->isephemeral) || + def->isephemeral) || (MATCH(VIR_CONNECT_LIST_SECRETS_NO_EPHEMERAL) && - !obj->def->isephemeral))) + !def->isephemeral))) return false;

/* filter by whether it's private */ if (MATCH(VIR_CONNECT_LIST_SECRETS_FILTERS_PRIVATE) && !((MATCH(VIR_CONNECT_LIST_SECRETS_PRIVATE) && - obj->def->isprivate) || + def->isprivate) || (MATCH(VIR_CONNECT_LIST_SECRETS_NO_PRIVATE) && - !obj->def->isprivate))) + !def->isprivate))) return false;

return true; @@ -515,14 +527,16 @@ virSecretObjListPopulate(void *payload, { struct virSecretObjListData *data = opaque; virSecretObjPtr obj = payload; + virSecretDefPtr def; virSecretPtr secret = NULL;

if (data->error) return 0;

virObjectLock(obj); + def = obj->def;

- if (data->filter && !data->filter(data->conn, obj->def)) + if (data->filter && !data->filter(data->conn, def)) goto cleanup;

if (!virSecretObjMatchFlags(obj, data->flags)) @@ -533,9 +547,9 @@ virSecretObjListPopulate(void *payload, goto cleanup; }

- if (!(secret = virGetSecret(data->conn, obj->def->uuid, - obj->def->usage_type, - obj->def->usage_id))) { + if (!(secret = virGetSecret(data->conn, def->uuid, + def->usage_type, + def->usage_id))) { data->error = true; goto cleanup; } @@ -624,7 +638,9 @@ virSecretObjListGetUUIDs(virSecretObjListPtr secrets, int virSecretObjDeleteConfig(virSecretObjPtr obj) { - if (!obj->def->isephemeral && + virSecretDefPtr def = obj->def; + + if (!def->isephemeral &&

here,

Some concept - obj->def->isphemeral becomes just def->isephemeral

...

...

unlink(obj->configFile) < 0 && errno != ENOENT) { virReportSystemError(errno, _("cannot unlink '%s'"), obj->configFile); @@ -653,10 +669,11 @@ virSecretObjDeleteData(virSecretObjPtr obj) int virSecretObjSaveConfig(virSecretObjPtr obj) { + virSecretDefPtr def = obj->def; char *xml = NULL; int ret = -1;

- if (!(xml = virSecretDefFormat(obj->def))) + if (!(xml = virSecretDefFormat(def))) goto cleanup;

here,

Sure it could eventually be a call instead, similar comment to above though w/r/t function call within a function call for readability

...

...

if (virFileRewriteStr(obj->configFile, S_IRUSR | S_IWUSR, xml) < 0) @@ -711,11 +728,12 @@ virSecretObjSetDef(virSecretObjPtr obj, unsigned char * virSecretObjGetValue(virSecretObjPtr obj) { + virSecretDefPtr def = obj->def; unsigned char *ret = NULL;

if (!obj->value) { char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(obj->def->uuid, uuidstr); + virUUIDFormat(def->uuid, uuidstr);

here,

obj->def->uuid is now just def->uuid

...

...

virReportError(VIR_ERR_NO_SECRET, _("secret '%s' does not have a value"), uuidstr); goto cleanup; @@ -735,6 +753,7 @@ virSecretObjSetValue(virSecretObjPtr obj, const unsigned char *value, size_t value_size) { + virSecretDefPtr def = obj->def; unsigned char *old_value, *new_value; size_t old_value_size;

@@ -748,7 +767,7 @@ virSecretObjSetValue(virSecretObjPtr obj, obj->value = new_value; obj->value_size = value_size;

- if (!obj->def->isephemeral && virSecretObjSaveData(obj) < 0) + if (!def->isephemeral && virSecretObjSaveData(obj) < 0) goto error;

and here.

similar to previous isephemeral.

John

...

Pavel

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 04/25/2017 06:05 AM, Pavel Hrdina wrote:

On Mon, Apr 24, 2017 at 02:00:18PM -0400, John Ferlan wrote:

...

Rather than overloading one function - split apart the logic to have separate interfaces and local/private structures to manage the data for which the helper is collecting.

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 98 +++++++++++++++++++++++++++++++------------------ src/conf/virsecretobj.h | 6 +-- 2 files changed, 65 insertions(+), 39 deletions(-)

diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index c410a6b..3717552 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -415,9 +415,54 @@ virSecretObjListAdd(virSecretObjListPtr secrets, }

-struct virSecretObjListGetHelperData { +struct secretCountData {

This doesn't follow our naming rules, all struct should be prefixed with *vir*.

I guess I went on the character reduction plan. I can adjust.

...

virConnectPtr conn; - virSecretObjListACLFilter filter; + virSecretObjListACLFilter aclfilter; + int count; +}; + +static int +virSecretObjListNumOfSecretsCallback(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + struct secretCountData *data = opaque; + virSecretObjPtr obj = payload; + virSecretDefPtr def; + + virObjectLock(obj); + def = obj->def; + + if (data->aclfilter && !data->aclfilter(data->conn, def)) + goto cleanup;

This follows previous patch, in this case having separate variable for virSecretDefPtr doesn't give us any benefit, just a noise in the code.

Well, yes it does eventually... It's a see the diff now or see more diffs later...

...

+ + data->count++; + + cleanup: + virObjectUnlock(obj); + return 0; +} + + +int +virSecretObjListNumOfSecrets(virSecretObjListPtr secrets, + virSecretObjListACLFilter aclfilter, + virConnectPtr conn) +{ + struct secretCountData data = { + .conn = conn, .aclfilter = aclfilter, .count = 0 }; + + virObjectLock(secrets); + virHashForEach(secrets->objs, virSecretObjListNumOfSecretsCallback, &data); + virObjectUnlock(secrets); + + return data.count; +}

Unnecessary movement of function.

Well if you look logically at the code the setup used to be virSecretObjListGetHelper <== Helper for both NumOf and GetUUIDs virSecretObjListNumOfSecrets <== Main API virSecretObjMatchFlags <== Helper for ListPopulate virSecretObjListPopulate <== Helper for ListExport (obvious, right) virSecretObjListExport <== Main API virSecretObjListGetUUIDs <== Main API The goal was to have "alike" code next to each other in the module and to be "similarly named", thus virSecretObjListNumOfSecretsCallback virSecretObjListNumOfSecrets virSecretObjListGetUUIDsCallback virSecretObjListGetUUIDs virSecretObjMatchFlags virSecretObjListExportCallback virSecretObjListExport I *get* the code motion thing and the git history/blame concerns - still they are *far worse* when moving code from one module to another. With graphical tools like gitk it makes it a lot easier to trace/track the history.

...

+ + +struct secretListData {

This should be virSecretListData.

Sure that can change - although long term it won't matter though as it'll go away to be replaced by a virObject* data structure that can handle NumOf, GetUUIDs, and ListExport generically John

...

+ virConnectPtr conn; + virSecretObjListACLFilter aclfilter; int nuuids; char **uuids; int maxuuids;

Pavel

On 04/25/2017 08:47 AM, Pavel Hrdina wrote:

On Tue, Apr 25, 2017 at 08:03:58AM -0400, John Ferlan wrote:

...

On 04/25/2017 06:05 AM, Pavel Hrdina wrote:

...

On Mon, Apr 24, 2017 at 02:00:18PM -0400, John Ferlan wrote:

...

Rather than overloading one function - split apart the logic to have separate interfaces and local/private structures to manage the data for which the helper is collecting.

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 98 +++++++++++++++++++++++++++++++------------------ src/conf/virsecretobj.h | 6 +-- 2 files changed, 65 insertions(+), 39 deletions(-)

diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index c410a6b..3717552 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -415,9 +415,54 @@ virSecretObjListAdd(virSecretObjListPtr secrets, }

-struct virSecretObjListGetHelperData { +struct secretCountData {

This doesn't follow our naming rules, all struct should be prefixed with *vir*.

I guess I went on the character reduction plan. I can adjust.

...

...

virConnectPtr conn; - virSecretObjListACLFilter filter; + virSecretObjListACLFilter aclfilter; + int count; +}; + +static int +virSecretObjListNumOfSecretsCallback(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + struct secretCountData *data = opaque; + virSecretObjPtr obj = payload; + virSecretDefPtr def; + + virObjectLock(obj); + def = obj->def; + + if (data->aclfilter && !data->aclfilter(data->conn, def)) + goto cleanup;

This follows previous patch, in this case having separate variable for virSecretDefPtr doesn't give us any benefit, just a noise in the code.

Well, yes it does eventually... It's a see the diff now or see more diffs later...

...

...

+ + data->count++; + + cleanup: + virObjectUnlock(obj); + return 0; +} + + +int +virSecretObjListNumOfSecrets(virSecretObjListPtr secrets, + virSecretObjListACLFilter aclfilter, + virConnectPtr conn) +{ + struct secretCountData data = { + .conn = conn, .aclfilter = aclfilter, .count = 0 }; + + virObjectLock(secrets); + virHashForEach(secrets->objs, virSecretObjListNumOfSecretsCallback, &data); + virObjectUnlock(secrets); + + return data.count; +}

Unnecessary movement of function.

Well if you look logically at the code the setup used to be

virSecretObjListGetHelper <== Helper for both NumOf and GetUUIDs virSecretObjListNumOfSecrets <== Main API virSecretObjMatchFlags <== Helper for ListPopulate virSecretObjListPopulate <== Helper for ListExport (obvious, right) virSecretObjListExport <== Main API virSecretObjListGetUUIDs <== Main API

The goal was to have "alike" code next to each other in the module and to be "similarly named", thus

virSecretObjListNumOfSecretsCallback virSecretObjListNumOfSecrets virSecretObjListGetUUIDsCallback virSecretObjListGetUUIDs virSecretObjMatchFlags virSecretObjListExportCallback virSecretObjListExport

I *get* the code motion thing and the git history/blame concerns - still they are *far worse* when moving code from one module to another. With graphical tools like gitk it makes it a lot easier to trace/track the history.

Yes, I understand the goal to have the related function next to each other, but we usually don't move the functions just to keep them together. It makes the code look&feel nicer but it's not something that give as any real value.

The "real value" is that I don't have to go searching for the companion code. The "static" helper is right next to the external API. Why have it be essentially 120 lines later (e.g. patch 7 where I did move it). It doesn't make sense visually to have: virSecretObjListNumOfSecretsCallback virSecretObjListGetUUIDsCallback virSecretObjListNumOfSecrets virSecretObjListGetUUIDs Ironically it's less that virSecretObjListNumOfSecrets moved and more that virSecretObjListGetHelper was split into two functions. The difference code caused it to look more like virSecretObjListGetUUIDsCallback replaces virSecretObjListGetHelper John

Pavel

...

...

...

+ + +struct secretListData {

This should be virSecretListData.

Sure that can change - although long term it won't matter though as it'll go away to be replaced by a virObject* data structure that can handle NumOf, GetUUIDs, and ListExport generically

John

...

...

+ virConnectPtr conn; + virSecretObjListACLFilter aclfilter; int nuuids; char **uuids; int maxuuids;

Pavel

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Apr 24, 2017 at 02:00:20PM -0400, John Ferlan wrote:

Since we're storing a virUUIDFormat'd string in our Hash Table, let's modify the Lookup API to receive a formatted string as well.

It's true that we use formatted UUID as a key for storing the secret in hash table we don't necessary need to change Lookup APIs to require formatted string as well. All other objects have similar APIs to Lookup an object by UUID and they don't require formatted UUID. I like the fact, that the formatting of UUID is hidden for the caller since we store RAW UUIDs in our object structures. I'm not sure about this change, I'm more inclined to NACK this patch. Pavel

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 18 +++++++----------- src/conf/virsecretobj.h | 2 +- src/secret/secret_driver.c | 10 +++++----- 3 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 5acda4c..ae2b287 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -163,12 +163,8 @@ virSecretObjListDispose(void *obj)

static virSecretObjPtr virSecretObjListFindByUUIDLocked(virSecretObjListPtr secrets, - const unsigned char *uuid) + const char *uuidstr) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(uuid, uuidstr); - return virObjectRef(virHashLookup(secrets->objs, uuidstr)); }

@@ -176,7 +172,7 @@ virSecretObjListFindByUUIDLocked(virSecretObjListPtr secrets, /** * virSecretObjFindByUUID: * @secrets: list of secret objects - * @uuid: secret uuid to find + * @uuidstr: secret uuid to find * * This function locks @secrets and finds the secret object which * corresponds to @uuid. @@ -185,12 +181,12 @@ virSecretObjListFindByUUIDLocked(virSecretObjListPtr secrets, */ virSecretObjPtr virSecretObjListFindByUUID(virSecretObjListPtr secrets, - const unsigned char *uuid) + const char *uuidstr) { virSecretObjPtr obj;

virObjectLock(secrets); - obj = virSecretObjListFindByUUIDLocked(secrets, uuid); + obj = virSecretObjListFindByUUIDLocked(secrets, uuidstr); virObjectUnlock(secrets); if (obj) virObjectLock(obj); @@ -328,13 +324,14 @@ virSecretObjListAdd(virSecretObjListPtr secrets, if (oldDef) *oldDef = NULL;

+ virUUIDFormat(newdef->uuid, uuidstr); + /* Is there a secret already matching this UUID */ - if ((obj = virSecretObjListFindByUUIDLocked(secrets, newdef->uuid))) { + if ((obj = virSecretObjListFindByUUIDLocked(secrets, uuidstr))) { virObjectLock(obj); def = obj->def;

if (STRNEQ_NULLABLE(def->usage_id, newdef->usage_id)) { - virUUIDFormat(def->uuid, uuidstr); virReportError(VIR_ERR_INTERNAL_ERROR, _("a secret with UUID %s is already defined for " "use with %s"), @@ -372,7 +369,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets, /* Generate the possible configFile and base64File strings * using the configDir, uuidstr, and appropriate suffix */ - virUUIDFormat(newdef->uuid, uuidstr); if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) || !(base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) goto cleanup; diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h index bd38f52..092f23c 100644 --- a/src/conf/virsecretobj.h +++ b/src/conf/virsecretobj.h @@ -40,7 +40,7 @@ virSecretObjListNew(void);

virSecretObjPtr virSecretObjListFindByUUID(virSecretObjListPtr secrets, - const unsigned char *uuid); + const char *uuidstr);

virSecretObjPtr virSecretObjListFindByUsage(virSecretObjListPtr secrets, diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index cc050ff..2d4091d 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -86,8 +86,8 @@ secretObjFromSecret(virSecretPtr secret) virSecretObjPtr obj; char uuidstr[VIR_UUID_STRING_BUFLEN];

- if (!(obj = virSecretObjListFindByUUID(driver->secrets, secret->uuid))) { - virUUIDFormat(secret->uuid, uuidstr); + virUUIDFormat(secret->uuid, uuidstr); + if (!(obj = virSecretObjListFindByUUID(driver->secrets, uuidstr))) { virReportError(VIR_ERR_NO_SECRET, _("no secret with matching uuid '%s'"), uuidstr); return NULL; @@ -159,10 +159,10 @@ secretLookupByUUID(virConnectPtr conn, virSecretPtr ret = NULL; virSecretObjPtr obj; virSecretDefPtr def; + char uuidstr[VIR_UUID_STRING_BUFLEN];

- if (!(obj = virSecretObjListFindByUUID(driver->secrets, uuid))) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(uuid, uuidstr); + virUUIDFormat(uuid, uuidstr); + if (!(obj = virSecretObjListFindByUUID(driver->secrets, uuidstr))) { virReportError(VIR_ERR_NO_SECRET, _("no secret with matching uuid '%s'"), uuidstr); goto cleanup; -- 2.9.3

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, Apr 25, 2017 at 08:05:13AM -0400, John Ferlan wrote:

On 04/25/2017 07:22 AM, Pavel Hrdina wrote:

...

On Mon, Apr 24, 2017 at 02:00:20PM -0400, John Ferlan wrote:

...

Since we're storing a virUUIDFormat'd string in our Hash Table, let's modify the Lookup API to receive a formatted string as well.

It's true that we use formatted UUID as a key for storing the secret in hash table we don't necessary need to change Lookup APIs to require formatted string as well. All other objects have similar APIs to Lookup an object by UUID and they don't require formatted UUID. I like the fact, that the formatting of UUID is hidden for the caller since we store RAW UUIDs in our object structures.

I'm not sure about this change, I'm more inclined to NACK this patch.

Go back to my comment in my response to patch 3 regarding a single function to perform the Find...

Take the abstraction back a level for every/any driver that uses UUID there is similar code to essentially format the UUID for "add" and "lookup" (or find). So lots of repetitive code, right? The UUID is always stored in the hash table using the formatted value - hence the movement to utilize that formatted UUID.

For some drivers (OK domain only because you don't have the context I have) - there's a second hash table for lookup (e.g. Name). That second lookup is done by "char *" as would the formatted UUID be done.

If you then further consider that the only difference between the hash tables is formatted UUID or name, then you can consolidate more code to perform that lookup by a "key" that is either primary or secondary.

That's one of few things why I don't like the "primary" and "secondary" hash table idea. It kind of forces you to use "string" as a key for hash table even though we can use whatever we want and implement appropriate "key*" callbacks and use virHashCreateFull instead of virHashCreate for storing objects by UUID, with that change we can avoid formatting the UUID at all for hash table. Formatting UUID should be done only for generating an XML or printing a log message. Our internal code shouldn't format it for our internal logic at all.

There is also similar code to format the UUID for any message printed. Sometimes that format is done more than once in a single function.

Formatting the UUID only once for the whole function is actual cleanup. I'm not against that change. Pavel

John

...

Pavel

...

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 18 +++++++----------- src/conf/virsecretobj.h | 2 +- src/secret/secret_driver.c | 10 +++++----- 3 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 5acda4c..ae2b287 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -163,12 +163,8 @@ virSecretObjListDispose(void *obj)

static virSecretObjPtr virSecretObjListFindByUUIDLocked(virSecretObjListPtr secrets, - const unsigned char *uuid) + const char *uuidstr) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(uuid, uuidstr); - return virObjectRef(virHashLookup(secrets->objs, uuidstr)); }

@@ -176,7 +172,7 @@ virSecretObjListFindByUUIDLocked(virSecretObjListPtr secrets, /** * virSecretObjFindByUUID: * @secrets: list of secret objects - * @uuid: secret uuid to find + * @uuidstr: secret uuid to find * * This function locks @secrets and finds the secret object which * corresponds to @uuid. @@ -185,12 +181,12 @@ virSecretObjListFindByUUIDLocked(virSecretObjListPtr secrets, */ virSecretObjPtr virSecretObjListFindByUUID(virSecretObjListPtr secrets, - const unsigned char *uuid) + const char *uuidstr) { virSecretObjPtr obj;

virObjectLock(secrets); - obj = virSecretObjListFindByUUIDLocked(secrets, uuid); + obj = virSecretObjListFindByUUIDLocked(secrets, uuidstr); virObjectUnlock(secrets); if (obj) virObjectLock(obj); @@ -328,13 +324,14 @@ virSecretObjListAdd(virSecretObjListPtr secrets, if (oldDef) *oldDef = NULL;

+ virUUIDFormat(newdef->uuid, uuidstr); + /* Is there a secret already matching this UUID */ - if ((obj = virSecretObjListFindByUUIDLocked(secrets, newdef->uuid))) { + if ((obj = virSecretObjListFindByUUIDLocked(secrets, uuidstr))) { virObjectLock(obj); def = obj->def;

if (STRNEQ_NULLABLE(def->usage_id, newdef->usage_id)) { - virUUIDFormat(def->uuid, uuidstr); virReportError(VIR_ERR_INTERNAL_ERROR, _("a secret with UUID %s is already defined for " "use with %s"), @@ -372,7 +369,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets, /* Generate the possible configFile and base64File strings * using the configDir, uuidstr, and appropriate suffix */ - virUUIDFormat(newdef->uuid, uuidstr); if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) || !(base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) goto cleanup; diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h index bd38f52..092f23c 100644 --- a/src/conf/virsecretobj.h +++ b/src/conf/virsecretobj.h @@ -40,7 +40,7 @@ virSecretObjListNew(void);

virSecretObjPtr virSecretObjListFindByUUID(virSecretObjListPtr secrets, - const unsigned char *uuid); + const char *uuidstr);

virSecretObjPtr virSecretObjListFindByUsage(virSecretObjListPtr secrets, diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index cc050ff..2d4091d 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -86,8 +86,8 @@ secretObjFromSecret(virSecretPtr secret) virSecretObjPtr obj; char uuidstr[VIR_UUID_STRING_BUFLEN];

- if (!(obj = virSecretObjListFindByUUID(driver->secrets, secret->uuid))) { - virUUIDFormat(secret->uuid, uuidstr); + virUUIDFormat(secret->uuid, uuidstr); + if (!(obj = virSecretObjListFindByUUID(driver->secrets, uuidstr))) { virReportError(VIR_ERR_NO_SECRET, _("no secret with matching uuid '%s'"), uuidstr); return NULL; @@ -159,10 +159,10 @@ secretLookupByUUID(virConnectPtr conn, virSecretPtr ret = NULL; virSecretObjPtr obj; virSecretDefPtr def; + char uuidstr[VIR_UUID_STRING_BUFLEN];

- if (!(obj = virSecretObjListFindByUUID(driver->secrets, uuid))) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(uuid, uuidstr); + virUUIDFormat(uuid, uuidstr); + if (!(obj = virSecretObjListFindByUUID(driver->secrets, uuidstr))) { virReportError(VIR_ERR_NO_SECRET, _("no secret with matching uuid '%s'"), uuidstr); goto cleanup; -- 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 07:45 AM, Pavel Hrdina wrote:

On Mon, Apr 24, 2017 at 02:00:22PM -0400, John Ferlan wrote:

...

Rather than being generated during virSecretObjListAdd, generate the file paths in each of the callers and then copy those paths into the object rather than stealing their pointers.

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 53 ++++++++++++++++++++++++---------------------- src/conf/virsecretobj.h | 3 ++- src/secret/secret_driver.c | 14 ++++++++++-- 3 files changed, 42 insertions(+), 28 deletions(-)

diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index ae2b287..7a9908d 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -300,7 +300,8 @@ virSecretObjListRemove(virSecretObjListPtr secrets, * virSecretObjListAdd: * @secrets: list of secret objects * @newdef: new secret definition - * @configDir: directory to place secret config files + * @configFile: secret config file + * @base64File: secret data file * @oldDef: Former secret def (e.g. a reload path perhaps) * * Add the new @newdef to the secret obj table hash @@ -310,14 +311,14 @@ virSecretObjListRemove(virSecretObjListPtr secrets, virSecretObjPtr virSecretObjListAdd(virSecretObjListPtr secrets, virSecretDefPtr newdef, - const char *configDir, + const char *configFile, + const char *base64File, virSecretDefPtr *oldDef) { virSecretObjPtr obj; virSecretDefPtr def; virSecretObjPtr ret = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; - char *configFile = NULL, *base64File = NULL;

virObjectLock(secrets);

@@ -366,13 +367,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets, goto cleanup; }

- /* Generate the possible configFile and base64File strings - * using the configDir, uuidstr, and appropriate suffix - */ - if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) || - !(base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) - goto cleanup; - if (!(obj = virSecretObjNew())) goto cleanup;

@@ -380,8 +374,10 @@ virSecretObjListAdd(virSecretObjListPtr secrets, goto cleanup;

obj->def = newdef; - VIR_STEAL_PTR(obj->configFile, configFile); - VIR_STEAL_PTR(obj->base64File, base64File); + if ((VIR_STRDUP(obj->configFile, configFile) < 0) || + (VIR_STRDUP(obj->base64File, base64File) < 0)) + goto cleanup; + virObjectRef(obj); }

@@ -390,8 +386,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets,

cleanup: virSecretObjEndAPI(&obj); - VIR_FREE(configFile); - VIR_FREE(base64File); virObjectUnlock(secrets); return ret; } @@ -899,21 +893,22 @@ virSecretLoadValue(virSecretObjPtr obj)

static virSecretObjPtr virSecretLoad(virSecretObjListPtr secrets, - const char *file, - const char *path, - const char *configDir) + const char *fname, + const char *configFile, + const char *base64File) { virSecretDefPtr def = NULL; virSecretObjPtr obj = NULL; virSecretObjPtr ret = NULL;

- if (!(def = virSecretDefParseFile(path))) + if (!(def = virSecretDefParseFile(configFile))) goto cleanup;

- if (virSecretLoadValidateUUID(def, file) < 0) + if (virSecretLoadValidateUUID(def, fname) < 0) goto cleanup;

- if (!(obj = virSecretObjListAdd(secrets, def, configDir, NULL))) + if (!(obj = virSecretObjListAdd(secrets, def, configFile, base64File, + NULL))) goto cleanup; def = NULL;

@@ -936,6 +931,8 @@ virSecretLoadAllConfigs(virSecretObjListPtr secrets, { DIR *dir = NULL; struct dirent *de; + char *configFile = NULL; + char *base64File = NULL; int rc;

if ((rc = virDirOpenIfExists(&dir, configDir)) <= 0) @@ -944,26 +941,32 @@ virSecretLoadAllConfigs(virSecretObjListPtr secrets, /* Ignore errors reported by readdir or other calls within the * loop (if any). It's better to keep the secrets we managed to find. */ while (virDirRead(dir, &de, NULL) > 0) { - char *path; virSecretObjPtr obj;

+ VIR_FREE(configFile); + VIR_FREE(base64File); + if (!virFileHasSuffix(de->d_name, ".xml")) continue;

- if (!(path = virFileBuildPath(configDir, de->d_name, NULL))) + if (!(configFile = virFileBuildPath(configDir, de->d_name, ".xml"))) + continue; + + if (!(base64File = virFileBuildPath(configDir, de->d_name, "base64"))) continue;

I don't like the fact that the ".xml" and ".base64" will be at two different places in our code, it's easy to miss one place and make things inconsistent like this patch. You have a typo here.

Oh - right the ".base64" - didn't catch that obviously and it took me a bit to notice ;-)

IMHO stealing pointer is better than allocate a new string, duplicate the string in order to be stored and free the allocated string.

Yeah and this is where things did get a bit tricky for me w/r/t pointer stealing. In future patches, configFile becomes part of an object since multiple drivers use configFile, while base64File is a specific concept to the secret driver. If the pointer is stolen by a subsequent API, then 'technically' none of the callers should be using it any more. Although it's perfectly normal for that to happen in our code because we don't pass by reference and have that steal. Because I knew what the end result is, I had to figure a way to get there somewhat logically. I'll look to rework this. John

NACK

Pavel

...

- if (!(obj = virSecretLoad(secrets, de->d_name, path, configDir))) { + if (!(obj = virSecretLoad(secrets, de->d_name, configFile, + base64File))) { VIR_ERROR(_("Error reading secret: %s"), virGetLastErrorMessage()); - VIR_FREE(path); continue; }

- VIR_FREE(path); virSecretObjEndAPI(&obj); }

+ VIR_FREE(configFile); + VIR_FREE(base64File); VIR_DIR_CLOSE(dir); return 0; } diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h index 092f23c..51adc98 100644 --- a/src/conf/virsecretobj.h +++ b/src/conf/virsecretobj.h @@ -54,7 +54,8 @@ virSecretObjListRemove(virSecretObjListPtr secrets, virSecretObjPtr virSecretObjListAdd(virSecretObjListPtr secrets, virSecretDefPtr newdef, - const char *configDir, + const char *configFile, + const char *base64File, virSecretDefPtr *oldDef);

typedef bool diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 8ddae57..3f8671b 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -213,6 +213,9 @@ secretDefineXML(virConnectPtr conn, virSecretDefPtr backup = NULL; virSecretDefPtr def; virObjectEventPtr event = NULL; + char *configFile = NULL; + char *base64File = NULL; + char uuidstr[VIR_UUID_STRING_BUFLEN];

virCheckFlags(0, NULL);

@@ -222,8 +225,13 @@ secretDefineXML(virConnectPtr conn, if (virSecretDefineXMLEnsureACL(conn, def) < 0) goto cleanup;

- if (!(obj = virSecretObjListAdd(driver->secrets, def, - driver->configDir, &backup))) + virUUIDFormat(def->uuid, uuidstr); + if (!(configFile = virFileBuildPath(driver->configDir, uuidstr, ".xml")) || + !(base64File = virFileBuildPath(driver->configDir, uuidstr, ".base64"))) + goto cleanup; + + if (!(obj = virSecretObjListAdd(driver->secrets, def, configFile, + base64File, &backup))) goto cleanup;

if (!def->isephemeral) { @@ -272,6 +280,8 @@ secretDefineXML(virConnectPtr conn, virSecretObjListRemove(driver->secrets, obj);

cleanup: + VIR_FREE(configFile); + VIR_FREE(base64File); virSecretDefFree(def); virSecretObjEndAPI(&obj); if (event) -- 2.9.3

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list