[libvirt] Parsing of memory.stat in libvirt
Hi Folks, libvirt currently spams the logs with error : virCgroupGetMemoryStat:2490 : internal error: Cannot parse 'memory.stat' cgroup file whenever someone does ps in a container. This is because the parser for memory/stat is incorrect: the `line' variable is never updated, so each time through the loop, the same start-of-line is compared for the token; and as all spaces are eventually replaced with NUL the error exit is taken instead of ending the loop properly. Here is a strawman patch to fix the problem. Please note, I'm not subscribed to this list; I reported the bug as https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=913023 and was asked to send the patch upstream. src/util/vircgroup.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) --- libvirt.orig/src/util/vircgroup.c +++ libvirt/src/util/vircgroup.c @@ -2477,7 +2477,7 @@ virCgroupGetMemoryStat(virCgroupPtr grou line = stat; - while (line) { + while (*line) { char *newLine = strchr(line, '\n'); char *valueStr = strchr(line, ' '); unsigned long long value; @@ -2507,6 +2507,11 @@ virCgroupGetMemoryStat(virCgroupPtr grou inactiveFileVal = value >> 10; else if (STREQ(line, "unevictable")) unevictableVal = value >> 10; + + if (newLine) + line = newLine + 1; + else + break; } *cache = cacheVal; -- Dr Peter Chubb Tel: +61 2 9490 5852 http://ts.data61.csiro.au/ Trustworthy Systems Group Data61 (formerly NICTA)
On 11/6/18 3:25 AM, Peter.Chubb@data61.csiro.au wrote:
Hi Folks, libvirt currently spams the logs with
error : virCgroupGetMemoryStat:2490 : internal error: Cannot parse 'memory.stat' cgroup file
whenever someone does ps in a container.
This is because the parser for memory/stat is incorrect: the `line' variable is never updated, so each time through the loop, the same start-of-line is compared for the token; and as all spaces are eventually replaced with NUL the error exit is taken instead of ending the loop properly.
Here is a strawman patch to fix the problem. Please note, I'm not subscribed to this list; I reported the bug as https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=913023 and was asked to send the patch upstream.
src/util/vircgroup.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
Hmmmm... well I see quite true... nice catch! Seems commit id 901d2b9c87 added the code and commit e634c7cd0 used it for LXC's virLXCCgroupGetMemStat (libvirt 4.7.0, so rather recent fortunately). However that conversion seemed to missed the nuance that getline() is a bit different than the algorithm generated parsing through the read file. Too bad an existing test wasn't utilized for it, because that would have failed miserably. An any case, the patch below is against an older version of libvirt, we typically work from top of the git tree, but since you're not a regular contributor I will put something together and CC you using top of tree. Since we adhere to usage of Signed-Off-By, I'd need to have you agree to have me add an S-O-B with your name. For now I'll put my name with you as the author.
--- libvirt.orig/src/util/vircgroup.c +++ libvirt/src/util/vircgroup.c @@ -2477,7 +2477,7 @@ virCgroupGetMemoryStat(virCgroupPtr grou
line = stat;
- while (line) { + while (*line) {
probably should be line && *line Since if line was for who knows what reason NULL, then life wouldn't be happy. Tks, John updated patch will be sent shortly (with any luck).
char *newLine = strchr(line, '\n'); char *valueStr = strchr(line, ' '); unsigned long long value; @@ -2507,6 +2507,11 @@ virCgroupGetMemoryStat(virCgroupPtr grou inactiveFileVal = value >> 10; else if (STREQ(line, "unevictable")) unevictableVal = value >> 10; + + if (newLine) + line = newLine + 1; + else + break; }
*cache = cacheVal;
"John" == John Ferlan <jferlan@redhat.com> writes:
On 11/6/18 3:25 AM, Peter.Chubb@data61.csiro.au wrote:
Hi Folks, libvirt currently spams the logs with
error : virCgroupGetMemoryStat:2490 : internal error: Cannot parse 'memory.stat' cgroup file
whenever someone does ps in a container.
This is because the parser for memory/stat is incorrect: the `line' variable is never updated, so each time through the loop, the same start-of-line is compared for the token; and as all spaces are eventually replaced with NUL the error exit is taken instead of ending the loop properly.
Here is a strawman patch to fix the problem. Please note, I'm not subscribed to this list; I reported the bug as https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=913023 and was asked to send the patch upstream.
src/util/vircgroup.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
John> Hmmmm... well I see quite true... nice catch! John> An any case, the patch below is against an older version of John> libvirt, we typically work from top of the git tree, but since John> you're not a regular contributor I will put something together John> and CC you using top of tree. Since we adhere to usage of John> Signed-Off-By, I'd need to have you agree to have me add an John> S-O-B with your name. For now I'll put my name with you as the John> author. Sure, I'll add it to the individual patches.
--- libvirt.orig/src/util/vircgroup.c +++ libvirt/src/util/vircgroup.c @@ -2477,7 +2477,7 @@ virCgroupGetMemoryStat(virCgroupPtr grou
line = stat;
- while (line) { + while (*line) {
John> probably should be line && *line Since if line was for who knows John> what reason NULL, then life wouldn't be happy. line can't be NULL at this point. virCgroupGetValueStr() returns non-zero if it hasn't set stat to something sensible. An extra check doesn't hurt though. Peter C -- Dr Peter Chubb Tel: +61 2 9490 5852 http://ts.data61.csiro.au/ Trustworthy Systems Group Data61, CSIRO (formerly NICTA)
participants (2)
-
John Ferlan -
Peter.Chubb＠data61.csiro.au