On Tue, Apr 25, 2017 at 07:58:58AM -0400, John Ferlan wrote:
>
>
> 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.
Like I replied to your replay, these change itself doesn't improve anything
and there is no benefit from these changes further in the series. If you
have some patches prepared in your branch that will benefit these changes
it should be part of that series, not this one.
>
> >> 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.
Once, but it's not a part of this series so without that patch it
doesn't add any benefit to the current code. This probably applies to
the remaining places.
And getting def->uuid instead of obj->def->uuid doesn't seem like an
improvement, it's used only once in this function. If there were more
usages of def it would make sense to do this change.
Pavel
> >> 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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
