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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
