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

[Pavel Hrdina]

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.

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170425/26b8c4d8/attachment-0001.sig>