On 04/25/2017 04:36 AM, Pavel Hrdina wrote:
> On Mon, Apr 24, 2017 at 02:00:15PM -0400, John Ferlan wrote:
>> Rather than dereferencing obj->def->X, create a local 'def' variable
>> variable that will dereference the def and use directly.
>>
>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
>> ---
>> src/conf/virsecretobj.c | 69 +++++++++++++++++++++++++++++++------------------
>> 1 file changed, 44 insertions(+), 25 deletions(-)
>>
>> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
>> index 42f36c8..413955d 100644
>> --- a/src/conf/virsecretobj.c
>> +++ b/src/conf/virsecretobj.c
>> @@ -139,8 +139,9 @@ static void
>> virSecretObjDispose(void *opaque)
>> {
>> virSecretObjPtr obj = opaque;
>> + virSecretDefPtr def = obj->def;
>>
>> - virSecretDefFree(obj->def);
>> + virSecretDefFree(def);
>
> Here it adds only a noise into the code, no actual improvement.
>
Well.... not entirely. Later on in patches that aren't posted yet the
"obj->def" is going to be replaced by virObjectPoolableDefGetDef() and
while yes, one could make that call inside the virSecretDefFree, it's
also just as simple to make it outside that call. It's a rote exercise.
Besides I find it "ugly" to read functionA(functionB()). Even worse if
functionB now could return NULL or some value one then has to split
apart the code anyway.
>> if (obj->value) {
>> /* Wipe before free to ensure we don't leave a secret on the heap */
>> memset(obj->value, 0, obj->value_size);
>> @@ -203,16 +204,18 @@ virSecretObjSearchName(const void *payload,
>> const void *opaque)
>> {
>> virSecretObjPtr obj = (virSecretObjPtr) payload;
>> + virSecretDefPtr def;
>> struct virSecretSearchData *data = (struct virSecretSearchData *) opaque;
>> int found = 0;
>>
>> virObjectLock(obj);
>> + def = obj->def;
>>
>> - if (obj->def->usage_type != data->usageType)
>> + if (def->usage_type != data->usageType)
>> goto cleanup;
>>
>> if (data->usageType != VIR_SECRET_USAGE_TYPE_NONE &&
>> - STREQ(obj->def->usage_id, data->usageID))
>> + STREQ(def->usage_id, data->usageID))
>> found = 1;
>>
>> cleanup:
>> @@ -278,11 +281,13 @@ virSecretObjListRemove(virSecretObjListPtr secrets,
>> virSecretObjPtr obj)
>> {
>> char uuidstr[VIR_UUID_STRING_BUFLEN];
>> + virSecretDefPtr def;
>>
>> if (!obj)
>> return;
>> + def = obj->def;
>>
>> - virUUIDFormat(obj->def->uuid, uuidstr);
>> + virUUIDFormat(def->uuid, uuidstr);
>
> Same here,
>
No this is "obj->def->uuid" into "def->uuid"... Once "->def" becomes
part of an "obj" it won't be visible and that would require more change
later on to essentially perform the same action.
>> virObjectRef(obj);
>> virObjectUnlock(obj);
>>
>> @@ -315,6 +320,7 @@ virSecretObjListAddLocked(virSecretObjListPtr secrets,
>> virSecretDefPtr *oldDef)
>> {
>> virSecretObjPtr obj;
>> + virSecretDefPtr def;
>> virSecretObjPtr ret = NULL;
>> char uuidstr[VIR_UUID_STRING_BUFLEN];
>> char *configFile = NULL, *base64File = NULL;
>> @@ -325,26 +331,27 @@ virSecretObjListAddLocked(virSecretObjListPtr secrets,
>> /* Is there a secret already matching this UUID */
>> if ((obj = virSecretObjListFindByUUIDLocked(secrets, newdef->uuid))) {
>> virObjectLock(obj);
>> + def = obj->def;
>>
>> - if (STRNEQ_NULLABLE(obj->def->usage_id, newdef->usage_id)) {
>> - virUUIDFormat(obj->def->uuid, uuidstr);
>> + 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"),
>> - uuidstr, obj->def->usage_id);
>> + uuidstr, def->usage_id);
>> goto cleanup;
>> }
>>
>> - if (obj->def->isprivate && !newdef->isprivate) {
>> + if (def->isprivate && !newdef->isprivate) {
>> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> _("cannot change private flag on existing secret"));
>> goto cleanup;
>> }
>>
>> if (oldDef)
>> - *oldDef = obj->def;
>> + *oldDef = def;
>> else
>> - virSecretDefFree(obj->def);
>> + virSecretDefFree(def);
>> obj->def = newdef;
>> } else {
>> /* No existing secret with same UUID,
>> @@ -353,7 +360,8 @@ virSecretObjListAddLocked(virSecretObjListPtr secrets,
>> newdef->usage_type,
>> newdef->usage_id))) {
>> virObjectLock(obj);
>> - virUUIDFormat(obj->def->uuid, uuidstr);
>> + def = obj->def;
>> + virUUIDFormat(def->uuid, uuidstr);
>> virReportError(VIR_ERR_INTERNAL_ERROR,
>> _("a secret with UUID %s already defined for "
>> "use with %s"),
>> @@ -424,6 +432,7 @@ virSecretObjListGetHelper(void *payload,
>> {
>> struct virSecretObjListGetHelperData *data = opaque;
>> virSecretObjPtr obj = payload;
>> + virSecretDefPtr def;
>>
>> if (data->error)
>> return 0;
>> @@ -432,8 +441,9 @@ virSecretObjListGetHelper(void *payload,
>> return 0;
>>
>> virObjectLock(obj);
>> + def = obj->def;
>>
>> - if (data->filter && !data->filter(data->conn, obj->def))
>> + if (data->filter && !data->filter(data->conn, def))
>> goto cleanup;
>>
>> if (data->uuids) {
>> @@ -444,7 +454,7 @@ virSecretObjListGetHelper(void *payload,
>> goto cleanup;
>> }
>>
>> - virUUIDFormat(obj->def->uuid, uuidstr);
>> + virUUIDFormat(def->uuid, uuidstr);
>> data->uuids[data->got] = uuidstr;
>> }
>>
>> @@ -478,20 +488,22 @@ static bool
>> virSecretObjMatchFlags(virSecretObjPtr obj,
>> unsigned int flags)
>> {
>> + virSecretDefPtr def = obj->def;
>> +
>> /* filter by whether it's ephemeral */
>> if (MATCH(VIR_CONNECT_LIST_SECRETS_FILTERS_EPHEMERAL) &&
>> !((MATCH(VIR_CONNECT_LIST_SECRETS_EPHEMERAL) &&
>> - obj->def->isephemeral) ||
>> + def->isephemeral) ||
>> (MATCH(VIR_CONNECT_LIST_SECRETS_NO_EPHEMERAL) &&
>> - !obj->def->isephemeral)))
>> + !def->isephemeral)))
>> return false;
>>
>> /* filter by whether it's private */
>> if (MATCH(VIR_CONNECT_LIST_SECRETS_FILTERS_PRIVATE) &&
>> !((MATCH(VIR_CONNECT_LIST_SECRETS_PRIVATE) &&
>> - obj->def->isprivate) ||
>> + def->isprivate) ||
>> (MATCH(VIR_CONNECT_LIST_SECRETS_NO_PRIVATE) &&
>> - !obj->def->isprivate)))
>> + !def->isprivate)))
>> return false;
>>
>> return true;
>> @@ -515,14 +527,16 @@ virSecretObjListPopulate(void *payload,
>> {
>> struct virSecretObjListData *data = opaque;
>> virSecretObjPtr obj = payload;
>> + virSecretDefPtr def;
>> virSecretPtr secret = NULL;
>>
>> if (data->error)
>> return 0;
>>
>> virObjectLock(obj);
>> + def = obj->def;
>>
>> - if (data->filter && !data->filter(data->conn, obj->def))
>> + if (data->filter && !data->filter(data->conn, def))
>> goto cleanup;
>>
>> if (!virSecretObjMatchFlags(obj, data->flags))
>> @@ -533,9 +547,9 @@ virSecretObjListPopulate(void *payload,
>> goto cleanup;
>> }
>>
>> - if (!(secret = virGetSecret(data->conn, obj->def->uuid,
>> - obj->def->usage_type,
>> - obj->def->usage_id))) {
>> + if (!(secret = virGetSecret(data->conn, def->uuid,
>> + def->usage_type,
>> + def->usage_id))) {
>> data->error = true;
>> goto cleanup;
>> }
>> @@ -624,7 +638,9 @@ virSecretObjListGetUUIDs(virSecretObjListPtr secrets,
>> int
>> virSecretObjDeleteConfig(virSecretObjPtr obj)
>> {
>> - if (!obj->def->isephemeral &&
>> + virSecretDefPtr def = obj->def;
>> +
>> + if (!def->isephemeral &&
>
> here,
>
Some concept - obj->def->isphemeral becomes just def->isephemeral
>> unlink(obj->configFile) < 0 && errno != ENOENT) {
>> virReportSystemError(errno, _("cannot unlink '%s'"),
>> obj->configFile);
>> @@ -653,10 +669,11 @@ virSecretObjDeleteData(virSecretObjPtr obj)
>> int
>> virSecretObjSaveConfig(virSecretObjPtr obj)
>> {
>> + virSecretDefPtr def = obj->def;
>> char *xml = NULL;
>> int ret = -1;
>>
>> - if (!(xml = virSecretDefFormat(obj->def)))
>> + if (!(xml = virSecretDefFormat(def)))
>> goto cleanup;
>
> here,
>
Sure it could eventually be a call instead, similar comment to above
though w/r/t function call within a function call for readability
>> if (virFileRewriteStr(obj->configFile, S_IRUSR | S_IWUSR, xml) < 0)
>> @@ -711,11 +728,12 @@ virSecretObjSetDef(virSecretObjPtr obj,
>> unsigned char *
>> virSecretObjGetValue(virSecretObjPtr obj)
>> {
>> + virSecretDefPtr def = obj->def;
>> unsigned char *ret = NULL;
>>
>> if (!obj->value) {
>> char uuidstr[VIR_UUID_STRING_BUFLEN];
>> - virUUIDFormat(obj->def->uuid, uuidstr);
>> + virUUIDFormat(def->uuid, uuidstr);
>
> here,
obj->def->uuid is now just def->uuid
>> virReportError(VIR_ERR_NO_SECRET,
>> _("secret '%s' does not have a value"), uuidstr);
>> goto cleanup;
>> @@ -735,6 +753,7 @@ virSecretObjSetValue(virSecretObjPtr obj,
>> const unsigned char *value,
>> size_t value_size)
>> {
>> + virSecretDefPtr def = obj->def;
>> unsigned char *old_value, *new_value;
>> size_t old_value_size;
>>
>> @@ -748,7 +767,7 @@ virSecretObjSetValue(virSecretObjPtr obj,
>> obj->value = new_value;
>> obj->value_size = value_size;
>>
>> - if (!obj->def->isephemeral && virSecretObjSaveData(obj) < 0)
>> + if (!def->isephemeral && virSecretObjSaveData(obj) < 0)
>> goto error;
>
> and here.
>
similar to previous isephemeral.
John
> Pavel
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
