[libvirt] [PATCH v2 00/11]
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
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 | 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
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@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
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@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
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@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
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 | 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
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@redhat.com> --- src/conf/virsecretobj.c | 63 +++++++++++++++++++++++++++++++------------------ 1 file changed, 40 insertions(+), 23 deletions(-)
Pavel
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@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
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@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
Makes it a bit more clear what it is. Signed-off-by: John Ferlan <jferlan@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
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 | 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
There's no need to separate, so just have one. Signed-off-by: John Ferlan <jferlan@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
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@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
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@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
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@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
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
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
participants (2)
-
John Ferlan -
Pavel Hrdina