[libvirt] [PATCH] qemu: Get memory balloon info correctly for text monitor
* src/qemu/qemu_monitor_text.c: BALLOON_PREFIX was defined as "balloon: actual=", which cause "actual=" is stripped early before the real parsing. This patch changes BALLOON_PREFIX into "balloon: ", and modifies related functions, also renames "qemuMonitorParseExtraBalloonInfo" to "qemuMonitorParseBalloonInfo", as after the changing, it parses all the info returned by "info balloon". --- src/qemu/qemu_monitor_text.c | 47 +++++++++++++++++++++++++---------------- 1 files changed, 29 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 7bf733d..d17d863 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -547,8 +547,12 @@ static int parseMemoryStat(char **text, unsigned int tag, return 0; } - /* Convert bytes to kilobytes for libvirt */ switch (tag) { + /* Convert megabytes to kilobytes for libvirt */ + case VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON: + value = value << 10; + break; + /* Convert bytes to kilobytes for libvirt */ case VIR_DOMAIN_MEMORY_STAT_SWAP_IN: case VIR_DOMAIN_MEMORY_STAT_SWAP_OUT: case VIR_DOMAIN_MEMORY_STAT_UNUSED: @@ -565,15 +569,17 @@ static int parseMemoryStat(char **text, unsigned int tag, /* The reply from the 'info balloon' command may contain additional memory * statistics in the form: '[,<tag>=<val>]*' */ -static int qemuMonitorParseExtraBalloonInfo(char *text, - virDomainMemoryStatPtr stats, - unsigned int nr_stats) +static int qemuMonitorParseBalloonInfo(char *text, + virDomainMemoryStatPtr stats, + unsigned int nr_stats) { char *p = text; unsigned int nr_stats_found = 0; while (*p && nr_stats_found < nr_stats) { - if (parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_SWAP_IN, + if (parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, + "actual=", &stats[nr_stats_found]) || + parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_SWAP_IN, ",mem_swapped_in=", &stats[nr_stats_found]) || parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_SWAP_OUT, ",mem_swapped_out=", &stats[nr_stats_found]) || @@ -584,9 +590,7 @@ static int qemuMonitorParseExtraBalloonInfo(char *text, parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_UNUSED, ",free_mem=", &stats[nr_stats_found]) || parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_AVAILABLE, - ",total_mem=", &stats[nr_stats_found]) || - parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, - ",actual=", &stats[nr_stats_found])) + ",total_mem=", &stats[nr_stats_found])) nr_stats_found++; /* Skip to the next label. When *p is ',' the last match attempt @@ -602,7 +606,7 @@ static int qemuMonitorParseExtraBalloonInfo(char *text, /* The reply from QEMU contains 'ballon: actual=421' where value is in MB */ -#define BALLOON_PREFIX "balloon: actual=" +#define BALLOON_PREFIX "balloon: " /* * Returns: 0 if balloon not supported, +1 if balloon query worked @@ -625,13 +629,22 @@ int qemuMonitorTextGetBalloonInfo(qemuMonitorPtr mon, unsigned int memMB; char *end; offset += strlen(BALLOON_PREFIX); - if (virStrToLong_ui(offset, &end, 10, &memMB) < 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - _("could not parse memory balloon allocation from '%s'"), reply); - goto cleanup; + + if (STRPREFIX(offset, "actual=")) { + offset += strlen("actual="); + + if (virStrToLong_ui(offset, &end, 10, &memMB) < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("could not parse memory balloon allocation from '%s'"), reply); + goto cleanup; + } + *currmem = memMB * 1024; + ret = 1; + } else { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected balloon information '%s'"), reply); + ret = -1; } - *currmem = memMB * 1024; - ret = 1; } else { /* We don't raise an error here, since its to be expected that * many QEMU's don't support ballooning @@ -660,9 +673,7 @@ int qemuMonitorTextGetMemoryStats(qemuMonitorPtr mon, if ((offset = strstr(reply, BALLOON_PREFIX)) != NULL) { offset += strlen(BALLOON_PREFIX); - if ((offset = strchr(offset, ',')) != NULL) { - ret = qemuMonitorParseExtraBalloonInfo(offset, stats, nr_stats); - } + ret = qemuMonitorParseBalloonInfo(offset, stats, nr_stats); } VIR_FREE(reply); -- 1.7.6
于 2011年08月15日 21:58, Osier Yang 写道:
* src/qemu/qemu_monitor_text.c: BALLOON_PREFIX was defined as "balloon: actual=", which cause "actual=" is stripped early before the real parsing. This patch changes BALLOON_PREFIX into "balloon: ", and modifies related functions, also renames "qemuMonitorParseExtraBalloonInfo" to "qemuMonitorParseBalloonInfo", as after the changing, it parses all the info returned by "info balloon".
Forgot to mention the problem, e.g. "virsh dommemstat $domain" returns empty result.
--- src/qemu/qemu_monitor_text.c | 47 +++++++++++++++++++++++++---------------- 1 files changed, 29 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 7bf733d..d17d863 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -547,8 +547,12 @@ static int parseMemoryStat(char **text, unsigned int tag, return 0; }
- /* Convert bytes to kilobytes for libvirt */ switch (tag) { + /* Convert megabytes to kilobytes for libvirt */ + case VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON: + value = value << 10; + break; + /* Convert bytes to kilobytes for libvirt */ case VIR_DOMAIN_MEMORY_STAT_SWAP_IN: case VIR_DOMAIN_MEMORY_STAT_SWAP_OUT: case VIR_DOMAIN_MEMORY_STAT_UNUSED: @@ -565,15 +569,17 @@ static int parseMemoryStat(char **text, unsigned int tag, /* The reply from the 'info balloon' command may contain additional memory * statistics in the form: '[,<tag>=<val>]*' */ -static int qemuMonitorParseExtraBalloonInfo(char *text, - virDomainMemoryStatPtr stats, - unsigned int nr_stats) +static int qemuMonitorParseBalloonInfo(char *text, + virDomainMemoryStatPtr stats, + unsigned int nr_stats) { char *p = text; unsigned int nr_stats_found = 0;
while (*p && nr_stats_found < nr_stats) { - if (parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_SWAP_IN, + if (parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, + "actual=", &stats[nr_stats_found]) || + parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_SWAP_IN, ",mem_swapped_in=", &stats[nr_stats_found]) || parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_SWAP_OUT, ",mem_swapped_out=", &stats[nr_stats_found]) || @@ -584,9 +590,7 @@ static int qemuMonitorParseExtraBalloonInfo(char *text, parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_UNUSED, ",free_mem=", &stats[nr_stats_found]) || parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_AVAILABLE, - ",total_mem=", &stats[nr_stats_found]) || - parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, - ",actual=", &stats[nr_stats_found])) + ",total_mem=", &stats[nr_stats_found])) nr_stats_found++;
/* Skip to the next label. When *p is ',' the last match attempt @@ -602,7 +606,7 @@ static int qemuMonitorParseExtraBalloonInfo(char *text,
/* The reply from QEMU contains 'ballon: actual=421' where value is in MB */ -#define BALLOON_PREFIX "balloon: actual=" +#define BALLOON_PREFIX "balloon: "
/* * Returns: 0 if balloon not supported, +1 if balloon query worked @@ -625,13 +629,22 @@ int qemuMonitorTextGetBalloonInfo(qemuMonitorPtr mon, unsigned int memMB; char *end; offset += strlen(BALLOON_PREFIX); - if (virStrToLong_ui(offset, &end, 10, &memMB) < 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - _("could not parse memory balloon allocation from '%s'"), reply); - goto cleanup; + + if (STRPREFIX(offset, "actual=")) { + offset += strlen("actual="); + + if (virStrToLong_ui(offset, &end, 10, &memMB) < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("could not parse memory balloon allocation from '%s'"), reply); + goto cleanup; + } + *currmem = memMB * 1024; + ret = 1; + } else { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected balloon information '%s'"), reply); + ret = -1; } - *currmem = memMB * 1024; - ret = 1; } else { /* We don't raise an error here, since its to be expected that * many QEMU's don't support ballooning @@ -660,9 +673,7 @@ int qemuMonitorTextGetMemoryStats(qemuMonitorPtr mon,
if ((offset = strstr(reply, BALLOON_PREFIX)) != NULL) { offset += strlen(BALLOON_PREFIX); - if ((offset = strchr(offset, ',')) != NULL) { - ret = qemuMonitorParseExtraBalloonInfo(offset, stats, nr_stats); - } + ret = qemuMonitorParseBalloonInfo(offset, stats, nr_stats); }
VIR_FREE(reply);
On 08/15/2011 08:23 AM, Osier Yang wrote:
于 2011年08月15日 21:58, Osier Yang 写道:
* src/qemu/qemu_monitor_text.c: BALLOON_PREFIX was defined as "balloon: actual=", which cause "actual=" is stripped early before the real parsing. This patch changes BALLOON_PREFIX into "balloon: ", and modifies related functions, also renames "qemuMonitorParseExtraBalloonInfo" to "qemuMonitorParseBalloonInfo", as after the changing, it parses all the info returned by "info balloon".
Forgot to mention the problem, e.g. "virsh dommemstat $domain" returns empty result.
That is because qemu has disabled stats reporting and so the extra fields are not present in the info balloon response. -- Adam Litke IBM Linux Technology Center
On Mon, Aug 15, 2011 at 11:27:43AM -0500, Adam Litke wrote:
On 08/15/2011 08:23 AM, Osier Yang wrote:
于 2011年08月15日 21:58, Osier Yang 写道:
* src/qemu/qemu_monitor_text.c: BALLOON_PREFIX was defined as "balloon: actual=", which cause "actual=" is stripped early before the real parsing. This patch changes BALLOON_PREFIX into "balloon: ", and modifies related functions, also renames "qemuMonitorParseExtraBalloonInfo" to "qemuMonitorParseBalloonInfo", as after the changing, it parses all the info returned by "info balloon".
Forgot to mention the problem, e.g. "virsh dommemstat $domain" returns empty result.
That is because qemu has disabled stats reporting and so the extra fields are not present in the info balloon response.
I'd completely forgotten about this problem. We really should try to get this fixed & renabled in QEMU sometime in the not too distant future. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 08/15/2011 11:50 AM, Daniel P. Berrange wrote:
On Mon, Aug 15, 2011 at 11:27:43AM -0500, Adam Litke wrote:
On 08/15/2011 08:23 AM, Osier Yang wrote:
于 2011年08月15日 21:58, Osier Yang 写道:
* src/qemu/qemu_monitor_text.c: BALLOON_PREFIX was defined as "balloon: actual=", which cause "actual=" is stripped early before the real parsing. This patch changes BALLOON_PREFIX into "balloon: ", and modifies related functions, also renames "qemuMonitorParseExtraBalloonInfo" to "qemuMonitorParseBalloonInfo", as after the changing, it parses all the info returned by "info balloon".
Forgot to mention the problem, e.g. "virsh dommemstat $domain" returns empty result.
That is because qemu has disabled stats reporting and so the extra fields are not present in the info balloon response.
I'd completely forgotten about this problem. We really should try to get this fixed & renabled in QEMU sometime in the not too distant future.
I agree. The problem is that qemu lacks a proper interface for asynchronous commands so an unresponsive guest could freeze the monitor. QMP is undergoing a significant overhaul as a result of the new QAPI framework. It is my understanding that this new framework will provide a robust async interface, allowing us to re-enable balloon stats. -- Adam Litke IBM Linux Technology Center
于 2011年08月16日 00:27, Adam Litke 写道:
On 08/15/2011 08:23 AM, Osier Yang wrote:
于 2011年08月15日 21:58, Osier Yang 写道:
* src/qemu/qemu_monitor_text.c: BALLOON_PREFIX was defined as "balloon: actual=", which cause "actual=" is stripped early before the real parsing. This patch changes BALLOON_PREFIX into "balloon: ", and modifies related functions, also renames "qemuMonitorParseExtraBalloonInfo" to "qemuMonitorParseBalloonInfo", as after the changing, it parses all the info returned by "info balloon". Forgot to mention the problem, e.g. "virsh dommemstat $domain" returns empty result. That is because qemu has disabled stats reporting and so the extra fields are not present in the info balloon response.
Yes, I made a patch for this before (commit 41514f7b), but I didn't realize the "actual=" is stripped early before the parsing, so with the commit, one will get value of "actual" at least in QMP mode even the extra fields are disabled by qemu. But for text monitor, the problem still exists, this patch is to fix it. Osier
Hi Osier, Just to be clear, this is a cleanup not a bugfix right? The current code should be working properly as written. On 08/15/2011 08:58 AM, Osier Yang wrote:
* src/qemu/qemu_monitor_text.c: BALLOON_PREFIX was defined as "balloon: actual=", which cause "actual=" is stripped early before the real parsing. This patch changes BALLOON_PREFIX into "balloon: ", and modifies related functions, also renames "qemuMonitorParseExtraBalloonInfo" to "qemuMonitorParseBalloonInfo", as after the changing, it parses all the info returned by "info balloon". --- src/qemu/qemu_monitor_text.c | 47 +++++++++++++++++++++++++---------------- 1 files changed, 29 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 7bf733d..d17d863 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -547,8 +547,12 @@ static int parseMemoryStat(char **text, unsigned int tag, return 0; }
- /* Convert bytes to kilobytes for libvirt */ switch (tag) { + /* Convert megabytes to kilobytes for libvirt */ + case VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON: + value = value << 10; + break; + /* Convert bytes to kilobytes for libvirt */ case VIR_DOMAIN_MEMORY_STAT_SWAP_IN: case VIR_DOMAIN_MEMORY_STAT_SWAP_OUT: case VIR_DOMAIN_MEMORY_STAT_UNUSED: @@ -565,15 +569,17 @@ static int parseMemoryStat(char **text, unsigned int tag, /* The reply from the 'info balloon' command may contain additional memory * statistics in the form: '[,<tag>=<val>]*' */ -static int qemuMonitorParseExtraBalloonInfo(char *text, - virDomainMemoryStatPtr stats, - unsigned int nr_stats) +static int qemuMonitorParseBalloonInfo(char *text, + virDomainMemoryStatPtr stats, + unsigned int nr_stats) { char *p = text; unsigned int nr_stats_found = 0;
while (*p && nr_stats_found < nr_stats) { - if (parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_SWAP_IN, + if (parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, + "actual=", &stats[nr_stats_found]) || + parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_SWAP_IN, ",mem_swapped_in=", &stats[nr_stats_found]) || parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_SWAP_OUT, ",mem_swapped_out=", &stats[nr_stats_found]) ||
According to this code (and actual monitor behavior) 'actual' always appears and has to come first. Therefore, I would still parse the 'actual' stat outside of the while loop.
@@ -584,9 +590,7 @@ static int qemuMonitorParseExtraBalloonInfo(char *text, parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_UNUSED, ",free_mem=", &stats[nr_stats_found]) || parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_AVAILABLE, - ",total_mem=", &stats[nr_stats_found]) || - parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, - ",actual=", &stats[nr_stats_found])) + ",total_mem=", &stats[nr_stats_found])) nr_stats_found++;
/* Skip to the next label. When *p is ',' the last match attempt @@ -602,7 +606,7 @@ static int qemuMonitorParseExtraBalloonInfo(char *text,
/* The reply from QEMU contains 'ballon: actual=421' where value is in MB */ -#define BALLOON_PREFIX "balloon: actual=" +#define BALLOON_PREFIX "balloon: "
/* * Returns: 0 if balloon not supported, +1 if balloon query worked @@ -625,13 +629,22 @@ int qemuMonitorTextGetBalloonInfo(qemuMonitorPtr mon, unsigned int memMB; char *end; offset += strlen(BALLOON_PREFIX); - if (virStrToLong_ui(offset, &end, 10, &memMB) < 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - _("could not parse memory balloon allocation from '%s'"), reply); - goto cleanup; + + if (STRPREFIX(offset, "actual=")) { + offset += strlen("actual="); + + if (virStrToLong_ui(offset, &end, 10, &memMB) < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("could not parse memory balloon allocation from '%s'"), reply); + goto cleanup; + } + *currmem = memMB * 1024; + ret = 1; + } else { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected balloon information '%s'"), reply); + ret = -1; } - *currmem = memMB * 1024; - ret = 1; } else { /* We don't raise an error here, since its to be expected that * many QEMU's don't support ballooning
Why not just use qemuMonitorParseBalloonInfo() for the logic above? You only need to request one stat since actual must be first.
@@ -660,9 +673,7 @@ int qemuMonitorTextGetMemoryStats(qemuMonitorPtr mon,
if ((offset = strstr(reply, BALLOON_PREFIX)) != NULL) { offset += strlen(BALLOON_PREFIX); - if ((offset = strchr(offset, ',')) != NULL) { - ret = qemuMonitorParseExtraBalloonInfo(offset, stats, nr_stats); - } + ret = qemuMonitorParseBalloonInfo(offset, stats, nr_stats); }
VIR_FREE(reply);
-- Adam Litke IBM Linux Technology Center
于 2011年08月16日 00:40, Adam Litke 写道:
Hi Osier,
Just to be clear, this is a cleanup not a bugfix right? The current code should be working properly as written.
No, "virDomainMemoryStats" will return empty result if libvirt communicates to qemu monitor in text mode. Actually this is reported by a upstream Debian user, It seems to me packager of Debian compiles libvirt without QMP support.
On 08/15/2011 08:58 AM, Osier Yang wrote:
* src/qemu/qemu_monitor_text.c: BALLOON_PREFIX was defined as "balloon: actual=", which cause "actual=" is stripped early before the real parsing. This patch changes BALLOON_PREFIX into "balloon: ", and modifies related functions, also renames "qemuMonitorParseExtraBalloonInfo" to "qemuMonitorParseBalloonInfo", as after the changing, it parses all the info returned by "info balloon". --- src/qemu/qemu_monitor_text.c | 47 +++++++++++++++++++++++++---------------- 1 files changed, 29 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 7bf733d..d17d863 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -547,8 +547,12 @@ static int parseMemoryStat(char **text, unsigned int tag, return 0; }
- /* Convert bytes to kilobytes for libvirt */ switch (tag) { + /* Convert megabytes to kilobytes for libvirt */ + case VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON: + value = value<< 10; + break; + /* Convert bytes to kilobytes for libvirt */ case VIR_DOMAIN_MEMORY_STAT_SWAP_IN: case VIR_DOMAIN_MEMORY_STAT_SWAP_OUT: case VIR_DOMAIN_MEMORY_STAT_UNUSED: @@ -565,15 +569,17 @@ static int parseMemoryStat(char **text, unsigned int tag, /* The reply from the 'info balloon' command may contain additional memory * statistics in the form: '[,<tag>=<val>]*' */ -static int qemuMonitorParseExtraBalloonInfo(char *text, - virDomainMemoryStatPtr stats, - unsigned int nr_stats) +static int qemuMonitorParseBalloonInfo(char *text, + virDomainMemoryStatPtr stats, + unsigned int nr_stats) { char *p = text; unsigned int nr_stats_found = 0;
while (*p&& nr_stats_found< nr_stats) { - if (parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_SWAP_IN, + if (parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, + "actual=",&stats[nr_stats_found]) || + parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_SWAP_IN, ",mem_swapped_in=",&stats[nr_stats_found]) || parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_SWAP_OUT, ",mem_swapped_out=",&stats[nr_stats_found]) || According to this code (and actual monitor behavior) 'actual' always appears and has to come first. Therefore, I would still parse the 'actual' stat outside of the while loop.
@@ -584,9 +590,7 @@ static int qemuMonitorParseExtraBalloonInfo(char *text, parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_UNUSED, ",free_mem=",&stats[nr_stats_found]) || parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_AVAILABLE, - ",total_mem=",&stats[nr_stats_found]) || - parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, - ",actual=",&stats[nr_stats_found])) + ",total_mem=",&stats[nr_stats_found])) nr_stats_found++;
/* Skip to the next label. When *p is ',' the last match attempt @@ -602,7 +606,7 @@ static int qemuMonitorParseExtraBalloonInfo(char *text,
/* The reply from QEMU contains 'ballon: actual=421' where value is in MB */ -#define BALLOON_PREFIX "balloon: actual=" +#define BALLOON_PREFIX "balloon: "
/* * Returns: 0 if balloon not supported, +1 if balloon query worked @@ -625,13 +629,22 @@ int qemuMonitorTextGetBalloonInfo(qemuMonitorPtr mon, unsigned int memMB; char *end; offset += strlen(BALLOON_PREFIX); - if (virStrToLong_ui(offset,&end, 10,&memMB)< 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - _("could not parse memory balloon allocation from '%s'"), reply); - goto cleanup; + + if (STRPREFIX(offset, "actual=")) { + offset += strlen("actual="); + + if (virStrToLong_ui(offset,&end, 10,&memMB)< 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("could not parse memory balloon allocation from '%s'"), reply); + goto cleanup; + } + *currmem = memMB * 1024; + ret = 1; + } else { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected balloon information '%s'"), reply); + ret = -1; } - *currmem = memMB * 1024; - ret = 1; } else { /* We don't raise an error here, since its to be expected that * many QEMU's don't support ballooning Why not just use qemuMonitorParseBalloonInfo() for the logic above? You only need to request one stat since actual must be first.
Not sure if I get your meaning correctly. If parsing "actual" in qemuMonitorParseBalloonInfo, then yes, agree using qemuMonitorParseballoonInfo here is more simpler. But it looks to me you object to parse "actual=" in qemuMonitorParseBalloonInfo. :) I want to explain more about this, all the purpose of this patch is to prevent "actual=" being stripped before the parsing. And it's fix a problem, not cleanup. Osier
On 08/15/2011 10:28 PM, Osier Yang wrote:
于 2011年08月16日 00:40, Adam Litke 写道:
Hi Osier,
Just to be clear, this is a cleanup not a bugfix right? The current code should be working properly as written.
No, "virDomainMemoryStats" will return empty result if libvirt communicates to qemu monitor in text mode. Actually this is reported by a upstream Debian user, It seems to me packager of Debian compiles libvirt without QMP support.
Ok, I am now understanding what you are trying to do. Originally, the virDomainMemoryStats API did not include the 'actual' field. At the time it seemed redundant to include it since that information is already returned by virDomainGetInfo(). However, I am not opposed to including it here.
On 08/15/2011 08:58 AM, Osier Yang wrote:
* src/qemu/qemu_monitor_text.c: BALLOON_PREFIX was defined as "balloon: actual=", which cause "actual=" is stripped early before the real parsing. This patch changes BALLOON_PREFIX into "balloon: ", and modifies related functions, also renames "qemuMonitorParseExtraBalloonInfo" to "qemuMonitorParseBalloonInfo", as after the changing, it parses all the info returned by "info balloon". --- src/qemu/qemu_monitor_text.c | 47 +++++++++++++++++++++++++---------------- 1 files changed, 29 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 7bf733d..d17d863 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -547,8 +547,12 @@ static int parseMemoryStat(char **text, unsigned int tag, return 0; }
- /* Convert bytes to kilobytes for libvirt */ switch (tag) { + /* Convert megabytes to kilobytes for libvirt */ + case VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON: + value = value<< 10; + break; + /* Convert bytes to kilobytes for libvirt */ case VIR_DOMAIN_MEMORY_STAT_SWAP_IN: case VIR_DOMAIN_MEMORY_STAT_SWAP_OUT: case VIR_DOMAIN_MEMORY_STAT_UNUSED: @@ -565,15 +569,17 @@ static int parseMemoryStat(char **text, unsigned int tag, /* The reply from the 'info balloon' command may contain additional memory * statistics in the form: '[,<tag>=<val>]*' */ -static int qemuMonitorParseExtraBalloonInfo(char *text, - virDomainMemoryStatPtr stats, - unsigned int nr_stats) +static int qemuMonitorParseBalloonInfo(char *text, + virDomainMemoryStatPtr stats, + unsigned int nr_stats) { char *p = text; unsigned int nr_stats_found = 0;
while (*p&& nr_stats_found< nr_stats) { - if (parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_SWAP_IN, + if (parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, + "actual=",&stats[nr_stats_found]) || + parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_SWAP_IN,
Also, don't forget to edit the comment at the top of the function. The expected format is now: actual=<val>[,<tag>=<val>]*
",mem_swapped_in=",&stats[nr_stats_found]) || parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_SWAP_OUT,
",mem_swapped_out=",&stats[nr_stats_found]) || According to this code (and actual monitor behavior) 'actual' always appears and has to come first. Therefore, I would still parse the 'actual' stat outside of the while loop.
@@ -584,9 +590,7 @@ static int qemuMonitorParseExtraBalloonInfo(char *text, parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_UNUSED, ",free_mem=",&stats[nr_stats_found]) || parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_AVAILABLE, - ",total_mem=",&stats[nr_stats_found]) || - parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, - ",actual=",&stats[nr_stats_found])) + ",total_mem=",&stats[nr_stats_found])) nr_stats_found++;
/* Skip to the next label. When *p is ',' the last match attempt @@ -602,7 +606,7 @@ static int qemuMonitorParseExtraBalloonInfo(char *text,
/* The reply from QEMU contains 'ballon: actual=421' where value is in MB */ -#define BALLOON_PREFIX "balloon: actual=" +#define BALLOON_PREFIX "balloon: "
/* * Returns: 0 if balloon not supported, +1 if balloon query worked @@ -625,13 +629,22 @@ int qemuMonitorTextGetBalloonInfo(qemuMonitorPtr mon, unsigned int memMB; char *end; offset += strlen(BALLOON_PREFIX); - if (virStrToLong_ui(offset,&end, 10,&memMB)< 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - _("could not parse memory balloon allocation from '%s'"), reply); - goto cleanup; + + if (STRPREFIX(offset, "actual=")) { + offset += strlen("actual="); + + if (virStrToLong_ui(offset,&end, 10,&memMB)< 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("could not parse memory balloon allocation from '%s'"), reply); + goto cleanup; + } + *currmem = memMB * 1024; + ret = 1; + } else { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected balloon information '%s'"), reply); + ret = -1; } - *currmem = memMB * 1024; - ret = 1; } else { /* We don't raise an error here, since its to be expected that * many QEMU's don't support ballooning Why not just use qemuMonitorParseBalloonInfo() for the logic above? You only need to request one stat since actual must be first.
Not sure if I get your meaning correctly.
If parsing "actual" in qemuMonitorParseBalloonInfo, then yes, agree using qemuMonitorParseballoonInfo here is more simpler.
But it looks to me you object to parse "actual=" in qemuMonitorParseBalloonInfo. :)
Nope, I agree that actual= should be parsed in qemuMonitorParseBalloonInfo(). I am just recommending that the parseMemoryStat() call for 'actual' be moved above the while loop so that it always occurs first.
I want to explain more about this, all the purpose of this patch is to prevent "actual=" being stripped before the parsing. And it's fix a problem, not cleanup.
Yep, got it. Thanks. -- Adam Litke IBM Linux Technology Center
* src/qemu/qemu_monitor_text.c: BALLOON_PREFIX was defined as "balloon: actual=", which cause "actual=" is stripped early before the real parsing. This patch changes BALLOON_PREFIX into "balloon: ", and modifies related functions, also renames "qemuMonitorParseExtraBalloonInfo" to "qemuMonitorParseBalloonInfo", as after the changing, it parses all the info returned by "info balloon". v2: Adopted Adam's suggestion, parse "actual=" outside of the loop of qemuMonitorParseBalloonInfo, and use qemuMonitorParseBalloonInfo for qemuMonitorTextGetBalloonInfo. --- src/qemu/qemu_monitor_text.c | 53 +++++++++++++++++++++++++++-------------- 1 files changed, 35 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 335e39e..a661626 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -547,8 +547,12 @@ static int parseMemoryStat(char **text, unsigned int tag, return 0; } - /* Convert bytes to kilobytes for libvirt */ switch (tag) { + /* Convert megabytes to kilobytes for libvirt */ + case VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON: + value = value << 10; + break; + /* Convert bytes to kilobytes for libvirt */ case VIR_DOMAIN_MEMORY_STAT_SWAP_IN: case VIR_DOMAIN_MEMORY_STAT_SWAP_OUT: case VIR_DOMAIN_MEMORY_STAT_UNUSED: @@ -563,15 +567,25 @@ static int parseMemoryStat(char **text, unsigned int tag, } /* The reply from the 'info balloon' command may contain additional memory - * statistics in the form: '[,<tag>=<val>]*' + * statistics in the form: 'actual=<val> [,<tag>=<val>]*' */ -static int qemuMonitorParseExtraBalloonInfo(char *text, - virDomainMemoryStatPtr stats, - unsigned int nr_stats) +static int qemuMonitorParseBalloonInfo(char *text, + virDomainMemoryStatPtr stats, + unsigned int nr_stats) { char *p = text; unsigned int nr_stats_found = 0; + /* Since "actual=" always comes first in the returned string, + * and sometime we only care about the value of "actual", such + * as qemuMonitorGetBalloonInfo, so parse it outside of the + * loop. + */ + if (parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, + "actual=", &stats[nr_stats_found]) == 1) { + nr_stats_found++; + } + while (*p && nr_stats_found < nr_stats) { if (parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_SWAP_IN, ",mem_swapped_in=", &stats[nr_stats_found]) || @@ -584,9 +598,7 @@ static int qemuMonitorParseExtraBalloonInfo(char *text, parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_UNUSED, ",free_mem=", &stats[nr_stats_found]) || parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_AVAILABLE, - ",total_mem=", &stats[nr_stats_found]) || - parseMemoryStat(&p, VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, - ",actual=", &stats[nr_stats_found])) + ",total_mem=", &stats[nr_stats_found])) nr_stats_found++; /* Skip to the next label. When *p is ',' the last match attempt @@ -602,7 +614,7 @@ static int qemuMonitorParseExtraBalloonInfo(char *text, /* The reply from QEMU contains 'ballon: actual=421' where value is in MB */ -#define BALLOON_PREFIX "balloon: actual=" +#define BALLOON_PREFIX "balloon: " /* * Returns: 0 if balloon not supported, +1 if balloon query worked @@ -622,15 +634,22 @@ int qemuMonitorTextGetBalloonInfo(qemuMonitorPtr mon, } if ((offset = strstr(reply, BALLOON_PREFIX)) != NULL) { - unsigned int memMB; - char *end; offset += strlen(BALLOON_PREFIX); - if (virStrToLong_ui(offset, &end, 10, &memMB) < 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - _("could not parse memory balloon allocation from '%s'"), reply); + struct _virDomainMemoryStat stats[1]; + + if (qemuMonitorParseBalloonInfo(offset, stats, 1) == 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected balloon information '%s'"), reply); goto cleanup; } - *currmem = memMB * 1024; + + if (stats[0].tag != VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected balloon information '%s'"), reply); + goto cleanup; + } + + *currmem = stats[0].val; ret = 1; } else { /* We don't raise an error here, since its to be expected that @@ -660,9 +679,7 @@ int qemuMonitorTextGetMemoryStats(qemuMonitorPtr mon, if ((offset = strstr(reply, BALLOON_PREFIX)) != NULL) { offset += strlen(BALLOON_PREFIX); - if ((offset = strchr(offset, ',')) != NULL) { - ret = qemuMonitorParseExtraBalloonInfo(offset, stats, nr_stats); - } + ret = qemuMonitorParseBalloonInfo(offset, stats, nr_stats); } VIR_FREE(reply); -- 1.7.6
On 08/19/2011 04:17 AM, Osier Yang wrote:
* src/qemu/qemu_monitor_text.c: BALLOON_PREFIX was defined as "balloon: actual=", which cause "actual=" is stripped early before the real parsing. This patch changes BALLOON_PREFIX into "balloon: ", and modifies related functions, also renames "qemuMonitorParseExtraBalloonInfo" to "qemuMonitorParseBalloonInfo", as after the changing, it parses all the info returned by "info balloon".
v2:
Adopted Adam's suggestion, parse "actual=" outside of the loop of qemuMonitorParseBalloonInfo, and use qemuMonitorParseBalloonInfo for qemuMonitorTextGetBalloonInfo. --- src/qemu/qemu_monitor_text.c | 53 +++++++++++++++++++++++++++-------------- 1 files changed, 35 insertions(+), 18 deletions(-)
Looks nicer.
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 335e39e..a661626 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -547,8 +547,12 @@ static int parseMemoryStat(char **text, unsigned int tag, return 0; }
- /* Convert bytes to kilobytes for libvirt */ switch (tag) { + /* Convert megabytes to kilobytes for libvirt */ + case VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON: + value = value<< 10;
Why not use 'value <<= 10;' for fewer characters?
char *p = text; unsigned int nr_stats_found = 0;
+ /* Since "actual=" always comes first in the returned string, + * and sometime we only care about the value of "actual", such
s/sometime/sometimes/
+ * as qemuMonitorGetBalloonInfo, so parse it outside of the
Either s/so/we/, or s/Since// - leaving both 'since' and 'so' sounds awkward in the same sentence. ACK with the nits fixed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年08月19日 21:26, Eric Blake 写道:
On 08/19/2011 04:17 AM, Osier Yang wrote:
* src/qemu/qemu_monitor_text.c: BALLOON_PREFIX was defined as "balloon: actual=", which cause "actual=" is stripped early before the real parsing. This patch changes BALLOON_PREFIX into "balloon: ", and modifies related functions, also renames "qemuMonitorParseExtraBalloonInfo" to "qemuMonitorParseBalloonInfo", as after the changing, it parses all the info returned by "info balloon".
v2:
Adopted Adam's suggestion, parse "actual=" outside of the loop of qemuMonitorParseBalloonInfo, and use qemuMonitorParseBalloonInfo for qemuMonitorTextGetBalloonInfo. --- src/qemu/qemu_monitor_text.c | 53 +++++++++++++++++++++++++++-------------- 1 files changed, 35 insertions(+), 18 deletions(-)
Looks nicer.
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 335e39e..a661626 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -547,8 +547,12 @@ static int parseMemoryStat(char **text, unsigned int tag, return 0; }
- /* Convert bytes to kilobytes for libvirt */ switch (tag) { + /* Convert megabytes to kilobytes for libvirt */ + case VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON: + value = value<< 10;
Why not use 'value <<= 10;' for fewer characters?
char *p = text; unsigned int nr_stats_found = 0;
+ /* Since "actual=" always comes first in the returned string, + * and sometime we only care about the value of "actual", such
s/sometime/sometimes/
+ * as qemuMonitorGetBalloonInfo, so parse it outside of the
Either s/so/we/, or s/Since// - leaving both 'since' and 'so' sounds awkward in the same sentence.
ACK with the nits fixed.
Pushed with the nits fixed, thanks Osier
participants (4)
-
Adam Litke -
Daniel P. Berrange -
Eric Blake -
Osier Yang