[libvirt] [PATCH 00/14] Secret driver updates
This started as a 'simple' change to use virFileRewrite instead of replaceFile (as pointed out by jtomko in his reply to my review comments of the safewrite_str series:) http://www.redhat.com/archives/libvir-list/2016-February/msg01141.html However, as I was looking at the code I thought - well rather than using forward linked lists to store secrets, this code should use the "more normal" VIR_{APPEND|DELETE}_ELEMENT in order to manage the secret list. So I jumped into the rabbit hole and wound my way through the code to get it to a point where it'll look a whole lot like other drivers. FWIW: Not for *this* release, but hopefully the "next" release! John Ferlan (14): secret: Various formatting cleanups secret: Use virFileRewrite instead of replaceFile secret: Rename virSecretEntry secret: Remove local virSecretPtr 'secret' secret: Rename virSecretObjPtr 'entry' to 'secret' secret: Use 'secret' instead of 's' for variable name secret: Rename directory to configDir secret: Adjust logic to build file path in secretLoad secret: Create a 'configFile' in virSecretObj secret: Create a 'base64File' in virSecretObj secret: Introduce listUnlinkSecret secret: Introduce secretAssignDef secret: Rename loadSecrets secret: Introduce virSecretObjList src/secret/secret_driver.c | 640 ++++++++++++++++++++++----------------------- 1 file changed, 315 insertions(+), 325 deletions(-) -- 2.5.0
Rather than having it interspersed with other changes, do it once. Remove a couple ^L, 1 argument per line for functions, less than 80 chars per line, use of spacing between logical groups of code, use of one line if statements when doing fetch followed by comparison, use direct return when no cleanup to be done. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 146 ++++++++++++++++++++++++--------------------- 1 file changed, 77 insertions(+), 69 deletions(-) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index fcdff1b..13456e8 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -1,7 +1,7 @@ /* * secret_driver.c: local driver for secret manipulation API * - * Copyright (C) 2009-2014 Red Hat, Inc. + * Copyright (C) 2009-2016 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -50,7 +50,7 @@ VIR_LOG_INIT("secret.secret_driver"); enum { SECRET_MAX_XML_FILE = 10*1024*1024 }; - /* Internal driver state */ +/* Internal driver state */ typedef struct _virSecretEntry virSecretEntry; typedef virSecretEntry *virSecretEntryPtr; @@ -94,7 +94,8 @@ listUnlink(virSecretEntryPtr *pptr) } static void -listInsert(virSecretEntryPtr *pptr, virSecretEntryPtr secret) +listInsert(virSecretEntryPtr *pptr, + virSecretEntryPtr secret) { secret->next = *pptr; *pptr = secret; @@ -128,7 +129,8 @@ secretFindByUUID(const unsigned char *uuid) } static virSecretEntryPtr -secretFindByUsage(int usageType, const char *usageID) +secretFindByUsage(int usageType, + const char *usageID) { virSecretEntryPtr *pptr, s; @@ -170,7 +172,9 @@ secretFindByUsage(int usageType, const char *usageID) "$basename.base64". "$basename" is in both cases the base64-encoded UUID. */ static int -replaceFile(const char *filename, void *data, size_t size) +replaceFile(const char *filename, + void *data, + size_t size) { char *tmp_path = NULL; int fd = -1, ret = -1; @@ -217,14 +221,16 @@ replaceFile(const char *filename, void *data, size_t size) } static char * -secretComputePath(const virSecretEntry *secret, const char *suffix) +secretComputePath(const virSecretEntry *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->directory, uuidstr, suffix)); + ignore_value(virAsprintf(&ret, "%s/%s%s", driver->directory, + uuidstr, suffix)); return ret; } @@ -260,11 +266,10 @@ secretSaveDef(const virSecretEntry *secret) if (secretEnsureDirectory() < 0) goto cleanup; - filename = secretXMLPath(secret); - if (filename == NULL) + if (!(filename = secretXMLPath(secret))) goto cleanup; - xml = virSecretDefFormat(secret->def); - if (xml == NULL) + + if (!(xml = virSecretDefFormat(secret->def))) goto cleanup; if (replaceFile(filename, xml, strlen(xml)) < 0) @@ -290,9 +295,9 @@ secretSaveValue(const virSecretEntry *secret) if (secretEnsureDirectory() < 0) goto cleanup; - filename = secretBase64Path(secret); - if (filename == NULL) + if (!(filename = secretBase64Path(secret))) goto cleanup; + base64_encode_alloc((const char *)secret->value, secret->value_size, &base64); if (base64 == NULL) { @@ -317,11 +322,10 @@ secretDeleteSaved(const virSecretEntry *secret) char *xml_filename = NULL, *value_filename = NULL; int ret = -1; - xml_filename = secretXMLPath(secret); - if (xml_filename == NULL) + if (!(xml_filename = secretXMLPath(secret))) goto cleanup; - value_filename = secretBase64Path(secret); - if (value_filename == NULL) + + if (!(value_filename = secretBase64Path(secret))) goto cleanup; if (unlink(xml_filename) < 0 && errno != ENOENT) @@ -364,12 +368,10 @@ secretLoadValue(virSecretEntryPtr secret) char *filename = NULL, *contents = NULL, *value = NULL; size_t value_size; - filename = secretBase64Path(secret); - if (filename == NULL) + if (!(filename = secretBase64Path(secret))) goto cleanup; - fd = open(filename, O_RDONLY); - if (fd == -1) { + if ((fd = open(filename, O_RDONLY)) == -1) { if (errno == ENOENT) { ret = 0; goto cleanup; @@ -377,10 +379,12 @@ secretLoadValue(virSecretEntryPtr secret) virReportSystemError(errno, _("cannot open '%s'"), filename); goto cleanup; } + if (fstat(fd, &st) < 0) { virReportSystemError(errno, _("cannot stat '%s'"), filename); goto cleanup; } + if ((size_t)st.st_size != st.st_size) { virReportError(VIR_ERR_INTERNAL_ERROR, _("'%s' file does not fit in memory"), filename); @@ -389,10 +393,12 @@ secretLoadValue(virSecretEntryPtr secret) if (VIR_ALLOC_N(contents, st.st_size) < 0) goto cleanup; + if (saferead(fd, contents, st.st_size) != st.st_size) { virReportSystemError(errno, _("cannot read '%s'"), filename); goto cleanup; } + VIR_FORCE_CLOSE(fd); if (!base64_decode_alloc(contents, st.st_size, &value, &value_size)) { @@ -433,9 +439,10 @@ secretLoad(const char *xml_basename) if (virAsprintf(&xml_filename, "%s/%s", driver->directory, xml_basename) < 0) goto cleanup; - def = virSecretDefParseFile(xml_filename); - if (def == NULL) + + if (!(def = virSecretDefParseFile(xml_filename))) goto cleanup; + VIR_FREE(xml_filename); if (secretLoadValidateUUID(def, xml_basename) < 0) @@ -462,29 +469,28 @@ secretLoad(const char *xml_basename) static int loadSecrets(virSecretEntryPtr *dest) { - int ret = -1; DIR *dir = NULL; struct dirent *de; virSecretEntryPtr list = NULL; - dir = opendir(driver->directory); - if (dir == NULL) { + if (!(dir = opendir(driver->directory))) { if (errno == ENOENT) return 0; virReportSystemError(errno, _("cannot open '%s'"), driver->directory); - goto cleanup; + return -1; } + while (virDirRead(dir, &de, NULL) > 0) { virSecretEntryPtr secret; if (STREQ(de->d_name, ".") || STREQ(de->d_name, "..")) continue; + if (!virFileHasSuffix(de->d_name, ".xml")) continue; - secret = secretLoad(de->d_name); - if (secret == NULL) { + if (!(secret = secretLoad(de->d_name))) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Error reading secret: %s"), @@ -492,6 +498,7 @@ loadSecrets(virSecretEntryPtr *dest) virResetError(err); continue; } + listInsert(&list, secret); } /* Ignore error reported by readdir, if any. It's better to keep the @@ -504,15 +511,11 @@ loadSecrets(virSecretEntryPtr *dest) listInsert(dest, s); } - ret = 0; - - cleanup: - if (dir != NULL) - closedir(dir); - return ret; + closedir(dir); + return 0; } - /* Driver functions */ +/* Driver functions */ static int secretConnectNumOfSecrets(virConnectPtr conn) @@ -537,7 +540,9 @@ secretConnectNumOfSecrets(virConnectPtr conn) } static int -secretConnectListSecrets(virConnectPtr conn, char **uuids, int maxuuids) +secretConnectListSecrets(virConnectPtr conn, + char **uuids, + int maxuuids) { size_t i; virSecretEntryPtr secret; @@ -679,15 +684,15 @@ secretConnectListAllSecrets(virConnectPtr conn, static virSecretPtr -secretLookupByUUID(virConnectPtr conn, const unsigned char *uuid) +secretLookupByUUID(virConnectPtr conn, + const unsigned char *uuid) { virSecretPtr ret = NULL; virSecretEntryPtr secret; secretDriverLock(); - secret = secretFindByUUID(uuid); - if (secret == NULL) { + if (!(secret = secretFindByUUID(uuid))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(uuid, uuidstr); virReportError(VIR_ERR_NO_SECRET, @@ -710,15 +715,16 @@ secretLookupByUUID(virConnectPtr conn, const unsigned char *uuid) static virSecretPtr -secretLookupByUsage(virConnectPtr conn, int usageType, const char *usageID) +secretLookupByUsage(virConnectPtr conn, + int usageType, + const char *usageID) { virSecretPtr ret = NULL; virSecretEntryPtr secret; secretDriverLock(); - secret = secretFindByUsage(usageType, usageID); - if (secret == NULL) { + if (!(secret = secretFindByUsage(usageType, usageID))) { virReportError(VIR_ERR_NO_SECRET, _("no secret with matching usage '%s'"), usageID); goto cleanup; @@ -739,7 +745,8 @@ secretLookupByUsage(virConnectPtr conn, int usageType, const char *usageID) static virSecretPtr -secretDefineXML(virConnectPtr conn, const char *xml, +secretDefineXML(virConnectPtr conn, + const char *xml, unsigned int flags) { virSecretPtr ret = NULL; @@ -749,8 +756,7 @@ secretDefineXML(virConnectPtr conn, const char *xml, virCheckFlags(0, NULL); - new_attrs = virSecretDefParseString(xml); - if (new_attrs == NULL) + if (!(new_attrs = virSecretDefParseString(xml))) return NULL; secretDriverLock(); @@ -758,16 +764,16 @@ secretDefineXML(virConnectPtr conn, const char *xml, if (virSecretDefineXMLEnsureACL(conn, new_attrs) < 0) goto cleanup; - secret = secretFindByUUID(new_attrs->uuid); - if (secret == NULL) { - /* No existing secret with same UUID, try look for matching usage instead */ + if (!(secret = secretFindByUUID(new_attrs->uuid))) { + /* No existing secret with same UUID, + * try look for matching usage instead */ const char *usageID = secretUsageIDForDef(new_attrs); - secret = secretFindByUsage(new_attrs->usage_type, usageID); - if (secret) { + if ((secret = secretFindByUsage(new_attrs->usage_type, usageID))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(secret->def->uuid, uuidstr); virReportError(VIR_ERR_INTERNAL_ERROR, - _("a secret with UUID %s already defined for use with %s"), + _("a secret with UUID %s already defined for " + "use with %s"), uuidstr, usageID); goto cleanup; } @@ -785,7 +791,8 @@ secretDefineXML(virConnectPtr conn, const char *xml, char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(secret->def->uuid, uuidstr); virReportError(VIR_ERR_INTERNAL_ERROR, - _("a secret with UUID %s is already defined for use with %s"), + _("a secret with UUID %s is already defined " + "for use with %s"), uuidstr, oldUsageID); goto cleanup; } @@ -853,7 +860,8 @@ secretDefineXML(virConnectPtr conn, const char *xml, } static char * -secretGetXMLDesc(virSecretPtr obj, unsigned int flags) +secretGetXMLDesc(virSecretPtr obj, + unsigned int flags) { char *ret = NULL; virSecretEntryPtr secret; @@ -862,8 +870,7 @@ secretGetXMLDesc(virSecretPtr obj, unsigned int flags) secretDriverLock(); - secret = secretFindByUUID(obj->uuid); - if (secret == NULL) { + if (!(secret = secretFindByUUID(obj->uuid))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(obj->uuid, uuidstr); virReportError(VIR_ERR_NO_SECRET, @@ -883,8 +890,10 @@ secretGetXMLDesc(virSecretPtr obj, unsigned int flags) } static int -secretSetValue(virSecretPtr obj, const unsigned char *value, - size_t value_size, unsigned int flags) +secretSetValue(virSecretPtr obj, + const unsigned char *value, + size_t value_size, + unsigned int flags) { int ret = -1; unsigned char *old_value, *new_value; @@ -898,8 +907,7 @@ secretSetValue(virSecretPtr obj, const unsigned char *value, secretDriverLock(); - secret = secretFindByUUID(obj->uuid); - if (secret == NULL) { + if (!(secret = secretFindByUUID(obj->uuid))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(obj->uuid, uuidstr); virReportError(VIR_ERR_NO_SECRET, @@ -945,7 +953,9 @@ secretSetValue(virSecretPtr obj, const unsigned char *value, } static unsigned char * -secretGetValue(virSecretPtr obj, size_t *value_size, unsigned int flags, +secretGetValue(virSecretPtr obj, + size_t *value_size, + unsigned int flags, unsigned int internalFlags) { unsigned char *ret = NULL; @@ -955,8 +965,7 @@ secretGetValue(virSecretPtr obj, size_t *value_size, unsigned int flags, secretDriverLock(); - secret = secretFindByUUID(obj->uuid); - if (secret == NULL) { + if (!(secret = secretFindByUUID(obj->uuid))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(obj->uuid, uuidstr); virReportError(VIR_ERR_NO_SECRET, @@ -1001,8 +1010,7 @@ secretUndefine(virSecretPtr obj) secretDriverLock(); - secret = secretFindByUUID(obj->uuid); - if (secret == NULL) { + if (!(secret = secretFindByUUID(obj->uuid))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(obj->uuid, uuidstr); virReportError(VIR_ERR_NO_SECRET, @@ -1079,8 +1087,7 @@ secretStateInitialize(bool privileged, if (VIR_STRDUP(base, SYSCONFDIR "/libvirt") < 0) goto error; } else { - base = virGetUserConfigDirectory(); - if (!base) + if (!(base = virGetUserConfigDirectory())) goto error; } if (virAsprintf(&driver->directory, "%s/secrets", base) < 0) @@ -1113,8 +1120,9 @@ secretStateReload(void) if (loadSecrets(&new_secrets) < 0) goto end; - /* Keep ephemeral secrets from current state. Discard non-ephemeral secrets - that were removed by the secrets directory. */ + /* Keep ephemeral secrets from current state. + * Discard non-ephemeral secrets that were removed + * by the secrets directory. */ while (driver->secrets != NULL) { virSecretEntryPtr s; -- 2.5.0
On Thu, Feb 25, 2016 at 09:03:05AM -0500, John Ferlan wrote:
Rather than having it interspersed with other changes, do it once.
Remove a couple ^L, 1 argument per line for functions, less than 80 chars per line, use of spacing between logical groups of code, use of one line if statements when doing fetch followed by comparison, use direct return when no cleanup to be done.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 146 ++++++++++++++++++++++++--------------------- 1 file changed, 77 insertions(+), 69 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Use the common API instead of essentially open coding same functionality. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 55 +++++++++------------------------------------- 1 file changed, 10 insertions(+), 45 deletions(-) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 13456e8..4f4582a 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -172,52 +172,15 @@ secretFindByUsage(int usageType, "$basename.base64". "$basename" is in both cases the base64-encoded UUID. */ static int -replaceFile(const char *filename, - void *data, - size_t size) +secretRewriteFile(int fd, + void *opaque) { - char *tmp_path = NULL; - int fd = -1, ret = -1; + char *data = opaque; - if (virAsprintf(&tmp_path, "%sXXXXXX", filename) < 0) - goto cleanup; - fd = mkostemp(tmp_path, O_CLOEXEC); - if (fd == -1) { - virReportSystemError(errno, _("mkostemp('%s') failed"), tmp_path); - goto cleanup; - } - if (fchmod(fd, S_IRUSR | S_IWUSR) != 0) { - virReportSystemError(errno, _("fchmod('%s') failed"), tmp_path); - goto cleanup; - } - - ret = safewrite(fd, data, size); - if (ret < 0) { - virReportSystemError(errno, _("error writing to '%s'"), - tmp_path); - goto cleanup; - } - if (VIR_CLOSE(fd) < 0) { - virReportSystemError(errno, _("error closing '%s'"), tmp_path); - goto cleanup; - } - fd = -1; - - if (rename(tmp_path, filename) < 0) { - virReportSystemError(errno, _("rename(%s, %s) failed"), tmp_path, - filename); - goto cleanup; - } - VIR_FREE(tmp_path); - ret = 0; + if (safewrite(fd, data, strlen(data)) < 0) + return -1; - cleanup: - VIR_FORCE_CLOSE(fd); - if (tmp_path != NULL) { - unlink(tmp_path); - VIR_FREE(tmp_path); - } - return ret; + return 0; } static char * @@ -272,7 +235,8 @@ secretSaveDef(const virSecretEntry *secret) if (!(xml = virSecretDefFormat(secret->def))) goto cleanup; - if (replaceFile(filename, xml, strlen(xml)) < 0) + if (virFileRewrite(filename, S_IRUSR | S_IWUSR, + secretRewriteFile, xml) < 0) goto cleanup; ret = 0; @@ -305,7 +269,8 @@ secretSaveValue(const virSecretEntry *secret) goto cleanup; } - if (replaceFile(filename, base64, strlen(base64)) < 0) + if (virFileRewrite(filename, S_IRUSR | S_IWUSR, + secretRewriteFile, base64) < 0) goto cleanup; ret = 0; -- 2.5.0
On Thu, Feb 25, 2016 at 09:03:06AM -0500, John Ferlan wrote:
Use the common API instead of essentially open coding same functionality.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 55 +++++++++------------------------------------- 1 file changed, 10 insertions(+), 45 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Rename to virSecretObj - preparation for future patch, but also follows similar code in other drivers. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 84 +++++++++++++++++++++++----------------------- 1 file changed, 42 insertions(+), 42 deletions(-) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 4f4582a..27c3f9e 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -52,10 +52,10 @@ enum { SECRET_MAX_XML_FILE = 10*1024*1024 }; /* Internal driver state */ -typedef struct _virSecretEntry virSecretEntry; -typedef virSecretEntry *virSecretEntryPtr; -struct _virSecretEntry { - virSecretEntryPtr next; +typedef struct _virSecretObj virSecretObj; +typedef virSecretObj *virSecretObjPtr; +struct _virSecretObj { + virSecretObjPtr next; virSecretDefPtr def; unsigned char *value; /* May be NULL */ size_t value_size; @@ -65,7 +65,7 @@ typedef struct _virSecretDriverState virSecretDriverState; typedef virSecretDriverState *virSecretDriverStatePtr; struct _virSecretDriverState { virMutex lock; - virSecretEntry *secrets; + virSecretObj *secrets; char *directory; }; @@ -83,10 +83,10 @@ secretDriverUnlock(void) virMutexUnlock(&driver->lock); } -static virSecretEntryPtr -listUnlink(virSecretEntryPtr *pptr) +static virSecretObjPtr +listUnlink(virSecretObjPtr *pptr) { - virSecretEntryPtr secret; + virSecretObjPtr secret; secret = *pptr; *pptr = secret->next; @@ -94,15 +94,15 @@ listUnlink(virSecretEntryPtr *pptr) } static void -listInsert(virSecretEntryPtr *pptr, - virSecretEntryPtr secret) +listInsert(virSecretObjPtr *pptr, + virSecretObjPtr secret) { secret->next = *pptr; *pptr = secret; } static void -secretFree(virSecretEntryPtr secret) +secretFree(virSecretObjPtr secret) { if (secret == NULL) return; @@ -115,10 +115,10 @@ secretFree(virSecretEntryPtr secret) VIR_FREE(secret); } -static virSecretEntryPtr +static virSecretObjPtr secretFindByUUID(const unsigned char *uuid) { - virSecretEntryPtr *pptr, s; + virSecretObjPtr *pptr, s; for (pptr = &driver->secrets; *pptr != NULL; pptr = &s->next) { s = *pptr; @@ -128,11 +128,11 @@ secretFindByUUID(const unsigned char *uuid) return NULL; } -static virSecretEntryPtr +static virSecretObjPtr secretFindByUsage(int usageType, const char *usageID) { - virSecretEntryPtr *pptr, s; + virSecretObjPtr *pptr, s; for (pptr = &driver->secrets; *pptr != NULL; pptr = &s->next) { s = *pptr; @@ -184,7 +184,7 @@ secretRewriteFile(int fd, } static char * -secretComputePath(const virSecretEntry *secret, +secretComputePath(const virSecretObj *secret, const char *suffix) { char *ret; @@ -198,13 +198,13 @@ secretComputePath(const virSecretEntry *secret, } static char * -secretXMLPath(const virSecretEntry *secret) +secretXMLPath(const virSecretObj *secret) { return secretComputePath(secret, ".xml"); } static char * -secretBase64Path(const virSecretEntry *secret) +secretBase64Path(const virSecretObj *secret) { return secretComputePath(secret, ".base64"); } @@ -221,7 +221,7 @@ secretEnsureDirectory(void) } static int -secretSaveDef(const virSecretEntry *secret) +secretSaveDef(const virSecretObj *secret) { char *filename = NULL, *xml = NULL; int ret = -1; @@ -248,7 +248,7 @@ secretSaveDef(const virSecretEntry *secret) } static int -secretSaveValue(const virSecretEntry *secret) +secretSaveValue(const virSecretObj *secret) { char *filename = NULL, *base64 = NULL; int ret = -1; @@ -282,7 +282,7 @@ secretSaveValue(const virSecretEntry *secret) } static int -secretDeleteSaved(const virSecretEntry *secret) +secretDeleteSaved(const virSecretObj *secret) { char *xml_filename = NULL, *value_filename = NULL; int ret = -1; @@ -326,7 +326,7 @@ secretLoadValidateUUID(virSecretDefPtr def, } static int -secretLoadValue(virSecretEntryPtr secret) +secretLoadValue(virSecretObjPtr secret) { int ret = -1, fd = -1; struct stat st; @@ -394,11 +394,11 @@ secretLoadValue(virSecretEntryPtr secret) return ret; } -static virSecretEntryPtr +static virSecretObjPtr secretLoad(const char *xml_basename) { virSecretDefPtr def = NULL; - virSecretEntryPtr secret = NULL, ret = NULL; + virSecretObjPtr secret = NULL, ret = NULL; char *xml_filename; if (virAsprintf(&xml_filename, "%s/%s", driver->directory, @@ -432,11 +432,11 @@ secretLoad(const char *xml_basename) } static int -loadSecrets(virSecretEntryPtr *dest) +loadSecrets(virSecretObjPtr *dest) { DIR *dir = NULL; struct dirent *de; - virSecretEntryPtr list = NULL; + virSecretObjPtr list = NULL; if (!(dir = opendir(driver->directory))) { if (errno == ENOENT) @@ -447,7 +447,7 @@ loadSecrets(virSecretEntryPtr *dest) } while (virDirRead(dir, &de, NULL) > 0) { - virSecretEntryPtr secret; + virSecretObjPtr secret; if (STREQ(de->d_name, ".") || STREQ(de->d_name, "..")) continue; @@ -470,7 +470,7 @@ loadSecrets(virSecretEntryPtr *dest) secrets we managed to find. */ while (list != NULL) { - virSecretEntryPtr s; + virSecretObjPtr s; s = listUnlink(&list); listInsert(dest, s); @@ -486,7 +486,7 @@ static int secretConnectNumOfSecrets(virConnectPtr conn) { size_t i; - virSecretEntryPtr secret; + virSecretObjPtr secret; if (virConnectNumOfSecretsEnsureACL(conn) < 0) return -1; @@ -510,7 +510,7 @@ secretConnectListSecrets(virConnectPtr conn, int maxuuids) { size_t i; - virSecretEntryPtr secret; + virSecretObjPtr secret; memset(uuids, 0, maxuuids * sizeof(*uuids)); @@ -577,7 +577,7 @@ secretConnectListAllSecrets(virConnectPtr conn, int nsecrets = 0; int ret_nsecrets = 0; virSecretPtr secret = NULL; - virSecretEntryPtr entry = NULL; + virSecretObjPtr entry = NULL; size_t i = 0; int ret = -1; @@ -653,7 +653,7 @@ secretLookupByUUID(virConnectPtr conn, const unsigned char *uuid) { virSecretPtr ret = NULL; - virSecretEntryPtr secret; + virSecretObjPtr secret; secretDriverLock(); @@ -685,7 +685,7 @@ secretLookupByUsage(virConnectPtr conn, const char *usageID) { virSecretPtr ret = NULL; - virSecretEntryPtr secret; + virSecretObjPtr secret; secretDriverLock(); @@ -715,7 +715,7 @@ secretDefineXML(virConnectPtr conn, unsigned int flags) { virSecretPtr ret = NULL; - virSecretEntryPtr secret; + virSecretObjPtr secret; virSecretDefPtr backup = NULL; virSecretDefPtr new_attrs; @@ -829,7 +829,7 @@ secretGetXMLDesc(virSecretPtr obj, unsigned int flags) { char *ret = NULL; - virSecretEntryPtr secret; + virSecretObjPtr secret; virCheckFlags(0, NULL); @@ -863,7 +863,7 @@ secretSetValue(virSecretPtr obj, int ret = -1; unsigned char *old_value, *new_value; size_t old_value_size; - virSecretEntryPtr secret; + virSecretObjPtr secret; virCheckFlags(0, -1); @@ -924,7 +924,7 @@ secretGetValue(virSecretPtr obj, unsigned int internalFlags) { unsigned char *ret = NULL; - virSecretEntryPtr secret; + virSecretObjPtr secret; virCheckFlags(0, NULL); @@ -971,7 +971,7 @@ static int secretUndefine(virSecretPtr obj) { int ret = -1; - virSecretEntryPtr secret; + virSecretObjPtr secret; secretDriverLock(); @@ -993,7 +993,7 @@ secretUndefine(virSecretPtr obj) if (driver->secrets == secret) { driver->secrets = secret->next; } else { - virSecretEntryPtr tmp = driver->secrets; + virSecretObjPtr tmp = driver->secrets; while (tmp && tmp->next != secret) tmp = tmp->next; if (tmp) @@ -1018,7 +1018,7 @@ secretStateCleanup(void) secretDriverLock(); while (driver->secrets != NULL) { - virSecretEntryPtr s; + virSecretObjPtr s; s = listUnlink(&driver->secrets); secretFree(s); @@ -1075,7 +1075,7 @@ secretStateInitialize(bool privileged, static int secretStateReload(void) { - virSecretEntryPtr new_secrets = NULL; + virSecretObjPtr new_secrets = NULL; if (!driver) return -1; @@ -1089,7 +1089,7 @@ secretStateReload(void) * Discard non-ephemeral secrets that were removed * by the secrets directory. */ while (driver->secrets != NULL) { - virSecretEntryPtr s; + virSecretObjPtr s; s = listUnlink(&driver->secrets); if (s->def->ephemeral) -- 2.5.0
On Thu, Feb 25, 2016 at 09:03:07AM -0500, John Ferlan wrote:
Rename to virSecretObj - preparation for future patch, but also follows similar code in other drivers.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 84 +++++++++++++++++++++++----------------------- 1 file changed, 42 insertions(+), 42 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Remove the need for the local 'secret' in secretConnectListAllSecrets. A subsequent patch will rename the ObjPtr entry to secret. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 27c3f9e..07b3257 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -576,7 +576,6 @@ secretConnectListAllSecrets(virConnectPtr conn, virSecretPtr *tmp_secrets = NULL; int nsecrets = 0; int ret_nsecrets = 0; - virSecretPtr secret = NULL; virSecretObjPtr entry = NULL; size_t i = 0; int ret = -1; @@ -616,12 +615,12 @@ secretConnectListAllSecrets(virConnectPtr conn, continue; if (secrets) { - if (!(secret = virGetSecret(conn, - entry->def->uuid, - entry->def->usage_type, - secretUsageIDForDef(entry->def)))) + if (!(tmp_secrets[ret_nsecrets] = + virGetSecret(conn, + entry->def->uuid, + entry->def->usage_type, + secretUsageIDForDef(entry->def)))) goto cleanup; - tmp_secrets[ret_nsecrets] = secret; } ret_nsecrets++; } -- 2.5.0
On Thu, Feb 25, 2016 at 09:03:08AM -0500, John Ferlan wrote:
Remove the need for the local 'secret' in secretConnectListAllSecrets. A subsequent patch will rename the ObjPtr entry to secret.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Just renaming the variable in secretConnectListAllSecrets. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 07b3257..4ee8ece 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -576,7 +576,7 @@ secretConnectListAllSecrets(virConnectPtr conn, virSecretPtr *tmp_secrets = NULL; int nsecrets = 0; int ret_nsecrets = 0; - virSecretObjPtr entry = NULL; + virSecretObjPtr secret = NULL; size_t i = 0; int ret = -1; @@ -587,39 +587,39 @@ secretConnectListAllSecrets(virConnectPtr conn, secretDriverLock(); - for (entry = driver->secrets; entry != NULL; entry = entry->next) + for (secret = driver->secrets; secret != NULL; secret = secret->next) nsecrets++; if (secrets && VIR_ALLOC_N(tmp_secrets, nsecrets + 1) < 0) goto cleanup; - for (entry = driver->secrets; entry != NULL; entry = entry->next) { + for (secret = driver->secrets; secret != NULL; secret = secret->next) { if (!virConnectListAllSecretsCheckACL(conn, - entry->def)) + secret->def)) continue; /* filter by whether it's ephemeral */ if (MATCH(VIR_CONNECT_LIST_SECRETS_FILTERS_EPHEMERAL) && !((MATCH(VIR_CONNECT_LIST_SECRETS_EPHEMERAL) && - entry->def->ephemeral) || + secret->def->ephemeral) || (MATCH(VIR_CONNECT_LIST_SECRETS_NO_EPHEMERAL) && - !entry->def->ephemeral))) + !secret->def->ephemeral))) continue; /* filter by whether it's private */ if (MATCH(VIR_CONNECT_LIST_SECRETS_FILTERS_PRIVATE) && !((MATCH(VIR_CONNECT_LIST_SECRETS_PRIVATE) && - entry->def->private) || + secret->def->private) || (MATCH(VIR_CONNECT_LIST_SECRETS_NO_PRIVATE) && - !entry->def->private))) + !secret->def->private))) continue; if (secrets) { if (!(tmp_secrets[ret_nsecrets] = virGetSecret(conn, - entry->def->uuid, - entry->def->usage_type, - secretUsageIDForDef(entry->def)))) + secret->def->uuid, + secret->def->usage_type, + secretUsageIDForDef(secret->def)))) goto cleanup; } ret_nsecrets++; -- 2.5.0
On Thu, Feb 25, 2016 at 09:03:09AM -0500, John Ferlan wrote:
Just renaming the variable in secretConnectListAllSecrets.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Remove one letter variable. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 52 +++++++++++++++++++++++----------------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 4ee8ece..c2bd72a 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -118,12 +118,12 @@ secretFree(virSecretObjPtr secret) static virSecretObjPtr secretFindByUUID(const unsigned char *uuid) { - virSecretObjPtr *pptr, s; + virSecretObjPtr *pptr, secret; - for (pptr = &driver->secrets; *pptr != NULL; pptr = &s->next) { - s = *pptr; - if (memcmp(s->def->uuid, uuid, VIR_UUID_BUFLEN) == 0) - return s; + for (pptr = &driver->secrets; *pptr != NULL; pptr = &secret->next) { + secret = *pptr; + if (memcmp(secret->def->uuid, uuid, VIR_UUID_BUFLEN) == 0) + return secret; } return NULL; } @@ -132,12 +132,12 @@ static virSecretObjPtr secretFindByUsage(int usageType, const char *usageID) { - virSecretObjPtr *pptr, s; + virSecretObjPtr *pptr, secret; - for (pptr = &driver->secrets; *pptr != NULL; pptr = &s->next) { - s = *pptr; + for (pptr = &driver->secrets; *pptr != NULL; pptr = &secret->next) { + secret = *pptr; - if (s->def->usage_type != usageType) + if (secret->def->usage_type != usageType) continue; switch (usageType) { @@ -146,18 +146,18 @@ secretFindByUsage(int usageType, break; case VIR_SECRET_USAGE_TYPE_VOLUME: - if (STREQ(s->def->usage.volume, usageID)) - return s; + if (STREQ(secret->def->usage.volume, usageID)) + return secret; break; case VIR_SECRET_USAGE_TYPE_CEPH: - if (STREQ(s->def->usage.ceph, usageID)) - return s; + if (STREQ(secret->def->usage.ceph, usageID)) + return secret; break; case VIR_SECRET_USAGE_TYPE_ISCSI: - if (STREQ(s->def->usage.target, usageID)) - return s; + if (STREQ(secret->def->usage.target, usageID)) + return secret; break; } } @@ -470,10 +470,10 @@ loadSecrets(virSecretObjPtr *dest) secrets we managed to find. */ while (list != NULL) { - virSecretObjPtr s; + virSecretObjPtr secret; - s = listUnlink(&list); - listInsert(dest, s); + secret = listUnlink(&list); + listInsert(dest, secret); } closedir(dir); @@ -1017,10 +1017,10 @@ secretStateCleanup(void) secretDriverLock(); while (driver->secrets != NULL) { - virSecretObjPtr s; + virSecretObjPtr secret; - s = listUnlink(&driver->secrets); - secretFree(s); + secret = listUnlink(&driver->secrets); + secretFree(secret); } VIR_FREE(driver->directory); @@ -1088,13 +1088,13 @@ secretStateReload(void) * Discard non-ephemeral secrets that were removed * by the secrets directory. */ while (driver->secrets != NULL) { - virSecretObjPtr s; + virSecretObjPtr secret; - s = listUnlink(&driver->secrets); - if (s->def->ephemeral) - listInsert(&new_secrets, s); + secret = listUnlink(&driver->secrets); + if (secret->def->ephemeral) + listInsert(&new_secrets, secret); else - secretFree(s); + secretFree(secret); } driver->secrets = new_secrets; -- 2.5.0
On Thu, Feb 25, 2016 at 09:03:10AM -0500, John Ferlan wrote:
Remove one letter variable.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 52 +++++++++++++++++++++++----------------------- 1 file changed, 26 insertions(+), 26 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
This follows other drivers usage model. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index c2bd72a..abeefc3 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -66,7 +66,7 @@ typedef virSecretDriverState *virSecretDriverStatePtr; struct _virSecretDriverState { virMutex lock; virSecretObj *secrets; - char *directory; + char *configDir; }; static virSecretDriverStatePtr driver; @@ -166,7 +166,7 @@ secretFindByUsage(int usageType, /* Permament secret storage */ -/* Secrets are stored in virSecretDriverStatePtr->directory. Each secret +/* Secrets are stored in virSecretDriverStatePtr->configDir. Each secret has virSecretDef stored as XML in "$basename.xml". If a value of the secret is defined, it is stored as base64 (with no formatting) in "$basename.base64". "$basename" is in both cases the base64-encoded UUID. */ @@ -192,7 +192,7 @@ secretComputePath(const virSecretObj *secret, virUUIDFormat(secret->def->uuid, uuidstr); - ignore_value(virAsprintf(&ret, "%s/%s%s", driver->directory, + ignore_value(virAsprintf(&ret, "%s/%s%s", driver->configDir, uuidstr, suffix)); return ret; } @@ -212,9 +212,9 @@ secretBase64Path(const virSecretObj *secret) static int secretEnsureDirectory(void) { - if (mkdir(driver->directory, S_IRWXU) < 0 && errno != EEXIST) { + if (mkdir(driver->configDir, S_IRWXU) < 0 && errno != EEXIST) { virReportSystemError(errno, _("cannot create '%s'"), - driver->directory); + driver->configDir); return -1; } return 0; @@ -401,7 +401,7 @@ secretLoad(const char *xml_basename) virSecretObjPtr secret = NULL, ret = NULL; char *xml_filename; - if (virAsprintf(&xml_filename, "%s/%s", driver->directory, + if (virAsprintf(&xml_filename, "%s/%s", driver->configDir, xml_basename) < 0) goto cleanup; @@ -438,11 +438,11 @@ loadSecrets(virSecretObjPtr *dest) struct dirent *de; virSecretObjPtr list = NULL; - if (!(dir = opendir(driver->directory))) { + if (!(dir = opendir(driver->configDir))) { if (errno == ENOENT) return 0; virReportSystemError(errno, _("cannot open '%s'"), - driver->directory); + driver->configDir); return -1; } @@ -1022,7 +1022,7 @@ secretStateCleanup(void) secret = listUnlink(&driver->secrets); secretFree(secret); } - VIR_FREE(driver->directory); + VIR_FREE(driver->configDir); secretDriverUnlock(); virMutexDestroy(&driver->lock); @@ -1054,7 +1054,7 @@ secretStateInitialize(bool privileged, if (!(base = virGetUserConfigDirectory())) goto error; } - if (virAsprintf(&driver->directory, "%s/secrets", base) < 0) + if (virAsprintf(&driver->configDir, "%s/secrets", base) < 0) goto error; VIR_FREE(base); @@ -1086,7 +1086,7 @@ secretStateReload(void) /* Keep ephemeral secrets from current state. * Discard non-ephemeral secrets that were removed - * by the secrets directory. */ + * by the secrets configDir. */ while (driver->secrets != NULL) { virSecretObjPtr secret; -- 2.5.0
On Thu, Feb 25, 2016 at 09:03:11AM -0500, John Ferlan wrote:
This follows other drivers usage model.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
The 'secretLoad' was essentially open coding virFileBuildPath. Adjust the logic to have the caller build the path and pass it. The net sum of ignoring the virFileBuildPath failure is the same as before where the failure to virAsprintf the path would have been ignored anyway in the secretLoad error path. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index abeefc3..4dee2a6 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -309,16 +309,16 @@ secretDeleteSaved(const virSecretObj *secret) static int secretLoadValidateUUID(virSecretDefPtr def, - const char *xml_basename) + const char *file) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(def->uuid, uuidstr); - if (!virFileMatchesNameSuffix(xml_basename, uuidstr, ".xml")) { + if (!virFileMatchesNameSuffix(file, uuidstr, ".xml")) { virReportError(VIR_ERR_INTERNAL_ERROR, _("<uuid> does not match secret file name '%s'"), - xml_basename); + file); return -1; } @@ -395,22 +395,16 @@ secretLoadValue(virSecretObjPtr secret) } static virSecretObjPtr -secretLoad(const char *xml_basename) +secretLoad(const char *file, + const char *path) { virSecretDefPtr def = NULL; virSecretObjPtr secret = NULL, ret = NULL; - char *xml_filename; - if (virAsprintf(&xml_filename, "%s/%s", driver->configDir, - xml_basename) < 0) + if (!(def = virSecretDefParseFile(path))) goto cleanup; - if (!(def = virSecretDefParseFile(xml_filename))) - goto cleanup; - - VIR_FREE(xml_filename); - - if (secretLoadValidateUUID(def, xml_basename) < 0) + if (secretLoadValidateUUID(def, file) < 0) goto cleanup; if (VIR_ALLOC(secret) < 0) @@ -427,7 +421,6 @@ secretLoad(const char *xml_basename) cleanup: secretFree(secret); virSecretDefFree(def); - VIR_FREE(xml_filename); return ret; } @@ -447,6 +440,7 @@ loadSecrets(virSecretObjPtr *dest) } while (virDirRead(dir, &de, NULL) > 0) { + char *path; virSecretObjPtr secret; if (STREQ(de->d_name, ".") || STREQ(de->d_name, "..")) @@ -455,15 +449,20 @@ loadSecrets(virSecretObjPtr *dest) if (!virFileHasSuffix(de->d_name, ".xml")) continue; - if (!(secret = secretLoad(de->d_name))) { + if (!(path = virFileBuildPath(driver->configDir, de->d_name, NULL))) + continue; + + if (!(secret = secretLoad(de->d_name, path))) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Error reading secret: %s"), err != NULL ? err->message: _("unknown error")); virResetError(err); + VIR_FREE(path); continue; } + VIR_FREE(path); listInsert(&list, secret); } /* Ignore error reported by readdir, if any. It's better to keep the -- 2.5.0
On Thu, Feb 25, 2016 at 09:03:12AM -0500, John Ferlan wrote:
The 'secretLoad' was essentially open coding virFileBuildPath.
Adjust the logic to have the caller build the path and pass it. The net sum of ignoring the virFileBuildPath failure is the same as before where the failure to virAsprintf the path would have been ignored anyway in the secretLoad error path.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
This patch removes the need for secretXMLPath. Instead save 'path' during loadSecret as 'configFile'. The secretXMLPath is nothing more than an open coded virFileBuildPath. All that code did was concantenate the driver->configDir, the UUID of the secret, and the ".xml" suffix to form the configFile name which we now will generate and save instead. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 39 ++++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 4dee2a6..d4b0207 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -56,6 +56,7 @@ typedef struct _virSecretObj virSecretObj; typedef virSecretObj *virSecretObjPtr; struct _virSecretObj { virSecretObjPtr next; + char *configFile; virSecretDefPtr def; unsigned char *value; /* May be NULL */ size_t value_size; @@ -112,6 +113,7 @@ secretFree(virSecretObjPtr secret) memset(secret->value, 0, secret->value_size); VIR_FREE(secret->value); } + VIR_FREE(secret->configFile); VIR_FREE(secret); } @@ -197,11 +199,6 @@ secretComputePath(const virSecretObj *secret, return ret; } -static char * -secretXMLPath(const virSecretObj *secret) -{ - return secretComputePath(secret, ".xml"); -} static char * secretBase64Path(const virSecretObj *secret) @@ -223,19 +220,16 @@ secretEnsureDirectory(void) static int secretSaveDef(const virSecretObj *secret) { - char *filename = NULL, *xml = NULL; + char *xml = NULL; int ret = -1; if (secretEnsureDirectory() < 0) goto cleanup; - if (!(filename = secretXMLPath(secret))) - goto cleanup; - if (!(xml = virSecretDefFormat(secret->def))) goto cleanup; - if (virFileRewrite(filename, S_IRUSR | S_IWUSR, + if (virFileRewrite(secret->configFile, S_IRUSR | S_IWUSR, secretRewriteFile, xml) < 0) goto cleanup; @@ -243,7 +237,6 @@ secretSaveDef(const virSecretObj *secret) cleanup: VIR_FREE(xml); - VIR_FREE(filename); return ret; } @@ -284,16 +277,13 @@ secretSaveValue(const virSecretObj *secret) static int secretDeleteSaved(const virSecretObj *secret) { - char *xml_filename = NULL, *value_filename = NULL; + char *value_filename = NULL; int ret = -1; - if (!(xml_filename = secretXMLPath(secret))) - goto cleanup; - if (!(value_filename = secretBase64Path(secret))) goto cleanup; - if (unlink(xml_filename) < 0 && errno != ENOENT) + if (unlink(secret->configFile) < 0 && errno != ENOENT) goto cleanup; /* When the XML is missing, the rest may waste disk space, but the secret won't be loaded again, so we have succeeded already. */ @@ -303,7 +293,6 @@ secretDeleteSaved(const virSecretObj *secret) cleanup: VIR_FREE(value_filename); - VIR_FREE(xml_filename); return ret; } @@ -412,6 +401,9 @@ secretLoad(const char *file, secret->def = def; def = NULL; + if (VIR_STRDUP(secret->configFile, path) < 0) + goto cleanup; + if (secretLoadValue(secret) < 0) goto cleanup; @@ -731,9 +723,10 @@ secretDefineXML(virConnectPtr conn, /* 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))) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(secret->def->uuid, uuidstr); virReportError(VIR_ERR_INTERNAL_ERROR, _("a secret with UUID %s already defined for " "use with %s"), @@ -745,6 +738,14 @@ secretDefineXML(virConnectPtr conn, if (VIR_ALLOC(secret) < 0) goto cleanup; + /* Generate configFile using driver->configDir, + * the uuidstr, and .xml suffix */ + if (!(secret->configFile = virFileBuildPath(driver->configDir, + uuidstr, ".xml"))) { + secretFree(secret); + goto cleanup; + } + listInsert(&driver->secrets, secret); secret->def = new_attrs; } else { -- 2.5.0
On 02/25/2016 09:03 AM, John Ferlan wrote:
This patch removes the need for secretXMLPath. Instead save 'path' during loadSecret as 'configFile'. The secretXMLPath is nothing more than an open coded virFileBuildPath. All that code did was concantenate the driver->configDir, the UUID of the secret, and the ".xml" suffix to form the configFile name which we now will generate and save instead.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 39 ++++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 19 deletions(-)
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 4dee2a6..d4b0207 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -56,6 +56,7 @@ typedef struct _virSecretObj virSecretObj; typedef virSecretObj *virSecretObjPtr; struct _virSecretObj { virSecretObjPtr next; + char *configFile; virSecretDefPtr def; unsigned char *value; /* May be NULL */ size_t value_size; @@ -112,6 +113,7 @@ secretFree(virSecretObjPtr secret) memset(secret->value, 0, secret->value_size); VIR_FREE(secret->value); } + VIR_FREE(secret->configFile); VIR_FREE(secret); }
@@ -197,11 +199,6 @@ secretComputePath(const virSecretObj *secret, return ret; }
-static char * -secretXMLPath(const virSecretObj *secret) -{ - return secretComputePath(secret, ".xml"); -}
static char * secretBase64Path(const virSecretObj *secret) @@ -223,19 +220,16 @@ secretEnsureDirectory(void) static int secretSaveDef(const virSecretObj *secret) { - char *filename = NULL, *xml = NULL; + char *xml = NULL; int ret = -1;
if (secretEnsureDirectory() < 0) goto cleanup;
- if (!(filename = secretXMLPath(secret))) - goto cleanup; - if (!(xml = virSecretDefFormat(secret->def))) goto cleanup;
- if (virFileRewrite(filename, S_IRUSR | S_IWUSR, + if (virFileRewrite(secret->configFile, S_IRUSR | S_IWUSR, secretRewriteFile, xml) < 0) goto cleanup;
@@ -243,7 +237,6 @@ secretSaveDef(const virSecretObj *secret)
cleanup: VIR_FREE(xml); - VIR_FREE(filename); return ret; }
@@ -284,16 +277,13 @@ secretSaveValue(const virSecretObj *secret) static int secretDeleteSaved(const virSecretObj *secret) { - char *xml_filename = NULL, *value_filename = NULL; + char *value_filename = NULL; int ret = -1;
- if (!(xml_filename = secretXMLPath(secret))) - goto cleanup; - if (!(value_filename = secretBase64Path(secret))) goto cleanup;
- if (unlink(xml_filename) < 0 && errno != ENOENT) + if (unlink(secret->configFile) < 0 && errno != ENOENT) goto cleanup; /* When the XML is missing, the rest may waste disk space, but the secret won't be loaded again, so we have succeeded already. */ @@ -303,7 +293,6 @@ secretDeleteSaved(const virSecretObj *secret)
cleanup: VIR_FREE(value_filename); - VIR_FREE(xml_filename); return ret; }
@@ -412,6 +401,9 @@ secretLoad(const char *file, secret->def = def; def = NULL;
+ if (VIR_STRDUP(secret->configFile, path) < 0) + goto cleanup; + if (secretLoadValue(secret) < 0) goto cleanup;
@@ -731,9 +723,10 @@ secretDefineXML(virConnectPtr conn, /* 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))) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(secret->def->uuid, uuidstr); virReportError(VIR_ERR_INTERNAL_ERROR, _("a secret with UUID %s already defined for " "use with %s"), @@ -745,6 +738,14 @@ secretDefineXML(virConnectPtr conn, if (VIR_ALLOC(secret) < 0) goto cleanup;
+ /* Generate configFile using driver->configDir, + * the uuidstr, and .xml suffix */ + if (!(secret->configFile = virFileBuildPath(driver->configDir, + uuidstr, ".xml"))) { + secretFree(secret); + goto cleanup; + } + listInsert(&driver->secrets, secret); secret->def = new_attrs; } else {
<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,
On Thu, Feb 25, 2016 at 09:03:13AM -0500, John Ferlan wrote:
This patch removes the need for secretXMLPath. Instead save 'path' during loadSecret as 'configFile'. The secretXMLPath is nothing more than an open coded virFileBuildPath. All that code did was concantenate the driver->configDir, the UUID of the secret, and the ".xml" suffix to form the configFile name which we now will generate and save instead.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 39 ++++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 19 deletions(-)
ACK with the followup squashed obviously Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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@redhat.com> --- 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
On Thu, Feb 25, 2016 at 09:03:14AM -0500, John Ferlan wrote:
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.
Yep, and better to have the failure upfront than later when we want to save/delete the secret
This also removes the need for secretComputPath and secretBase64Path.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 107 ++++++++++++++++++++-------------------------
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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@redhat.com> --- 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
On Thu, Feb 25, 2016 at 09:03:15AM -0500, John Ferlan wrote:
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@redhat.com> --- src/secret/secret_driver.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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@redhat.com> --- 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
On Thu, Feb 25, 2016 at 09:03:16AM -0500, John Ferlan wrote:
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@redhat.com> --- 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; +}
Could probably argue this function ultimately belongs in src/conf/secret_conf.c to match domain_conf.c virDomainObjAssignDef ACK anyway. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Rename to secretLoadAllConfigs and add the 'driver->configDir' as a parameter. Signed-off-by: John Ferlan <jferlan@redhat.com> --- 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
On Thu, Feb 25, 2016 at 09:03:17AM -0500, John Ferlan wrote:
Rename to secretLoadAllConfigs and add the 'driver->configDir' as a parameter.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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@redhat.com> --- 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 Thu, Feb 25, 2016 at 09:03:18AM -0500, John Ferlan wrote:
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@redhat.com> --- 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; };
No objections to using an array, but if we consider the possibility of having many 100's of guests, all using encrypted disks, or network disks needing auth, then we would probably be better having a hash table indexed on uuid, like we do for domains. ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 02/26/2016 12:16 PM, Daniel P. Berrange wrote:
On Thu, Feb 25, 2016 at 09:03:18AM -0500, John Ferlan wrote:
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@redhat.com> --- 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; };
No objections to using an array, but if we consider the possibility of having many 100's of guests, all using encrypted disks, or network disks needing auth, then we would probably be better having a hash table indexed on uuid, like we do for domains.
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
participants (2)
-
Daniel P. Berrange -
John Ferlan