[libvirt] [PATCH 09/14] secret: Split apart NumOfSecrets and GetUUIDs callback function

[John Ferlan]

On 04/25/2017 08:47 AM, Pavel Hrdina wrote:
> On Tue, Apr 25, 2017 at 08:03:58AM -0400, John Ferlan wrote:
>>
>>
>> On 04/25/2017 06:05 AM, Pavel Hrdina wrote:
>>> On Mon, Apr 24, 2017 at 02:00:18PM -0400, John Ferlan wrote:
>>>> Rather than overloading one function - split apart the logic to have
>>>> separate interfaces and local/private structures to manage the data
>>>> for which the helper is collecting.
>>>>
>>>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>>>> ---
>>>>  src/conf/virsecretobj.c | 98 +++++++++++++++++++++++++++++++------------------
>>>>  src/conf/virsecretobj.h |  6 +--
>>>>  2 files changed, 65 insertions(+), 39 deletions(-)
>>>>
>>>> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
>>>> index c410a6b..3717552 100644
>>>> --- a/src/conf/virsecretobj.c
>>>> +++ b/src/conf/virsecretobj.c
>>>> @@ -415,9 +415,54 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>>>>  }
>>>>  
>>>>  
>>>> -struct virSecretObjListGetHelperData {
>>>> +struct secretCountData {
>>>
>>> This doesn't follow our naming rules, all struct should be prefixed with *vir*.
>>>
>>
>> I guess I went on the character reduction plan. I can adjust.
>>
>>>>      virConnectPtr conn;
>>>> -    virSecretObjListACLFilter filter;
>>>> +    virSecretObjListACLFilter aclfilter;
>>>> +    int count;
>>>> +};
>>>> +
>>>> +static int
>>>> +virSecretObjListNumOfSecretsCallback(void *payload,
>>>> +                                     const void *name ATTRIBUTE_UNUSED,
>>>> +                                     void *opaque)
>>>> +{
>>>> +    struct secretCountData *data = opaque;
>>>> +    virSecretObjPtr obj = payload;
>>>> +    virSecretDefPtr def;
>>>> +
>>>> +    virObjectLock(obj);
>>>> +    def = obj->def;
>>>> +
>>>> +    if (data->aclfilter && !data->aclfilter(data->conn, def))
>>>> +        goto cleanup;
>>>
>>> This follows previous patch, in this case having separate variable for
>>> virSecretDefPtr doesn't give us any benefit, just a noise in the code.
>>>
>>
>> Well, yes it does eventually... It's a see the diff now or see more
>> diffs later...
>>
>>>> +
>>>> +    data->count++;
>>>> +
>>>> + cleanup:
>>>> +    virObjectUnlock(obj);
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +
>>>> +int
>>>> +virSecretObjListNumOfSecrets(virSecretObjListPtr secrets,
>>>> +                             virSecretObjListACLFilter aclfilter,
>>>> +                             virConnectPtr conn)
>>>> +{
>>>> +    struct secretCountData data = {
>>>> +        .conn = conn, .aclfilter = aclfilter, .count = 0 };
>>>> +
>>>> +    virObjectLock(secrets);
>>>> +    virHashForEach(secrets->objs, virSecretObjListNumOfSecretsCallback, &data);
>>>> +    virObjectUnlock(secrets);
>>>> +
>>>> +    return data.count;
>>>> +}
>>>
>>> Unnecessary movement of function.
>>>
>>
>> Well if you look logically at the code the setup used to be
>>
>> virSecretObjListGetHelper  <== Helper for both NumOf and GetUUIDs
>> virSecretObjListNumOfSecrets  <== Main API
>> virSecretObjMatchFlags <== Helper for ListPopulate
>> virSecretObjListPopulate <== Helper for ListExport (obvious, right)
>> virSecretObjListExport <== Main API
>> virSecretObjListGetUUIDs <== Main API
>>
>> The goal was to have "alike" code next to each other in the module and
>> to be "similarly named", thus
>>
>> virSecretObjListNumOfSecretsCallback
>> virSecretObjListNumOfSecrets
>> virSecretObjListGetUUIDsCallback
>> virSecretObjListGetUUIDs
>> virSecretObjMatchFlags
>> virSecretObjListExportCallback
>> virSecretObjListExport
>>
>> I *get* the code motion thing and the git history/blame concerns - still
>> they are *far worse* when moving code from one module to another. With
>> graphical tools like gitk it makes it a lot easier to trace/track the
>> history.
> 
> Yes, I understand the goal to have the related function next to each other,
> but we usually don't move the functions just to keep them together.  It makes
> the code look&feel nicer but it's not something that give as any real value.
> 

The "real value" is that I don't have to go searching for the companion
code. The "static" helper is right next to the external API.  Why have
it be essentially 120 lines later (e.g. patch 7 where I did move it).

It doesn't make sense visually to have:

virSecretObjListNumOfSecretsCallback
virSecretObjListGetUUIDsCallback
virSecretObjListNumOfSecrets
virSecretObjListGetUUIDs


Ironically it's less that virSecretObjListNumOfSecrets moved and more
that virSecretObjListGetHelper was split into two functions. The
difference code caused it to look more like
virSecretObjListGetUUIDsCallback replaces virSecretObjListGetHelper


John

> Pavel
> 
>>>> +
>>>> +
>>>> +struct secretListData {
>>>
>>> This should be virSecretListData.
>>>
>>
>> Sure that can change - although long term it won't matter though as
>> it'll go away to be replaced by a virObject* data structure that can
>> handle NumOf, GetUUIDs, and ListExport generically
>>
>> John
>>
>>>> +    virConnectPtr conn;
>>>> +    virSecretObjListACLFilter aclfilter;
>>>>      int nuuids;
>>>>      char **uuids;
>>>>      int maxuuids;
>>>
>>> Pavel
>>>
>>
>> --
>> libvir-list mailing list
>> libvir-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list