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