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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
