[libvirt] [PATCH v2 00/14] Convert secret driver to use hashed object
v1: http://www.redhat.com/archives/libvir-list/2016-March/msg00099.html Differences high level: - Use virsecretobj.{c,h} instead of secret_conf.{c,h} - Work in v1 review comments Difference details for those that care to know: Note: Former patch2 has already been pushed. Patch 1: Is combination of v1's patch1 and patch3 w/ adjustments: - Create virsecretobj.c and virsecretobj.h and move sources there. - Add virsecretobj.{c,h} to src/Makefile.am - Use order as suggested by eblake review of patch3 for forward static decls, removed the != NULL from virSecretObjDispose, and add comment regarding why memset() is being done Patch 2: Former patch4 - Move code to virsecretobj.{c,h} instead of secret_conf.{c,h} - Change the virSecretObjFindByUUIDLocked to use the construct "return virObjectRef(virHashLookup(...));" as suggested by eblake Patch3: Former patch5 (split) - Split the format patch5 to have virSecretUsageIDForDef changes in one patch and the remainder in the followup patch. Patch4: Former patch5 (split) - The rest of former patch5 - Removed the extraneous "VIR_FREE(configFile);" as noted by eblake. - NOTE: eblake queried about the condition: if (secret->def->private && !def->private) in virSecretObjListAddLocked. This same check came from the former secretDefineXML in the else of the "secretFindByUUID(new_attrs->uuid)". IOW: Redefining a secret. The code didn't allow a "new" secret to be not private if the currently defined one was private. I left it as is. Patch5->Patch9: Former patch6->patch10: - All that's different is using virsecretobj instead of secret_conf Patch10: Former patch11 - Using virsecretobj instead of secret_conf Patch11: Former patch10 - Adjusted and moved the comment from virSecretObjDeleteConfig to virSecretObjDeleteData - Added a virReportSystemError for the DeleteConfig error Patch12: Former patch13: - All that's different is using virsecretobj instead of secret_conf Patch13: Former patch14 - Delete the extra space in the "return 0" as noted by Cole's review. Patch14: Former patch15: - All that's different is using virsecretobj instead of secret_conf After all is said done, I did a sdiff between the v1 secret_conf.h and v2 virsecretobj.h as well as the v1 secret_conf.c and v2 virsecretobj.c and found only the differences as noted above, plus removed a duplicated virSecretUsageIDForDef prototype I found in secret_conf.h. I also went through the painful process of make check syntax-check at each step of the way and built using Coverity. John Ferlan (14): secret: Create virsecretobj.c and virsecretconf.h secret: Introduce virSecretObjListFindBy{UUID|Usage} support secret: Introduce virSecretUsageIDForDef secret: Introduce virSecretObjListAdd* and virSecretObjListRemove secret: Introduce virSecretObjListNumOfSecrets secret: Introduce virSecretObjListExport secret: Introduce virSecretObjListGetUUIDs secret: Use the hashed virSecretObjList secret: Move and rename secretLoadAllConfigs secret: Introduce virSecretObjDelete{Config|Data} secret: Introduce virSecretObjSave{Config|Data} secret: Introduce virSecretObj{Get|Set}Def secret: Introduce virSecretObjGetValue and virSecretObjGetValueSize secret: Change virSecretDef variable names po/POTFILES.in | 1 + src/Makefile.am | 3 +- src/conf/secret_conf.c | 37 +- src/conf/secret_conf.h | 9 +- src/conf/virsecretobj.c | 1011 +++++++++++++++++++++++++++++++++++++++++ src/conf/virsecretobj.h | 110 +++++ src/libvirt_private.syms | 24 + src/secret/secret_driver.c | 823 ++++----------------------------- src/storage/storage_backend.c | 4 +- 9 files changed, 1279 insertions(+), 743 deletions(-) create mode 100644 src/conf/virsecretobj.c create mode 100644 src/conf/virsecretobj.h -- 2.5.5
Move virSecretObj from secret_driver.c to virsecretobj.h To support being able to create a hashed secrets list, move the virSecretObj to virsecretobj.h so that the code can at least find the definition. This should be a temporary situation while the virsecretobj.c code is patched in order to support a hashed secret object while still having the linked list support in secret_driver.c. Eventually, the goal is to move the virSecretObj into virsecretobj.c, although it is notable that the existing model from which virSecretObj was derived has virDomainObj in src/conf/domain_conf.h and virNetworkObj in src/conf/network_conf.h, so virSecretObj wouldn't be unique if it were to remain in virsecretobj.h Still adding accessors to fetch and store hashed object data will be the end goal. Add definitions and infrastucture in virsecretobj.c to create and handle a hashed virSecretObj and virSecretObjList including the class, object, lock setup, and disposal API's. Nothing will call these yet. This infrastructure will replace the forward linked list logic within the secret_driver, eventually. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/Makefile.am | 3 +- src/conf/virsecretobj.c | 138 +++++++++++++++++++++++++++++++++++++++++++++ src/conf/virsecretobj.h | 49 ++++++++++++++++ src/secret/secret_driver.c | 12 +--- 4 files changed, 190 insertions(+), 12 deletions(-) create mode 100644 src/conf/virsecretobj.c create mode 100644 src/conf/virsecretobj.h diff --git a/src/Makefile.am b/src/Makefile.am index ad1c0c3..b31f363 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -365,7 +365,8 @@ INTERFACE_CONF_SOURCES = \ # Secret driver generic impl APIs SECRET_CONF_SOURCES = \ - conf/secret_conf.h conf/secret_conf.c + conf/secret_conf.h conf/secret_conf.c \ + conf/virsecretobj.h conf/virsecretobj.c # Network driver generic impl APIs NODE_DEVICE_CONF_SOURCES = \ diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c new file mode 100644 index 0000000..905e138 --- /dev/null +++ b/src/conf/virsecretobj.c @@ -0,0 +1,138 @@ +/* + * virsecretobj.c: internal <secret> objects handling + * + * 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 + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> + +#include "datatypes.h" +#include "virsecretobj.h" +#include "viralloc.h" +#include "virhash.h" + + +static virClassPtr virSecretObjClass; +static virClassPtr virSecretObjListClass; +static void virSecretObjDispose(void *obj); +static void virSecretObjListDispose(void *obj); + +struct _virSecretObjList { + virObjectLockable parent; + + /* uuid string -> virSecretObj mapping + * for O(1), lockless lookup-by-uuid */ + virHashTable *objs; +}; + +struct virSecretSearchData { + int usageType; + const char *usageID; +}; + + +static int +virSecretObjOnceInit(void) +{ + if (!(virSecretObjClass = virClassNew(virClassForObjectLockable(), + "virSecretObj", + sizeof(virSecretObj), + virSecretObjDispose))) + return -1; + + if (!(virSecretObjListClass = virClassNew(virClassForObjectLockable(), + "virSecretObjList", + sizeof(virSecretObjList), + virSecretObjListDispose))) + return -1; + + return 0; +} + + +VIR_ONCE_GLOBAL_INIT(virSecretObj) + +virSecretObjPtr +virSecretObjNew(void) +{ + virSecretObjPtr secret; + + if (virSecretObjInitialize() < 0) + return NULL; + + if (!(secret = virObjectLockableNew(virSecretObjClass))) + return NULL; + + return secret; +} + + +void +virSecretObjEndAPI(virSecretObjPtr *secret) +{ + if (!*secret) + return; + + virObjectUnlock(*secret); + virObjectUnref(*secret); + *secret = NULL; +} + + +virSecretObjListPtr +virSecretObjListNew(void) +{ + virSecretObjListPtr secrets; + + if (virSecretObjInitialize() < 0) + return NULL; + + if (!(secrets = virObjectLockableNew(virSecretObjListClass))) + return NULL; + + if (!(secrets->objs = virHashCreate(50, virObjectFreeHashData))) { + virObjectUnref(secrets); + return NULL; + } + + return secrets; +} + + +static void +virSecretObjDispose(void *obj) +{ + virSecretObjPtr secret = obj; + + virSecretDefFree(secret->def); + if (secret->value) { + /* Wipe before free to ensure we don't leave a secret on the heap */ + memset(secret->value, 0, secret->value_size); + VIR_FREE(secret->value); + } + VIR_FREE(secret->configFile); + VIR_FREE(secret->base64File); +} + + +static void +virSecretObjListDispose(void *obj) +{ + virSecretObjListPtr secrets = obj; + + virHashFree(secrets->objs); +} diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h new file mode 100644 index 0000000..3fc0844 --- /dev/null +++ b/src/conf/virsecretobj.h @@ -0,0 +1,49 @@ +/* + * virsecretobj.h: internal <secret> objects handling + * + * Copyright (C) 2009-2010, 2013-2014, 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 + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#ifndef __VIRSECRETOBJ_H__ +# define __VIRSECRETOBJ_H__ + +# include "internal.h" + +# include "secret_conf.h" + +typedef struct _virSecretObj virSecretObj; +typedef virSecretObj *virSecretObjPtr; +struct _virSecretObj { + virSecretObjPtr next; + char *configFile; + char *base64File; + virSecretDefPtr def; + unsigned char *value; /* May be NULL */ + size_t value_size; +}; + + +virSecretObjPtr virSecretObjNew(void); + +void virSecretObjEndAPI(virSecretObjPtr *secret); + +typedef struct _virSecretObjList virSecretObjList; +typedef virSecretObjList *virSecretObjListPtr; + +virSecretObjListPtr virSecretObjListNew(void); + +#endif /* __VIRSECRETOBJ_H__ */ diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 4d15797..9165a9f 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -35,6 +35,7 @@ #include "virlog.h" #include "viralloc.h" #include "secret_conf.h" +#include "virsecretobj.h" #include "secret_driver.h" #include "virthread.h" #include "viruuid.h" @@ -52,17 +53,6 @@ enum { SECRET_MAX_XML_FILE = 10*1024*1024 }; /* Internal driver state */ -typedef struct _virSecretObj virSecretObj; -typedef virSecretObj *virSecretObjPtr; -struct _virSecretObj { - virSecretObjPtr next; - char *configFile; - char *base64File; - virSecretDefPtr def; - unsigned char *value; /* May be NULL */ - size_t value_size; -}; - typedef struct _virSecretDriverState virSecretDriverState; typedef virSecretDriverState *virSecretDriverStatePtr; struct _virSecretDriverState { -- 2.5.5
New API's including unlocked and Locked versions in order to be able to use in either manner. Support for searching hash object lists instead of linked lists will replace existing secret_driver functions secretFindByUUID and secretFindByUsage Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 140 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/virsecretobj.h | 14 +++++ 2 files changed, 154 insertions(+) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 905e138..1fd9f57 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -136,3 +136,143 @@ virSecretObjListDispose(void *obj) virHashFree(secrets->objs); } + + +/** + * virSecretObjFindByUUIDLocked: + * @secrets: list of secret objects + * @uuid: secret uuid to find + * + * This functions requires @secrets to be locked already! + * + * Returns: not locked, but ref'd secret object. + */ +virSecretObjPtr +virSecretObjListFindByUUIDLocked(virSecretObjListPtr secrets, + const unsigned char *uuid) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(uuid, uuidstr); + + return virObjectRef(virHashLookup(secrets->objs, uuidstr)); +} + + +/** + * virSecretObjFindByUUID: + * @secrets: list of secret objects + * @uuid: secret uuid to find + * + * This function locks @secrets and finds the secret object which + * corresponds to @uuid. + * + * Returns: locked and ref'd secret object. + */ +virSecretObjPtr +virSecretObjListFindByUUID(virSecretObjListPtr secrets, + const unsigned char *uuid) +{ + virSecretObjPtr ret; + + virObjectLock(secrets); + ret = virSecretObjListFindByUUIDLocked(secrets, uuid); + virObjectUnlock(secrets); + if (ret) + virObjectLock(ret); + return ret; +} + + +static int +virSecretObjSearchName(const void *payload, + const void *name ATTRIBUTE_UNUSED, + const void *opaque) +{ + virSecretObjPtr secret = (virSecretObjPtr) payload; + struct virSecretSearchData *data = (struct virSecretSearchData *) opaque; + int found = 0; + + virObjectLock(secret); + + if (secret->def->usage_type != data->usageType) + goto cleanup; + + switch (data->usageType) { + case VIR_SECRET_USAGE_TYPE_NONE: + /* never match this */ + break; + + case VIR_SECRET_USAGE_TYPE_VOLUME: + if (STREQ(secret->def->usage.volume, data->usageID)) + found = 1; + break; + + case VIR_SECRET_USAGE_TYPE_CEPH: + if (STREQ(secret->def->usage.ceph, data->usageID)) + found = 1; + break; + + case VIR_SECRET_USAGE_TYPE_ISCSI: + if (STREQ(secret->def->usage.target, data->usageID)) + found = 1; + break; + } + + cleanup: + virObjectUnlock(secret); + return found; +} + + +/** + * virSecretObjFindByUsageLocked: + * @secrets: list of secret objects + * @usageType: secret usageType to find + * @usageID: secret usage string + * + * This functions requires @secrets to be locked already! + * + * Returns: not locked, but ref'd secret object. + */ +virSecretObjPtr +virSecretObjListFindByUsageLocked(virSecretObjListPtr secrets, + int usageType, + const char *usageID) +{ + virSecretObjPtr ret = NULL; + struct virSecretSearchData data = { .usageType = usageType, + .usageID = usageID }; + + ret = virHashSearch(secrets->objs, virSecretObjSearchName, &data); + if (ret) + virObjectRef(ret); + return ret; +} + + +/** + * virSecretObjFindByUsage: + * @secrets: list of secret objects + * @usageType: secret usageType to find + * @usageID: secret usage string + * + * This function locks @secrets and finds the secret object which + * corresponds to @usageID of @usageType. + * + * Returns: locked and ref'd secret object. + */ +virSecretObjPtr +virSecretObjListFindByUsage(virSecretObjListPtr secrets, + int usageType, + const char *usageID) +{ + virSecretObjPtr ret; + + virObjectLock(secrets); + ret = virSecretObjListFindByUsageLocked(secrets, usageType, usageID); + virObjectUnlock(secrets); + if (ret) + virObjectLock(ret); + return ret; +} diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h index 3fc0844..0effcd6 100644 --- a/src/conf/virsecretobj.h +++ b/src/conf/virsecretobj.h @@ -46,4 +46,18 @@ typedef virSecretObjList *virSecretObjListPtr; virSecretObjListPtr virSecretObjListNew(void); +virSecretObjPtr virSecretObjListFindByUUIDLocked(virSecretObjListPtr secrets, + const unsigned char *uuid); + +virSecretObjPtr virSecretObjListFindByUUID(virSecretObjListPtr secrets, + const unsigned char *uuid); + +virSecretObjPtr virSecretObjListFindByUsageLocked(virSecretObjListPtr secrets, + int usageType, + const char *usageID); + +virSecretObjPtr virSecretObjListFindByUsage(virSecretObjListPtr secrets, + int usageType, + const char *usageID); + #endif /* __VIRSECRETOBJ_H__ */ -- 2.5.5
Move the driver specific secretUsageIDForDef into secret_conf.c. It could be more of a general purpose API. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/secret_conf.c | 24 +++++++++++++++++++++++- src/conf/secret_conf.h | 4 +++- src/libvirt_private.syms | 1 + src/secret/secret_driver.c | 34 +++++++--------------------------- 4 files changed, 34 insertions(+), 29 deletions(-) diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index 4eebae5..8373051 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -1,7 +1,7 @@ /* * secret_conf.c: internal <secret> XML handling * - * Copyright (C) 2009-2014 Red Hat, Inc. + * Copyright (C) 2009-2014, 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 @@ -39,6 +39,28 @@ VIR_LOG_INIT("conf.secret_conf"); VIR_ENUM_IMPL(virSecretUsage, VIR_SECRET_USAGE_TYPE_LAST, "none", "volume", "ceph", "iscsi") +const char * +virSecretUsageIDForDef(virSecretDefPtr def) +{ + switch (def->usage_type) { + case VIR_SECRET_USAGE_TYPE_NONE: + return ""; + + case VIR_SECRET_USAGE_TYPE_VOLUME: + return def->usage.volume; + + case VIR_SECRET_USAGE_TYPE_CEPH: + return def->usage.ceph; + + case VIR_SECRET_USAGE_TYPE_ISCSI: + return def->usage.target; + + default: + return NULL; + } +} + + void virSecretDefFree(virSecretDefPtr def) { diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index 9c13f05..c87efe4 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -1,7 +1,7 @@ /* * secret_conf.h: internal <secret> XML handling API * - * Copyright (C) 2009-2010, 2013-2014 Red Hat, Inc. + * Copyright (C) 2009-2010, 2013-2014, 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 @@ -43,6 +43,8 @@ struct _virSecretDef { } usage; }; +const char *virSecretUsageIDForDef(virSecretDefPtr def); + void virSecretDefFree(virSecretDefPtr def); virSecretDefPtr virSecretDefParseString(const char *xml); virSecretDefPtr virSecretDefParseFile(const char *filename); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2b55369..b2eb43f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -786,6 +786,7 @@ virSecretDefFormat; virSecretDefFree; virSecretDefParseFile; virSecretDefParseString; +virSecretUsageIDForDef; virSecretUsageTypeFromString; virSecretUsageTypeToString; diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 9165a9f..336f00f 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -567,26 +567,6 @@ secretConnectListSecrets(virConnectPtr conn, return -1; } -static const char * -secretUsageIDForDef(virSecretDefPtr def) -{ - switch (def->usage_type) { - case VIR_SECRET_USAGE_TYPE_NONE: - return ""; - - case VIR_SECRET_USAGE_TYPE_VOLUME: - return def->usage.volume; - - case VIR_SECRET_USAGE_TYPE_CEPH: - return def->usage.ceph; - - case VIR_SECRET_USAGE_TYPE_ISCSI: - return def->usage.target; - - default: - return NULL; - } -} #define MATCH(FLAG) (flags & (FLAG)) static int @@ -640,7 +620,7 @@ secretConnectListAllSecrets(virConnectPtr conn, virGetSecret(conn, secret->def->uuid, secret->def->usage_type, - secretUsageIDForDef(secret->def)))) + virSecretUsageIDForDef(secret->def)))) goto cleanup; } ret_nsecrets++; @@ -691,7 +671,7 @@ secretLookupByUUID(virConnectPtr conn, ret = virGetSecret(conn, secret->def->uuid, secret->def->usage_type, - secretUsageIDForDef(secret->def)); + virSecretUsageIDForDef(secret->def)); cleanup: secretDriverUnlock(); @@ -721,7 +701,7 @@ secretLookupByUsage(virConnectPtr conn, ret = virGetSecret(conn, secret->def->uuid, secret->def->usage_type, - secretUsageIDForDef(secret->def)); + virSecretUsageIDForDef(secret->def)); cleanup: secretDriverUnlock(); @@ -752,7 +732,7 @@ secretDefineXML(virConnectPtr conn, if (!(secret = secretFindByUUID(new_attrs->uuid))) { /* No existing secret with same UUID, * try look for matching usage instead */ - const char *usageID = secretUsageIDForDef(new_attrs); + const char *usageID = virSecretUsageIDForDef(new_attrs); char uuidstr[VIR_UUID_STRING_BUFLEN]; if ((secret = secretFindByUsage(new_attrs->usage_type, usageID))) { @@ -785,8 +765,8 @@ secretDefineXML(virConnectPtr conn, goto cleanup; } } else { - const char *newUsageID = secretUsageIDForDef(new_attrs); - const char *oldUsageID = secretUsageIDForDef(secret->def); + const char *newUsageID = virSecretUsageIDForDef(new_attrs); + const char *oldUsageID = virSecretUsageIDForDef(secret->def); if (STRNEQ(oldUsageID, newUsageID)) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(secret->def->uuid, uuidstr); @@ -831,7 +811,7 @@ secretDefineXML(virConnectPtr conn, ret = virGetSecret(conn, secret->def->uuid, secret->def->usage_type, - secretUsageIDForDef(secret->def)); + virSecretUsageIDForDef(secret->def)); goto cleanup; restore_backup: -- 2.5.5
Add the functions to add/remove elements from the hashed secret obj list. These will replace secret_driver functions secretAssignDef and secretObjRemove. The virSecretObjListAddLocked will perform the necessary lookups and decide whether to replace an existing hash entry or create a new one. This includes setting up the configPath and base64Path as well as being able to support the caller's need to restore from a previous definition in case something goes wrong in the caller. Signed-off-by: John Ferlan <jferlan@redhat.com> --- po/POTFILES.in | 1 + src/conf/virsecretobj.c | 154 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/virsecretobj.h | 13 ++++ 3 files changed, 168 insertions(+) diff --git a/po/POTFILES.in b/po/POTFILES.in index 2f53079..506d535 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -39,6 +39,7 @@ src/conf/snapshot_conf.c src/conf/storage_conf.c src/conf/virchrdev.c src/conf/virdomainobjlist.c +src/conf/virsecretobj.c src/cpu/cpu.c src/cpu/cpu_generic.c src/cpu/cpu_map.c diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 1fd9f57..b808902 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -23,8 +23,14 @@ #include "datatypes.h" #include "virsecretobj.h" #include "viralloc.h" +#include "virerror.h" +#include "virfile.h" #include "virhash.h" +#include "virlog.h" +#define VIR_FROM_THIS VIR_FROM_SECRET + +VIR_LOG_INIT("conf.virsecretobj"); static virClassPtr virSecretObjClass; static virClassPtr virSecretObjListClass; @@ -276,3 +282,151 @@ virSecretObjListFindByUsage(virSecretObjListPtr secrets, virObjectLock(ret); return ret; } + + +/* + * virSecretObjListRemove: + * @secrets: list of secret objects + * @secret: a secret object + * + * Remove the object from the hash table. The caller must hold the lock + * on the driver owning @secrets and must have also locked @secret to + * ensure no one else is either waiting for @secret or still using it. + */ +void +virSecretObjListRemove(virSecretObjListPtr secrets, + virSecretObjPtr secret) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(secret->def->uuid, uuidstr); + virObjectRef(secret); + virObjectUnlock(secret); + + virObjectLock(secrets); + virObjectLock(secret); + virHashRemoveEntry(secrets->objs, uuidstr); + virObjectUnlock(secret); + virObjectUnref(secret); + virObjectUnlock(secrets); +} + + +/* + * virSecretObjListAddLocked: + * @secrets: list of secret objects + * @def: new secret definition + * @configDir: directory to place secret config files + * @oldDef: Former secret def (e.g. a reload path perhaps) + * + * Add the new def to the secret obj table hash + * + * This functions requires @secrets to be locked already! + * + * Returns pointer to secret or NULL if failure to add + */ +virSecretObjPtr +virSecretObjListAddLocked(virSecretObjListPtr secrets, + virSecretDefPtr def, + const char *configDir, + virSecretDefPtr *oldDef) +{ + virSecretObjPtr secret; + virSecretObjPtr ret = NULL; + const char *newUsageID = virSecretUsageIDForDef(def); + char uuidstr[VIR_UUID_STRING_BUFLEN]; + char *configFile = NULL, *base64File = NULL; + + if (oldDef) + *oldDef = NULL; + + /* Is there a secret already matching this UUID */ + if ((secret = virSecretObjListFindByUUIDLocked(secrets, def->uuid))) { + const char *oldUsageID; + + virObjectLock(secret); + + oldUsageID = virSecretUsageIDForDef(secret->def); + if (STRNEQ(oldUsageID, newUsageID)) { + virUUIDFormat(secret->def->uuid, uuidstr); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("a secret with UUID %s is already defined for " + "use with %s"), + uuidstr, oldUsageID); + goto cleanup; + } + + if (secret->def->private && !def->private) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot change private flag on existing secret")); + goto cleanup; + } + + if (oldDef) + *oldDef = secret->def; + else + virSecretDefFree(secret->def); + secret->def = def; + } else { + /* No existing secret with same UUID, + * try look for matching usage instead */ + if ((secret = virSecretObjListFindByUsageLocked(secrets, + def->usage_type, + newUsageID))) { + virObjectLock(secret); + virUUIDFormat(secret->def->uuid, uuidstr); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("a secret with UUID %s already defined for " + "use with %s"), + uuidstr, newUsageID); + goto cleanup; + } + + /* Generate the possible configFile and base64File strings + * using the configDir, uuidstr, and appropriate suffix + */ + virUUIDFormat(def->uuid, uuidstr); + if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) || + !(base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) + goto cleanup; + + if (!(secret = virSecretObjNew())) + goto cleanup; + + virObjectLock(secret); + + if (virHashAddEntry(secrets->objs, uuidstr, secret) < 0) + goto cleanup; + + secret->def = def; + secret->configFile = configFile; + secret->base64File = base64File; + configFile = NULL; + base64File = NULL; + virObjectRef(secret); + } + + ret = secret; + secret = NULL; + + cleanup: + virSecretObjEndAPI(&secret); + VIR_FREE(configFile); + VIR_FREE(base64File); + return ret; +} + + +virSecretObjPtr +virSecretObjListAdd(virSecretObjListPtr secrets, + virSecretDefPtr def, + const char *configDir, + virSecretDefPtr *oldDef) +{ + virSecretObjPtr ret; + + virObjectLock(secrets); + ret = virSecretObjListAddLocked(secrets, def, configDir, oldDef); + virObjectUnlock(secrets); + return ret; +} diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h index 0effcd6..290e91b 100644 --- a/src/conf/virsecretobj.h +++ b/src/conf/virsecretobj.h @@ -60,4 +60,17 @@ virSecretObjPtr virSecretObjListFindByUsage(virSecretObjListPtr secrets, int usageType, const char *usageID); +void virSecretObjListRemove(virSecretObjListPtr secrets, + virSecretObjPtr secret); + +virSecretObjPtr virSecretObjListAddLocked(virSecretObjListPtr secrets, + virSecretDefPtr def, + const char *configDir, + virSecretDefPtr *oldDef); + +virSecretObjPtr virSecretObjListAdd(virSecretObjListPtr secrets, + virSecretDefPtr def, + const char *configDir, + virSecretDefPtr *oldDef); + #endif /* __VIRSECRETOBJ_H__ */ -- 2.5.5
Add function to count the hashed secret obj list with filters. This will replace the guts of secret_driver's secretConnectNumOfSecrets. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ src/conf/virsecretobj.h | 7 +++++++ 2 files changed, 51 insertions(+) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index b808902..7f103e3 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -430,3 +430,47 @@ virSecretObjListAdd(virSecretObjListPtr secrets, virObjectUnlock(secrets); return ret; } + + +struct virSecretObjListGetHelperData { + virConnectPtr conn; + virSecretObjListACLFilter filter; + int got; +}; + + +static int +virSecretObjListGetHelper(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + struct virSecretObjListGetHelperData *data = opaque; + virSecretObjPtr obj = payload; + + virObjectLock(obj); + + if (data->filter && !data->filter(data->conn, obj->def)) + goto cleanup; + + data->got++; + + cleanup: + virObjectUnlock(obj); + return 0; +} + + +int +virSecretObjListNumOfSecrets(virSecretObjListPtr secrets, + virSecretObjListACLFilter filter, + virConnectPtr conn) +{ + struct virSecretObjListGetHelperData data = { + .conn = conn, .filter = filter, .got = 0 }; + + virObjectLock(secrets); + virHashForEach(secrets->objs, virSecretObjListGetHelper, &data); + virObjectUnlock(secrets); + + return data.got; +} diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h index 290e91b..7f0d40a 100644 --- a/src/conf/virsecretobj.h +++ b/src/conf/virsecretobj.h @@ -73,4 +73,11 @@ virSecretObjPtr virSecretObjListAdd(virSecretObjListPtr secrets, const char *configDir, virSecretDefPtr *oldDef); +typedef bool (*virSecretObjListACLFilter)(virConnectPtr conn, + virSecretDefPtr def); + +int virSecretObjListNumOfSecrets(virSecretObjListPtr secrets, + virSecretObjListACLFilter filter, + virConnectPtr conn); + #endif /* __VIRSECRETOBJ_H__ */ -- 2.5.5
Add function to return a "match" filtered list of secret objects. This function replaces the guts of secretConnectListAllSecrets. Need to also move and make global virSecretUsageIDForDef since it'll be used by both secret_driver.c and secret_conf.c Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/virsecretobj.h | 6 +++ 2 files changed, 123 insertions(+) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 7f103e3..3b218d5 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -474,3 +474,120 @@ virSecretObjListNumOfSecrets(virSecretObjListPtr secrets, return data.got; } + + +#define MATCH(FLAG) (flags & (FLAG)) +static bool +virSecretObjMatchFlags(virSecretObjPtr secret, + unsigned int flags) +{ + /* filter by whether it's ephemeral */ + if (MATCH(VIR_CONNECT_LIST_SECRETS_FILTERS_EPHEMERAL) && + !((MATCH(VIR_CONNECT_LIST_SECRETS_EPHEMERAL) && + secret->def->ephemeral) || + (MATCH(VIR_CONNECT_LIST_SECRETS_NO_EPHEMERAL) && + !secret->def->ephemeral))) + return false; + + /* filter by whether it's private */ + if (MATCH(VIR_CONNECT_LIST_SECRETS_FILTERS_PRIVATE) && + !((MATCH(VIR_CONNECT_LIST_SECRETS_PRIVATE) && + secret->def->private) || + (MATCH(VIR_CONNECT_LIST_SECRETS_NO_PRIVATE) && + !secret->def->private))) + return false; + + return true; +} +#undef MATCH + + +struct virSecretObjListData { + virConnectPtr conn; + virSecretPtr *secrets; + virSecretObjListACLFilter filter; + unsigned int flags; + int nsecrets; + bool error; +}; + +static int +virSecretObjListPopulate(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + struct virSecretObjListData *data = opaque; + virSecretObjPtr obj = payload; + virSecretPtr secret = NULL; + + if (data->error) + return 0; + + virObjectLock(obj); + + if (data->filter && !data->filter(data->conn, obj->def)) + goto cleanup; + + if (!virSecretObjMatchFlags(obj, data->flags)) + goto cleanup; + + if (!data->secrets) { + data->nsecrets++; + goto cleanup; + } + + if (!(secret = virGetSecret(data->conn, obj->def->uuid, + obj->def->usage_type, + virSecretUsageIDForDef(obj->def)))) { + data->error = true; + goto cleanup; + } + + data->secrets[data->nsecrets++] = secret; + + cleanup: + virObjectUnlock(obj); + return 0; +} + + +int +virSecretObjListExport(virConnectPtr conn, + virSecretObjListPtr secretobjs, + virSecretPtr **secrets, + virSecretObjListACLFilter filter, + unsigned int flags) +{ + int ret = -1; + struct virSecretObjListData data = { + .conn = conn, .secrets = NULL, + .filter = filter, .flags = flags, + .nsecrets = 0, .error = false }; + + virObjectLock(secretobjs); + if (secrets && + VIR_ALLOC_N(data.secrets, virHashSize(secretobjs->objs) + 1) < 0) + goto cleanup; + + virHashForEach(secretobjs->objs, virSecretObjListPopulate, &data); + + if (data.error) + goto cleanup; + + if (data.secrets) { + /* trim the array to the final size */ + ignore_value(VIR_REALLOC_N(data.secrets, data.nsecrets + 1)); + *secrets = data.secrets; + data.secrets = NULL; + } + + ret = data.nsecrets; + + cleanup: + virObjectUnlock(secretobjs); + while (data.secrets && data.nsecrets) + virObjectUnref(data.secrets[--data.nsecrets]); + + VIR_FREE(data.secrets); + return ret; +} diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h index 7f0d40a..761f69e 100644 --- a/src/conf/virsecretobj.h +++ b/src/conf/virsecretobj.h @@ -80,4 +80,10 @@ int virSecretObjListNumOfSecrets(virSecretObjListPtr secrets, virSecretObjListACLFilter filter, virConnectPtr conn); +int virSecretObjListExport(virConnectPtr conn, + virSecretObjListPtr secretobjs, + virSecretPtr **secrets, + virSecretObjListACLFilter filter, + unsigned int flags); + #endif /* __VIRSECRETOBJ_H__ */ -- 2.5.5
Add function to return counted listed of uuids to from the hashed secrets object list. This will replace the guts of secretConnectListSecrets. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++- src/conf/virsecretobj.h | 6 ++++++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 3b218d5..eab4e30 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -436,6 +436,9 @@ struct virSecretObjListGetHelperData { virConnectPtr conn; virSecretObjListACLFilter filter; int got; + char **uuids; + int nuuids; + bool error; }; @@ -447,11 +450,27 @@ virSecretObjListGetHelper(void *payload, struct virSecretObjListGetHelperData *data = opaque; virSecretObjPtr obj = payload; + if (data->error) + return 0; + + if (data->nuuids >= 0 && data->got == data->nuuids) + return 0; + virObjectLock(obj); if (data->filter && !data->filter(data->conn, obj->def)) goto cleanup; + if (data->uuids) { + char *uuidstr; + + if (VIR_ALLOC_N(uuidstr, VIR_UUID_STRING_BUFLEN) < 0) + goto cleanup; + + virUUIDFormat(obj->def->uuid, uuidstr); + data->uuids[data->got] = uuidstr; + } + data->got++; cleanup: @@ -466,7 +485,8 @@ virSecretObjListNumOfSecrets(virSecretObjListPtr secrets, virConnectPtr conn) { struct virSecretObjListGetHelperData data = { - .conn = conn, .filter = filter, .got = 0 }; + .conn = conn, .filter = filter, .got = 0, + .uuids = NULL, .nuuids = -1, .error = false }; virObjectLock(secrets); virHashForEach(secrets->objs, virSecretObjListGetHelper, &data); @@ -591,3 +611,34 @@ virSecretObjListExport(virConnectPtr conn, VIR_FREE(data.secrets); return ret; } + + +int +virSecretObjListGetUUIDs(virSecretObjListPtr secrets, + char **uuids, + int nuuids, + virSecretObjListACLFilter filter, + virConnectPtr conn) +{ + int ret = -1; + + struct virSecretObjListGetHelperData data = { + .conn = conn, .filter = filter, .got = 0, + .uuids = uuids, .nuuids = nuuids, .error = false }; + + virObjectLock(secrets); + virHashForEach(secrets->objs, virSecretObjListGetHelper, &data); + virObjectUnlock(secrets); + + if (data.error) + goto cleanup; + + ret = data.got; + + cleanup: + if (ret < 0) { + while (data.got) + VIR_FREE(data.uuids[--data.got]); + } + return ret; +} diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h index 761f69e..50e31e0 100644 --- a/src/conf/virsecretobj.h +++ b/src/conf/virsecretobj.h @@ -86,4 +86,10 @@ int virSecretObjListExport(virConnectPtr conn, virSecretObjListACLFilter filter, unsigned int flags); +int virSecretObjListGetUUIDs(virSecretObjListPtr secrets, + char **uuids, + int nuuids, + virSecretObjListACLFilter filter, + virConnectPtr conn); + #endif /* __VIRSECRETOBJ_H__ */ -- 2.5.5
This patch replaces most of the guts of secret_driver.c with recently added secret_conf.c APIs in order manage secret lists and objects using the hashed virSecretObjList* lookup API's. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.h | 3 +- src/libvirt_private.syms | 12 ++ src/secret/secret_driver.c | 437 ++++++--------------------------------------- 3 files changed, 64 insertions(+), 388 deletions(-) diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h index 50e31e0..514db2f 100644 --- a/src/conf/virsecretobj.h +++ b/src/conf/virsecretobj.h @@ -24,11 +24,12 @@ # include "internal.h" # include "secret_conf.h" +# include "virobject.h" typedef struct _virSecretObj virSecretObj; typedef virSecretObj *virSecretObjPtr; struct _virSecretObj { - virSecretObjPtr next; + virObjectLockable parent; char *configFile; char *base64File; virSecretDefPtr def; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b2eb43f..603eba5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -894,6 +894,18 @@ virDomainObjListRemoveLocked; virDomainObjListRename; +# conf/virsecretobj.h +virSecretObjEndAPI; +virSecretObjListAdd; +virSecretObjListExport; +virSecretObjListFindByUsage; +virSecretObjListFindByUUID; +virSecretObjListGetUUIDs; +virSecretObjListNew; +virSecretObjListNumOfSecrets; +virSecretObjListRemove; + + # cpu/cpu.h cpuBaseline; cpuBaselineXML; diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 336f00f..90ec4ba 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -57,7 +57,7 @@ typedef struct _virSecretDriverState virSecretDriverState; typedef virSecretDriverState *virSecretDriverStatePtr; struct _virSecretDriverState { virMutex lock; - virSecretObj *secrets; + virSecretObjListPtr secrets; char *configDir; }; @@ -75,104 +75,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) -{ - if (secret == NULL) - return; - - virSecretDefFree(secret->def); - if (secret->value != NULL) { - memset(secret->value, 0, secret->value_size); - VIR_FREE(secret->value); - } - VIR_FREE(secret->configFile); - VIR_FREE(secret->base64File); - VIR_FREE(secret); -} - -static virSecretObjPtr -secretFindByUUID(const unsigned char *uuid) -{ - virSecretObjPtr *pptr, secret; - - 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; -} - -static virSecretObjPtr -secretFindByUsage(int usageType, - const char *usageID) -{ - virSecretObjPtr *pptr, secret; - - for (pptr = &driver->secrets; *pptr != NULL; pptr = &secret->next) { - secret = *pptr; - - if (secret->def->usage_type != usageType) - continue; - - switch (usageType) { - case VIR_SECRET_USAGE_TYPE_NONE: - /* never match this */ - break; - - case VIR_SECRET_USAGE_TYPE_VOLUME: - if (STREQ(secret->def->usage.volume, usageID)) - return secret; - break; - - case VIR_SECRET_USAGE_TYPE_CEPH: - if (STREQ(secret->def->usage.ceph, usageID)) - return secret; - break; - - case VIR_SECRET_USAGE_TYPE_ISCSI: - if (STREQ(secret->def->usage.target, usageID)) - return secret; - break; - } - } - 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; -} static virSecretObjPtr @@ -181,7 +83,7 @@ secretObjFromSecret(virSecretPtr secret) virSecretObjPtr obj; char uuidstr[VIR_UUID_STRING_BUFLEN]; - if (!(obj = secretFindByUUID(secret->uuid))) { + if (!(obj = virSecretObjListFindByUUID(driver->secrets, secret->uuid))) { virUUIDFormat(secret->uuid, uuidstr); virReportError(VIR_ERR_NO_SECRET, _("no secret with matching uuid '%s'"), uuidstr); @@ -377,30 +279,11 @@ 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 secrets, const char *file, const char *path, - const char *base64path) + const char *configDir) { virSecretDefPtr def = NULL; virSecretObjPtr secret = NULL, ret = NULL; @@ -411,16 +294,10 @@ secretLoad(virSecretObjPtr *list, if (secretLoadValidateUUID(def, file) < 0) goto cleanup; - if (!(secret = secretAssignDef(list, def))) + if (!(secret = virSecretObjListAdd(secrets, def, configDir, NULL))) goto cleanup; def = NULL; - if (VIR_STRDUP(secret->configFile, path) < 0) - goto cleanup; - - if (VIR_STRDUP(secret->base64File, base64path) < 0) - goto cleanup; - if (secretLoadValue(secret) < 0) goto cleanup; @@ -428,19 +305,19 @@ secretLoad(virSecretObjPtr *list, secret = NULL; cleanup: - listUnlinkSecret(list, secret); - secretFree(secret); + if (secret) + virSecretObjListRemove(secrets, 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) @@ -449,8 +326,10 @@ 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; + char *path; virSecretObjPtr secret; if (STREQ(de->d_name, ".") || STREQ(de->d_name, "..")) @@ -462,39 +341,18 @@ secretLoadAllConfigs(virSecretObjPtr *dest, 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(configDir, - base64name, ".base64"))) { - VIR_FREE(path); - VIR_FREE(base64name); - continue; - } - VIR_FREE(base64name); - - if (!(secret = secretLoad(&list, de->d_name, path, base64path))) { + if (!(secret = secretLoad(secrets, de->d_name, path, configDir))) { 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); - } - /* 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); + virSecretObjEndAPI(&secret); } closedir(dir); @@ -506,23 +364,12 @@ secretLoadAllConfigs(virSecretObjPtr *dest, static int secretConnectNumOfSecrets(virConnectPtr conn) { - size_t i; - virSecretObjPtr secret; - if (virConnectNumOfSecretsEnsureACL(conn) < 0) return -1; - secretDriverLock(); - - i = 0; - for (secret = driver->secrets; secret != NULL; secret = secret->next) { - if (virConnectNumOfSecretsCheckACL(conn, - secret->def)) - i++; - } - - secretDriverUnlock(); - return i; + return virSecretObjListNumOfSecrets(driver->secrets, + virConnectNumOfSecretsCheckACL, + conn); } static int @@ -530,122 +377,30 @@ secretConnectListSecrets(virConnectPtr conn, char **uuids, int maxuuids) { - size_t i; - virSecretObjPtr secret; - memset(uuids, 0, maxuuids * sizeof(*uuids)); if (virConnectListSecretsEnsureACL(conn) < 0) return -1; - secretDriverLock(); - - i = 0; - for (secret = driver->secrets; secret != NULL; secret = secret->next) { - char *uuidstr; - if (!virConnectListSecretsCheckACL(conn, - secret->def)) - continue; - if (i == maxuuids) - break; - if (VIR_ALLOC_N(uuidstr, VIR_UUID_STRING_BUFLEN) < 0) - goto cleanup; - virUUIDFormat(secret->def->uuid, uuidstr); - uuids[i] = uuidstr; - i++; - } - - secretDriverUnlock(); - return i; - - cleanup: - secretDriverUnlock(); - - for (i = 0; i < maxuuids; i++) - VIR_FREE(uuids[i]); - - return -1; + return virSecretObjListGetUUIDs(driver->secrets, uuids, maxuuids, + virConnectListSecretsCheckACL, conn); } -#define MATCH(FLAG) (flags & (FLAG)) static int secretConnectListAllSecrets(virConnectPtr conn, virSecretPtr **secrets, unsigned int flags) { - virSecretPtr *tmp_secrets = NULL; - int nsecrets = 0; - int ret_nsecrets = 0; - virSecretObjPtr secret = NULL; - size_t i = 0; - int ret = -1; - virCheckFlags(VIR_CONNECT_LIST_SECRETS_FILTERS_ALL, -1); if (virConnectListAllSecretsEnsureACL(conn) < 0) return -1; - secretDriverLock(); - - for (secret = driver->secrets; secret != NULL; secret = secret->next) - nsecrets++; - - if (secrets && VIR_ALLOC_N(tmp_secrets, nsecrets + 1) < 0) - goto cleanup; - - for (secret = driver->secrets; secret != NULL; secret = secret->next) { - if (!virConnectListAllSecretsCheckACL(conn, - secret->def)) - continue; - - /* filter by whether it's ephemeral */ - if (MATCH(VIR_CONNECT_LIST_SECRETS_FILTERS_EPHEMERAL) && - !((MATCH(VIR_CONNECT_LIST_SECRETS_EPHEMERAL) && - secret->def->ephemeral) || - (MATCH(VIR_CONNECT_LIST_SECRETS_NO_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) && - secret->def->private) || - (MATCH(VIR_CONNECT_LIST_SECRETS_NO_PRIVATE) && - !secret->def->private))) - continue; - - if (secrets) { - if (!(tmp_secrets[ret_nsecrets] = - virGetSecret(conn, - secret->def->uuid, - secret->def->usage_type, - virSecretUsageIDForDef(secret->def)))) - goto cleanup; - } - ret_nsecrets++; - } - - if (tmp_secrets) { - /* trim the array to the final size */ - ignore_value(VIR_REALLOC_N(tmp_secrets, ret_nsecrets + 1)); - *secrets = tmp_secrets; - tmp_secrets = NULL; - } - - ret = ret_nsecrets; - - cleanup: - secretDriverUnlock(); - if (tmp_secrets) { - for (i = 0; i < ret_nsecrets; i ++) - virObjectUnref(tmp_secrets[i]); - } - VIR_FREE(tmp_secrets); - - return ret; + return virSecretObjListExport(conn, driver->secrets, secrets, + virConnectListAllSecretsCheckACL, + flags); } -#undef MATCH static virSecretPtr @@ -655,9 +410,7 @@ secretLookupByUUID(virConnectPtr conn, virSecretPtr ret = NULL; virSecretObjPtr secret; - secretDriverLock(); - - if (!(secret = secretFindByUUID(uuid))) { + if (!(secret = virSecretObjListFindByUUID(driver->secrets, uuid))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(uuid, uuidstr); virReportError(VIR_ERR_NO_SECRET, @@ -674,7 +427,7 @@ secretLookupByUUID(virConnectPtr conn, virSecretUsageIDForDef(secret->def)); cleanup: - secretDriverUnlock(); + virSecretObjEndAPI(&secret); return ret; } @@ -687,9 +440,8 @@ secretLookupByUsage(virConnectPtr conn, virSecretPtr ret = NULL; virSecretObjPtr secret; - secretDriverLock(); - - if (!(secret = secretFindByUsage(usageType, usageID))) { + if (!(secret = virSecretObjListFindByUsage(driver->secrets, + usageType, usageID))) { virReportError(VIR_ERR_NO_SECRET, _("no secret with matching usage '%s'"), usageID); goto cleanup; @@ -704,7 +456,7 @@ secretLookupByUsage(virConnectPtr conn, virSecretUsageIDForDef(secret->def)); cleanup: - secretDriverUnlock(); + virSecretObjEndAPI(&secret); return ret; } @@ -724,69 +476,12 @@ secretDefineXML(virConnectPtr conn, if (!(new_attrs = virSecretDefParseString(xml))) return NULL; - secretDriverLock(); - if (virSecretDefineXMLEnsureACL(conn, new_attrs) < 0) goto cleanup; - if (!(secret = secretFindByUUID(new_attrs->uuid))) { - /* No existing secret with same UUID, - * try look for matching usage instead */ - const char *usageID = virSecretUsageIDForDef(new_attrs); - char uuidstr[VIR_UUID_STRING_BUFLEN]; - - 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"), - uuidstr, usageID); - goto cleanup; - } - - /* No existing secret at all, create one */ - if (!(secret = secretAssignDef(&driver->secrets, new_attrs))) - goto cleanup; - - virUUIDFormat(secret->def->uuid, uuidstr); - - /* Generate configFile using driver->configDir, - * the uuidstr, and .xml suffix */ - if (!(secret->configFile = virFileBuildPath(driver->configDir, - uuidstr, ".xml"))) { - 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; - } - } else { - const char *newUsageID = virSecretUsageIDForDef(new_attrs); - const char *oldUsageID = virSecretUsageIDForDef(secret->def); - if (STRNEQ(oldUsageID, newUsageID)) { - 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"), - uuidstr, oldUsageID); - goto cleanup; - } - - if (secret->def->private && !new_attrs->private) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot change private flag on existing secret")); - goto cleanup; - } - - /* Got an existing secret matches attrs, so reuse that */ - backup = secret->def; - secret->def = new_attrs; - } + if (!(secret = virSecretObjListAdd(driver->secrets, new_attrs, + driver->configDir, &backup))) + goto cleanup; if (!new_attrs->ephemeral) { if (backup && backup->ephemeral) { @@ -815,21 +510,18 @@ secretDefineXML(virConnectPtr conn, goto cleanup; restore_backup: - if (backup) { - /* Error - restore previous state and free new attributes */ + /* If we have a backup, then secret was defined before, so just restore + * the backup. The current secret->def (new_attrs) will be handled below. + * Otherwise, this is a new secret, thus remove it. + */ + if (backup) 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); - } + else + virSecretObjListRemove(driver->secrets, secret); cleanup: virSecretDefFree(new_attrs); - secretDriverUnlock(); + virSecretObjEndAPI(&secret); return ret; } @@ -843,8 +535,6 @@ secretGetXMLDesc(virSecretPtr obj, virCheckFlags(0, NULL); - secretDriverLock(); - if (!(secret = secretObjFromSecret(obj))) goto cleanup; @@ -854,7 +544,7 @@ secretGetXMLDesc(virSecretPtr obj, ret = virSecretDefFormat(secret->def); cleanup: - secretDriverUnlock(); + virSecretObjEndAPI(&secret); return ret; } @@ -875,8 +565,6 @@ secretSetValue(virSecretPtr obj, if (VIR_ALLOC_N(new_value, value_size) < 0) return -1; - secretDriverLock(); - if (!(secret = secretObjFromSecret(obj))) goto cleanup; @@ -910,7 +598,7 @@ secretSetValue(virSecretPtr obj, memset(new_value, 0, value_size); cleanup: - secretDriverUnlock(); + virSecretObjEndAPI(&secret); VIR_FREE(new_value); @@ -928,8 +616,6 @@ secretGetValue(virSecretPtr obj, virCheckFlags(0, NULL); - secretDriverLock(); - if (!(secret = secretObjFromSecret(obj))) goto cleanup; @@ -957,7 +643,7 @@ secretGetValue(virSecretPtr obj, *value_size = secret->value_size; cleanup: - secretDriverUnlock(); + virSecretObjEndAPI(&secret); return ret; } @@ -968,8 +654,6 @@ secretUndefine(virSecretPtr obj) int ret = -1; virSecretObjPtr secret; - secretDriverLock(); - if (!(secret = secretObjFromSecret(obj))) goto cleanup; @@ -980,13 +664,12 @@ secretUndefine(virSecretPtr obj) secretDeleteSaved(secret) < 0) goto cleanup; - listUnlinkSecret(&driver->secrets, secret); - secretFree(secret); + virSecretObjListRemove(driver->secrets, secret); ret = 0; cleanup: - secretDriverUnlock(); + virSecretObjEndAPI(&secret); return ret; } @@ -994,17 +677,12 @@ secretUndefine(virSecretPtr obj) static int secretStateCleanup(void) { - if (driver == NULL) + if (!driver) return -1; secretDriverLock(); - while (driver->secrets != NULL) { - virSecretObjPtr secret; - - secret = listUnlink(&driver->secrets); - secretFree(secret); - } + virObjectUnref(driver->secrets); VIR_FREE(driver->configDir); secretDriverUnlock(); @@ -1041,7 +719,10 @@ secretStateInitialize(bool privileged, goto error; VIR_FREE(base); - if (secretLoadAllConfigs(&driver->secrets, driver->configDir) < 0) + if (!(driver->secrets = virSecretObjListNew())) + goto error; + + if (secretLoadAllConfigs(driver->secrets, driver->configDir) < 0) goto error; secretDriverUnlock(); @@ -1057,31 +738,13 @@ secretStateInitialize(bool privileged, static int secretStateReload(void) { - virSecretObjPtr new_secrets = NULL; - if (!driver) return -1; secretDriverLock(); - if (secretLoadAllConfigs(&new_secrets, driver->configDir) < 0) - goto end; - - /* 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); - } - driver->secrets = new_secrets; + ignore_value(secretLoadAllConfigs(driver->secrets, driver->configDir)); - end: secretDriverUnlock(); return 0; } -- 2.5.5
Move to secret_conf.c and rename to virSecretLoadAllConfigs. Also includes moving/renaming the supporting virSecretLoad, virSecretLoadValue, and virSecretLoadValidateUUID. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/secret_conf.c | 1 + src/conf/secret_conf.h | 1 + src/conf/virsecretobj.c | 175 +++++++++++++++++++++++++++++++++++++++++++++ src/conf/virsecretobj.h | 2 + src/libvirt_private.syms | 1 + src/secret/secret_driver.c | 174 +------------------------------------------- 6 files changed, 182 insertions(+), 172 deletions(-) diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index 8373051..5c39f24 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -28,6 +28,7 @@ #include "virlog.h" #include "viralloc.h" #include "secret_conf.h" +#include "virsecretobj.h" #include "virerror.h" #include "virxml.h" #include "viruuid.h" diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index c87efe4..5ca4ecd 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -62,4 +62,5 @@ char *virSecretDefFormat(const virSecretDef *def); (VIR_CONNECT_LIST_SECRETS_FILTERS_EPHEMERAL | \ VIR_CONNECT_LIST_SECRETS_FILTERS_PRIVATE) + #endif diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index eab4e30..e5dafa4 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -19,6 +19,9 @@ */ #include <config.h> +#include <dirent.h> +#include <fcntl.h> +#include <sys/stat.h> #include "datatypes.h" #include "virsecretobj.h" @@ -27,6 +30,7 @@ #include "virfile.h" #include "virhash.h" #include "virlog.h" +#include "base64.h" #define VIR_FROM_THIS VIR_FROM_SECRET @@ -642,3 +646,174 @@ virSecretObjListGetUUIDs(virSecretObjListPtr secrets, } return ret; } + + +static int +virSecretLoadValidateUUID(virSecretDefPtr def, + const char *file) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(def->uuid, uuidstr); + + if (!virFileMatchesNameSuffix(file, uuidstr, ".xml")) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("<uuid> does not match secret file name '%s'"), + file); + return -1; + } + + return 0; +} + + +static int +virSecretLoadValue(virSecretObjPtr secret) +{ + int ret = -1, fd = -1; + struct stat st; + char *contents = NULL, *value = NULL; + size_t value_size; + + if ((fd = open(secret->base64File, O_RDONLY)) == -1) { + if (errno == ENOENT) { + ret = 0; + goto cleanup; + } + virReportSystemError(errno, _("cannot open '%s'"), + secret->base64File); + goto cleanup; + } + + if (fstat(fd, &st) < 0) { + 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"), + secret->base64File); + goto cleanup; + } + + 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'"), + secret->base64File); + goto cleanup; + } + + VIR_FORCE_CLOSE(fd); + + if (!base64_decode_alloc(contents, st.st_size, &value, &value_size)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid base64 in '%s'"), + secret->base64File); + goto cleanup; + } + if (value == NULL) + goto cleanup; + + secret->value = (unsigned char *)value; + value = NULL; + secret->value_size = value_size; + + ret = 0; + + cleanup: + if (value != NULL) { + memset(value, 0, value_size); + VIR_FREE(value); + } + if (contents != NULL) { + memset(contents, 0, st.st_size); + VIR_FREE(contents); + } + VIR_FORCE_CLOSE(fd); + return ret; +} + + +static virSecretObjPtr +virSecretLoad(virSecretObjListPtr secrets, + const char *file, + const char *path, + const char *configDir) +{ + virSecretDefPtr def = NULL; + virSecretObjPtr secret = NULL, ret = NULL; + + if (!(def = virSecretDefParseFile(path))) + goto cleanup; + + if (virSecretLoadValidateUUID(def, file) < 0) + goto cleanup; + + if (!(secret = virSecretObjListAdd(secrets, def, configDir, NULL))) + goto cleanup; + def = NULL; + + if (virSecretLoadValue(secret) < 0) + goto cleanup; + + ret = secret; + secret = NULL; + + cleanup: + if (secret) + virSecretObjListRemove(secrets, secret); + virSecretDefFree(def); + return ret; +} + + +int +virSecretLoadAllConfigs(virSecretObjListPtr secrets, + const char *configDir) +{ + DIR *dir = NULL; + struct dirent *de; + + if (!(dir = opendir(configDir))) { + if (errno == ENOENT) + return 0; + virReportSystemError(errno, _("cannot open '%s'"), configDir); + 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; + virSecretObjPtr secret; + + if (STREQ(de->d_name, ".") || STREQ(de->d_name, "..")) + continue; + + if (!virFileHasSuffix(de->d_name, ".xml")) + continue; + + if (!(path = virFileBuildPath(configDir, de->d_name, NULL))) + continue; + + if (!(secret = virSecretLoad(secrets, de->d_name, path, configDir))) { + virErrorPtr err = virGetLastError(); + + VIR_ERROR(_("Error reading secret: %s"), + err != NULL ? err->message: _("unknown error")); + virResetError(err); + VIR_FREE(path); + continue; + } + + VIR_FREE(path); + virSecretObjEndAPI(&secret); + } + + closedir(dir); + return 0; +} diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h index 514db2f..2e8dcf6 100644 --- a/src/conf/virsecretobj.h +++ b/src/conf/virsecretobj.h @@ -93,4 +93,6 @@ int virSecretObjListGetUUIDs(virSecretObjListPtr secrets, virSecretObjListACLFilter filter, virConnectPtr conn); +int virSecretLoadAllConfigs(virSecretObjListPtr secrets, + const char *configDir); #endif /* __VIRSECRETOBJ_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 603eba5..5a6265f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -895,6 +895,7 @@ virDomainObjListRename; # conf/virsecretobj.h +virSecretLoadAllConfigs; virSecretObjEndAPI; virSecretObjListAdd; virSecretObjListExport; diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 90ec4ba..c8b4163 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -22,7 +22,6 @@ #include <config.h> -#include <dirent.h> #include <fcntl.h> #include <string.h> #include <sys/stat.h> @@ -190,175 +189,6 @@ secretDeleteSaved(const virSecretObj *secret) return 0; } -static int -secretLoadValidateUUID(virSecretDefPtr def, - const char *file) -{ - char uuidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(def->uuid, uuidstr); - - if (!virFileMatchesNameSuffix(file, uuidstr, ".xml")) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("<uuid> does not match secret file name '%s'"), - file); - return -1; - } - - return 0; -} - -static int -secretLoadValue(virSecretObjPtr secret) -{ - int ret = -1, fd = -1; - struct stat st; - char *contents = NULL, *value = NULL; - size_t value_size; - - if ((fd = open(secret->base64File, O_RDONLY)) == -1) { - if (errno == ENOENT) { - ret = 0; - goto cleanup; - } - virReportSystemError(errno, _("cannot open '%s'"), - secret->base64File); - goto cleanup; - } - - if (fstat(fd, &st) < 0) { - 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"), - secret->base64File); - goto cleanup; - } - - 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'"), - secret->base64File); - goto cleanup; - } - - VIR_FORCE_CLOSE(fd); - - if (!base64_decode_alloc(contents, st.st_size, &value, &value_size)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("invalid base64 in '%s'"), - secret->base64File); - goto cleanup; - } - if (value == NULL) - goto cleanup; - - secret->value = (unsigned char *)value; - value = NULL; - secret->value_size = value_size; - - ret = 0; - - cleanup: - if (value != NULL) { - memset(value, 0, value_size); - VIR_FREE(value); - } - if (contents != NULL) { - memset(contents, 0, st.st_size); - VIR_FREE(contents); - } - VIR_FORCE_CLOSE(fd); - return ret; -} - - -static virSecretObjPtr -secretLoad(virSecretObjListPtr secrets, - const char *file, - const char *path, - const char *configDir) -{ - virSecretDefPtr def = NULL; - virSecretObjPtr secret = NULL, ret = NULL; - - if (!(def = virSecretDefParseFile(path))) - goto cleanup; - - if (secretLoadValidateUUID(def, file) < 0) - goto cleanup; - - if (!(secret = virSecretObjListAdd(secrets, def, configDir, NULL))) - goto cleanup; - def = NULL; - - if (secretLoadValue(secret) < 0) - goto cleanup; - - ret = secret; - secret = NULL; - - cleanup: - if (secret) - virSecretObjListRemove(secrets, secret); - virSecretDefFree(def); - return ret; -} - - -static int -secretLoadAllConfigs(virSecretObjListPtr secrets, - const char *configDir) -{ - DIR *dir = NULL; - struct dirent *de; - - if (!(dir = opendir(configDir))) { - if (errno == ENOENT) - return 0; - virReportSystemError(errno, _("cannot open '%s'"), configDir); - 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; - virSecretObjPtr secret; - - if (STREQ(de->d_name, ".") || STREQ(de->d_name, "..")) - continue; - - if (!virFileHasSuffix(de->d_name, ".xml")) - continue; - - if (!(path = virFileBuildPath(configDir, de->d_name, NULL))) - continue; - - if (!(secret = secretLoad(secrets, de->d_name, path, configDir))) { - virErrorPtr err = virGetLastError(); - - VIR_ERROR(_("Error reading secret: %s"), - err != NULL ? err->message: _("unknown error")); - virResetError(err); - VIR_FREE(path); - continue; - } - - VIR_FREE(path); - virSecretObjEndAPI(&secret); - } - - closedir(dir); - return 0; -} - /* Driver functions */ static int @@ -722,7 +552,7 @@ secretStateInitialize(bool privileged, if (!(driver->secrets = virSecretObjListNew())) goto error; - if (secretLoadAllConfigs(driver->secrets, driver->configDir) < 0) + if (virSecretLoadAllConfigs(driver->secrets, driver->configDir) < 0) goto error; secretDriverUnlock(); @@ -743,7 +573,7 @@ secretStateReload(void) secretDriverLock(); - ignore_value(secretLoadAllConfigs(driver->secrets, driver->configDir)); + ignore_value(virSecretLoadAllConfigs(driver->secrets, driver->configDir)); secretDriverUnlock(); return 0; -- 2.5.5
Move and rename secretDeleteSaved from secret_driver into virsecretobj and split it up into two parts since there is error path code that looks to just delete the secret data file Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 23 +++++++++++++++++++++++ src/conf/virsecretobj.h | 4 ++++ src/libvirt_private.syms | 2 ++ src/secret/secret_driver.c | 22 ++++++---------------- 4 files changed, 35 insertions(+), 16 deletions(-) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index e5dafa4..7ad77c7 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -648,6 +648,29 @@ virSecretObjListGetUUIDs(virSecretObjListPtr secrets, } +int +virSecretObjDeleteConfig(virSecretObjPtr secret) +{ + if (!secret->def->ephemeral && + unlink(secret->configFile) < 0 && errno != ENOENT) { + virReportSystemError(errno, _("cannot unlink '%s'"), + secret->configFile); + return -1; + } + + return 0; +} + + +void +virSecretObjDeleteData(virSecretObjPtr secret) +{ + /* The configFile will already be removed, so secret won't be + * loaded again if this fails */ + (void)unlink(secret->base64File); +} + + static int virSecretLoadValidateUUID(virSecretDefPtr def, const char *file) diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h index 2e8dcf6..8f1247a 100644 --- a/src/conf/virsecretobj.h +++ b/src/conf/virsecretobj.h @@ -93,6 +93,10 @@ int virSecretObjListGetUUIDs(virSecretObjListPtr secrets, virSecretObjListACLFilter filter, virConnectPtr conn); +int virSecretObjDeleteConfig(virSecretObjPtr secret); + +void virSecretObjDeleteData(virSecretObjPtr secret); + int virSecretLoadAllConfigs(virSecretObjListPtr secrets, const char *configDir); #endif /* __VIRSECRETOBJ_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5a6265f..6134ac2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -896,6 +896,8 @@ virDomainObjListRename; # conf/virsecretobj.h virSecretLoadAllConfigs; +virSecretObjDeleteConfig; +virSecretObjDeleteData; virSecretObjEndAPI; virSecretObjListAdd; virSecretObjListExport; diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index c8b4163..0767424 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -176,19 +176,6 @@ secretSaveValue(const virSecretObj *secret) return ret; } -static int -secretDeleteSaved(const virSecretObj *secret) -{ - if (unlink(secret->configFile) < 0 && errno != ENOENT) - 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. */ - (void)unlink(secret->base64File); - - return 0; -} - /* Driver functions */ static int @@ -326,8 +313,10 @@ secretDefineXML(virConnectPtr conn, goto restore_backup; } } else if (backup && !backup->ephemeral) { - if (secretDeleteSaved(secret) < 0) + if (virSecretObjDeleteConfig(secret) < 0) goto restore_backup; + + virSecretObjDeleteData(secret); } /* Saved successfully - drop old values */ new_attrs = NULL; @@ -490,10 +479,11 @@ secretUndefine(virSecretPtr obj) if (virSecretUndefineEnsureACL(obj->conn, secret->def) < 0) goto cleanup; - if (!secret->def->ephemeral && - secretDeleteSaved(secret) < 0) + if (virSecretObjDeleteConfig(secret) < 0) goto cleanup; + virSecretObjDeleteData(secret); + virSecretObjListRemove(driver->secrets, secret); ret = 0; -- 2.5.5
Move and rename the secretRewriteFile, secretSaveDef, and secretSaveValue from secret_driver to virsecretobj Need to make some slight adjustments since the secretSave* functions called secretEnsureDirectory, but otherwise mostly just a move of code. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 69 +++++++++++++++++++++++++++++++++++ src/conf/virsecretobj.h | 4 +++ src/libvirt_private.syms | 2 ++ src/secret/secret_driver.c | 90 +++++++--------------------------------------- 4 files changed, 87 insertions(+), 78 deletions(-) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 7ad77c7..a67ffd6 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -671,6 +671,75 @@ virSecretObjDeleteData(virSecretObjPtr secret) } +/* Permanent secret storage */ + +/* 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. */ + +static int +virSecretRewriteFile(int fd, + void *opaque) +{ + char *data = opaque; + + if (safewrite(fd, data, strlen(data)) < 0) + return -1; + + return 0; +} + + +int +virSecretObjSaveConfig(virSecretObjPtr secret) +{ + char *xml = NULL; + int ret = -1; + + if (!(xml = virSecretDefFormat(secret->def))) + goto cleanup; + + if (virFileRewrite(secret->configFile, S_IRUSR | S_IWUSR, + virSecretRewriteFile, xml) < 0) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(xml); + return ret; +} + + +int +virSecretObjSaveData(virSecretObjPtr secret) +{ + char *base64 = NULL; + int ret = -1; + + if (!secret->value) + return 0; + + base64_encode_alloc((const char *)secret->value, secret->value_size, + &base64); + if (base64 == NULL) { + virReportOOMError(); + goto cleanup; + } + + if (virFileRewrite(secret->base64File, S_IRUSR | S_IWUSR, + virSecretRewriteFile, base64) < 0) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(base64); + return ret; +} + + static int virSecretLoadValidateUUID(virSecretDefPtr def, const char *file) diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h index 8f1247a..176896a 100644 --- a/src/conf/virsecretobj.h +++ b/src/conf/virsecretobj.h @@ -97,6 +97,10 @@ int virSecretObjDeleteConfig(virSecretObjPtr secret); void virSecretObjDeleteData(virSecretObjPtr secret); +int virSecretObjSaveConfig(virSecretObjPtr secret); + +int virSecretObjSaveData(virSecretObjPtr secret); + int virSecretLoadAllConfigs(virSecretObjListPtr secrets, const char *configDir); #endif /* __VIRSECRETOBJ_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6134ac2..f03d6a9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -907,6 +907,8 @@ virSecretObjListGetUUIDs; virSecretObjListNew; virSecretObjListNumOfSecrets; virSecretObjListRemove; +virSecretObjSaveConfig; +virSecretObjSaveData; # cpu/cpu.h diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 0767424..5657825 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -92,26 +92,6 @@ secretObjFromSecret(virSecretPtr secret) } -/* Permament secret storage */ - -/* 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. */ - -static int -secretRewriteFile(int fd, - void *opaque) -{ - char *data = opaque; - - if (safewrite(fd, data, strlen(data)) < 0) - return -1; - - return 0; -} - - static int secretEnsureDirectory(void) { @@ -123,59 +103,6 @@ secretEnsureDirectory(void) return 0; } -static int -secretSaveDef(const virSecretObj *secret) -{ - char *xml = NULL; - int ret = -1; - - if (secretEnsureDirectory() < 0) - goto cleanup; - - if (!(xml = virSecretDefFormat(secret->def))) - goto cleanup; - - if (virFileRewrite(secret->configFile, S_IRUSR | S_IWUSR, - secretRewriteFile, xml) < 0) - goto cleanup; - - ret = 0; - - cleanup: - VIR_FREE(xml); - return ret; -} - -static int -secretSaveValue(const virSecretObj *secret) -{ - char *base64 = NULL; - int ret = -1; - - if (secret->value == NULL) - return 0; - - if (secretEnsureDirectory() < 0) - goto cleanup; - - base64_encode_alloc((const char *)secret->value, secret->value_size, - &base64); - if (base64 == NULL) { - virReportOOMError(); - goto cleanup; - } - - if (virFileRewrite(secret->base64File, S_IRUSR | S_IWUSR, - secretRewriteFile, base64) < 0) - goto cleanup; - - ret = 0; - - cleanup: - VIR_FREE(base64); - return ret; -} - /* Driver functions */ static int @@ -301,14 +228,18 @@ secretDefineXML(virConnectPtr conn, goto cleanup; if (!new_attrs->ephemeral) { + if (secretEnsureDirectory() < 0) + goto cleanup; + if (backup && backup->ephemeral) { - if (secretSaveValue(secret) < 0) + if (virSecretObjSaveData(secret) < 0) goto restore_backup; } - if (secretSaveDef(secret) < 0) { + + if (virSecretObjSaveConfig(secret) < 0) { if (backup && backup->ephemeral) { - /* Undo the secretSaveValue() above; ignore errors */ - (void)unlink(secret->base64File); + /* Undo the virSecretObjSaveData() above; ignore errors */ + virSecretObjDeleteData(secret); } goto restore_backup; } @@ -397,7 +328,10 @@ secretSetValue(virSecretPtr obj, secret->value = new_value; secret->value_size = value_size; if (!secret->def->ephemeral) { - if (secretSaveValue(secret) < 0) + if (secretEnsureDirectory() < 0) + goto cleanup; + + if (virSecretObjSaveData(secret) < 0) goto restore_backup; } /* Saved successfully - drop old value */ -- 2.5.5
Introduce fetch and set accessor to the secretObj->def field for usage by the driver to avoid the driver needing to know the format of virSecretObj Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 15 +++++++++++++ src/conf/virsecretobj.h | 4 ++++ src/libvirt_private.syms | 2 ++ src/secret/secret_driver.c | 54 ++++++++++++++++++++++++++++------------------ 4 files changed, 54 insertions(+), 21 deletions(-) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index a67ffd6..8854a32 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -740,6 +740,21 @@ virSecretObjSaveData(virSecretObjPtr secret) } +virSecretDefPtr +virSecretObjGetDef(virSecretObjPtr secret) +{ + return secret->def; +} + + +void +virSecretObjSetDef(virSecretObjPtr secret, + virSecretDefPtr def) +{ + secret->def = def; +} + + static int virSecretLoadValidateUUID(virSecretDefPtr def, const char *file) diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h index 176896a..a9b3103 100644 --- a/src/conf/virsecretobj.h +++ b/src/conf/virsecretobj.h @@ -101,6 +101,10 @@ int virSecretObjSaveConfig(virSecretObjPtr secret); int virSecretObjSaveData(virSecretObjPtr secret); +virSecretDefPtr virSecretObjGetDef(virSecretObjPtr secret); + +void virSecretObjSetDef(virSecretObjPtr secret, virSecretDefPtr def); + int virSecretLoadAllConfigs(virSecretObjListPtr secrets, const char *configDir); #endif /* __VIRSECRETOBJ_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f03d6a9..c2e20b3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -899,6 +899,7 @@ virSecretLoadAllConfigs; virSecretObjDeleteConfig; virSecretObjDeleteData; virSecretObjEndAPI; +virSecretObjGetDef; virSecretObjListAdd; virSecretObjListExport; virSecretObjListFindByUsage; @@ -909,6 +910,7 @@ virSecretObjListNumOfSecrets; virSecretObjListRemove; virSecretObjSaveConfig; virSecretObjSaveData; +virSecretObjSetDef; # cpu/cpu.h diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 5657825..61de3cb 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -153,6 +153,7 @@ secretLookupByUUID(virConnectPtr conn, { virSecretPtr ret = NULL; virSecretObjPtr secret; + virSecretDefPtr def; if (!(secret = virSecretObjListFindByUUID(driver->secrets, uuid))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -162,13 +163,14 @@ secretLookupByUUID(virConnectPtr conn, goto cleanup; } - if (virSecretLookupByUUIDEnsureACL(conn, secret->def) < 0) + def = virSecretObjGetDef(secret); + if (virSecretLookupByUUIDEnsureACL(conn, def) < 0) goto cleanup; ret = virGetSecret(conn, - secret->def->uuid, - secret->def->usage_type, - virSecretUsageIDForDef(secret->def)); + def->uuid, + def->usage_type, + virSecretUsageIDForDef(def)); cleanup: virSecretObjEndAPI(&secret); @@ -183,6 +185,7 @@ secretLookupByUsage(virConnectPtr conn, { virSecretPtr ret = NULL; virSecretObjPtr secret; + virSecretDefPtr def; if (!(secret = virSecretObjListFindByUsage(driver->secrets, usageType, usageID))) { @@ -191,13 +194,14 @@ secretLookupByUsage(virConnectPtr conn, goto cleanup; } - if (virSecretLookupByUsageEnsureACL(conn, secret->def) < 0) + def = virSecretObjGetDef(secret); + if (virSecretLookupByUsageEnsureACL(conn, def) < 0) goto cleanup; ret = virGetSecret(conn, - secret->def->uuid, - secret->def->usage_type, - virSecretUsageIDForDef(secret->def)); + def->uuid, + def->usage_type, + virSecretUsageIDForDef(def)); cleanup: virSecretObjEndAPI(&secret); @@ -250,22 +254,22 @@ secretDefineXML(virConnectPtr conn, virSecretObjDeleteData(secret); } /* Saved successfully - drop old values */ - new_attrs = NULL; virSecretDefFree(backup); ret = virGetSecret(conn, - secret->def->uuid, - secret->def->usage_type, - virSecretUsageIDForDef(secret->def)); + new_attrs->uuid, + new_attrs->usage_type, + virSecretUsageIDForDef(new_attrs)); + new_attrs = NULL; goto cleanup; restore_backup: /* If we have a backup, then secret was defined before, so just restore - * the backup. The current secret->def (new_attrs) will be handled below. + * the backup. The current (new_attrs) will be handled below. * Otherwise, this is a new secret, thus remove it. */ if (backup) - secret->def = backup; + virSecretObjSetDef(secret, backup); else virSecretObjListRemove(driver->secrets, secret); @@ -282,16 +286,18 @@ secretGetXMLDesc(virSecretPtr obj, { char *ret = NULL; virSecretObjPtr secret; + virSecretDefPtr def; virCheckFlags(0, NULL); if (!(secret = secretObjFromSecret(obj))) goto cleanup; - if (virSecretGetXMLDescEnsureACL(obj->conn, secret->def) < 0) + def = virSecretObjGetDef(secret); + if (virSecretGetXMLDescEnsureACL(obj->conn, def) < 0) goto cleanup; - ret = virSecretDefFormat(secret->def); + ret = virSecretDefFormat(def); cleanup: virSecretObjEndAPI(&secret); @@ -309,6 +315,7 @@ secretSetValue(virSecretPtr obj, unsigned char *old_value, *new_value; size_t old_value_size; virSecretObjPtr secret; + virSecretDefPtr def; virCheckFlags(0, -1); @@ -318,7 +325,8 @@ secretSetValue(virSecretPtr obj, if (!(secret = secretObjFromSecret(obj))) goto cleanup; - if (virSecretSetValueEnsureACL(obj->conn, secret->def) < 0) + def = virSecretObjGetDef(secret); + if (virSecretSetValueEnsureACL(obj->conn, def) < 0) goto cleanup; old_value = secret->value; @@ -327,7 +335,7 @@ secretSetValue(virSecretPtr obj, memcpy(new_value, value, value_size); secret->value = new_value; secret->value_size = value_size; - if (!secret->def->ephemeral) { + if (!def->ephemeral) { if (secretEnsureDirectory() < 0) goto cleanup; @@ -366,13 +374,15 @@ secretGetValue(virSecretPtr obj, { unsigned char *ret = NULL; virSecretObjPtr secret; + virSecretDefPtr def; virCheckFlags(0, NULL); if (!(secret = secretObjFromSecret(obj))) goto cleanup; - if (virSecretGetValueEnsureACL(obj->conn, secret->def) < 0) + def = virSecretObjGetDef(secret); + if (virSecretGetValueEnsureACL(obj->conn, def) < 0) goto cleanup; if (secret->value == NULL) { @@ -384,7 +394,7 @@ secretGetValue(virSecretPtr obj, } if ((internalFlags & VIR_SECRET_GET_VALUE_INTERNAL_CALL) == 0 && - secret->def->private) { + def->private) { virReportError(VIR_ERR_INVALID_SECRET, "%s", _("secret is private")); goto cleanup; @@ -406,11 +416,13 @@ secretUndefine(virSecretPtr obj) { int ret = -1; virSecretObjPtr secret; + virSecretDefPtr def; if (!(secret = secretObjFromSecret(obj))) goto cleanup; - if (virSecretUndefineEnsureACL(obj->conn, secret->def) < 0) + def = virSecretObjGetDef(secret); + if (virSecretUndefineEnsureACL(obj->conn, def) < 0) goto cleanup; if (virSecretObjDeleteConfig(secret) < 0) -- 2.5.5
Introduce the final accessor's to _virSecretObject data and move the structure from virsecretobj.h to virsecretobj.c The virSecretObjSetValue logic will handle setting both the secret value and the value_size. Some slight adjustments to the error path over what was in secretSetValue were made. Additionally, a slight logic change in secretGetValue where we'll check for the internalFlags and error out before checking for and erroring out for a NULL secret->value. That way, it won't be obvious to anyone that the secret value wasn't set rather they'll just know they cannot get the secret value since it's private. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/virsecretobj.h | 18 +++++----- src/libvirt_private.syms | 4 +++ src/secret/secret_driver.c | 50 ++++----------------------- 4 files changed, 105 insertions(+), 52 deletions(-) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 8854a32..4f28392 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -36,6 +36,15 @@ VIR_LOG_INIT("conf.virsecretobj"); +struct _virSecretObj { + virObjectLockable parent; + char *configFile; + char *base64File; + virSecretDefPtr def; + unsigned char *value; /* May be NULL */ + size_t value_size; +}; + static virClassPtr virSecretObjClass; static virClassPtr virSecretObjListClass; static void virSecretObjDispose(void *obj); @@ -755,6 +764,82 @@ virSecretObjSetDef(virSecretObjPtr secret, } +unsigned char * +virSecretObjGetValue(virSecretObjPtr secret) +{ + unsigned char *ret = NULL; + + if (!secret->value) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(secret->def->uuid, uuidstr); + virReportError(VIR_ERR_NO_SECRET, + _("secret '%s' does not have a value"), uuidstr); + goto cleanup; + } + + if (VIR_ALLOC_N(ret, secret->value_size) < 0) + goto cleanup; + memcpy(ret, secret->value, secret->value_size); + + cleanup: + return ret; +} + + +int +virSecretObjSetValue(virSecretObjPtr secret, + const unsigned char *value, + size_t value_size) +{ + unsigned char *old_value, *new_value; + size_t old_value_size; + + if (VIR_ALLOC_N(new_value, value_size) < 0) + return -1; + + old_value = secret->value; + old_value_size = secret->value_size; + + memcpy(new_value, value, value_size); + secret->value = new_value; + secret->value_size = value_size; + + if (!secret->def->ephemeral && virSecretObjSaveData(secret) < 0) + goto error; + + /* Saved successfully - drop old value */ + if (old_value) { + memset(old_value, 0, old_value_size); + VIR_FREE(old_value); + } + + return 0; + + error: + /* Error - restore previous state and free new value */ + secret->value = old_value; + secret->value_size = old_value_size; + memset(new_value, 0, value_size); + VIR_FREE(new_value); + return -1; +} + + +size_t +virSecretObjGetValueSize(virSecretObjPtr secret) +{ + return secret->value_size; +} + + +void +virSecretObjSetValueSize(virSecretObjPtr secret, + size_t value_size) +{ + secret->value_size = value_size; +} + + static int virSecretLoadValidateUUID(virSecretDefPtr def, const char *file) diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h index a9b3103..fa45b42 100644 --- a/src/conf/virsecretobj.h +++ b/src/conf/virsecretobj.h @@ -28,15 +28,6 @@ typedef struct _virSecretObj virSecretObj; typedef virSecretObj *virSecretObjPtr; -struct _virSecretObj { - virObjectLockable parent; - char *configFile; - char *base64File; - virSecretDefPtr def; - unsigned char *value; /* May be NULL */ - size_t value_size; -}; - virSecretObjPtr virSecretObjNew(void); @@ -105,6 +96,15 @@ virSecretDefPtr virSecretObjGetDef(virSecretObjPtr secret); void virSecretObjSetDef(virSecretObjPtr secret, virSecretDefPtr def); +unsigned char *virSecretObjGetValue(virSecretObjPtr secret); + +int virSecretObjSetValue(virSecretObjPtr secret, + const unsigned char *value, size_t value_size); + +size_t virSecretObjGetValueSize(virSecretObjPtr secret); + +void virSecretObjSetValueSize(virSecretObjPtr secret, size_t value_size); + int virSecretLoadAllConfigs(virSecretObjListPtr secrets, const char *configDir); #endif /* __VIRSECRETOBJ_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c2e20b3..ad5c382 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -900,6 +900,8 @@ virSecretObjDeleteConfig; virSecretObjDeleteData; virSecretObjEndAPI; virSecretObjGetDef; +virSecretObjGetValue; +virSecretObjGetValueSize; virSecretObjListAdd; virSecretObjListExport; virSecretObjListFindByUsage; @@ -911,6 +913,8 @@ virSecretObjListRemove; virSecretObjSaveConfig; virSecretObjSaveData; virSecretObjSetDef; +virSecretObjSetValue; +virSecretObjSetValueSize; # cpu/cpu.h diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 61de3cb..bbfb382 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -312,16 +312,11 @@ secretSetValue(virSecretPtr obj, unsigned int flags) { int ret = -1; - unsigned char *old_value, *new_value; - size_t old_value_size; virSecretObjPtr secret; virSecretDefPtr def; virCheckFlags(0, -1); - if (VIR_ALLOC_N(new_value, value_size) < 0) - return -1; - if (!(secret = secretObjFromSecret(obj))) goto cleanup; @@ -329,40 +324,17 @@ secretSetValue(virSecretPtr obj, if (virSecretSetValueEnsureACL(obj->conn, def) < 0) goto cleanup; - old_value = secret->value; - old_value_size = secret->value_size; - - memcpy(new_value, value, value_size); - secret->value = new_value; - secret->value_size = value_size; - if (!def->ephemeral) { - if (secretEnsureDirectory() < 0) - goto cleanup; + if (secretEnsureDirectory() < 0) + goto cleanup; - if (virSecretObjSaveData(secret) < 0) - goto restore_backup; - } - /* Saved successfully - drop old value */ - if (old_value != NULL) { - memset(old_value, 0, old_value_size); - VIR_FREE(old_value); - } - new_value = NULL; + if (virSecretObjSetValue(secret, value, value_size) < 0) + goto cleanup; ret = 0; - goto cleanup; - - restore_backup: - /* Error - restore previous state and free new value */ - secret->value = old_value; - secret->value_size = old_value_size; - memset(new_value, 0, value_size); cleanup: virSecretObjEndAPI(&secret); - VIR_FREE(new_value); - return ret; } @@ -385,14 +357,6 @@ secretGetValue(virSecretPtr obj, if (virSecretGetValueEnsureACL(obj->conn, def) < 0) goto cleanup; - if (secret->value == NULL) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(obj->uuid, uuidstr); - virReportError(VIR_ERR_NO_SECRET, - _("secret '%s' does not have a value"), uuidstr); - goto cleanup; - } - if ((internalFlags & VIR_SECRET_GET_VALUE_INTERNAL_CALL) == 0 && def->private) { virReportError(VIR_ERR_INVALID_SECRET, "%s", @@ -400,10 +364,10 @@ secretGetValue(virSecretPtr obj, goto cleanup; } - if (VIR_ALLOC_N(ret, secret->value_size) < 0) + if (!(ret = virSecretObjGetValue(secret))) goto cleanup; - memcpy(ret, secret->value, secret->value_size); - *value_size = secret->value_size; + + *value_size = virSecretObjGetValueSize(secret); cleanup: virSecretObjEndAPI(&secret); -- 2.5.5
Change 'ephemeral' to 'isephemeral' and 'private' to 'isprivate' since both are bools. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/secret_conf.c | 12 ++++++------ src/conf/secret_conf.h | 4 ++-- src/conf/virsecretobj.c | 14 +++++++------- src/secret/secret_driver.c | 10 +++++----- src/storage/storage_backend.c | 4 ++-- 5 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index 5c39f24..de9e6cf 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -183,9 +183,9 @@ secretXMLParseNode(xmlDocPtr xml, xmlNodePtr root) prop = virXPathString("string(./@ephemeral)", ctxt); if (prop != NULL) { if (STREQ(prop, "yes")) { - def->ephemeral = true; + def->isephemeral = true; } else if (STREQ(prop, "no")) { - def->ephemeral = false; + def->isephemeral = false; } else { virReportError(VIR_ERR_XML_ERROR, "%s", _("invalid value of 'ephemeral'")); @@ -197,9 +197,9 @@ secretXMLParseNode(xmlDocPtr xml, xmlNodePtr root) prop = virXPathString("string(./@private)", ctxt); if (prop != NULL) { if (STREQ(prop, "yes")) { - def->private = true; + def->isprivate = true; } else if (STREQ(prop, "no")) { - def->private = false; + def->isprivate = false; } else { virReportError(VIR_ERR_XML_ERROR, "%s", _("invalid value of 'private'")); @@ -325,8 +325,8 @@ virSecretDefFormat(const virSecretDef *def) char uuidstr[VIR_UUID_STRING_BUFLEN]; virBufferAsprintf(&buf, "<secret ephemeral='%s' private='%s'>\n", - def->ephemeral ? "yes" : "no", - def->private ? "yes" : "no"); + def->isephemeral ? "yes" : "no", + def->isprivate ? "yes" : "no"); uuid = def->uuid; virUUIDFormat(uuid, uuidstr); diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index 5ca4ecd..ca1afec 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -31,8 +31,8 @@ VIR_ENUM_DECL(virSecretUsage) typedef struct _virSecretDef virSecretDef; typedef virSecretDef *virSecretDefPtr; struct _virSecretDef { - bool ephemeral; - bool private; + bool isephemeral; + bool isprivate; unsigned char uuid[VIR_UUID_BUFLEN]; char *description; /* May be NULL */ int usage_type; diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 4f28392..721e0b4 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -369,7 +369,7 @@ virSecretObjListAddLocked(virSecretObjListPtr secrets, goto cleanup; } - if (secret->def->private && !def->private) { + if (secret->def->isprivate && !def->isprivate) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot change private flag on existing secret")); goto cleanup; @@ -517,17 +517,17 @@ virSecretObjMatchFlags(virSecretObjPtr secret, /* filter by whether it's ephemeral */ if (MATCH(VIR_CONNECT_LIST_SECRETS_FILTERS_EPHEMERAL) && !((MATCH(VIR_CONNECT_LIST_SECRETS_EPHEMERAL) && - secret->def->ephemeral) || + secret->def->isephemeral) || (MATCH(VIR_CONNECT_LIST_SECRETS_NO_EPHEMERAL) && - !secret->def->ephemeral))) + !secret->def->isephemeral))) return false; /* filter by whether it's private */ if (MATCH(VIR_CONNECT_LIST_SECRETS_FILTERS_PRIVATE) && !((MATCH(VIR_CONNECT_LIST_SECRETS_PRIVATE) && - secret->def->private) || + secret->def->isprivate) || (MATCH(VIR_CONNECT_LIST_SECRETS_NO_PRIVATE) && - !secret->def->private))) + !secret->def->isprivate))) return false; return true; @@ -660,7 +660,7 @@ virSecretObjListGetUUIDs(virSecretObjListPtr secrets, int virSecretObjDeleteConfig(virSecretObjPtr secret) { - if (!secret->def->ephemeral && + if (!secret->def->isephemeral && unlink(secret->configFile) < 0 && errno != ENOENT) { virReportSystemError(errno, _("cannot unlink '%s'"), secret->configFile); @@ -804,7 +804,7 @@ virSecretObjSetValue(virSecretObjPtr secret, secret->value = new_value; secret->value_size = value_size; - if (!secret->def->ephemeral && virSecretObjSaveData(secret) < 0) + if (!secret->def->isephemeral && virSecretObjSaveData(secret) < 0) goto error; /* Saved successfully - drop old value */ diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index bbfb382..b5b0bea 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -231,23 +231,23 @@ secretDefineXML(virConnectPtr conn, driver->configDir, &backup))) goto cleanup; - if (!new_attrs->ephemeral) { + if (!new_attrs->isephemeral) { if (secretEnsureDirectory() < 0) goto cleanup; - if (backup && backup->ephemeral) { + if (backup && backup->isephemeral) { if (virSecretObjSaveData(secret) < 0) goto restore_backup; } if (virSecretObjSaveConfig(secret) < 0) { - if (backup && backup->ephemeral) { + if (backup && backup->isephemeral) { /* Undo the virSecretObjSaveData() above; ignore errors */ virSecretObjDeleteData(secret); } goto restore_backup; } - } else if (backup && !backup->ephemeral) { + } else if (backup && !backup->isephemeral) { if (virSecretObjDeleteConfig(secret) < 0) goto restore_backup; @@ -358,7 +358,7 @@ secretGetValue(virSecretPtr obj, goto cleanup; if ((internalFlags & VIR_SECRET_GET_VALUE_INTERNAL_CALL) == 0 && - def->private) { + def->isprivate) { virReportError(VIR_ERR_INVALID_SECRET, "%s", _("secret is private")); goto cleanup; diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a8fff14..0e2fd10 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -626,8 +626,8 @@ virStorageGenerateQcowEncryption(virConnectPtr conn, VIR_ALLOC(def) < 0) goto cleanup; - def->ephemeral = false; - def->private = false; + def->isephemeral = false; + def->isprivate = false; if (virStorageGenerateSecretUUID(conn, def->uuid) < 0) goto cleanup; -- 2.5.5
On 04/20/2016 07:40 AM, John Ferlan wrote:
v1: http://www.redhat.com/archives/libvir-list/2016-March/msg00099.html
Differences high level: - Use virsecretobj.{c,h} instead of secret_conf.{c,h} - Work in v1 review comments
Difference details for those that care to know:
Note: Former patch2 has already been pushed.
Patch 1: Is combination of v1's patch1 and patch3 w/ adjustments:
- Create virsecretobj.c and virsecretobj.h and move sources there. - Add virsecretobj.{c,h} to src/Makefile.am - Use order as suggested by eblake review of patch3 for forward static decls, removed the != NULL from virSecretObjDispose, and add comment regarding why memset() is being done
Patch 2: Former patch4 - Move code to virsecretobj.{c,h} instead of secret_conf.{c,h} - Change the virSecretObjFindByUUIDLocked to use the construct "return virObjectRef(virHashLookup(...));" as suggested by eblake
Patch3: Former patch5 (split) - Split the format patch5 to have virSecretUsageIDForDef changes in one patch and the remainder in the followup patch.
Patch4: Former patch5 (split) - The rest of former patch5 - Removed the extraneous "VIR_FREE(configFile);" as noted by eblake. - NOTE: eblake queried about the condition:
if (secret->def->private && !def->private)
in virSecretObjListAddLocked. This same check came from the former secretDefineXML in the else of the "secretFindByUUID(new_attrs->uuid)". IOW: Redefining a secret. The code didn't allow a "new" secret to be not private if the currently defined one was private. I left it as is.
Patch5->Patch9: Former patch6->patch10: - All that's different is using virsecretobj instead of secret_conf
Patch10: Former patch11 - Using virsecretobj instead of secret_conf
Patch11: Former patch10 - Adjusted and moved the comment from virSecretObjDeleteConfig to virSecretObjDeleteData - Added a virReportSystemError for the DeleteConfig error
Patch12: Former patch13: - All that's different is using virsecretobj instead of secret_conf
Patch13: Former patch14 - Delete the extra space in the "return 0" as noted by Cole's review.
Patch14: Former patch15: - All that's different is using virsecretobj instead of secret_conf
After all is said done, I did a sdiff between the v1 secret_conf.h and v2 virsecretobj.h as well as the v1 secret_conf.c and v2 virsecretobj.c and found only the differences as noted above, plus removed a duplicated virSecretUsageIDForDef prototype I found in secret_conf.h.
I also went through the painful process of make check syntax-check at each step of the way and built using Coverity.
John Ferlan (14): secret: Create virsecretobj.c and virsecretconf.h secret: Introduce virSecretObjListFindBy{UUID|Usage} support secret: Introduce virSecretUsageIDForDef secret: Introduce virSecretObjListAdd* and virSecretObjListRemove secret: Introduce virSecretObjListNumOfSecrets secret: Introduce virSecretObjListExport secret: Introduce virSecretObjListGetUUIDs secret: Use the hashed virSecretObjList secret: Move and rename secretLoadAllConfigs secret: Introduce virSecretObjDelete{Config|Data} secret: Introduce virSecretObjSave{Config|Data} secret: Introduce virSecretObj{Get|Set}Def secret: Introduce virSecretObjGetValue and virSecretObjGetValueSize secret: Change virSecretDef variable names
po/POTFILES.in | 1 + src/Makefile.am | 3 +- src/conf/secret_conf.c | 37 +- src/conf/secret_conf.h | 9 +- src/conf/virsecretobj.c | 1011 +++++++++++++++++++++++++++++++++++++++++ src/conf/virsecretobj.h | 110 +++++ src/libvirt_private.syms | 24 + src/secret/secret_driver.c | 823 ++++----------------------------- src/storage/storage_backend.c | 4 +- 9 files changed, 1279 insertions(+), 743 deletions(-) create mode 100644 src/conf/virsecretobj.c create mode 100644 src/conf/virsecretobj.h
ACK (I reviewed the changes you mention above, but just skimmed the rest, though i reviewed majority of v1) - Cole
participants (2)
-
Cole Robinson -
John Ferlan