On 04/25/2017 07:45 AM, Pavel Hrdina wrote:
> On Mon, Apr 24, 2017 at 02:00:22PM -0400, John Ferlan wrote:
>> Rather than being generated during virSecretObjListAdd, generate the file
>> paths in each of the callers and then copy those paths into the object
>> rather than stealing their pointers.
>>
>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
>> ---
>> src/conf/virsecretobj.c | 53 ++++++++++++++++++++++++----------------------
>> src/conf/virsecretobj.h | 3 ++-
>> src/secret/secret_driver.c | 14 ++++++++++--
>> 3 files changed, 42 insertions(+), 28 deletions(-)
>>
>> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
>> index ae2b287..7a9908d 100644
>> --- a/src/conf/virsecretobj.c
>> +++ b/src/conf/virsecretobj.c
>> @@ -300,7 +300,8 @@ virSecretObjListRemove(virSecretObjListPtr secrets,
>> * virSecretObjListAdd:
>> * @secrets: list of secret objects
>> * @newdef: new secret definition
>> - * @configDir: directory to place secret config files
>> + * @configFile: secret config file
>> + * @base64File: secret data file
>> * @oldDef: Former secret def (e.g. a reload path perhaps)
>> *
>> * Add the new @newdef to the secret obj table hash
>> @@ -310,14 +311,14 @@ virSecretObjListRemove(virSecretObjListPtr secrets,
>> virSecretObjPtr
>> virSecretObjListAdd(virSecretObjListPtr secrets,
>> virSecretDefPtr newdef,
>> - const char *configDir,
>> + const char *configFile,
>> + const char *base64File,
>> virSecretDefPtr *oldDef)
>> {
>> virSecretObjPtr obj;
>> virSecretDefPtr def;
>> virSecretObjPtr ret = NULL;
>> char uuidstr[VIR_UUID_STRING_BUFLEN];
>> - char *configFile = NULL, *base64File = NULL;
>>
>> virObjectLock(secrets);
>>
>> @@ -366,13 +367,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>> goto cleanup;
>> }
>>
>> - /* Generate the possible configFile and base64File strings
>> - * using the configDir, uuidstr, and appropriate suffix
>> - */
>> - if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) ||
>> - !(base64File = virFileBuildPath(configDir, uuidstr, ".base64")))
>> - goto cleanup;
>> -
>> if (!(obj = virSecretObjNew()))
>> goto cleanup;
>>
>> @@ -380,8 +374,10 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>> goto cleanup;
>>
>> obj->def = newdef;
>> - VIR_STEAL_PTR(obj->configFile, configFile);
>> - VIR_STEAL_PTR(obj->base64File, base64File);
>> + if ((VIR_STRDUP(obj->configFile, configFile) < 0) ||
>> + (VIR_STRDUP(obj->base64File, base64File) < 0))
>> + goto cleanup;
>> +
>> virObjectRef(obj);
>> }
>>
>> @@ -390,8 +386,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>>
>> cleanup:
>> virSecretObjEndAPI(&obj);
>> - VIR_FREE(configFile);
>> - VIR_FREE(base64File);
>> virObjectUnlock(secrets);
>> return ret;
>> }
>> @@ -899,21 +893,22 @@ virSecretLoadValue(virSecretObjPtr obj)
>>
>> static virSecretObjPtr
>> virSecretLoad(virSecretObjListPtr secrets,
>> - const char *file,
>> - const char *path,
>> - const char *configDir)
>> + const char *fname,
>> + const char *configFile,
>> + const char *base64File)
>> {
>> virSecretDefPtr def = NULL;
>> virSecretObjPtr obj = NULL;
>> virSecretObjPtr ret = NULL;
>>
>> - if (!(def = virSecretDefParseFile(path)))
>> + if (!(def = virSecretDefParseFile(configFile)))
>> goto cleanup;
>>
>> - if (virSecretLoadValidateUUID(def, file) < 0)
>> + if (virSecretLoadValidateUUID(def, fname) < 0)
>> goto cleanup;
>>
>> - if (!(obj = virSecretObjListAdd(secrets, def, configDir, NULL)))
>> + if (!(obj = virSecretObjListAdd(secrets, def, configFile, base64File,
>> + NULL)))
>> goto cleanup;
>> def = NULL;
>>
>> @@ -936,6 +931,8 @@ virSecretLoadAllConfigs(virSecretObjListPtr secrets,
>> {
>> DIR *dir = NULL;
>> struct dirent *de;
>> + char *configFile = NULL;
>> + char *base64File = NULL;
>> int rc;
>>
>> if ((rc = virDirOpenIfExists(&dir, configDir)) <= 0)
>> @@ -944,26 +941,32 @@ virSecretLoadAllConfigs(virSecretObjListPtr secrets,
>> /* 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 obj;
>>
>> + VIR_FREE(configFile);
>> + VIR_FREE(base64File);
>> +
>> if (!virFileHasSuffix(de->d_name, ".xml"))
>> continue;
>>
>> - if (!(path = virFileBuildPath(configDir, de->d_name, NULL)))
>> + if (!(configFile = virFileBuildPath(configDir, de->d_name, ".xml")))
>> + continue;
>> +
>> + if (!(base64File = virFileBuildPath(configDir, de->d_name, "base64")))
>> continue;
>
> I don't like the fact that the ".xml" and ".base64" will be at two different
> places in our code, it's easy to miss one place and make things inconsistent
> like this patch. You have a typo here.
Oh - right the ".base64" - didn't catch that obviously and it took me a
bit to notice ;-)
>
> IMHO stealing pointer is better than allocate a new string, duplicate the
> string in order to be stored and free the allocated string.
Yeah and this is where things did get a bit tricky for me w/r/t pointer
stealing. In future patches, configFile becomes part of an object since
multiple drivers use configFile, while base64File is a specific concept
to the secret driver. If the pointer is stolen by a subsequent API, then
'technically' none of the callers should be using it any more. Although
it's perfectly normal for that to happen in our code because we don't
pass by reference and have that steal.
Because I knew what the end result is, I had to figure a way to get
there somewhat logically. I'll look to rework this.
John
>
> NACK
>
> Pavel
>
>>
>> - if (!(obj = virSecretLoad(secrets, de->d_name, path, configDir))) {
>> + if (!(obj = virSecretLoad(secrets, de->d_name, configFile,
>> + base64File))) {
>> VIR_ERROR(_("Error reading secret: %s"),
>> virGetLastErrorMessage());
>> - VIR_FREE(path);
>> continue;
>> }
>>
>> - VIR_FREE(path);
>> virSecretObjEndAPI(&obj);
>> }
>>
>> + VIR_FREE(configFile);
>> + VIR_FREE(base64File);
>> VIR_DIR_CLOSE(dir);
>> return 0;
>> }
>> diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h
>> index 092f23c..51adc98 100644
>> --- a/src/conf/virsecretobj.h
>> +++ b/src/conf/virsecretobj.h
>> @@ -54,7 +54,8 @@ virSecretObjListRemove(virSecretObjListPtr secrets,
>> virSecretObjPtr
>> virSecretObjListAdd(virSecretObjListPtr secrets,
>> virSecretDefPtr newdef,
>> - const char *configDir,
>> + const char *configFile,
>> + const char *base64File,
>> virSecretDefPtr *oldDef);
>>
>> typedef bool
>> diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
>> index 8ddae57..3f8671b 100644
>> --- a/src/secret/secret_driver.c
>> +++ b/src/secret/secret_driver.c
>> @@ -213,6 +213,9 @@ secretDefineXML(virConnectPtr conn,
>> virSecretDefPtr backup = NULL;
>> virSecretDefPtr def;
>> virObjectEventPtr event = NULL;
>> + char *configFile = NULL;
>> + char *base64File = NULL;
>> + char uuidstr[VIR_UUID_STRING_BUFLEN];
>>
>> virCheckFlags(0, NULL);
>>
>> @@ -222,8 +225,13 @@ secretDefineXML(virConnectPtr conn,
>> if (virSecretDefineXMLEnsureACL(conn, def) < 0)
>> goto cleanup;
>>
>> - if (!(obj = virSecretObjListAdd(driver->secrets, def,
>> - driver->configDir, &backup)))
>> + virUUIDFormat(def->uuid, uuidstr);
>> + if (!(configFile = virFileBuildPath(driver->configDir, uuidstr, ".xml")) ||
>> + !(base64File = virFileBuildPath(driver->configDir, uuidstr, ".base64")))
>> + goto cleanup;
>> +
>> + if (!(obj = virSecretObjListAdd(driver->secrets, def, configFile,
>> + base64File, &backup)))
>> goto cleanup;
>>
>> if (!def->isephemeral) {
>> @@ -272,6 +280,8 @@ secretDefineXML(virConnectPtr conn,
>> virSecretObjListRemove(driver->secrets, obj);
>>
>> cleanup:
>> + VIR_FREE(configFile);
>> + VIR_FREE(base64File);
>> virSecretDefFree(def);
>> virSecretObjEndAPI(&obj);
>> if (event)
>> --
>> 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
