On 04/25/2017 08:38 AM, Pavel Hrdina wrote:
> 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.
>
You want to see a 90 patch series? That includes all secret, nwfilter,
and interface changes plus a bunch of object changes? No one's even
commented on the object RFC I posted, but I need to make some progress;
otherwise, I end up with way too many patches to manage in my tree.
>>
>>>> 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.
>
Again, it's all about frame of reference. If this patch is unacceptable
then I may as well just stop doing any of this work. Really - there's
so much built up upon the separation of 'def' into it's own variable
that it's untenable to continue.
I think the question should be - is what I did technically wrong and not
is what I did seem unnecessary? Would the compiler actually do something
different (probably not). I personally find the code more readable to
have the separate defs. Are the "just" obj->def deref's absolutely
necessary - probably not for now and I could change those back. But for
any others where its obj->def->X, I feel the local def is a better
answer *regardless* of whether it's only used once.
I have an end goal in mind, but I can pretty much guarantee if I posted
all 90+ patches I have it'd probably mean the patches would languish on
the list because no one wants to review 90+ patches.
I contemplated splitting these up into 5-8 patch series, but that felt
way too tedious. So the two 15 patch series felt like a happy medium.
I do greatly appreciate you jumping right in right away and providing
reviews! While I may not agree exactly - I can only hope if you
understand the frame of reference it'll help understand why I think this
should be done now.
John
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
