[libvirt] [PATCH 00/10] Convert secret driver to use hashed object
This series replaces the usage of linked list in secret driver with a hashed object list as was suggested in the review in the last patch of the previous secret driver changes series: http://www.redhat.com/archives/libvir-list/2016-February/msg01274.html Patch 1 - starts the ball rolling Patch 2 - is yet another code optimization (as found in bridge_driver) Patches 3-8 - Add the new API's "slowly" (for review purposes) Patch 9 - replaces the usage of linked lists with the hashed object Patch 10 - moves the secretLoadAllConfigs to secret_conf The changes are loosely modeled after both virdomainobj and network_conf functionality. The real meat and potato(e)s is found in patches 5 and 9. Other functions should be relatively straightforward. I've done testing of various virsh secret-* commands (both valid and invalid options) as well as performing driver start, stop, and reload testing. Each patch was built using 'check' and 'syntax-check'. The end result is much more code in secret_conf and much less in secret_driver. John Ferlan (10): secret: Move virSecretObj to secret_conf.h Add secretObjFromSecret secret: Add hashed virSecretObj and virSecretObjList secret: Introduce virSecretObjListFindBy{UUID|Usage} support secret: Introduce virSecretObjListAdd* and virSecretObjListRemove secret: Introduce virSecretObjListNumOfSecrets secret: Introduce virSecretObjListExport secret: Introduce virSecretObjListGetUUIDs secret: Use the hashed virSecretObjList secret: Move and rename secretLoadAllConfigs src/conf/secret_conf.c | 819 ++++++++++++++++++++++++++++++++++++++++++++- src/conf/secret_conf.h | 76 ++++- src/libvirt_private.syms | 11 + src/secret/secret_driver.c | 647 +++-------------------------------- 4 files changed, 957 insertions(+), 596 deletions(-) -- 2.5.0
To support being able to create a hashed secrets list, move the virSecretObj to secret_conf.h so that secret_conf.c can at least find the definition. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/secret_conf.h | 13 ++++++++++++- src/secret/secret_driver.c | 11 ----------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index 9c13f05..8eedb40 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,17 @@ struct _virSecretDef { } usage; }; +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; +}; + void virSecretDefFree(virSecretDefPtr def); virSecretDefPtr virSecretDefParseString(const char *xml); virSecretDefPtr virSecretDefParseFile(const char *filename); diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 36e28b9..b07cc6f 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -52,17 +52,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.0
On Wed, Mar 02, 2016 at 13:54:58 -0500, John Ferlan wrote:
To support being able to create a hashed secrets list, move the virSecretObj to secret_conf.h so that secret_conf.c can at least find the definition.
I don't think this is necessary. You still can create accessors and have the actual implementation of the struct private in the .c file. That way it's guaranteed that nobody will touch the fields directly accross the source files. Peter
On 03/07/2016 10:50 AM, Peter Krempa wrote:
On Wed, Mar 02, 2016 at 13:54:58 -0500, John Ferlan wrote:
To support being able to create a hashed secrets list, move the virSecretObj to secret_conf.h so that secret_conf.c can at least find the definition.
I don't think this is necessary. You still can create accessors and have the actual implementation of the struct private in the .c file. That way it's guaranteed that nobody will touch the fields directly accross the source files.
Suffice to say there's still more work to do; however, short term I think it needs to move so I can make some progress. I will have a followup series that will move _virSecretObj into secret_conf.c. The goal for this series was to build up the code to support a hashed secret object while still having that linked list in the driver. Patch 10 was added to this series mainly since it was discussed in danpb's review of my last set of secret driver changes. Moving the saving of the config file and secret data file was next on the todo list. I should note that the existing model has virDomainObj in src/conf/domain_conf.h and virNetworkObj in src/conf/network_conf.h, so virSecretObj wouldn't be unique... I honestly didn't want another 15-20 patch series floating around. Make some progress, share, get buy in, continue. The question then becomes does the rest of the series look good? Tks - John
On 03/07/2016 04:47 PM, John Ferlan wrote:
On 03/07/2016 10:50 AM, Peter Krempa wrote:
On Wed, Mar 02, 2016 at 13:54:58 -0500, John Ferlan wrote:
To support being able to create a hashed secrets list, move the virSecretObj to secret_conf.h so that secret_conf.c can at least find the definition.
I don't think this is necessary. You still can create accessors and have the actual implementation of the struct private in the .c file. That way it's guaranteed that nobody will touch the fields directly accross the source files.
Suffice to say there's still more work to do; however, short term I think it needs to move so I can make some progress. I will have a followup series that will move _virSecretObj into secret_conf.c.
The goal for this series was to build up the code to support a hashed secret object while still having that linked list in the driver. Patch 10 was added to this series mainly since it was discussed in danpb's review of my last set of secret driver changes. Moving the saving of the config file and secret data file was next on the todo list.
I should note that the existing model has virDomainObj in src/conf/domain_conf.h and virNetworkObj in src/conf/network_conf.h, so virSecretObj wouldn't be unique...
Precedence is a strong argument; so make mention of that in the commit message. I'm personally okay with this patch, but I'll go ahead and review the rest of the series first before deciding if attempting to use accessors and keeping the struct private is worth a respin, or whether to ack this as-is.
I honestly didn't want another 15-20 patch series floating around. Make some progress, share, get buy in, continue. The question then becomes does the rest of the series look good?
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 45 +++++++++++++++++++++------------------------ 1 file changed, 21 insertions(+), 24 deletions(-) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index b07cc6f..8c1f79a 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -173,6 +173,23 @@ secretAssignDef(virSecretObjPtr *list, return secret; } + +static virSecretObjPtr +secretObjFromSecret(virSecretPtr secret) +{ + virSecretObjPtr obj; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + if (!(obj = secretFindByUUID(secret->uuid))) { + virUUIDFormat(secret->uuid, uuidstr); + virReportError(VIR_ERR_NO_SECRET, + _("no secret with matching uuid '%s'"), uuidstr); + return NULL; + } + return obj; +} + + /* Permament secret storage */ /* Secrets are stored in virSecretDriverStatePtr->configDir. Each secret @@ -847,13 +864,8 @@ secretGetXMLDesc(virSecretPtr obj, secretDriverLock(); - if (!(secret = secretFindByUUID(obj->uuid))) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(obj->uuid, uuidstr); - virReportError(VIR_ERR_NO_SECRET, - _("no secret with matching uuid '%s'"), uuidstr); + if (!(secret = secretObjFromSecret(obj))) goto cleanup; - } if (virSecretGetXMLDescEnsureACL(obj->conn, secret->def) < 0) goto cleanup; @@ -884,13 +896,8 @@ secretSetValue(virSecretPtr obj, secretDriverLock(); - if (!(secret = secretFindByUUID(obj->uuid))) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(obj->uuid, uuidstr); - virReportError(VIR_ERR_NO_SECRET, - _("no secret with matching uuid '%s'"), uuidstr); + if (!(secret = secretObjFromSecret(obj))) goto cleanup; - } if (virSecretSetValueEnsureACL(obj->conn, secret->def) < 0) goto cleanup; @@ -942,13 +949,8 @@ secretGetValue(virSecretPtr obj, secretDriverLock(); - if (!(secret = secretFindByUUID(obj->uuid))) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(obj->uuid, uuidstr); - virReportError(VIR_ERR_NO_SECRET, - _("no secret with matching uuid '%s'"), uuidstr); + if (!(secret = secretObjFromSecret(obj))) goto cleanup; - } if (virSecretGetValueEnsureACL(obj->conn, secret->def) < 0) goto cleanup; @@ -987,13 +989,8 @@ secretUndefine(virSecretPtr obj) secretDriverLock(); - if (!(secret = secretFindByUUID(obj->uuid))) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(obj->uuid, uuidstr); - virReportError(VIR_ERR_NO_SECRET, - _("no secret with matching uuid '%s'"), uuidstr); + if (!(secret = secretObjFromSecret(obj))) goto cleanup; - } if (virSecretUndefineEnsureACL(obj->conn, secret->def) < 0) goto cleanup; -- 2.5.0
On Wed, Mar 02, 2016 at 13:54:59 -0500, John Ferlan wrote:
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 45 +++++++++++++++++++++------------------------ 1 file changed, 21 insertions(+), 24 deletions(-)
ACK
On 03/02/2016 11:54 AM, John Ferlan wrote:
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 45 +++++++++++++++++++++------------------------ 1 file changed, 21 insertions(+), 24 deletions(-)
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index b07cc6f..8c1f79a 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -173,6 +173,23 @@ secretAssignDef(virSecretObjPtr *list, return secret; }
+ +static virSecretObjPtr +secretObjFromSecret(virSecretPtr secret) +{ + virSecretObjPtr obj; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + if (!(obj = secretFindByUUID(secret->uuid))) { + virUUIDFormat(secret->uuid, uuidstr); + virReportError(VIR_ERR_NO_SECRET, + _("no secret with matching uuid '%s'"), uuidstr); + return NULL; + } + return obj; +} + + /* Permament secret storage */
s/Permament/Permanent/ while you are touching this -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Add definitions and infrastucture required for 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. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/secret_conf.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++- src/conf/secret_conf.h | 9 ++++ 2 files changed, 123 insertions(+), 1 deletion(-) diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index 4eebae5..675fc3f 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 @@ -31,6 +31,7 @@ #include "virerror.h" #include "virxml.h" #include "viruuid.h" +#include "virhash.h" #define VIR_FROM_THIS VIR_FROM_SECRET @@ -39,6 +40,118 @@ VIR_LOG_INIT("conf.secret_conf"); VIR_ENUM_IMPL(virSecretUsage, VIR_SECRET_USAGE_TYPE_LAST, "none", "volume", "ceph", "iscsi") + +static virClassPtr virSecretObjClass; +static void virSecretObjDispose(void *obj); +static virClassPtr virSecretObjListClass; +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 != NULL) { + 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); +} + + void virSecretDefFree(virSecretDefPtr def) { diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index 8eedb40..76b805c 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -54,6 +54,15 @@ struct _virSecretObj { size_t value_size; }; +virSecretObjPtr virSecretObjNew(void); + +void virSecretObjEndAPI(virSecretObjPtr *secret); + +typedef struct _virSecretObjList virSecretObjList; +typedef virSecretObjList *virSecretObjListPtr; + +virSecretObjListPtr virSecretObjListNew(void); + void virSecretDefFree(virSecretDefPtr def); virSecretDefPtr virSecretDefParseString(const char *xml); virSecretDefPtr virSecretDefParseFile(const char *filename); -- 2.5.0
On 03/02/2016 11:55 AM, John Ferlan wrote:
Add definitions and infrastucture required for 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.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/secret_conf.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++- src/conf/secret_conf.h | 9 ++++ 2 files changed, 123 insertions(+), 1 deletion(-)
+ +static virClassPtr virSecretObjClass; +static void virSecretObjDispose(void *obj); +static virClassPtr virSecretObjListClass; +static void virSecretObjListDispose(void *obj);
I'm comparing against src/conf/network_conf.c, which lists the two objects prior to the two function forward declarations (that is, swap lines 2 and 3 of the above).
+ +struct _virSecretObjList { + virObjectLockable parent; + + /* uuid string -> virSecretObj mapping + * for O(1), lockless lookup-by-uuid */ + virHashTable *objs; +};
network_conf.c doesn't have the nice comment; but I like it here.
+ +static void +virSecretObjDispose(void *obj) +{ + virSecretObjPtr secret = obj; + + virSecretDefFree(secret->def); + if (secret->value != NULL) {
I might have dropped the '!= NULL', but that's cosmetic.
+ memset(secret->value, 0, secret->value_size); + VIR_FREE(secret->value);
Maybe worth a comment that we intentionally wipe before free, to minimize the chance of leaving a no-longer-used secret on the heap? The name 'Secret' kind of makes it apparent why we spend the extra time doing that, when most uses of VIR_FREE intentionally avoid the extra wipe in the name of speed, but the comment may still be useful. Addressing any of my comments shouldn't affect behavior, so: ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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/secret_conf.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/secret_conf.h | 14 +++++ 2 files changed, 158 insertions(+) diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index 675fc3f..d082ad8 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -152,6 +152,150 @@ virSecretObjListDispose(void *obj) } +/** + * 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) +{ + virSecretObjPtr ret = NULL; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(uuid, uuidstr); + + ret = virHashLookup(secrets->objs, uuidstr); + if (ret) + virObjectRef(ret); + return ret; +} + + +/** + * 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; +} + + void virSecretDefFree(virSecretDefPtr def) { diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index 76b805c..53e03fe 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -63,6 +63,20 @@ 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); + void virSecretDefFree(virSecretDefPtr def); virSecretDefPtr virSecretDefParseString(const char *xml); virSecretDefPtr virSecretDefParseFile(const char *filename); -- 2.5.0
On 03/02/2016 11:55 AM, John Ferlan wrote:
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/secret_conf.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/secret_conf.h | 14 +++++ 2 files changed, 158 insertions(+)
+/** + * virSecretObjFindByUUIDLocked:
+ ret = virHashLookup(secrets->objs, uuidstr); + if (ret) + virObjectRef(ret); + return ret;
It's safe to write: return virObjectRef(virHashLookup(...)); since virObjectRef intentionally has sane handling of NULL. Of course, you were just copying existing practice, and maybe the more verbose form is arguably more legible, so I don't care if you change it. (Or maybe a followup patch the other code you were copying from to use the compact form...) ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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. 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/secret_conf.c | 173 +++++++++++++++++++++++++++++++++++++++++++++ src/conf/secret_conf.h | 15 ++++ src/libvirt_private.syms | 1 + src/secret/secret_driver.c | 34 ++------- 4 files changed, 196 insertions(+), 27 deletions(-) diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index d082ad8..d45910f 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -32,6 +32,7 @@ #include "virxml.h" #include "viruuid.h" #include "virhash.h" +#include "virfile.h" #define VIR_FROM_THIS VIR_FROM_SECRET @@ -296,6 +297,178 @@ virSecretObjListFindByUsage(virSecretObjListPtr secrets, } +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; + } +} + + +/* + * 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"))) { + VIR_FREE(configFile); + 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; +} + + void virSecretDefFree(virSecretDefPtr def) { diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index 53e03fe..7e0651d 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -77,6 +77,21 @@ virSecretObjPtr virSecretObjListFindByUsage(virSecretObjListPtr secrets, int usageType, const char *usageID); +const char *virSecretUsageIDForDef(virSecretDefPtr def); + +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); + 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 5251dc0..3810604 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -785,6 +785,7 @@ virSecretDefFormat; virSecretDefFree; virSecretDefParseFile; virSecretDefParseString; +virSecretUsageIDForDef; virSecretUsageTypeFromString; virSecretUsageTypeToString; diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 8c1f79a..9d92619 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -566,26 +566,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 @@ -639,7 +619,7 @@ secretConnectListAllSecrets(virConnectPtr conn, virGetSecret(conn, secret->def->uuid, secret->def->usage_type, - secretUsageIDForDef(secret->def)))) + virSecretUsageIDForDef(secret->def)))) goto cleanup; } ret_nsecrets++; @@ -690,7 +670,7 @@ secretLookupByUUID(virConnectPtr conn, ret = virGetSecret(conn, secret->def->uuid, secret->def->usage_type, - secretUsageIDForDef(secret->def)); + virSecretUsageIDForDef(secret->def)); cleanup: secretDriverUnlock(); @@ -720,7 +700,7 @@ secretLookupByUsage(virConnectPtr conn, ret = virGetSecret(conn, secret->def->uuid, secret->def->usage_type, - secretUsageIDForDef(secret->def)); + virSecretUsageIDForDef(secret->def)); cleanup: secretDriverUnlock(); @@ -751,7 +731,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))) { @@ -784,8 +764,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); @@ -830,7 +810,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.0
On 03/02/2016 11:55 AM, John Ferlan wrote:
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.
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/secret_conf.c | 173 +++++++++++++++++++++++++++++++++++++++++++++ src/conf/secret_conf.h | 15 ++++ src/libvirt_private.syms | 1 + src/secret/secret_driver.c | 34 ++------- 4 files changed, 196 insertions(+), 27 deletions(-)
+virSecretObjPtr +virSecretObjListAddLocked(virSecretObjListPtr secrets, + virSecretDefPtr def, + const char *configDir, + virSecretDefPtr *oldDef)
+ + if (secret->def->private && !def->private) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot change private flag on existing secret")); + goto cleanup; + }
This says we can't turn an existing public secret into a private one, but nothing is protecting us from the converse of someone attempting to redefine a private secret into a public one. I think this should be: if (secret->def->private != def->private)
+ + /* 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"))) { + VIR_FREE(configFile);
Redundant; the cleanup label also takes care of freeing configFile.
+ goto cleanup; + } +
Everything else looked okay. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 03/18/2016 05:31 PM, Eric Blake wrote:
On 03/02/2016 11:55 AM, John Ferlan wrote:
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.
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/secret_conf.c | 173 +++++++++++++++++++++++++++++++++++++++++++++ src/conf/secret_conf.h | 15 ++++ src/libvirt_private.syms | 1 + src/secret/secret_driver.c | 34 ++------- 4 files changed, 196 insertions(+), 27 deletions(-)
+virSecretObjPtr +virSecretObjListAddLocked(virSecretObjListPtr secrets, + virSecretDefPtr def, + const char *configDir, + virSecretDefPtr *oldDef)
+ + if (secret->def->private && !def->private) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot change private flag on existing secret")); + goto cleanup; + }
This says we can't turn an existing public secret into a private one, but nothing is protecting us from the converse of someone attempting to redefine a private secret into a public one. I think this should be:
if (secret->def->private != def->private)
This is a copy of current check in secretDefineXML... The secret->def->private is our stored one and def->private is the new one. As I'm reading it, it looks right; however, going with the suggestion means one could not change either way (e.g. public->private or private->public). Then again maybe I'm reading your comment wrong. FWIW: I've changed the name later to "isprivate" to make it clearer (and to avoid that potential magic word private).
+ + /* 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"))) { + VIR_FREE(configFile);
Redundant; the cleanup label also takes care of freeing configFile.
Oh right - relic of intermediate edits where goto label didn't exist Thanks for sticking with this so far - John
+ goto cleanup; + } +
Everything else looked okay.
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/secret_conf.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ src/conf/secret_conf.h | 7 +++++++ 2 files changed, 51 insertions(+) diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index d45910f..6f11317 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -469,6 +469,50 @@ virSecretObjListAdd(virSecretObjListPtr secrets, } +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; +} + + void virSecretDefFree(virSecretDefPtr def) { diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index 7e0651d..59eb339 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -92,6 +92,13 @@ virSecretObjPtr virSecretObjListAdd(virSecretObjListPtr secrets, const char *configDir, virSecretDefPtr *oldDef); +typedef bool (*virSecretObjListACLFilter)(virConnectPtr conn, + virSecretDefPtr def); + +int virSecretObjListNumOfSecrets(virSecretObjListPtr secrets, + virSecretObjListACLFilter filter, + virConnectPtr conn); + void virSecretDefFree(virSecretDefPtr def); virSecretDefPtr virSecretDefParseString(const char *xml); virSecretDefPtr virSecretDefParseFile(const char *filename); -- 2.5.0
On 03/02/2016 11:55 AM, John Ferlan wrote:
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/secret_conf.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ src/conf/secret_conf.h | 7 +++++++ 2 files changed, 51 insertions(+)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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/secret_conf.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/secret_conf.h | 8 ++++ 2 files changed, 125 insertions(+) diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index 6f11317..92ac4cd 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -513,6 +513,123 @@ virSecretObjListNumOfSecrets(virSecretObjListPtr secrets, } +#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; +} + + void virSecretDefFree(virSecretDefPtr def) { diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index 59eb339..a3acd54 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -99,6 +99,14 @@ int virSecretObjListNumOfSecrets(virSecretObjListPtr secrets, virSecretObjListACLFilter filter, virConnectPtr conn); +const char *virSecretUsageIDForDef(virSecretDefPtr def); + +int virSecretObjListExport(virConnectPtr conn, + virSecretObjListPtr secretobjs, + virSecretPtr **secrets, + virSecretObjListACLFilter filter, + unsigned int flags); + void virSecretDefFree(virSecretDefPtr def); virSecretDefPtr virSecretDefParseString(const char *xml); virSecretDefPtr virSecretDefParseFile(const char *filename); -- 2.5.0
On 03/02/2016 11:55 AM, John Ferlan wrote:
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/secret_conf.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/secret_conf.h | 8 ++++ 2 files changed, 125 insertions(+)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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/secret_conf.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++- src/conf/secret_conf.h | 6 ++++++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index 92ac4cd..0e73c2d 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -473,6 +473,9 @@ struct virSecretObjListGetHelperData { virConnectPtr conn; virSecretObjListACLFilter filter; int got; + char **uuids; + int nuuids; + bool error; }; @@ -484,11 +487,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: @@ -503,7 +522,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); @@ -630,6 +650,37 @@ virSecretObjListExport(virConnectPtr conn, } +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; +} + + void virSecretDefFree(virSecretDefPtr def) { diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index a3acd54..15b07d5 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -107,6 +107,12 @@ int virSecretObjListExport(virConnectPtr conn, virSecretObjListACLFilter filter, unsigned int flags); +int virSecretObjListGetUUIDs(virSecretObjListPtr secrets, + char **uuids, + int nuuids, + virSecretObjListACLFilter filter, + virConnectPtr conn); + void virSecretDefFree(virSecretDefPtr def); virSecretDefPtr virSecretDefParseString(const char *xml); virSecretDefPtr virSecretDefParseFile(const char *filename); -- 2.5.0
On 03/02/2016 01:55 PM, John Ferlan wrote:
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/secret_conf.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++- src/conf/secret_conf.h | 6 ++++++ 2 files changed, 58 insertions(+), 1 deletion(-)
ACK, mirrors virNetworkObjListGetNames and looks sensible on its own. - Cole
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/secret_conf.h | 3 +- src/libvirt_private.syms | 9 + src/secret/secret_driver.c | 437 ++++++--------------------------------------- 3 files changed, 61 insertions(+), 388 deletions(-) diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index 15b07d5..1c9de52 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -25,6 +25,7 @@ # include "internal.h" # include "virutil.h" +# include "virobject.h" VIR_ENUM_DECL(virSecretUsage) @@ -46,7 +47,7 @@ struct _virSecretDef { 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 3810604..18a30ce 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -785,6 +785,15 @@ virSecretDefFormat; virSecretDefFree; virSecretDefParseFile; virSecretDefParseString; +virSecretObjEndAPI; +virSecretObjListAdd; +virSecretObjListExport; +virSecretObjListFindByUsage; +virSecretObjListFindByUUID; +virSecretObjListGetUUIDs; +virSecretObjListNew; +virSecretObjListNumOfSecrets; +virSecretObjListRemove; virSecretUsageIDForDef; virSecretUsageTypeFromString; virSecretUsageTypeToString; diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 9d92619..13ab365 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -56,7 +56,7 @@ typedef struct _virSecretDriverState virSecretDriverState; typedef virSecretDriverState *virSecretDriverStatePtr; struct _virSecretDriverState { virMutex lock; - virSecretObj *secrets; + virSecretObjListPtr secrets; char *configDir; }; @@ -74,104 +74,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 @@ -180,7 +82,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); @@ -376,30 +278,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; @@ -410,16 +293,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; @@ -427,19 +304,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) @@ -448,8 +325,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, "..")) @@ -461,39 +340,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); @@ -505,23 +363,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 @@ -529,122 +376,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 @@ -654,9 +409,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, @@ -673,7 +426,7 @@ secretLookupByUUID(virConnectPtr conn, virSecretUsageIDForDef(secret->def)); cleanup: - secretDriverUnlock(); + virSecretObjEndAPI(&secret); return ret; } @@ -686,9 +439,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; @@ -703,7 +455,7 @@ secretLookupByUsage(virConnectPtr conn, virSecretUsageIDForDef(secret->def)); cleanup: - secretDriverUnlock(); + virSecretObjEndAPI(&secret); return ret; } @@ -723,69 +475,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) { @@ -814,21 +509,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; } @@ -842,8 +534,6 @@ secretGetXMLDesc(virSecretPtr obj, virCheckFlags(0, NULL); - secretDriverLock(); - if (!(secret = secretObjFromSecret(obj))) goto cleanup; @@ -853,7 +543,7 @@ secretGetXMLDesc(virSecretPtr obj, ret = virSecretDefFormat(secret->def); cleanup: - secretDriverUnlock(); + virSecretObjEndAPI(&secret); return ret; } @@ -874,8 +564,6 @@ secretSetValue(virSecretPtr obj, if (VIR_ALLOC_N(new_value, value_size) < 0) return -1; - secretDriverLock(); - if (!(secret = secretObjFromSecret(obj))) goto cleanup; @@ -909,7 +597,7 @@ secretSetValue(virSecretPtr obj, memset(new_value, 0, value_size); cleanup: - secretDriverUnlock(); + virSecretObjEndAPI(&secret); VIR_FREE(new_value); @@ -927,8 +615,6 @@ secretGetValue(virSecretPtr obj, virCheckFlags(0, NULL); - secretDriverLock(); - if (!(secret = secretObjFromSecret(obj))) goto cleanup; @@ -956,7 +642,7 @@ secretGetValue(virSecretPtr obj, *value_size = secret->value_size; cleanup: - secretDriverUnlock(); + virSecretObjEndAPI(&secret); return ret; } @@ -967,8 +653,6 @@ secretUndefine(virSecretPtr obj) int ret = -1; virSecretObjPtr secret; - secretDriverLock(); - if (!(secret = secretObjFromSecret(obj))) goto cleanup; @@ -979,13 +663,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; } @@ -993,17 +676,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(); @@ -1040,7 +718,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(); @@ -1056,31 +737,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.0
On 03/02/2016 01:55 PM, John Ferlan wrote:
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/secret_conf.h | 3 +- src/libvirt_private.syms | 9 + src/secret/secret_driver.c | 437 ++++++--------------------------------------- 3 files changed, 61 insertions(+), 388 deletions(-)
ACK, mirrors the network driver usage (where similar), and all the custom logic beats seem to be handled in the previous patches - Cole
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 | 175 +++++++++++++++++++++++++++++++++++++++++++++ src/conf/secret_conf.h | 3 + src/libvirt_private.syms | 1 + src/secret/secret_driver.c | 174 +------------------------------------------- 4 files changed, 181 insertions(+), 172 deletions(-) diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index 0e73c2d..f6eee6f 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -21,6 +21,9 @@ */ #include <config.h> +#include <dirent.h> +#include <fcntl.h> +#include <sys/stat.h> #include "internal.h" #include "virbuffer.h" @@ -33,6 +36,7 @@ #include "viruuid.h" #include "virhash.h" #include "virfile.h" +#include "base64.h" #define VIR_FROM_THIS VIR_FROM_SECRET @@ -969,3 +973,174 @@ virSecretDefFormat(const virSecretDef *def) virBufferFreeAndReset(&buf); return NULL; } + + +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/secret_conf.h b/src/conf/secret_conf.h index 1c9de52..d3bd10c 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -131,4 +131,7 @@ char *virSecretDefFormat(const virSecretDef *def); (VIR_CONNECT_LIST_SECRETS_FILTERS_EPHEMERAL | \ VIR_CONNECT_LIST_SECRETS_FILTERS_PRIVATE) +int virSecretLoadAllConfigs(virSecretObjListPtr secrets, + const char *configDir); + #endif diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 18a30ce..4e1ee8e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -785,6 +785,7 @@ virSecretDefFormat; virSecretDefFree; virSecretDefParseFile; virSecretDefParseString; +virSecretLoadAllConfigs; virSecretObjEndAPI; virSecretObjListAdd; virSecretObjListExport; diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 13ab365..b8d9ecc 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> @@ -189,175 +188,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 @@ -721,7 +551,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(); @@ -742,7 +572,7 @@ secretStateReload(void) secretDriverLock(); - ignore_value(secretLoadAllConfigs(driver->secrets, driver->configDir)); + ignore_value(virSecretLoadAllConfigs(driver->secrets, driver->configDir)); secretDriverUnlock(); return 0; -- 2.5.0
On 03/02/2016 01:55 PM, John Ferlan wrote:
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 | 175 +++++++++++++++++++++++++++++++++++++++++++++ src/conf/secret_conf.h | 3 + src/libvirt_private.syms | 1 + src/secret/secret_driver.c | 174 +------------------------------------------- 4 files changed, 181 insertions(+), 172 deletions(-)
ACK, mirrors network_conf.c layout. (though honestly I'd rather we have separate files for XML handling and object handling... the existing conf.c files are too large anyways. I put an entry on the LibvirtFirstBugs wiki page about that type of code reorg though it's probably over an initial contributors head) - Cole
On 04/18/2016 05:48 PM, Cole Robinson wrote:
On 03/02/2016 01:55 PM, John Ferlan wrote:
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 | 175 +++++++++++++++++++++++++++++++++++++++++++++ src/conf/secret_conf.h | 3 + src/libvirt_private.syms | 1 + src/secret/secret_driver.c | 174 +------------------------------------------- 4 files changed, 181 insertions(+), 172 deletions(-)
ACK, mirrors network_conf.c layout.
(though honestly I'd rather we have separate files for XML handling and object handling... the existing conf.c files are too large anyways. I put an entry on the LibvirtFirstBugs wiki page about that type of code reorg though it's probably over an initial contributors head)
I could go the route of a "virsecretobj.c" (to partially mimic the virdomainobjlist.c) instead of cramming everything into secret_conf - there's about 1000 new lines in secret_conf just for this series (with the reduction of lines in secret_driver). It would mean a complete v2 of this series - although considering most things have been ACK'd the second review should be easier. I think there's only a couple of issues/questions to resolve from reviews. Thanks for the review - John
On 04/19/2016 08:01 AM, John Ferlan wrote:
On 04/18/2016 05:48 PM, Cole Robinson wrote:
On 03/02/2016 01:55 PM, John Ferlan wrote:
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 | 175 +++++++++++++++++++++++++++++++++++++++++++++ src/conf/secret_conf.h | 3 + src/libvirt_private.syms | 1 + src/secret/secret_driver.c | 174 +------------------------------------------- 4 files changed, 181 insertions(+), 172 deletions(-)
ACK, mirrors network_conf.c layout.
(though honestly I'd rather we have separate files for XML handling and object handling... the existing conf.c files are too large anyways. I put an entry on the LibvirtFirstBugs wiki page about that type of code reorg though it's probably over an initial contributors head)
I could go the route of a "virsecretobj.c" (to partially mimic the virdomainobjlist.c) instead of cramming everything into secret_conf - there's about 1000 new lines in secret_conf just for this series (with the reduction of lines in secret_driver).
It would mean a complete v2 of this series - although considering most things have been ACK'd the second review should be easier. I think there's only a couple of issues/questions to resolve from reviews.
Oh I wasn't trying to imply making it a requirement of this series. Definitely an additive thing, and I'm not even trying to pin you with the work, just floating the general idea - Cole
On 04/19/2016 11:10 AM, Cole Robinson wrote:
On 04/19/2016 08:01 AM, John Ferlan wrote:
On 04/18/2016 05:48 PM, Cole Robinson wrote:
On 03/02/2016 01:55 PM, John Ferlan wrote:
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 | 175 +++++++++++++++++++++++++++++++++++++++++++++ src/conf/secret_conf.h | 3 + src/libvirt_private.syms | 1 + src/secret/secret_driver.c | 174 +------------------------------------------- 4 files changed, 181 insertions(+), 172 deletions(-)
ACK, mirrors network_conf.c layout.
(though honestly I'd rather we have separate files for XML handling and object handling... the existing conf.c files are too large anyways. I put an entry on the LibvirtFirstBugs wiki page about that type of code reorg though it's probably over an initial contributors head)
I could go the route of a "virsecretobj.c" (to partially mimic the virdomainobjlist.c) instead of cramming everything into secret_conf - there's about 1000 new lines in secret_conf just for this series (with the reduction of lines in secret_driver).
It would mean a complete v2 of this series - although considering most things have been ACK'd the second review should be easier. I think there's only a couple of issues/questions to resolve from reviews.
Oh I wasn't trying to imply making it a requirement of this series. Definitely an additive thing, and I'm not even trying to pin you with the work, just floating the general idea
Understood - I'd rather do it now than take a hit at some unknown future point in time to essentially do the same thing. In the long run it's the "right thing" to do. John
On 04/19/2016 12:30 PM, John Ferlan wrote:
On 04/19/2016 11:10 AM, Cole Robinson wrote:
On 04/19/2016 08:01 AM, John Ferlan wrote:
On 04/18/2016 05:48 PM, Cole Robinson wrote:
On 03/02/2016 01:55 PM, John Ferlan wrote:
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 | 175 +++++++++++++++++++++++++++++++++++++++++++++ src/conf/secret_conf.h | 3 + src/libvirt_private.syms | 1 + src/secret/secret_driver.c | 174 +------------------------------------------- 4 files changed, 181 insertions(+), 172 deletions(-)
ACK, mirrors network_conf.c layout.
(though honestly I'd rather we have separate files for XML handling and object handling... the existing conf.c files are too large anyways. I put an entry on the LibvirtFirstBugs wiki page about that type of code reorg though it's probably over an initial contributors head)
I could go the route of a "virsecretobj.c" (to partially mimic the virdomainobjlist.c) instead of cramming everything into secret_conf - there's about 1000 new lines in secret_conf just for this series (with the reduction of lines in secret_driver).
It would mean a complete v2 of this series - although considering most things have been ACK'd the second review should be easier. I think there's only a couple of issues/questions to resolve from reviews.
Oh I wasn't trying to imply making it a requirement of this series. Definitely an additive thing, and I'm not even trying to pin you with the work, just floating the general idea
Understood - I'd rather do it now than take a hit at some unknown future point in time to essentially do the same thing. In the long run it's the "right thing" to do.
Yeah but I hate to see self contained changes get held up just to accommodate an additional cleanup. Reviewer bandwidth is the bottleneck here - Cole
Move and rename secretDeleteSaved from secret_driver into secret_conf 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/secret_conf.c | 21 +++++++++++++++++++++ src/conf/secret_conf.h | 4 ++++ src/libvirt_private.syms | 2 ++ src/secret/secret_driver.c | 22 ++++++---------------- 4 files changed, 33 insertions(+), 16 deletions(-) diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index f6eee6f..52f78bd 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -685,6 +685,27 @@ virSecretObjListGetUUIDs(virSecretObjListPtr secrets, } +int +virSecretObjDeleteConfig(virSecretObjPtr secret) +{ + /* When the XML is missing, we'll still allow the attempt to + * delete the secret data. Without a configFile, the secret + won't be loaded again, so we have succeeded already. */ + if (!secret->def->ephemeral && + unlink(secret->configFile) < 0 && errno != ENOENT) + return -1; + + return 0; +} + + +void +virSecretObjDeleteData(virSecretObjPtr secret) +{ + (void)unlink(secret->base64File); +} + + void virSecretDefFree(virSecretDefPtr def) { diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index d3bd10c..e2f69b5 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -114,6 +114,10 @@ int virSecretObjListGetUUIDs(virSecretObjListPtr secrets, virSecretObjListACLFilter filter, virConnectPtr conn); +int virSecretObjDeleteConfig(virSecretObjPtr secret); + +void virSecretObjDeleteData(virSecretObjPtr secret); + 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 cbc36de..2437b0b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -787,6 +787,8 @@ virSecretDefFree; virSecretDefParseFile; virSecretDefParseString; virSecretLoadAllConfigs; +virSecretObjDeleteConfig; +virSecretObjDeleteData; virSecretObjEndAPI; virSecretObjListAdd; virSecretObjListExport; diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index b8d9ecc..e4315f3 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -175,19 +175,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 @@ -325,8 +312,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; @@ -489,10 +478,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.0
On 03/08/2016 12:35 PM, John Ferlan wrote:
Move and rename secretDeleteSaved from secret_driver into secret_conf 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/secret_conf.c | 21 +++++++++++++++++++++ src/conf/secret_conf.h | 4 ++++ src/libvirt_private.syms | 2 ++ src/secret/secret_driver.c | 22 ++++++---------------- 4 files changed, 33 insertions(+), 16 deletions(-)
diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index f6eee6f..52f78bd 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -685,6 +685,27 @@ virSecretObjListGetUUIDs(virSecretObjListPtr secrets, }
+int +virSecretObjDeleteConfig(virSecretObjPtr secret) +{ + /* When the XML is missing, we'll still allow the attempt to + * delete the secret data. Without a configFile, the secret + won't be loaded again, so we have succeeded already. */
This comment seems weirdly placed now. If it's kept at all it should be placed at the ObjDeleteData call sites. Or rework it as a comment in ObjDeleteData to explain why we don't care about failure in this case.
+ if (!secret->def->ephemeral && + unlink(secret->configFile) < 0 && errno != ENOENT) + return -1; +
This should report have a virReportSystemError call. The original code doesn't have one, but that's a bug Minor stuff though so ACK in general, I don't care if you fix before pushing but not sure if there's a formal policy on that - Cole
+ return 0; +} + + +void +virSecretObjDeleteData(virSecretObjPtr secret) +{ + (void)unlink(secret->base64File); +} + + void virSecretDefFree(virSecretDefPtr def) { diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index d3bd10c..e2f69b5 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -114,6 +114,10 @@ int virSecretObjListGetUUIDs(virSecretObjListPtr secrets, virSecretObjListACLFilter filter, virConnectPtr conn);
+int virSecretObjDeleteConfig(virSecretObjPtr secret); + +void virSecretObjDeleteData(virSecretObjPtr secret); + 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 cbc36de..2437b0b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -787,6 +787,8 @@ virSecretDefFree; virSecretDefParseFile; virSecretDefParseString; virSecretLoadAllConfigs; +virSecretObjDeleteConfig; +virSecretObjDeleteData; virSecretObjEndAPI; virSecretObjListAdd; virSecretObjListExport; diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index b8d9ecc..e4315f3 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -175,19 +175,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 @@ -325,8 +312,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; @@ -489,10 +478,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;
Move and rename the secretRewriteFile, secretSaveDef, and secretSaveValue from secret_driver to secret_conf 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/secret_conf.c | 69 +++++++++++++++++++++++++++++++++++ src/conf/secret_conf.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/secret_conf.c b/src/conf/secret_conf.c index 52f78bd..3528288 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -706,6 +706,75 @@ virSecretObjDeleteData(virSecretObjPtr 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 +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; +} + + void virSecretDefFree(virSecretDefPtr def) { diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index e2f69b5..d40b510 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -118,6 +118,10 @@ int virSecretObjDeleteConfig(virSecretObjPtr secret); void virSecretObjDeleteData(virSecretObjPtr secret); +int virSecretObjSaveConfig(virSecretObjPtr secret); + +int virSecretObjSaveData(virSecretObjPtr secret); + 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 2437b0b..9e1a09e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -798,6 +798,8 @@ virSecretObjListGetUUIDs; virSecretObjListNew; virSecretObjListNumOfSecrets; virSecretObjListRemove; +virSecretObjSaveConfig; +virSecretObjSaveData; virSecretUsageIDForDef; virSecretUsageTypeFromString; virSecretUsageTypeToString; diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index e4315f3..1b4dfea 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -91,26 +91,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) { @@ -122,59 +102,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 @@ -300,14 +227,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; } @@ -396,7 +327,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.0
On 03/08/2016 12:36 PM, John Ferlan wrote:
Move and rename the secretRewriteFile, secretSaveDef, and secretSaveValue from secret_driver to secret_conf
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/secret_conf.c | 69 +++++++++++++++++++++++++++++++++++ src/conf/secret_conf.h | 4 +++ src/libvirt_private.syms | 2 ++ src/secret/secret_driver.c | 90 +++++++--------------------------------------- 4 files changed, 87 insertions(+), 78 deletions(-)
ACK Though there should probably be explicit virfile.c support for a generic 'rewrite file with this passed string', rather than requiring a callback. src/network/leaseshelper.c already has something similar - Cole
On 04/18/2016 06:12 PM, Cole Robinson wrote:
On 03/08/2016 12:36 PM, John Ferlan wrote:
Move and rename the secretRewriteFile, secretSaveDef, and secretSaveValue from secret_driver to secret_conf
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/secret_conf.c | 69 +++++++++++++++++++++++++++++++++++ src/conf/secret_conf.h | 4 +++ src/libvirt_private.syms | 2 ++ src/secret/secret_driver.c | 90 +++++++--------------------------------------- 4 files changed, 87 insertions(+), 78 deletions(-)
ACK
Though there should probably be explicit virfile.c support for a generic 'rewrite file with this passed string', rather than requiring a callback. src/network/leaseshelper.c already has something similar
This patch was just moving existing code. I cannot say for sure, but if the 'opaque' data had some formatting considerations rather than just a string, such as virXMLSaveFile which also uses virFileRewrite, then the existing virFileRewrite is doing just what it was designed to do. I didn't spend any cycles looks which came first. John
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/secret_conf.c | 15 +++++++++++++ src/conf/secret_conf.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/secret_conf.c b/src/conf/secret_conf.c index 3528288..0410328 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -775,6 +775,21 @@ virSecretObjSaveData(virSecretObjPtr secret) } +virSecretDefPtr +virSecretObjGetDef(virSecretObjPtr secret) +{ + return secret->def; +} + + +void +virSecretObjSetDef(virSecretObjPtr secret, + virSecretDefPtr def) +{ + secret->def = def; +} + + void virSecretDefFree(virSecretDefPtr def) { diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index d40b510..ce714c1 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -122,6 +122,10 @@ int virSecretObjSaveConfig(virSecretObjPtr secret); int virSecretObjSaveData(virSecretObjPtr secret); +virSecretDefPtr virSecretObjGetDef(virSecretObjPtr secret); + +void virSecretObjSetDef(virSecretObjPtr secret, 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 9e1a09e..3a417f0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -790,6 +790,7 @@ virSecretLoadAllConfigs; virSecretObjDeleteConfig; virSecretObjDeleteData; virSecretObjEndAPI; +virSecretObjGetDef; virSecretObjListAdd; virSecretObjListExport; virSecretObjListFindByUsage; @@ -800,6 +801,7 @@ virSecretObjListNumOfSecrets; virSecretObjListRemove; virSecretObjSaveConfig; virSecretObjSaveData; +virSecretObjSetDef; virSecretUsageIDForDef; virSecretUsageTypeFromString; virSecretUsageTypeToString; diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 1b4dfea..676c02e 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -152,6 +152,7 @@ secretLookupByUUID(virConnectPtr conn, { virSecretPtr ret = NULL; virSecretObjPtr secret; + virSecretDefPtr def; if (!(secret = virSecretObjListFindByUUID(driver->secrets, uuid))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -161,13 +162,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); @@ -182,6 +184,7 @@ secretLookupByUsage(virConnectPtr conn, { virSecretPtr ret = NULL; virSecretObjPtr secret; + virSecretDefPtr def; if (!(secret = virSecretObjListFindByUsage(driver->secrets, usageType, usageID))) { @@ -190,13 +193,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); @@ -249,22 +253,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); @@ -281,16 +285,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); @@ -308,6 +314,7 @@ secretSetValue(virSecretPtr obj, unsigned char *old_value, *new_value; size_t old_value_size; virSecretObjPtr secret; + virSecretDefPtr def; virCheckFlags(0, -1); @@ -317,7 +324,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; @@ -326,7 +334,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; @@ -365,13 +373,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) { @@ -383,7 +393,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; @@ -405,11 +415,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.0
On 03/08/2016 12:39 PM, John Ferlan wrote:
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/secret_conf.c | 15 +++++++++++++ src/conf/secret_conf.h | 4 ++++ src/libvirt_private.syms | 2 ++ src/secret/secret_driver.c | 54 ++++++++++++++++++++++++++++------------------ 4 files changed, 54 insertions(+), 21 deletions(-)
ACK - Cole
Introduce the final accessor's to _virSecretObject data and move the structure from secret_conf.h to secret_conf.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/secret_conf.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/secret_conf.h | 17 +++++----- src/libvirt_private.syms | 4 +++ src/secret/secret_driver.c | 50 ++++----------------------- 4 files changed, 104 insertions(+), 51 deletions(-) diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index 0410328..4d0cb9c 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -45,6 +45,14 @@ VIR_LOG_INIT("conf.secret_conf"); VIR_ENUM_IMPL(virSecretUsage, VIR_SECRET_USAGE_TYPE_LAST, "none", "volume", "ceph", "iscsi") +struct _virSecretObj { + virObjectLockable parent; + char *configFile; + char *base64File; + virSecretDefPtr def; + unsigned char *value; /* May be NULL */ + size_t value_size; +}; static virClassPtr virSecretObjClass; static void virSecretObjDispose(void *obj); @@ -790,6 +798,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; +} + + void virSecretDefFree(virSecretDefPtr def) { diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index ce714c1..be3bff9 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -46,14 +46,6 @@ struct _virSecretDef { 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); @@ -126,6 +118,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); + 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 3a417f0..15370f6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -791,6 +791,8 @@ virSecretObjDeleteConfig; virSecretObjDeleteData; virSecretObjEndAPI; virSecretObjGetDef; +virSecretObjGetValue; +virSecretObjGetValueSize; virSecretObjListAdd; virSecretObjListExport; virSecretObjListFindByUsage; @@ -802,6 +804,8 @@ virSecretObjListRemove; virSecretObjSaveConfig; virSecretObjSaveData; virSecretObjSetDef; +virSecretObjSetValue; +virSecretObjSetValueSize; virSecretUsageIDForDef; virSecretUsageTypeFromString; virSecretUsageTypeToString; diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 676c02e..ab58115 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -311,16 +311,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; @@ -328,40 +323,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; } @@ -384,14 +356,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", @@ -399,10 +363,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.0
On 03/08/2016 12:39 PM, John Ferlan wrote:
Introduce the final accessor's to _virSecretObject data and move the structure from secret_conf.h to secret_conf.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/secret_conf.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/secret_conf.h | 17 +++++----- src/libvirt_private.syms | 4 +++ src/secret/secret_driver.c | 50 ++++----------------------- 4 files changed, 104 insertions(+), 51 deletions(-)
diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index 0410328..4d0cb9c 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c
...
+ + +int +virSecretObjSetValue(virSecretObjPtr secret, + const unsigned char *value, + size_t value_size) +{
...
+ + return 0;
Weird spacing here ACK otherwise - Cole
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 | 26 +++++++++++++------------- src/conf/secret_conf.h | 4 ++-- src/secret/secret_driver.c | 10 +++++----- src/storage/storage_backend.c | 4 ++-- 4 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index 4d0cb9c..6d4de7c 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -403,7 +403,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; @@ -553,17 +553,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; @@ -699,7 +699,7 @@ virSecretObjDeleteConfig(virSecretObjPtr secret) /* When the XML is missing, we'll still allow the attempt to * delete the secret data. Without a configFile, the secret won't be loaded again, so we have succeeded already. */ - if (!secret->def->ephemeral && + if (!secret->def->isephemeral && unlink(secret->configFile) < 0 && errno != ENOENT) return -1; @@ -838,7 +838,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 */ @@ -995,9 +995,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'")); @@ -1009,9 +1009,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'")); @@ -1137,8 +1137,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 be3bff9..a64a4d9 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -32,8 +32,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/secret/secret_driver.c b/src/secret/secret_driver.c index ab58115..cdf8d80 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -230,23 +230,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; @@ -357,7 +357,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 231eccf..0a8469a 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.0
On 03/08/2016 12:39 PM, John Ferlan wrote:
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 | 26 +++++++++++++------------- src/conf/secret_conf.h | 4 ++-- src/secret/secret_driver.c | 10 +++++----- src/storage/storage_backend.c | 4 ++-- 4 files changed, 22 insertions(+), 22 deletions(-)
ACK - Cole
On 03/02/2016 01:54 PM, John Ferlan wrote:
This series replaces the usage of linked list in secret driver with a hashed object list as was suggested in the review in the last patch of the previous secret driver changes series:
http://www.redhat.com/archives/libvir-list/2016-February/msg01274.html
Patch 1 - starts the ball rolling Patch 2 - is yet another code optimization (as found in bridge_driver) Patches 3-8 - Add the new API's "slowly" (for review purposes) Patch 9 - replaces the usage of linked lists with the hashed object Patch 10 - moves the secretLoadAllConfigs to secret_conf
The changes are loosely modeled after both virdomainobj and network_conf functionality. The real meat and potato(e)s is found in patches 5 and 9. Other functions should be relatively straightforward.
I've done testing of various virsh secret-* commands (both valid and invalid options) as well as performing driver start, stop, and reload testing. Each patch was built using 'check' and 'syntax-check'.
The end result is much more code in secret_conf and much less in secret_driver.
John Ferlan (10): secret: Move virSecretObj to secret_conf.h Add secretObjFromSecret secret: Add hashed virSecretObj and virSecretObjList secret: Introduce virSecretObjListFindBy{UUID|Usage} support secret: Introduce virSecretObjListAdd* and virSecretObjListRemove secret: Introduce virSecretObjListNumOfSecrets secret: Introduce virSecretObjListExport secret: Introduce virSecretObjListGetUUIDs secret: Use the hashed virSecretObjList secret: Move and rename secretLoadAllConfigs
src/conf/secret_conf.c | 819 ++++++++++++++++++++++++++++++++++++++++++++- src/conf/secret_conf.h | 76 ++++- src/libvirt_private.syms | 11 + src/secret/secret_driver.c | 647 +++-------------------------------- 4 files changed, 957 insertions(+), 596 deletions(-)
Ping - any chance these can be finished up? Would it be better to repost the entire series. It should apply cleanly as there hasn't been changes in this space. Tks - John
participants (4)
-
Cole Robinson -
Eric Blake -
John Ferlan -
Peter Krempa