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

[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.

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