8:41 a.m.
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(a)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.
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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list