Re: [libvirt] [PATCH 11/14] secret: Alter FindByUUID to expect the formatted uuidstr

On 04/25/2017 07:22 AM, Pavel Hrdina wrote: > On Mon, Apr 24, 2017 at 02:00:20PM -0400, John Ferlan wrote: >> Since we're storing a virUUIDFormat'd string in our Hash Table, let's >> modify the Lookup API to receive a formatted string as well. > > It's true that we use formatted UUID as a key for storing the secret in hash > table we don't necessary need to change Lookup APIs to require formatted string > as well. All other objects have similar APIs to Lookup an object by UUID and > they don't require formatted UUID. I like the fact, that the formatting of > UUID is hidden for the caller since we store RAW UUIDs in our object structures. > > I'm not sure about this change, I'm more inclined to NACK this patch. > Go back to my comment in my response to patch 3 regarding a single function to perform the Find... Take the abstraction back a level for every/any driver that uses UUID there is similar code to essentially format the UUID for "add" and "lookup" (or find). So lots of repetitive code, right? The UUID is always stored in the hash table using the formatted value - hence the movement to utilize that formatted UUID. For some drivers (OK domain only because you don't have the context I have) - there's a second hash table for lookup (e.g. Name). That second lookup is done by "char *" as would the formatted UUID be done. If you then further consider that the only difference between the hash tables is formatted UUID or name, then you can consolidate more code to perform that lookup by a "key" that is either primary or secondary.

There is also similar code to format the UUID for any message printed. Sometimes that format is done more than once in a single function.

John > Pavel > >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >> --- >> src/conf/virsecretobj.c | 18 +++++++----------- >> src/conf/virsecretobj.h | 2 +- >> src/secret/secret_driver.c | 10 +++++----- >> 3 files changed, 13 insertions(+), 17 deletions(-) >> >> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c >> index 5acda4c..ae2b287 100644 >> --- a/src/conf/virsecretobj.c >> +++ b/src/conf/virsecretobj.c >> @@ -163,12 +163,8 @@ virSecretObjListDispose(void *obj) >> >> static virSecretObjPtr >> virSecretObjListFindByUUIDLocked(virSecretObjListPtr secrets, >> - const unsigned char *uuid) >> + const char *uuidstr) >> { >> - char uuidstr[VIR_UUID_STRING_BUFLEN]; >> - >> - virUUIDFormat(uuid, uuidstr); >> - >> return virObjectRef(virHashLookup(secrets->objs, uuidstr)); >> } >> >> @@ -176,7 +172,7 @@ virSecretObjListFindByUUIDLocked(virSecretObjListPtr secrets, >> /** >> * virSecretObjFindByUUID: >> * @secrets: list of secret objects >> - * @uuid: secret uuid to find >> + * @uuidstr: secret uuid to find >> * >> * This function locks @secrets and finds the secret object which >> * corresponds to @uuid. >> @@ -185,12 +181,12 @@ virSecretObjListFindByUUIDLocked(virSecretObjListPtr secrets, >> */ >> virSecretObjPtr >> virSecretObjListFindByUUID(virSecretObjListPtr secrets, >> - const unsigned char *uuid) >> + const char *uuidstr) >> { >> virSecretObjPtr obj; >> >> virObjectLock(secrets); >> - obj = virSecretObjListFindByUUIDLocked(secrets, uuid); >> + obj = virSecretObjListFindByUUIDLocked(secrets, uuidstr); >> virObjectUnlock(secrets); >> if (obj) >> virObjectLock(obj); >> @@ -328,13 +324,14 @@ virSecretObjListAdd(virSecretObjListPtr secrets, >> if (oldDef) >> *oldDef = NULL; >> >> + virUUIDFormat(newdef->uuid, uuidstr); >> + >> /* Is there a secret already matching this UUID */ >> - if ((obj = virSecretObjListFindByUUIDLocked(secrets, newdef->uuid))) { >> + if ((obj = virSecretObjListFindByUUIDLocked(secrets, uuidstr))) { >> virObjectLock(obj); >> def = obj->def; >> >> if (STRNEQ_NULLABLE(def->usage_id, newdef->usage_id)) { >> - virUUIDFormat(def->uuid, uuidstr); >> virReportError(VIR_ERR_INTERNAL_ERROR, >> _("a secret with UUID %s is already defined for " >> "use with %s"), >> @@ -372,7 +369,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets, >> /* Generate the possible configFile and base64File strings >> * using the configDir, uuidstr, and appropriate suffix >> */ >> - virUUIDFormat(newdef->uuid, uuidstr); >> if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) || >> !(base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) >> goto cleanup; >> diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h >> index bd38f52..092f23c 100644 >> --- a/src/conf/virsecretobj.h >> +++ b/src/conf/virsecretobj.h >> @@ -40,7 +40,7 @@ virSecretObjListNew(void); >> >> virSecretObjPtr >> virSecretObjListFindByUUID(virSecretObjListPtr secrets, >> - const unsigned char *uuid); >> + const char *uuidstr); >> >> virSecretObjPtr >> virSecretObjListFindByUsage(virSecretObjListPtr secrets, >> diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c >> index cc050ff..2d4091d 100644 >> --- a/src/secret/secret_driver.c >> +++ b/src/secret/secret_driver.c >> @@ -86,8 +86,8 @@ secretObjFromSecret(virSecretPtr secret) >> virSecretObjPtr obj; >> char uuidstr[VIR_UUID_STRING_BUFLEN]; >> >> - if (!(obj = virSecretObjListFindByUUID(driver->secrets, secret->uuid))) { >> - virUUIDFormat(secret->uuid, uuidstr); >> + virUUIDFormat(secret->uuid, uuidstr); >> + if (!(obj = virSecretObjListFindByUUID(driver->secrets, uuidstr))) { >> virReportError(VIR_ERR_NO_SECRET, >> _("no secret with matching uuid '%s'"), uuidstr); >> return NULL; >> @@ -159,10 +159,10 @@ secretLookupByUUID(virConnectPtr conn, >> virSecretPtr ret = NULL; >> virSecretObjPtr obj; >> virSecretDefPtr def; >> + char uuidstr[VIR_UUID_STRING_BUFLEN]; >> >> - if (!(obj = virSecretObjListFindByUUID(driver->secrets, uuid))) { >> - char uuidstr[VIR_UUID_STRING_BUFLEN]; >> - virUUIDFormat(uuid, uuidstr); >> + virUUIDFormat(uuid, uuidstr); >> + if (!(obj = virSecretObjListFindByUUID(driver->secrets, uuidstr))) { >> virReportError(VIR_ERR_NO_SECRET, >> _("no secret with matching uuid '%s'"), uuidstr); >> goto cleanup; >> -- >> 2.9.3 >> >> -- >> libvir-list mailing list >> libvir-list(a)redhat.com >> https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list