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