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

[John Ferlan]

On 11/13/18 8:01 AM, Pavel Hrdina wrote:
> 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.
> 

Right - just some copy-pasta here.

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

That's fine - either/or.  I forgot to note the values were "sourced
from" the original commit d14524701 MAKE_FILE mocking logic and the
fetch/store logic in virCgroupGetMemoryStat which does the >> 10.

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

Oh yeah - forgot to do that after I realized the >> was necessary... Off
by a magnitude of 1024 is always easy to figure out though. Still the
"correct number" could be a matter of opinion, too. Do you view the
expected number as seen in the array without the shift or with it?  e.g.
for 'cache' is 1336619008 (expected w/o shift) or 1305292 (value w/
shift) the correct value?

> 
> Note: please keep the lines under 80 characters.

Hey, that's my line ;-)

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

Thanks I went with displaying the shifted value:

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

But I won't push right away just in case someone prefers the unshifted
from the expected array.

John