On 04/25/2017 08:57 AM, Pavel Hrdina wrote:
> 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.
>
Well if a hash table can take any format, then I don't understand the
argument against using primary and secondary, but that's other code...
Using consistently formatted keys makes a lot of things "easier" as does
using existing virHash functions. Having differences in key types and
having to know or somehow indicate what format the key is in so that
some underlying object code can create/use it's own set of APIs could
certainly be done, but for the purposes of how libvirt currently uses
things I don't see the "extra" value in writing that code. Use what we
have - format the UUID and use it. Having object code have to know
*what* format the key is in and make decisions based off of that for me
is unnecessary if we can provide a format that's already managed.
In the long run, does it really matter what the key is? The goal is to
have a unique key. We could swap the order and it wouldn't matter. Still
in our parlance UUID the more prevalent key and it's *always* been
formatted to a string. Regardless if it was formatted or not, the
comparison is still the same - it's just the length that differs.
FWIW, in the long run:
driver primary secondary
--------------------------------
secret UUID usage
nwfilter UUID name
interface UUID MAC
nodedev name n/a
volume name n/a
storage UUID name
network UUID name
domain UUID name <= "working" example/reference
I just don't see the value in virHashCreateFull if virHashCreate has
existing comparisons based off of char strings that for UUID's are
easily converted. Maybe I have to get closer to the finish line to see a
value in type key object code.
John
>> 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@xxxxxxxxxx>
>>>> ---
>>>> 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@xxxxxxxxxx
>>>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
>> --
>> libvir-list mailing list
>> libvir-list@xxxxxxxxxx
>> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
