[libvirt] [PATCH 2/2] tests: Augment vcgrouptest to add virCgroupGetMemoryStat

[Pavel Hrdina]

On Wed, Nov 07, 2018 at 06:57:40PM -0500, John Ferlan wrote:
> Add a test to fetch the GetMemoryStat output. This only gets
> data for v1 only right now since the v2 data from commit 61ff6021
> is rather useless returning all 0's.
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  tests/vircgrouptest.c | 61 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/tests/vircgrouptest.c b/tests/vircgrouptest.c
> index 310e1fb6a2..06c4a8ef5c 100644
> --- a/tests/vircgrouptest.c
> +++ b/tests/vircgrouptest.c
> @@ -802,6 +802,64 @@ static int testCgroupGetMemoryUsage(const void *args ATTRIBUTE_UNUSED)
>      return ret;
>  }
>  
> +
> +static int
> +testCgroupGetMemoryStat(const void *args ATTRIBUTE_UNUSED)
> +{
> +    virCgroupPtr cgroup = NULL;
> +    int rv, ret = -1;

Please each variable on separate line.  Once you need to change/remove
any of the variable the diff is way better.

> +    size_t i;
> +
> +    const unsigned long long expected_values[] = {
> +        1336619008ULL,
> +        67100672ULL,
> +        145887232ULL,
> +        661872640ULL,
> +        627400704UL,
> +        3690496ULL
> +    };
> +    const char* names[] = {
> +        "cache",
> +        "active_anon",
> +        "inactive_anon",
> +        "active_file",
> +        "inactive_file",
> +        "unevictable"
> +    };
> +    unsigned long long values[ARRAY_CARDINALITY(expected_values)];
> +
> +    if ((rv = virCgroupNewPartition("/virtualmachines", true,
> +                                    (1 << VIR_CGROUP_CONTROLLER_MEMORY),
> +                                    &cgroup)) < 0) {
> +        fprintf(stderr, "Could not create /virtualmachines cgroup: %d\n", -rv);
> +        goto cleanup;
> +    }
> +
> +    if ((rv = virCgroupGetMemoryStat(cgroup, &values[0],
> +                                     &values[1], &values[2],
> +                                     &values[3], &values[4],
> +                                     &values[5])) < 0) {
> +        fprintf(stderr, "Could not retrieve GetMemoryStat for /virtualmachines cgroup: %d\n", -rv);
> +        goto cleanup;
> +    }
> +
> +    for (i = 0; i < ARRAY_CARDINALITY(expected_values); i++) {
> +        if (expected_values[i] != (values[i] << 10)) {

This feels wrong and it's just a lucky coincidence that it works with
these values. It's basically the same operation as 'x * 1024'.

I would rather change it into this:

         if ((expected_values[i] >> 10) != values[i]) {

because we know that we do the same operation after reading these values
from memory.stat file.

> +            fprintf(stderr,
> +                    "Wrong value (%llu) for %s from virCgroupGetMemoryStat (expected %llu)\n",
> +                    values[i], names[i], expected_values[i]);

This would print wrong values, we need to print shifted values.
Probably the best solution would be to have "expected_values" with the
correct number from the start.

Note: please keep the lines under 80 characters.

Because it's a test I'm OK with both solutions, modifying
"expected_values" in place or to have them correct from the start and
I'll leave it up to you.  There is no need to resend it.

Reviewed-by: Pavel Hrdina <phrdina at redhat.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20181113/9564e83b/attachment-0001.sig>