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 <[email protected]> > --- > 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 <[email protected]>
signature.asc
Description: PGP signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
