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

[Osier Yang]

于 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