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