Re: [libvirt] [PATCH 06/14] secret: Use virSecretDefPtr rather than deref from virSecretObjPtr

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(a)redhat.com&gt; >> --- >> 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(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list