4:15 p.m.
v1: https://www.redhat.com/archives/libvir-list/2017-April/msg01051.html
Changes since v1:
- Patches 1, 2, 4, and 14 were pushed since they were ACK and "separable"
- Former patch 3 is now patch 1
-> Restore the comments for the functions.
- Former patch 5 is now split into patch 2 and 3
-> Patch 2 addresses the "if (!obj)" check in virSecretObjListRemove
-> Patch 3 handles the naming of the variables
- Former patch 6 is now patch 4
-> Alterations here are not creating a @def for the express purpose
of only dereferencing obj->def if that's all that's used. If though
there is an obj->def->X, then the @def is created
- Former patch 7 is removed (e.g. keep virSecretObjListGetUUIDs where
it was). A battle for a different day perhaps.
- Former patch 8 is now patches 5 and 6
-> Patch 5 handles just the cleanup path for virSecretObjListGetUUIDs
-> Patch 6 changes the variable names
- Former patch 9 is now patches 7 and 8
-> Patch 7 focuses only on changing the name of @filter to @aclfilter
(something that wasn't called out specifically in the initial review
but that I figured I'd do anyway)
-> Patch 8 does the split and keeps the Callback functions together
so it doesn't appear that virSecretObjListNumOfSecrets moved. Again
I'll pick that battle another day. I did change the name of the
structures to include "virSecret" prefix though.
- Former patch 10 is now patch 9
-> This was ACK'd but not "easy" to extract out, so nothing to do
here.
- Former patch 11 is now patch 10
-> Perhaps merge conflicts were changed here. The algorithm hasn't
changed though.
- Former patch 12 is now patch 11
-> This was ACKd already - just forgot to extract it out for a push.
- Former patch 13 is removed
So I've tried to go with the spirit of the review even though I disagree
with the don't do the code motion part. I'll deal with that later when
things really converge and the functions are shorter.
John Ferlan (11):
secret: Make some virSecretObj* functions static
secret: Add NULL obj check to virSecretObjListRemove
secret: Use consistent naming for variables
secret: Use virSecretDefPtr rather than deref from virSecretObjPtr
secret: Alter cleanup path for virSecretObjListGetUUIDs
secret: Change variable names for list traversals
secret: Rename 'filter' to 'aclfilter'
secret: Split apart NumOfSecrets and GetUUIDs callback function
secret: Combine virSecretObjListAdd with Locked function
secret: Alter FindByUUID to expect the formatted uuidstr
secret: Generate configDir during driver initialization
src/conf/virsecretobj.c | 428 ++++++++++++++++++++++++---------------------
src/conf/virsecretobj.h | 54 ++----
src/secret/secret_driver.c | 170 +++++++++---------
3 files changed, 328 insertions(+), 324 deletions(-)
--
2.9.3
4:15 p.m.
New subject: [libvirt] [PATCH v2 01/11] secret: Make some virSecretObj* functions static
Make various virSecretObjList*Locked functions static and make
virSecretObjNew static since they're only called within virtsecretobj.c
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virsecretobj.c | 8 ++++----
src/conf/virsecretobj.h | 18 ------------------
2 files changed, 4 insertions(+), 22 deletions(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index 5626387..169a60b 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;
@@ -169,7 +169,7 @@ virSecretObjListDispose(void *obj)
*
* Returns: not locked, but ref'd secret object.
*/
-virSecretObjPtr
+static virSecretObjPtr
virSecretObjListFindByUUIDLocked(virSecretObjListPtr secrets,
const unsigned char *uuid)
{
@@ -240,7 +240,7 @@ virSecretObjSearchName(const void *payload,
*
* Returns: not locked, but ref'd secret object.
*/
-virSecretObjPtr
+static virSecretObjPtr
virSecretObjListFindByUsageLocked(virSecretObjListPtr secrets,
int usageType,
const char *usageID)
@@ -324,7 +324,7 @@ virSecretObjListRemove(virSecretObjListPtr secrets,
*
* Returns pointer to secret or NULL if failure to add
*/
-virSecretObjPtr
+static virSecretObjPtr
virSecretObjListAddLocked(virSecretObjListPtr secrets,
virSecretDefPtr def,
const char *configDir,
diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h
index b26061a..9638b69 100644
--- a/src/conf/virsecretobj.h
+++ b/src/conf/virsecretobj.h
@@ -29,9 +29,6 @@
typedef struct _virSecretObj virSecretObj;
typedef virSecretObj *virSecretObjPtr;
-virSecretObjPtr
-virSecretObjNew(void);
-
void
virSecretObjEndAPI(virSecretObjPtr *secret);
@@ -42,19 +39,10 @@ virSecretObjListPtr
virSecretObjListNew(void);
virSecretObjPtr
-virSecretObjListFindByUUIDLocked(virSecretObjListPtr secrets,
- const unsigned char *uuid);
-
-virSecretObjPtr
virSecretObjListFindByUUID(virSecretObjListPtr secrets,
const unsigned char *uuid);
virSecretObjPtr
-virSecretObjListFindByUsageLocked(virSecretObjListPtr secrets,
- int usageType,
- const char *usageID);
-
-virSecretObjPtr
virSecretObjListFindByUsage(virSecretObjListPtr secrets,
int usageType,
const char *usageID);
@@ -64,12 +52,6 @@ virSecretObjListRemove(virSecretObjListPtr secrets,
virSecretObjPtr secret);
virSecretObjPtr
-virSecretObjListAddLocked(virSecretObjListPtr secrets,
- virSecretDefPtr def,
- const char *configDir,
- virSecretDefPtr *oldDef);
-
-virSecretObjPtr
virSecretObjListAdd(virSecretObjListPtr secrets,
virSecretDefPtr def,
const char *configDir,
--
2.9.3
4:15 p.m.
New subject: [libvirt] [PATCH v2 02/11] secret: Add NULL obj check to virSecretObjListRemove
Rather than have the caller check if !obj before calling, just check
in the function for !obj and return.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virsecretobj.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index 169a60b..7c2ad90 100644
--- a/src/conf/virsecretobj.c
+++ b/src/conf/virsecretobj.c
@@ -298,6 +298,9 @@ virSecretObjListRemove(virSecretObjListPtr secrets,
{
char uuidstr[VIR_UUID_STRING_BUFLEN];
+ if (!obj)
+ return;
+
virUUIDFormat(secret->def->uuid, uuidstr);
virObjectRef(secret);
virObjectUnlock(secret);
@@ -915,8 +918,7 @@ virSecretLoad(virSecretObjListPtr secrets,
secret = NULL;
cleanup:
- if (secret)
- virSecretObjListRemove(secrets, secret);
+ virSecretObjListRemove(secrets, secret);
virSecretDefFree(def);
return ret;
}
--
2.9.3
4:15 p.m.
New subject: [libvirt] [PATCH v2 03/11] secret: Use consistent naming for variables
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).
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virsecretobj.c | 271 ++++++++++++++++++++++-----------------------
src/conf/virsecretobj.h | 26 ++---
src/secret/secret_driver.c | 139 ++++++++++++-----------
3 files changed, 223 insertions(+), 213 deletions(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index 7c2ad90..ac3d5be 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);
}
@@ -195,14 +195,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;
}
@@ -211,21 +211,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;
}
@@ -245,14 +245,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;
}
@@ -272,14 +272,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;
}
@@ -294,22 +294,22 @@ virSecretObjListFindByUsage(virSecretObjListPtr secrets,
*/
void
virSecretObjListRemove(virSecretObjListPtr secrets,
- virSecretObjPtr secret)
+ virSecretObjPtr obj)
{
char uuidstr[VIR_UUID_STRING_BUFLEN];
if (!obj)
return;
- virUUIDFormat(secret->def->uuid, uuidstr);
- virObjectRef(secret);
- virObjectUnlock(secret);
+ 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);
}
@@ -317,11 +317,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!
*
@@ -329,11 +329,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;
@@ -342,71 +342,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;
@@ -415,16 +413,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;
}
@@ -496,23 +494,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;
@@ -640,12 +638,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;
}
@@ -654,11 +652,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);
}
@@ -669,15 +667,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;
@@ -689,18 +687,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;
@@ -712,36 +710,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;
@@ -749,7 +747,7 @@ virSecretObjGetValue(virSecretObjPtr secret)
int
-virSecretObjSetValue(virSecretObjPtr secret,
+virSecretObjSetValue(virSecretObjPtr obj,
const unsigned char *value,
size_t value_size)
{
@@ -759,14 +757,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 */
@@ -779,8 +777,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;
@@ -788,17 +786,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;
}
@@ -822,33 +820,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;
}
@@ -857,7 +855,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;
}
@@ -866,15 +864,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;
@@ -899,7 +897,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;
@@ -907,18 +906,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:
- virSecretObjListRemove(secrets, secret);
+ virSecretObjListRemove(secrets, obj);
virSecretDefFree(def);
return ret;
}
@@ -939,7 +938,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;
@@ -947,7 +946,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);
@@ -955,7 +954,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
3:29 a.m.
New subject: [libvirt] [PATCH v2 03/11] secret: Use consistent naming for variables
On Tue, Apr 25, 2017 at 05:15:27PM -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).
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virsecretobj.c | 271 ++++++++++++++++++++++-----------------------
src/conf/virsecretobj.h | 26 ++---
src/secret/secret_driver.c | 139 ++++++++++++-----------
3 files changed, 223 insertions(+), 213 deletions(-)
This patch has a some unrelated newline changes, that's for a separate patch.
ACK if you remove them.
Pavel
4:15 p.m.
New subject: [libvirt] [PATCH v2 04/11] secret: Use virSecretDefPtr rather than deref from virSecretObjPtr
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(a)redhat.com>
---
src/conf/virsecretobj.c | 63 +++++++++++++++++++++++++++++++------------------
1 file changed, 40 insertions(+), 23 deletions(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index ac3d5be..f4ec4ff 100644
--- a/src/conf/virsecretobj.c
+++ b/src/conf/virsecretobj.c
@@ -212,16 +212,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:
@@ -297,11 +299,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);
virObjectRef(obj);
virObjectUnlock(obj);
@@ -334,6 +338,7 @@ virSecretObjListAddLocked(virSecretObjListPtr secrets,
virSecretDefPtr *oldDef)
{
virSecretObjPtr obj;
+ virSecretDefPtr def;
virSecretObjPtr ret = NULL;
char uuidstr[VIR_UUID_STRING_BUFLEN];
char *configFile = NULL, *base64File = NULL;
@@ -344,26 +349,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,
@@ -372,7 +378,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"),
@@ -443,6 +450,7 @@ virSecretObjListGetHelper(void *payload,
{
struct virSecretObjListGetHelperData *data = opaque;
virSecretObjPtr obj = payload;
+ virSecretDefPtr def;
if (data->error)
return 0;
@@ -451,8 +459,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) {
@@ -463,7 +472,7 @@ virSecretObjListGetHelper(void *payload,
goto cleanup;
}
- virUUIDFormat(obj->def->uuid, uuidstr);
+ virUUIDFormat(def->uuid, uuidstr);
data->uuids[data->got] = uuidstr;
}
@@ -497,20 +506,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;
@@ -534,14 +545,16 @@ virSecretObjListExportCallback(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))
@@ -552,9 +565,9 @@ virSecretObjListExportCallback(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;
}
@@ -640,7 +653,9 @@ virSecretObjListGetUUIDs(virSecretObjListPtr secrets,
int
virSecretObjDeleteConfig(virSecretObjPtr obj)
{
- if (!obj->def->isephemeral &&
+ virSecretDefPtr def = obj->def;
+
+ if (!def->isephemeral &&
unlink(obj->configFile) < 0 && errno != ENOENT) {
virReportSystemError(errno, _("cannot unlink '%s'"),
obj->configFile);
@@ -727,11 +742,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);
virReportError(VIR_ERR_NO_SECRET,
_("secret '%s' does not have a value"),
uuidstr);
goto cleanup;
@@ -751,6 +767,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;
@@ -764,7 +781,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;
/* Saved successfully - drop old value */
--
2.9.3
3:30 a.m.
New subject: [libvirt] [PATCH v2 04/11] secret: Use virSecretDefPtr rather than deref from virSecretObjPtr
On Tue, Apr 25, 2017 at 05:15:28PM -0400, John Ferlan wrote:
Rather than dereferencing obj->def->X, create a local
'def' variable
variable that will dereference the def and use directly.
s/variable//
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virsecretobj.c | 63 +++++++++++++++++++++++++++++++------------------
1 file changed, 40 insertions(+), 23 deletions(-)
Pavel
4:15 p.m.
New subject: [libvirt] [PATCH v2 05/11] secret: Alter cleanup path for virSecretObjListGetUUIDs
Rather than using "ret = -1" and cleanup processing, alter the return
path on failure to goto error and then just return the data.got.
In the error path, we no longer check for ret < 0, we just can free
anything added to the array and return -1 directly
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virsecretobj.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index f4ec4ff..b5a001d 100644
--- a/src/conf/virsecretobj.c
+++ b/src/conf/virsecretobj.c
@@ -626,8 +626,6 @@ virSecretObjListGetUUIDs(virSecretObjListPtr secrets,
virSecretObjListACLFilter filter,
virConnectPtr conn)
{
- int ret = -1;
-
struct virSecretObjListGetHelperData data = {
.conn = conn, .filter = filter, .got = 0,
.uuids = uuids, .nuuids = nuuids, .error = false };
@@ -637,16 +635,14 @@ virSecretObjListGetUUIDs(virSecretObjListPtr secrets,
virObjectUnlock(secrets);
if (data.error)
- goto cleanup;
+ goto error;
- ret = data.got;
+ return data.got;
- cleanup:
- if (ret < 0) {
- while (data.got)
- VIR_FREE(data.uuids[--data.got]);
- }
- return ret;
+ error:
+ while (data.got)
+ VIR_FREE(data.uuids[--data.got]);
+ return -1;
}
--
2.9.3
4:15 p.m.
New subject: [libvirt] [PATCH v2 06/11] secret: Change variable names for list traversals
Rather than 'nuuids' it should be 'maxuuids' and rather than
'got'
it should be 'nuuids'. Alter the logic of the list traversal to
utilize those names.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virsecretobj.c | 28 ++++++++++++++--------------
src/conf/virsecretobj.h | 2 +-
2 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index b5a001d..eae735f 100644
--- a/src/conf/virsecretobj.c
+++ b/src/conf/virsecretobj.c
@@ -436,9 +436,9 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
struct virSecretObjListGetHelperData {
virConnectPtr conn;
virSecretObjListACLFilter filter;
- int got;
- char **uuids;
int nuuids;
+ char **uuids;
+ int maxuuids;
bool error;
};
@@ -455,7 +455,7 @@ virSecretObjListGetHelper(void *payload,
if (data->error)
return 0;
- if (data->nuuids >= 0 && data->got == data->nuuids)
+ if (data->maxuuids >= 0 && data->nuuids == data->maxuuids)
return 0;
virObjectLock(obj);
@@ -473,10 +473,10 @@ virSecretObjListGetHelper(void *payload,
}
virUUIDFormat(def->uuid, uuidstr);
- data->uuids[data->got] = uuidstr;
+ data->uuids[data->nuuids] = uuidstr;
}
- data->got++;
+ data->nuuids++;
cleanup:
virObjectUnlock(obj);
@@ -490,14 +490,14 @@ virSecretObjListNumOfSecrets(virSecretObjListPtr secrets,
virConnectPtr conn)
{
struct virSecretObjListGetHelperData data = {
- .conn = conn, .filter = filter, .got = 0,
- .uuids = NULL, .nuuids = -1, .error = false };
+ .conn = conn, .filter = filter, .nuuids = 0,
+ .uuids = NULL, .maxuuids = -1, .error = false };
virObjectLock(secrets);
virHashForEach(secrets->objs, virSecretObjListGetHelper, &data);
virObjectUnlock(secrets);
- return data.got;
+ return data.nuuids;
}
@@ -622,13 +622,13 @@ virSecretObjListExport(virConnectPtr conn,
int
virSecretObjListGetUUIDs(virSecretObjListPtr secrets,
char **uuids,
- int nuuids,
+ int maxuuids,
virSecretObjListACLFilter filter,
virConnectPtr conn)
{
struct virSecretObjListGetHelperData data = {
- .conn = conn, .filter = filter, .got = 0,
- .uuids = uuids, .nuuids = nuuids, .error = false };
+ .conn = conn, .filter = filter, .nuuids = 0,
+ .uuids = uuids, .maxuuids = maxuuids, .error = false };
virObjectLock(secrets);
virHashForEach(secrets->objs, virSecretObjListGetHelper, &data);
@@ -637,11 +637,11 @@ virSecretObjListGetUUIDs(virSecretObjListPtr secrets,
if (data.error)
goto error;
- return data.got;
+ return data.nuuids;
error:
- while (data.got)
- VIR_FREE(data.uuids[--data.got]);
+ while (--data.nuuids)
+ VIR_FREE(data.uuids[data.nuuids]);
return -1;
}
diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h
index 8038faa..2cba731 100644
--- a/src/conf/virsecretobj.h
+++ b/src/conf/virsecretobj.h
@@ -76,7 +76,7 @@ virSecretObjListExport(virConnectPtr conn,
int
virSecretObjListGetUUIDs(virSecretObjListPtr secrets,
char **uuids,
- int nuuids,
+ int maxuuids,
virSecretObjListACLFilter filter,
virConnectPtr conn);
--
2.9.3
4:15 p.m.
New subject: [libvirt] [PATCH v2 07/11] secret: Rename 'filter' to 'aclfilter'
Makes it a bit more clear what it is.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virsecretobj.c | 20 ++++++++++----------
src/conf/virsecretobj.h | 6 +++---
2 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index eae735f..c02558d 100644
--- a/src/conf/virsecretobj.c
+++ b/src/conf/virsecretobj.c
@@ -435,7 +435,7 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
struct virSecretObjListGetHelperData {
virConnectPtr conn;
- virSecretObjListACLFilter filter;
+ virSecretObjListACLFilter aclfilter;
int nuuids;
char **uuids;
int maxuuids;
@@ -461,7 +461,7 @@ virSecretObjListGetHelper(void *payload,
virObjectLock(obj);
def = obj->def;
- if (data->filter && !data->filter(data->conn, def))
+ if (data->aclfilter && !data->aclfilter(data->conn, def))
goto cleanup;
if (data->uuids) {
@@ -486,11 +486,11 @@ virSecretObjListGetHelper(void *payload,
int
virSecretObjListNumOfSecrets(virSecretObjListPtr secrets,
- virSecretObjListACLFilter filter,
+ virSecretObjListACLFilter aclfilter,
virConnectPtr conn)
{
struct virSecretObjListGetHelperData data = {
- .conn = conn, .filter = filter, .nuuids = 0,
+ .conn = conn, .aclfilter = aclfilter, .nuuids = 0,
.uuids = NULL, .maxuuids = -1, .error = false };
virObjectLock(secrets);
@@ -532,7 +532,7 @@ virSecretObjMatchFlags(virSecretObjPtr obj,
struct virSecretObjListData {
virConnectPtr conn;
virSecretPtr *secrets;
- virSecretObjListACLFilter filter;
+ virSecretObjListACLFilter aclfilter;
unsigned int flags;
int nsecrets;
bool error;
@@ -554,7 +554,7 @@ virSecretObjListExportCallback(void *payload,
virObjectLock(obj);
def = obj->def;
- if (data->filter && !data->filter(data->conn, def))
+ if (data->aclfilter && !data->aclfilter(data->conn, def))
goto cleanup;
if (!virSecretObjMatchFlags(obj, data->flags))
@@ -584,12 +584,12 @@ int
virSecretObjListExport(virConnectPtr conn,
virSecretObjListPtr secretobjs,
virSecretPtr **secrets,
- virSecretObjListACLFilter filter,
+ virSecretObjListACLFilter aclfilter,
unsigned int flags)
{
struct virSecretObjListData data = {
.conn = conn, .secrets = NULL,
- .filter = filter, .flags = flags,
+ .aclfilter = aclfilter, .flags = flags,
.nsecrets = 0, .error = false };
virObjectLock(secretobjs);
@@ -623,11 +623,11 @@ int
virSecretObjListGetUUIDs(virSecretObjListPtr secrets,
char **uuids,
int maxuuids,
- virSecretObjListACLFilter filter,
+ virSecretObjListACLFilter aclfilter,
virConnectPtr conn)
{
struct virSecretObjListGetHelperData data = {
- .conn = conn, .filter = filter, .nuuids = 0,
+ .conn = conn, .aclfilter = aclfilter, .nuuids = 0,
.uuids = uuids, .maxuuids = maxuuids, .error = false };
virObjectLock(secrets);
diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h
index 2cba731..a355da7 100644
--- a/src/conf/virsecretobj.h
+++ b/src/conf/virsecretobj.h
@@ -63,21 +63,21 @@ typedef bool
int
virSecretObjListNumOfSecrets(virSecretObjListPtr secrets,
- virSecretObjListACLFilter filter,
+ virSecretObjListACLFilter aclfilter,
virConnectPtr conn);
int
virSecretObjListExport(virConnectPtr conn,
virSecretObjListPtr secretobjs,
virSecretPtr **secrets,
- virSecretObjListACLFilter filter,
+ virSecretObjListACLFilter aclfilter,
unsigned int flags);
int
virSecretObjListGetUUIDs(virSecretObjListPtr secrets,
char **uuids,
int maxuuids,
- virSecretObjListACLFilter filter,
+ virSecretObjListACLFilter aclfilter,
virConnectPtr conn);
int
--
2.9.3
4:15 p.m.
New subject: [libvirt] [PATCH v2 08/11] secret: Split apart NumOfSecrets and GetUUIDs callback function
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(a)redhat.com>
---
src/conf/virsecretobj.c | 60 +++++++++++++++++++++++++++++++++++--------------
1 file changed, 43 insertions(+), 17 deletions(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index c02558d..e9a36b2 100644
--- a/src/conf/virsecretobj.c
+++ b/src/conf/virsecretobj.c
@@ -433,7 +433,36 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
}
-struct virSecretObjListGetHelperData {
+struct virSecretCountData {
+ virConnectPtr conn;
+ virSecretObjListACLFilter aclfilter;
+ int count;
+};
+
+static int
+virSecretObjListNumOfSecretsCallback(void *payload,
+ const void *name ATTRIBUTE_UNUSED,
+ void *opaque)
+{
+ struct virSecretCountData *data = opaque;
+ virSecretObjPtr obj = payload;
+ virSecretDefPtr def;
+
+ virObjectLock(obj);
+ def = obj->def;
+
+ if (data->aclfilter && !data->aclfilter(data->conn, def))
+ goto cleanup;
+
+ data->count++;
+
+ cleanup:
+ virObjectUnlock(obj);
+ return 0;
+}
+
+
+struct virSecretListData {
virConnectPtr conn;
virSecretObjListACLFilter aclfilter;
int nuuids;
@@ -444,11 +473,11 @@ struct virSecretObjListGetHelperData {
static int
-virSecretObjListGetHelper(void *payload,
- const void *name ATTRIBUTE_UNUSED,
- void *opaque)
+virSecretObjListGetUUIDsCallback(void *payload,
+ const void *name ATTRIBUTE_UNUSED,
+ void *opaque)
{
- struct virSecretObjListGetHelperData *data = opaque;
+ struct virSecretListData *data = opaque;
virSecretObjPtr obj = payload;
virSecretDefPtr def;
@@ -473,11 +502,9 @@ virSecretObjListGetHelper(void *payload,
}
virUUIDFormat(def->uuid, uuidstr);
- data->uuids[data->nuuids] = uuidstr;
+ data->uuids[data->nuuids++] = uuidstr;
}
- data->nuuids++;
-
cleanup:
virObjectUnlock(obj);
return 0;
@@ -489,15 +516,14 @@ virSecretObjListNumOfSecrets(virSecretObjListPtr secrets,
virSecretObjListACLFilter aclfilter,
virConnectPtr conn)
{
- struct virSecretObjListGetHelperData data = {
- .conn = conn, .aclfilter = aclfilter, .nuuids = 0,
- .uuids = NULL, .maxuuids = -1, .error = false };
+ struct virSecretCountData data = {
+ .conn = conn, .aclfilter = aclfilter, .count = 0 };
virObjectLock(secrets);
- virHashForEach(secrets->objs, virSecretObjListGetHelper, &data);
+ virHashForEach(secrets->objs, virSecretObjListNumOfSecretsCallback, &data);
virObjectUnlock(secrets);
- return data.nuuids;
+ return data.count;
}
@@ -626,12 +652,12 @@ virSecretObjListGetUUIDs(virSecretObjListPtr secrets,
virSecretObjListACLFilter aclfilter,
virConnectPtr conn)
{
- struct virSecretObjListGetHelperData data = {
- .conn = conn, .aclfilter = aclfilter, .nuuids = 0,
- .uuids = uuids, .maxuuids = maxuuids, .error = false };
+ struct virSecretListData data = {
+ .conn = conn, .aclfilter = aclfilter, .uuids = uuids, .nuuids = 0,
+ .maxuuids = maxuuids, .error = false };
virObjectLock(secrets);
- virHashForEach(secrets->objs, virSecretObjListGetHelper, &data);
+ virHashForEach(secrets->objs, virSecretObjListGetUUIDsCallback, &data);
virObjectUnlock(secrets);
if (data.error)
--
2.9.3
4:15 p.m.
New subject: [libvirt] [PATCH v2 09/11] secret: Combine virSecretObjListAdd with Locked function
There's no need to separate, so just have one.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virsecretobj.c | 34 ++++++++++------------------------
1 file changed, 10 insertions(+), 24 deletions(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index e9a36b2..0311335 100644
--- a/src/conf/virsecretobj.c
+++ b/src/conf/virsecretobj.c
@@ -319,7 +319,7 @@ virSecretObjListRemove(virSecretObjListPtr secrets,
/*
- * virSecretObjListAddLocked:
+ * virSecretObjListAdd:
* @secrets: list of secret objects
* @newdef: new secret definition
* @configDir: directory to place secret config files
@@ -327,15 +327,13 @@ virSecretObjListRemove(virSecretObjListPtr secrets,
*
* Add the new @newdef to the secret obj table hash
*
- * This functions requires @secrets to be locked already!
- *
- * Returns pointer to secret or NULL if failure to add
+ * Returns: locked and ref'd secret or NULL if failure to add
*/
-static virSecretObjPtr
-virSecretObjListAddLocked(virSecretObjListPtr secrets,
- virSecretDefPtr newdef,
- const char *configDir,
- virSecretDefPtr *oldDef)
+virSecretObjPtr
+virSecretObjListAdd(virSecretObjListPtr secrets,
+ virSecretDefPtr newdef,
+ const char *configDir,
+ virSecretDefPtr *oldDef)
{
virSecretObjPtr obj;
virSecretDefPtr def;
@@ -343,6 +341,8 @@ virSecretObjListAddLocked(virSecretObjListPtr secrets,
char uuidstr[VIR_UUID_STRING_BUFLEN];
char *configFile = NULL, *base64File = NULL;
+ virObjectLock(secrets);
+
if (oldDef)
*oldDef = NULL;
@@ -414,22 +414,8 @@ virSecretObjListAddLocked(virSecretObjListPtr secrets,
virSecretObjEndAPI(&obj);
VIR_FREE(configFile);
VIR_FREE(base64File);
- return ret;
-}
-
-
-virSecretObjPtr
-virSecretObjListAdd(virSecretObjListPtr secrets,
- virSecretDefPtr newdef,
- const char *configDir,
- virSecretDefPtr *oldDef)
-{
- virSecretObjPtr obj;
-
- virObjectLock(secrets);
- obj = virSecretObjListAddLocked(secrets, newdef, configDir, oldDef);
virObjectUnlock(secrets);
- return obj;
+ return ret;
}
--
2.9.3
4:15 p.m.
New subject: [libvirt] [PATCH v2 10/11] secret: Alter FindByUUID to expect the formatted uuidstr
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.
Signed-off-by: John Ferlan <jferlan(a)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 0311335..62b86b7 100644
--- a/src/conf/virsecretobj.c
+++ b/src/conf/virsecretobj.c
@@ -171,12 +171,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));
}
@@ -184,7 +180,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.
@@ -193,12 +189,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);
@@ -346,13 +342,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"),
@@ -390,7 +387,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 a355da7..0196813 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
3:44 a.m.
New subject: [libvirt] [PATCH v2 10/11] secret: Alter FindByUUID to expect the formatted uuidstr
On Tue, Apr 25, 2017 at 05:15:34PM -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.
Signed-off-by: John Ferlan <jferlan(a)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(-)
I still don't like this patch and I see why you want to keep converting
UUID to string, it is necessary for the future patches because the *primary*
and *secondary* keys assumes that they will be strings.
I still think that if we will cleanup the UUID lookup somehow, we should stop
converting it to a string and make proper "key*" callbacks that will be used
by our hash table. I'll send a patches to do that.
Let's drop this patch for now to not hold the whole series and move it to
a discussion about the new virObjectPoolableHashTable and
virObjectPoolableHashElement classes.
Pavel
4:15 p.m.
New subject: [libvirt] [PATCH v2 11/11] secret: Generate configDir during driver initialization
Rather than waiting for the first save to fail, let's generate the
directory with the correct privs during initialization
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/secret/secret_driver.c | 23 ++++++-----------------
1 file changed, 6 insertions(+), 17 deletions(-)
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
index 2d4091d..8ddae57 100644
--- a/src/secret/secret_driver.c
+++ b/src/secret/secret_driver.c
@@ -96,17 +96,6 @@ secretObjFromSecret(virSecretPtr secret)
}
-static int
-secretEnsureDirectory(void)
-{
- if (mkdir(driver->configDir, S_IRWXU) < 0 && errno != EEXIST) {
- virReportSystemError(errno, _("cannot create '%s'"),
- driver->configDir);
- return -1;
- }
- return 0;
-}
-
/* Driver functions */
static int
@@ -238,9 +227,6 @@ secretDefineXML(virConnectPtr conn,
goto cleanup;
if (!def->isephemeral) {
- if (secretEnsureDirectory() < 0)
- goto cleanup;
-
if (backup && backup->isephemeral) {
if (virSecretObjSaveData(obj) < 0)
goto restore_backup;
@@ -341,9 +327,6 @@ secretSetValue(virSecretPtr secret,
if (virSecretSetValueEnsureACL(secret->conn, def) < 0)
goto cleanup;
- if (secretEnsureDirectory() < 0)
- goto cleanup;
-
if (virSecretObjSetValue(obj, value, value_size) < 0)
goto cleanup;
@@ -488,6 +471,12 @@ secretStateInitialize(bool privileged,
goto error;
VIR_FREE(base);
+ if (virFileMakePathWithMode(driver->configDir, S_IRWXU) < 0) {
+ virReportSystemError(errno, _("cannot create config directory
'%s'"),
+ driver->configDir);
+ goto error;
+ }
+
if (!(driver->secrets = virSecretObjListNew()))
goto error;
--
2.9.3
3:46 a.m.
On Tue, Apr 25, 2017 at 05:15:24PM -0400, John Ferlan wrote:
v1:
https://www.redhat.com/archives/libvir-list/2017-April/msg01051.html
Changes since v1:
- Patches 1, 2, 4, and 14 were pushed since they were ACK and "separable"
- Former patch 3 is now patch 1
-> Restore the comments for the functions.
- Former patch 5 is now split into patch 2 and 3
-> Patch 2 addresses the "if (!obj)" check in virSecretObjListRemove
-> Patch 3 handles the naming of the variables
- Former patch 6 is now patch 4
-> Alterations here are not creating a @def for the express purpose
of only dereferencing obj->def if that's all that's used. If though
there is an obj->def->X, then the @def is created
- Former patch 7 is removed (e.g. keep virSecretObjListGetUUIDs where
it was). A battle for a different day perhaps.
- Former patch 8 is now patches 5 and 6
-> Patch 5 handles just the cleanup path for virSecretObjListGetUUIDs
-> Patch 6 changes the variable names
- Former patch 9 is now patches 7 and 8
-> Patch 7 focuses only on changing the name of @filter to @aclfilter
(something that wasn't called out specifically in the initial review
but that I figured I'd do anyway)
-> Patch 8 does the split and keeps the Callback functions together
so it doesn't appear that virSecretObjListNumOfSecrets moved. Again
I'll pick that battle another day. I did change the name of the
structures to include "virSecret" prefix though.
- Former patch 10 is now patch 9
-> This was ACK'd but not "easy" to extract out, so nothing to do
here.
- Former patch 11 is now patch 10
-> Perhaps merge conflicts were changed here. The algorithm hasn't
changed though.
- Former patch 12 is now patch 11
-> This was ACKd already - just forgot to extract it out for a push.
- Former patch 13 is removed
So I've tried to go with the spirit of the review even though I disagree
with the don't do the code motion part. I'll deal with that later when
things really converge and the functions are shorter.
ACK series if you fix the nits pointed out for patch 03 and 04 and if you
drop patch 10.
Pavel
12:34 p.m.
On 04/26/2017 04:46 AM, Pavel Hrdina wrote:
On Tue, Apr 25, 2017 at 05:15:24PM -0400, John Ferlan wrote:
> v1: https://www.redhat.com/archives/libvir-list/2017-April/msg01051.html
>
> Changes since v1:
>
> - Patches 1, 2, 4, and 14 were pushed since they were ACK and "separable"
>
> - Former patch 3 is now patch 1
> -> Restore the comments for the functions.
>
> - Former patch 5 is now split into patch 2 and 3
> -> Patch 2 addresses the "if (!obj)" check in virSecretObjListRemove
>
> -> Patch 3 handles the naming of the variables
>
> - Former patch 6 is now patch 4
> -> Alterations here are not creating a @def for the express purpose
> of only dereferencing obj->def if that's all that's used. If though
> there is an obj->def->X, then the @def is created
>
> - Former patch 7 is removed (e.g. keep virSecretObjListGetUUIDs where
> it was). A battle for a different day perhaps.
>
> - Former patch 8 is now patches 5 and 6
> -> Patch 5 handles just the cleanup path for virSecretObjListGetUUIDs
>
> -> Patch 6 changes the variable names
>
> - Former patch 9 is now patches 7 and 8
> -> Patch 7 focuses only on changing the name of @filter to @aclfilter
> (something that wasn't called out specifically in the initial review
> but that I figured I'd do anyway)
>
> -> Patch 8 does the split and keeps the Callback functions together
> so it doesn't appear that virSecretObjListNumOfSecrets moved. Again
> I'll pick that battle another day. I did change the name of the
> structures to include "virSecret" prefix though.
>
> - Former patch 10 is now patch 9
> -> This was ACK'd but not "easy" to extract out, so nothing to do
here.
>
> - Former patch 11 is now patch 10
> -> Perhaps merge conflicts were changed here. The algorithm hasn't
> changed though.
>
> - Former patch 12 is now patch 11
> -> This was ACKd already - just forgot to extract it out for a push.
>
> - Former patch 13 is removed
>
> So I've tried to go with the spirit of the review even though I disagree
> with the don't do the code motion part. I'll deal with that later when
> things really converge and the functions are shorter.
ACK series if you fix the nits pointed out for patch 03 and 04 and if you
drop patch 10.
Removed extraneous newline adjustments from secret_driver in patch 3
(that's a type A type thing and I forgot to separate it...)... Fixed
commit message in patch 4, dropped patch 10...
Thanks for the reviews for this and the nwfilter series - it also helps
me for "direction" for the other vir*obj and *drivers that will be
adjusted as well...
John
2767
days inactive
2768
days old
16 comments
2 participants
participants (2)
-
John Ferlan -
Pavel Hrdina