On 02/25/2016 09:03 AM, John Ferlan wrote: <sigh> Move code and neglected to retest... Anyway, consider the following squashed in (it has ramifications a few patches later too. Essentially, cannot deref secret-> yet - do it only after we're sure we have a non NULL secret pointer): diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index d4b0207..9ad3d68 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -725,8 +725,8 @@ secretDefineXML(virConnectPtr conn, const char *usageID = secretUsageIDForDef(new_attrs); char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(secret->def->uuid, uuidstr); if ((secret = secretFindByUsage(new_attrs->usage_type, usageID))) { + virUUIDFormat(secret->def->uuid, uuidstr); virReportError(VIR_ERR_INTERNAL_ERROR, _("a secret with UUID %s already defined for " "use with %s"), @@ -738,6 +738,8 @@ secretDefineXML(virConnectPtr conn, if (VIR_ALLOC(secret) < 0) goto cleanup; + virUUIDFormat(secret->def->uuid, uuidstr); + /* Generate configFile using driver->configDir, * the uuidstr, and .xml suffix */ if (!(secret->configFile = virFileBuildPath(driver->configDir,

This patch removes need for secretBase64Path and secretComputePath. Similar to the configFile, create an entry for base64File, which will be generated as the driver->configDir, the UUID value, plus the ".base" suffix. Rather than generating on the fly, store this in the virSecretObj. The buildup of the pathname done in loadSecrets where the failure to build is ignored which is no different than the failure to generate the name in secretLoadValue which would have been ignored in the failure path after secretLoad. This also removes the need for secretComputPath and secretBase64Path. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/secret/secret_driver.c | 107 ++++++++++++++++++++------------------------- 1 file changed, 48 insertions(+), 59 deletions(-) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index d4b0207..f1ab58c 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -57,6 +57,7 @@ typedef virSecretObj *virSecretObjPtr; struct _virSecretObj { virSecretObjPtr next; char *configFile; + char *base64File; virSecretDefPtr def; unsigned char *value; /* May be NULL */ size_t value_size; @@ -114,6 +115,7 @@ secretFree(virSecretObjPtr secret) VIR_FREE(secret->value); } VIR_FREE(secret->configFile); + VIR_FREE(secret->base64File); VIR_FREE(secret); } @@ -185,26 +187,6 @@ secretRewriteFile(int fd, return 0; } -static char * -secretComputePath(const virSecretObj *secret, - const char *suffix) -{ - char *ret; - char uuidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(secret->def->uuid, uuidstr); - - ignore_value(virAsprintf(&ret, "%s/%s%s", driver->configDir, - uuidstr, suffix)); - return ret; -} - - -static char * -secretBase64Path(const virSecretObj *secret) -{ - return secretComputePath(secret, ".base64"); -} static int secretEnsureDirectory(void) @@ -243,7 +225,7 @@ secretSaveDef(const virSecretObj *secret) static int secretSaveValue(const virSecretObj *secret) { - char *filename = NULL, *base64 = NULL; + char *base64 = NULL; int ret = -1; if (secret->value == NULL) @@ -252,9 +234,6 @@ secretSaveValue(const virSecretObj *secret) if (secretEnsureDirectory() < 0) goto cleanup; - if (!(filename = secretBase64Path(secret))) - goto cleanup; - base64_encode_alloc((const char *)secret->value, secret->value_size, &base64); if (base64 == NULL) { @@ -262,7 +241,7 @@ secretSaveValue(const virSecretObj *secret) goto cleanup; } - if (virFileRewrite(filename, S_IRUSR | S_IWUSR, + if (virFileRewrite(secret->base64File, S_IRUSR | S_IWUSR, secretRewriteFile, base64) < 0) goto cleanup; @@ -270,30 +249,20 @@ secretSaveValue(const virSecretObj *secret) cleanup: VIR_FREE(base64); - VIR_FREE(filename); return ret; } static int secretDeleteSaved(const virSecretObj *secret) { - char *value_filename = NULL; - int ret = -1; - - if (!(value_filename = secretBase64Path(secret))) - goto cleanup; - if (unlink(secret->configFile) < 0 && errno != ENOENT) - goto cleanup; + return -1; + /* When the XML is missing, the rest may waste disk space, but the secret won't be loaded again, so we have succeeded already. */ - ret = 0; - - (void)unlink(value_filename); + (void)unlink(secret->base64File); - cleanup: - VIR_FREE(value_filename); - return ret; + return 0; } static int @@ -319,29 +288,29 @@ secretLoadValue(virSecretObjPtr secret) { int ret = -1, fd = -1; struct stat st; - char *filename = NULL, *contents = NULL, *value = NULL; + char *contents = NULL, *value = NULL; size_t value_size; - if (!(filename = secretBase64Path(secret))) - goto cleanup; - - if ((fd = open(filename, O_RDONLY)) == -1) { + if ((fd = open(secret->base64File, O_RDONLY)) == -1) { if (errno == ENOENT) { ret = 0; goto cleanup; } - virReportSystemError(errno, _("cannot open '%s'"), filename); + virReportSystemError(errno, _("cannot open '%s'"), + secret->base64File); goto cleanup; } if (fstat(fd, &st) < 0) { - virReportSystemError(errno, _("cannot stat '%s'"), filename); + virReportSystemError(errno, _("cannot stat '%s'"), + secret->base64File); goto cleanup; } if ((size_t)st.st_size != st.st_size) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("'%s' file does not fit in memory"), filename); + _("'%s' file does not fit in memory"), + secret->base64File); goto cleanup; } @@ -349,7 +318,8 @@ secretLoadValue(virSecretObjPtr secret) goto cleanup; if (saferead(fd, contents, st.st_size) != st.st_size) { - virReportSystemError(errno, _("cannot read '%s'"), filename); + virReportSystemError(errno, _("cannot read '%s'"), + secret->base64File); goto cleanup; } @@ -357,7 +327,8 @@ secretLoadValue(virSecretObjPtr secret) if (!base64_decode_alloc(contents, st.st_size, &value, &value_size)) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("invalid base64 in '%s'"), filename); + _("invalid base64 in '%s'"), + secret->base64File); goto cleanup; } if (value == NULL) @@ -379,13 +350,13 @@ secretLoadValue(virSecretObjPtr secret) VIR_FREE(contents); } VIR_FORCE_CLOSE(fd); - VIR_FREE(filename); return ret; } static virSecretObjPtr secretLoad(const char *file, - const char *path) + const char *path, + const char *base64path) { virSecretDefPtr def = NULL; virSecretObjPtr secret = NULL, ret = NULL; @@ -404,6 +375,9 @@ secretLoad(const char *file, if (VIR_STRDUP(secret->configFile, path) < 0) goto cleanup; + if (VIR_STRDUP(secret->base64File, base64path) < 0) + goto cleanup; + if (secretLoadValue(secret) < 0) goto cleanup; @@ -432,7 +406,7 @@ loadSecrets(virSecretObjPtr *dest) } while (virDirRead(dir, &de, NULL) > 0) { - char *path; + char *path, *base64name, *base64path; virSecretObjPtr secret; if (STREQ(de->d_name, ".") || STREQ(de->d_name, "..")) @@ -444,17 +418,30 @@ loadSecrets(virSecretObjPtr *dest) if (!(path = virFileBuildPath(driver->configDir, de->d_name, NULL))) continue; - if (!(secret = secretLoad(de->d_name, path))) { + /* Copy the .xml file name, but use suffix ".base64" instead */ + if (VIR_STRDUP(base64name, de->d_name) < 0 || + !virFileStripSuffix(base64name, ".xml") || + !(base64path = virFileBuildPath(driver->configDir, + base64name, ".base64"))) { + VIR_FREE(path); + VIR_FREE(base64name); + continue; + } + VIR_FREE(base64name); + + if (!(secret = secretLoad(de->d_name, path, base64path))) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Error reading secret: %s"), err != NULL ? err->message: _("unknown error")); virResetError(err); VIR_FREE(path); + VIR_FREE(base64path); continue; } VIR_FREE(path); + VIR_FREE(base64path); listInsert(&list, secret); } /* Ignore error reported by readdir, if any. It's better to keep the @@ -745,6 +732,13 @@ secretDefineXML(virConnectPtr conn, secretFree(secret); goto cleanup; } + /* Generate base64File using driver->configDir, + * the uuidstr, and .base64 suffix */ + if (!(secret->base64File = virFileBuildPath(driver->configDir, + uuidstr, ".base64"))) { + secretFree(secret); + goto cleanup; + } listInsert(&driver->secrets, secret); secret->def = new_attrs; @@ -779,13 +773,8 @@ secretDefineXML(virConnectPtr conn, } if (secretSaveDef(secret) < 0) { if (backup && backup->ephemeral) { - char *filename; - /* Undo the secretSaveValue() above; ignore errors */ - filename = secretBase64Path(secret); - if (filename != NULL) - (void)unlink(filename); - VIR_FREE(filename); + (void)unlink(secret->base64File); } goto restore_backup; } -- 2.5.0

Add a temporary helper to search for a specific secret by address on the list and remove it if it's found. The following patch will introduce a common allocation and listInsert helper. That means error paths of the routines calling would need a way to remove the secret off the list. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/secret/secret_driver.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index f1ab58c..70bd2f3 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -353,6 +353,26 @@ secretLoadValue(virSecretObjPtr secret) return ret; } + +static void +listUnlinkSecret(virSecretObjPtr *pptr, + virSecretObjPtr secret) +{ + if (!secret) + return; + + if (*pptr == secret) { + *pptr = secret->next; + } else { + virSecretObjPtr tmp = *pptr; + while (tmp && tmp->next != secret) + tmp = tmp->next; + if (tmp) + tmp->next = secret->next; + } +} + + static virSecretObjPtr secretLoad(const char *file, const char *path, @@ -978,15 +998,7 @@ secretUndefine(virSecretPtr obj) secretDeleteSaved(secret) < 0) goto cleanup; - if (driver->secrets == secret) { - driver->secrets = secret->next; - } else { - virSecretObjPtr tmp = driver->secrets; - while (tmp && tmp->next != secret) - tmp = tmp->next; - if (tmp) - tmp->next = secret->next; - } + listUnlinkSecret(&driver->secrets, secret); secretFree(secret); ret = 0; -- 2.5.0

This new API will allocate the secret, assign the def pointer, and insert the secret onto the passed list. Whether that's the temporary list in loadSecrets which gets loaded into the driver list or driver list during secretDefineXML. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/secret/secret_driver.c | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 70bd2f3..8ebe79d 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -168,6 +168,22 @@ secretFindByUsage(int usageType, return NULL; } + +static virSecretObjPtr +secretAssignDef(virSecretObjPtr *list, + virSecretDefPtr def) +{ + virSecretObjPtr secret; + + if (VIR_ALLOC(secret) < 0) + return NULL; + + listInsert(list, secret); + secret->def = def; + + return secret; +} + /* Permament secret storage */ /* Secrets are stored in virSecretDriverStatePtr->configDir. Each secret @@ -374,7 +390,8 @@ listUnlinkSecret(virSecretObjPtr *pptr, static virSecretObjPtr -secretLoad(const char *file, +secretLoad(virSecretObjPtr *list, + const char *file, const char *path, const char *base64path) { @@ -387,9 +404,8 @@ secretLoad(const char *file, if (secretLoadValidateUUID(def, file) < 0) goto cleanup; - if (VIR_ALLOC(secret) < 0) + if (!(secret = secretAssignDef(list, def))) goto cleanup; - secret->def = def; def = NULL; if (VIR_STRDUP(secret->configFile, path) < 0) @@ -405,6 +421,7 @@ secretLoad(const char *file, secret = NULL; cleanup: + listUnlinkSecret(list, secret); secretFree(secret); virSecretDefFree(def); return ret; @@ -449,7 +466,7 @@ loadSecrets(virSecretObjPtr *dest) } VIR_FREE(base64name); - if (!(secret = secretLoad(de->d_name, path, base64path))) { + if (!(secret = secretLoad(&list, de->d_name, path, base64path))) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Error reading secret: %s"), @@ -462,7 +479,6 @@ loadSecrets(virSecretObjPtr *dest) VIR_FREE(path); VIR_FREE(base64path); - listInsert(&list, secret); } /* Ignore error reported by readdir, if any. It's better to keep the secrets we managed to find. */ @@ -712,7 +728,7 @@ secretDefineXML(virConnectPtr conn, unsigned int flags) { virSecretPtr ret = NULL; - virSecretObjPtr secret; + virSecretObjPtr secret = NULL; virSecretDefPtr backup = NULL; virSecretDefPtr new_attrs; @@ -742,7 +758,7 @@ secretDefineXML(virConnectPtr conn, } /* No existing secret at all, create one */ - if (VIR_ALLOC(secret) < 0) + if (!(secret = secretAssignDef(&driver->secrets, new_attrs))) goto cleanup; /* Generate configFile using driver->configDir, @@ -759,9 +775,6 @@ secretDefineXML(virConnectPtr conn, secretFree(secret); goto cleanup; } - - listInsert(&driver->secrets, secret); - secret->def = new_attrs; } else { const char *newUsageID = secretUsageIDForDef(new_attrs); const char *oldUsageID = secretUsageIDForDef(secret->def); -- 2.5.0

Rename to secretLoadAllConfigs and add the 'driver->configDir' as a parameter. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/secret/secret_driver.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 8ebe79d..fb24237 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -428,17 +428,17 @@ secretLoad(virSecretObjPtr *list, } static int -loadSecrets(virSecretObjPtr *dest) +secretLoadAllConfigs(virSecretObjPtr *dest, + const char *configDir) { DIR *dir = NULL; struct dirent *de; virSecretObjPtr list = NULL; - if (!(dir = opendir(driver->configDir))) { + if (!(dir = opendir(configDir))) { if (errno == ENOENT) return 0; - virReportSystemError(errno, _("cannot open '%s'"), - driver->configDir); + virReportSystemError(errno, _("cannot open '%s'"), configDir); return -1; } @@ -452,13 +452,13 @@ loadSecrets(virSecretObjPtr *dest) if (!virFileHasSuffix(de->d_name, ".xml")) continue; - if (!(path = virFileBuildPath(driver->configDir, de->d_name, NULL))) + if (!(path = virFileBuildPath(configDir, de->d_name, NULL))) continue; /* Copy the .xml file name, but use suffix ".base64" instead */ if (VIR_STRDUP(base64name, de->d_name) < 0 || !virFileStripSuffix(base64name, ".xml") || - !(base64path = virFileBuildPath(driver->configDir, + !(base64path = virFileBuildPath(configDir, base64name, ".base64"))) { VIR_FREE(path); VIR_FREE(base64name); @@ -1072,7 +1072,7 @@ secretStateInitialize(bool privileged, goto error; VIR_FREE(base); - if (loadSecrets(&driver->secrets) < 0) + if (secretLoadAllConfigs(&driver->secrets, driver->configDir) < 0) goto error; secretDriverUnlock(); @@ -1095,7 +1095,7 @@ secretStateReload(void) secretDriverLock(); - if (loadSecrets(&new_secrets) < 0) + if (secretLoadAllConfigs(&new_secrets, driver->configDir) < 0) goto end; /* Keep ephemeral secrets from current state. -- 2.5.0

This patch replaces the listInsert and listUnlink with the more commonly used VIR_APPEND_ELEMENT and VIR_REMOVE_ELEMENT macros used for list mgmt. Of course that does require any code walking the ->next list to instead use the ->count for loop Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/secret/secret_driver.c | 244 +++++++++++++++++++++++---------------------- 1 file changed, 124 insertions(+), 120 deletions(-) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index fb24237..deb8c59 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -55,7 +55,6 @@ enum { SECRET_MAX_XML_FILE = 10*1024*1024 }; typedef struct _virSecretObj virSecretObj; typedef virSecretObj *virSecretObjPtr; struct _virSecretObj { - virSecretObjPtr next; char *configFile; char *base64File; virSecretDefPtr def; @@ -63,11 +62,18 @@ struct _virSecretObj { size_t value_size; }; +typedef struct _virSecretObjList virSecretObjList; +typedef virSecretObjList *virSecretObjListPtr; +struct _virSecretObjList { + size_t count; + virSecretObjPtr *objs; +}; + typedef struct _virSecretDriverState virSecretDriverState; typedef virSecretDriverState *virSecretDriverStatePtr; struct _virSecretDriverState { virMutex lock; - virSecretObj *secrets; + virSecretObjList secrets; char *configDir; }; @@ -85,23 +91,6 @@ secretDriverUnlock(void) virMutexUnlock(&driver->lock); } -static virSecretObjPtr -listUnlink(virSecretObjPtr *pptr) -{ - virSecretObjPtr secret; - - secret = *pptr; - *pptr = secret->next; - return secret; -} - -static void -listInsert(virSecretObjPtr *pptr, - virSecretObjPtr secret) -{ - secret->next = *pptr; - *pptr = secret; -} static void secretFree(virSecretObjPtr secret) @@ -119,13 +108,47 @@ secretFree(virSecretObjPtr secret) VIR_FREE(secret); } + +static void +secretObjListFree(virSecretObjListPtr secrets) +{ + size_t i; + + for (i = 0; i < secrets->count; i++) + secretFree(secrets->objs[i]); + VIR_FREE(secrets->objs); + secrets->count = 0; +} + + +static void +secretObjRemove(virSecretObjListPtr secrets, + virSecretObjPtr secret) +{ + size_t i; + + if (!secret) + return; + + for (i = 0; i < secrets->count; i++) { + if (secrets->objs[i] == secret) { + secretFree(secrets->objs[i]); + VIR_DELETE_ELEMENT(secrets->objs, i, secrets->count); + break; + } + } +} + + static virSecretObjPtr -secretFindByUUID(const unsigned char *uuid) +secretFindByUUID(virSecretObjListPtr secrets, + const unsigned char *uuid) { - virSecretObjPtr *pptr, secret; + size_t i; + virSecretObjPtr secret; - for (pptr = &driver->secrets; *pptr != NULL; pptr = &secret->next) { - secret = *pptr; + for (i = 0; i < secrets->count; i++) { + secret = secrets->objs[i]; if (memcmp(secret->def->uuid, uuid, VIR_UUID_BUFLEN) == 0) return secret; } @@ -133,13 +156,15 @@ secretFindByUUID(const unsigned char *uuid) } static virSecretObjPtr -secretFindByUsage(int usageType, +secretFindByUsage(virSecretObjListPtr secrets, + int usageType, const char *usageID) { - virSecretObjPtr *pptr, secret; + size_t i; + virSecretObjPtr secret; - for (pptr = &driver->secrets; *pptr != NULL; pptr = &secret->next) { - secret = *pptr; + for (i = 0; i < secrets->count; i++) { + secret = secrets->objs[i]; if (secret->def->usage_type != usageType) continue; @@ -170,7 +195,7 @@ secretFindByUsage(int usageType, static virSecretObjPtr -secretAssignDef(virSecretObjPtr *list, +secretAssignDef(virSecretObjListPtr list, virSecretDefPtr def) { virSecretObjPtr secret; @@ -178,7 +203,11 @@ secretAssignDef(virSecretObjPtr *list, if (VIR_ALLOC(secret) < 0) return NULL; - listInsert(list, secret); + if (VIR_APPEND_ELEMENT_COPY(list->objs, list->count, secret) < 0) { + secretFree(secret); + return NULL; + } + secret->def = def; return secret; @@ -370,27 +399,8 @@ secretLoadValue(virSecretObjPtr secret) } -static void -listUnlinkSecret(virSecretObjPtr *pptr, - virSecretObjPtr secret) -{ - if (!secret) - return; - - if (*pptr == secret) { - *pptr = secret->next; - } else { - virSecretObjPtr tmp = *pptr; - while (tmp && tmp->next != secret) - tmp = tmp->next; - if (tmp) - tmp->next = secret->next; - } -} - - static virSecretObjPtr -secretLoad(virSecretObjPtr *list, +secretLoad(virSecretObjListPtr list, const char *file, const char *path, const char *base64path) @@ -421,19 +431,18 @@ secretLoad(virSecretObjPtr *list, secret = NULL; cleanup: - listUnlinkSecret(list, secret); - secretFree(secret); + secretObjRemove(list, secret); virSecretDefFree(def); return ret; } + static int -secretLoadAllConfigs(virSecretObjPtr *dest, +secretLoadAllConfigs(virSecretObjListPtr secrets, const char *configDir) { DIR *dir = NULL; struct dirent *de; - virSecretObjPtr list = NULL; if (!(dir = opendir(configDir))) { if (errno == ENOENT) @@ -442,6 +451,8 @@ secretLoadAllConfigs(virSecretObjPtr *dest, return -1; } + /* Ignore errors reported by readdir or other calls within the + * loop (if any). It's better to keep the secrets we managed to find. */ while (virDirRead(dir, &de, NULL) > 0) { char *path, *base64name, *base64path; virSecretObjPtr secret; @@ -466,7 +477,7 @@ secretLoadAllConfigs(virSecretObjPtr *dest, } VIR_FREE(base64name); - if (!(secret = secretLoad(&list, de->d_name, path, base64path))) { + if (!(secret = secretLoad(secrets, de->d_name, path, base64path))) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Error reading secret: %s"), @@ -480,15 +491,6 @@ secretLoadAllConfigs(virSecretObjPtr *dest, VIR_FREE(path); VIR_FREE(base64path); } - /* Ignore error reported by readdir, if any. It's better to keep the - secrets we managed to find. */ - - while (list != NULL) { - virSecretObjPtr secret; - - secret = listUnlink(&list); - listInsert(dest, secret); - } closedir(dir); return 0; @@ -500,6 +502,7 @@ static int secretConnectNumOfSecrets(virConnectPtr conn) { size_t i; + int count = 0; virSecretObjPtr secret; if (virConnectNumOfSecretsEnsureACL(conn) < 0) @@ -507,15 +510,14 @@ secretConnectNumOfSecrets(virConnectPtr conn) secretDriverLock(); - i = 0; - for (secret = driver->secrets; secret != NULL; secret = secret->next) { - if (virConnectNumOfSecretsCheckACL(conn, - secret->def)) - i++; + for (i = 0; i < driver->secrets.count; i++) { + secret = driver->secrets.objs[i]; + if (virConnectNumOfSecretsCheckACL(conn, secret->def)) + count++; } secretDriverUnlock(); - return i; + return count; } static int @@ -524,6 +526,7 @@ secretConnectListSecrets(virConnectPtr conn, int maxuuids) { size_t i; + int count = 0; virSecretObjPtr secret; memset(uuids, 0, maxuuids * sizeof(*uuids)); @@ -533,23 +536,26 @@ secretConnectListSecrets(virConnectPtr conn, secretDriverLock(); - i = 0; - for (secret = driver->secrets; secret != NULL; secret = secret->next) { + for (i = 0; i < driver->secrets.count; i++) { char *uuidstr; - if (!virConnectListSecretsCheckACL(conn, - secret->def)) + + secret = driver->secrets.objs[i]; + if (!virConnectListSecretsCheckACL(conn, secret->def)) continue; - if (i == maxuuids) + + if (count == maxuuids) break; + if (VIR_ALLOC_N(uuidstr, VIR_UUID_STRING_BUFLEN) < 0) goto cleanup; + virUUIDFormat(secret->def->uuid, uuidstr); - uuids[i] = uuidstr; - i++; + uuids[count] = uuidstr; + count++; } secretDriverUnlock(); - return i; + return count; cleanup: secretDriverUnlock(); @@ -588,10 +594,9 @@ secretConnectListAllSecrets(virConnectPtr conn, unsigned int flags) { virSecretPtr *tmp_secrets = NULL; - int nsecrets = 0; int ret_nsecrets = 0; virSecretObjPtr secret = NULL; - size_t i = 0; + size_t i; int ret = -1; virCheckFlags(VIR_CONNECT_LIST_SECRETS_FILTERS_ALL, -1); @@ -601,15 +606,13 @@ secretConnectListAllSecrets(virConnectPtr conn, secretDriverLock(); - for (secret = driver->secrets; secret != NULL; secret = secret->next) - nsecrets++; - - if (secrets && VIR_ALLOC_N(tmp_secrets, nsecrets + 1) < 0) + if (secrets && VIR_ALLOC_N(tmp_secrets, driver->secrets.count + 1) < 0) goto cleanup; - for (secret = driver->secrets; secret != NULL; secret = secret->next) { - if (!virConnectListAllSecretsCheckACL(conn, - secret->def)) + for (i = 0; i < driver->secrets.count; i++) { + secret = driver->secrets.objs[i]; + + if (!virConnectListAllSecretsCheckACL(conn, secret->def)) continue; /* filter by whether it's ephemeral */ @@ -670,7 +673,7 @@ secretLookupByUUID(virConnectPtr conn, secretDriverLock(); - if (!(secret = secretFindByUUID(uuid))) { + if (!(secret = secretFindByUUID(&driver->secrets, uuid))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(uuid, uuidstr); virReportError(VIR_ERR_NO_SECRET, @@ -702,7 +705,7 @@ secretLookupByUsage(virConnectPtr conn, secretDriverLock(); - if (!(secret = secretFindByUsage(usageType, usageID))) { + if (!(secret = secretFindByUsage(&driver->secrets, usageType, usageID))) { virReportError(VIR_ERR_NO_SECRET, _("no secret with matching usage '%s'"), usageID); goto cleanup; @@ -742,14 +745,15 @@ secretDefineXML(virConnectPtr conn, if (virSecretDefineXMLEnsureACL(conn, new_attrs) < 0) goto cleanup; - if (!(secret = secretFindByUUID(new_attrs->uuid))) { + if (!(secret = secretFindByUUID(&driver->secrets, new_attrs->uuid))) { /* No existing secret with same UUID, * try look for matching usage instead */ const char *usageID = secretUsageIDForDef(new_attrs); char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(secret->def->uuid, uuidstr); - if ((secret = secretFindByUsage(new_attrs->usage_type, usageID))) { + if ((secret = secretFindByUsage(&driver->secrets, + new_attrs->usage_type, usageID))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("a secret with UUID %s already defined for " "use with %s"), @@ -757,7 +761,7 @@ secretDefineXML(virConnectPtr conn, goto cleanup; } - /* No existing secret at all, create one */ + /* No existing secret at all, create one, add to driver secrets list */ if (!(secret = secretAssignDef(&driver->secrets, new_attrs))) goto cleanup; @@ -765,14 +769,14 @@ secretDefineXML(virConnectPtr conn, * the uuidstr, and .xml suffix */ if (!(secret->configFile = virFileBuildPath(driver->configDir, uuidstr, ".xml"))) { - secretFree(secret); + secretObjRemove(&driver->secrets, secret); goto cleanup; } /* Generate base64File using driver->configDir, * the uuidstr, and .base64 suffix */ if (!(secret->base64File = virFileBuildPath(driver->configDir, uuidstr, ".base64"))) { - secretFree(secret); + secretObjRemove(&driver->secrets, secret); goto cleanup; } } else { @@ -830,12 +834,8 @@ secretDefineXML(virConnectPtr conn, /* Error - restore previous state and free new attributes */ secret->def = backup; } else { - /* "secret" was added to the head of the list above */ - if (listUnlink(&driver->secrets) != secret) - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("list of secrets is inconsistent")); - else - secretFree(secret); + /* "secret" was added to driver listthe head of the list above */ + secretObjRemove(&driver->secrets, secret); } cleanup: @@ -856,7 +856,7 @@ secretGetXMLDesc(virSecretPtr obj, secretDriverLock(); - if (!(secret = secretFindByUUID(obj->uuid))) { + if (!(secret = secretFindByUUID(&driver->secrets, obj->uuid))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(obj->uuid, uuidstr); virReportError(VIR_ERR_NO_SECRET, @@ -893,7 +893,7 @@ secretSetValue(virSecretPtr obj, secretDriverLock(); - if (!(secret = secretFindByUUID(obj->uuid))) { + if (!(secret = secretFindByUUID(&driver->secrets, obj->uuid))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(obj->uuid, uuidstr); virReportError(VIR_ERR_NO_SECRET, @@ -951,7 +951,7 @@ secretGetValue(virSecretPtr obj, secretDriverLock(); - if (!(secret = secretFindByUUID(obj->uuid))) { + if (!(secret = secretFindByUUID(&driver->secrets, obj->uuid))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(obj->uuid, uuidstr); virReportError(VIR_ERR_NO_SECRET, @@ -996,7 +996,7 @@ secretUndefine(virSecretPtr obj) secretDriverLock(); - if (!(secret = secretFindByUUID(obj->uuid))) { + if (!(secret = secretFindByUUID(&driver->secrets, obj->uuid))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(obj->uuid, uuidstr); virReportError(VIR_ERR_NO_SECRET, @@ -1011,8 +1011,7 @@ secretUndefine(virSecretPtr obj) secretDeleteSaved(secret) < 0) goto cleanup; - listUnlinkSecret(&driver->secrets, secret); - secretFree(secret); + secretObjRemove(&driver->secrets, secret); ret = 0; @@ -1030,12 +1029,7 @@ secretStateCleanup(void) secretDriverLock(); - while (driver->secrets != NULL) { - virSecretObjPtr secret; - - secret = listUnlink(&driver->secrets); - secretFree(secret); - } + secretObjListFree(&driver->secrets); VIR_FREE(driver->configDir); secretDriverUnlock(); @@ -1088,7 +1082,8 @@ secretStateInitialize(bool privileged, static int secretStateReload(void) { - virSecretObjPtr new_secrets = NULL; + size_t i; + virSecretObjList new_secrets = {0, NULL}; if (!driver) return -1; @@ -1101,15 +1096,24 @@ secretStateReload(void) /* Keep ephemeral secrets from current state. * Discard non-ephemeral secrets that were removed * by the secrets configDir. */ - while (driver->secrets != NULL) { - virSecretObjPtr secret; - - secret = listUnlink(&driver->secrets); - if (secret->def->ephemeral) - listInsert(&new_secrets, secret); - else - secretFree(secret); + for (i = 0; i < driver->secrets.count; i++) { + virSecretObjPtr secret = driver->secrets.objs[i]; + + if (secret->def->ephemeral) { + /* Remove from current, place on to be current soon */ + VIR_DELETE_ELEMENT(driver->secrets.objs, i, driver->secrets.count); + if (VIR_APPEND_ELEMENT(new_secrets.objs, new_secrets.count, + secret) < 0) { + virErrorPtr err = virGetLastError(); + + VIR_ERROR(_("Error reloading ephemeral secret: %s"), + err != NULL ? err->message : _("unknown error")); + virResetError(err); + } + } + secretFree(secret); } + secretObjListFree(&driver->secrets); driver->secrets = new_secrets; end: -- 2.5.0

On 02/26/2016 12:16 PM, Daniel P. Berrange wrote: Hmm.. Yeah - that's probably the better solution. Once this release closes I'll push patches 1-13 and rework this change to use secret objects similar to domain objects... Even though patch 12 introduces secretAssignDef that could conceptually go into secret_conf.c - I'll keep it in secret_driver.c for now and then when implementing a secret obj like domain obj a virSecret[Obj]AssignDef can be generated. It'll be temporary similar to listUnlinkSecret is/was. Thanks - John