On Wed, Nov 07, 2018 at 06:57:39PM -0500, John Ferlan wrote:

From: Peter Chubb <Peter.Chubb@data61.csiro.au>

Commit 901d2b9c introduced virCgroupGetMemoryStat and replaced the LXC virLXCCgroupGetMemStat logic in commit e634c7cd0. However, in doing so the replacement wasn't exact as the LXC logic used getline() to process the cgroup controller data, while the new virCgroupGetMemoryStat used "memory.stat" manual buffer read/ processing which neglected to forward through @line in order to read each line in the output.

To fix that, we should be sure to carry forward the @line value for each line read updating it beyond that current @newLine value once we've calculated the values that we want.

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/vircgroupv1.c | 7 ++++++- src/util/vircgroupv2.c | 7 ++++++- 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c index 28a74474ee..678ffda35c 100644 --- a/src/util/vircgroupv1.c +++ b/src/util/vircgroupv1.c @@ -1476,7 +1476,7 @@ virCgroupV1GetMemoryStat(virCgroupPtr group,

line = stat;

- while (line) { + while (line && *line) {

There is no need to check line, it cannot be NULL.

char *newLine = strchr(line, '\n'); char *valueStr = strchr(line, ' '); unsigned long long value; @@ -1506,6 +1506,11 @@ virCgroupV1GetMemoryStat(virCgroupPtr group, inactiveFileVal = value >> 10; else if (STREQ(line, "unevictable")) unevictableVal = value >> 10; + + if (newLine) + line = newLine + 1; + else + break; }

*cache = cacheVal; diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c index 32adb06784..541e8e790e 100644 --- a/src/util/vircgroupv2.c +++ b/src/util/vircgroupv2.c @@ -1068,7 +1068,7 @@ virCgroupV2GetMemoryStat(virCgroupPtr group,

line = stat;

- while (line) { + while (line && *line) {

Same here.

char *newLine = strchr(line, '\n'); char *valueStr = strchr(line, ' '); unsigned long long value; @@ -1102,6 +1102,11 @@ virCgroupV2GetMemoryStat(virCgroupPtr group, inactiveFileVal = value >> 10; else if (STREQ(line, "unevictable")) unevictableVal = value >> 10; + + if (newLine) + line = newLine + 1; + else + break; }

*cache = cacheVal; --

Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

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.

+ 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@redhat.com>