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@redhat.com> --- 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