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

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(a)redhat.com&gt; >> --- >> 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 > -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list