Re: [libvirt] [PATCH] qemu: Get memory balloon info correctly for text monitor

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.