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

[John Ferlan]

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.

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