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