[libvirt] [PATCH 11/14] secret: Alter FindByUUID to expect the formatted uuidstr
Pavel Hrdina
phrdina at redhat.com
Tue Apr 25 12:57:00 UTC 2017
On Tue, Apr 25, 2017 at 08:05:13AM -0400, John Ferlan wrote:
>
>
> 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.
That's one of few things why I don't like the "primary" and "secondary" hash
table idea. It kind of forces you to use "string" as a key for hash table
even though we can use whatever we want and implement appropriate "key*"
callbacks and use virHashCreateFull instead of virHashCreate for storing
objects by UUID, with that change we can avoid formatting the UUID at all
for hash table. Formatting UUID should be done only for generating an XML
or printing a log message. Our internal code shouldn't format it for our
internal logic at all.
> 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.
Formatting the UUID only once for the whole function is actual cleanup.
I'm not against that change.
Pavel
> John
>
> > Pavel
> >
> >>
> >> Signed-off-by: John Ferlan <jferlan at redhat.com>
> >> ---
> >> 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 at redhat.com
> >> https://www.redhat.com/mailman/listinfo/libvir-list
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170425/9a5005ae/attachment-0001.sig>
More information about the libvir-list
mailing list