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

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.

> 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,

> 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,

> 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,

> 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,

> 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.

Pavel