Re: [libvirt] [PATCHv2 2/2] conf: allow fuzz in XML with cur balloon > max
On 03/31/2012 01:19 AM, Osier Yang wrote:
> On 03/30/2012 11:56 PM, Eric Blake wrote:
>> Commit 1b1402b introduced a regression. Since older libvirt versions
>> would silently round memory up (until the previous patch), but populated
>> current memory based on querying the guest, it was possible to have
>> current> maximum by the amount of the rounding. Accept this fuzz
>> factor, and silently clamp current down to maximum in that case,
>> rather than failing to reparse XML for an existing VM.
>>
>> * src/conf/domain_conf.c (virDomainDefParseXML): Don't reject
>> existing XML.
>> Based on a report by Zhou Peng.
>> ---
>> src/conf/domain_conf.c | 19 +++
>> src/qemu/qemu_monitor_json.c |2 +-
>> 2 files changed, 16 insertions(+), 5 deletions(-)
>>
>> -goto error;
>> +/* Older libvirt could get into this situation due to
>> + * rounding; if the discrepancy is less than 1MiB, we silently
>> + * round down, otherwise we flag the issue. */
>> +if (VIR_DIV_UP(def->mem.cur_balloon, 1024)>
>> +VIR_DIV_UP(def->mem.max_balloon, 1024)) {
>> +virDomainReportError(VIR_ERR_XML_ERROR,
>> + _("current memory '%lluk' exceeds "
>> + "maximum '%lluk'"),
>> + def->mem.cur_balloon,
>> def->mem.max_balloon);
>> +goto error;
>> +} else {
>> +VIR_DEBUG("Truncating current %lluk to maximum %lluk",
>> + def->mem.cur_balloon, def->mem.max_balloon);
>> +def->mem.cur_balloon = def->mem.max_balloon;
>> +}
>
> But this needs the documentation IMHO, it actually changes
> the behaviours.
I don't see that this needs to be documented in user documentation, only
in the commit message.
In 0.9.10, there was no checking done at all. If libvirt output cur >
max (possible only up to the fuzz of a megabyte boundary), then this
still made no difference: the guest was running with max at the megabyte
boundary, so the current fit within the actual max of the guest. If you
hand-crafted xml with cur > max and outside the fuzz, you couldn't run
anyways, because qemu would complain that cur was out of range.
With these two patches, libvirt will now adjust max to the actual
megabyte boundary, at which point cur is no longer > max. And when
importing an older xml where cur > max, this silently clamps cur down to
max, but then libvirt rounds it back up again when passing it to qemu,
so you still end up with the same max at the megabyte boundary and cur
rounded up to match max. Meanwhile, if the user hand-writes xml with
too large of cur, we error out up front rather than letting qemu
complain, but it doesn't change the fact that it gets rejected at some
point in the chain.
That is, I don't see how this fuzz is user visible, so I don't think it
needs user visible documentation, only more details in the commit message.
v2 with improved commit message coming up.
--
Eric Blake ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 2/2] conf: allow fuzz in XML with cur balloon > max
On 03/30/2012 08:36 PM, Zhou Peng wrote:
> Thanks for your patch serials.
> I think they fix the true bug.
>
> But I have a little doubt on the fuzz allowance, pls have a see
> comment in line below.
>
>> if (def->mem.cur_balloon > def->mem.max_balloon) {
>> -virDomainReportError(VIR_ERR_XML_ERROR,
>> - _("current memory '%lluk' exceeds maximum
>> '%lluk'"),
>> - def->mem.cur_balloon, def->mem.max_balloon);
>> -goto error;
>> +/* Older libvirt could get into this situation due to
> Do you mean to allow the scene:
> * The checkpoint( thought invalid by old libvirt) produced by older libvirt
> can be thought valid if restored by the patched newer libvirt?
Yes. It is indeed possible to have an older libvirt produce XML where
the maximum memory was less than a megabyte boundary, and the current
memory is anywhere between that maximum and the actual megabyte boundary.
> But it will also allow a typo xml.
Yes, but in practice, it won't make a difference, since it will only
allow a typo if the difference falls in the rounding error up to the
megabyte boundary.
> * Another scene may be live migration between old libvirt
> and newer libvirt (pls correct me if err)
Yes, that is another situation where old libvirt xml can be output where
cur is greater than max, but only by at most the difference due to
rounding up to the megabyte boundary.
>
> If so, I think these scene should be documented or commented here or
> in the commit msg explicitly, otherwise this piece of code may confuse many
> readers, because I don't think cur_balloon can exceed max_balloon
> again after patched
> (so if exceed must be typo xml).
I thought I did document and comment that, but I can add more words
since both you and Osier seemed to be confused. v2 coming up.
--
Eric Blake ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 2/2] conf: allow fuzz in XML with cur balloon > max
On 03/30/2012 11:56 PM, Eric Blake wrote:
Commit 1b1402b introduced a regression. Since older libvirt versions
would silently round memory up (until the previous patch), but populated
current memory based on querying the guest, it was possible to have
current> maximum by the amount of the rounding. Accept this fuzz
factor, and silently clamp current down to maximum in that case,
rather than failing to reparse XML for an existing VM.
* src/conf/domain_conf.c (virDomainDefParseXML): Don't reject
existing XML.
Based on a report by Zhou Peng.
---
src/conf/domain_conf.c | 19 +++
src/qemu/qemu_monitor_json.c |2 +-
2 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 4caef6f..7c95920 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7776,10 +7776,21 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr
caps,
goto error;
if (def->mem.cur_balloon> def->mem.max_balloon) {
-virDomainReportError(VIR_ERR_XML_ERROR,
- _("current memory '%lluk' exceeds maximum
'%lluk'"),
- def->mem.cur_balloon, def->mem.max_balloon);
-goto error;
+/* Older libvirt could get into this situation due to
+ * rounding; if the discrepancy is less than 1MiB, we silently
+ * round down, otherwise we flag the issue. */
+if (VIR_DIV_UP(def->mem.cur_balloon, 1024)>
+VIR_DIV_UP(def->mem.max_balloon, 1024)) {
+virDomainReportError(VIR_ERR_XML_ERROR,
+ _("current memory '%lluk' exceeds "
+ "maximum '%lluk'"),
+ def->mem.cur_balloon, def->mem.max_balloon);
+goto error;
+} else {
+VIR_DEBUG("Truncating current %lluk to maximum %lluk",
+ def->mem.cur_balloon, def->mem.max_balloon);
+def->mem.cur_balloon = def->mem.max_balloon;
+}
But this needs the documentation IMHO, it actually changes
the behaviours.
} else if (def->mem.cur_balloon == 0) {
def->mem.cur_balloon = def->mem.max_balloon;
}
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 7093e1d..6d66587 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2022,7 +2022,7 @@ int qemuMonitorJSONSetBalloon(qemuMonitorPtr mon,
{
int ret;
virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("balloon",
- "U:value", ((unsigned
long long)newmem)*1024,
+ "U:value", newmem*1024ULL,
NULL);
virJSONValuePtr reply = NULL;
if (!cmd)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 2/2] conf: allow fuzz in XML with cur balloon > max
Thanks for your patch serials.
I think they fix the true bug.
But I have a little doubt on the fuzz allowance, pls have a see
comment in line below.
On Fri, Mar 30, 2012 at 11:56 PM, Eric Blake wrote:
>
> Commit 1b1402b introduced a regression. Since older libvirt versions
> would silently round memory up (until the previous patch), but populated
> current memory based on querying the guest, it was possible to have
> current > maximum by the amount of the rounding. Accept this fuzz
> factor, and silently clamp current down to maximum in that case,
> rather than failing to reparse XML for an existing VM.
>
> * src/conf/domain_conf.c (virDomainDefParseXML): Don't reject
> existing XML.
> Based on a report by Zhou Peng.
> ---
> src/conf/domain_conf.c | 19 +++
> src/qemu/qemu_monitor_json.c | 2 +-
> 2 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 4caef6f..7c95920 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -7776,10 +7776,21 @@ static virDomainDefPtr
> virDomainDefParseXML(virCapsPtr caps,
> goto error;
>
> if (def->mem.cur_balloon > def->mem.max_balloon) {
> - virDomainReportError(VIR_ERR_XML_ERROR,
> - _("current memory '%lluk' exceeds maximum
> '%lluk'"),
> - def->mem.cur_balloon, def->mem.max_balloon);
> - goto error;
> + /* Older libvirt could get into this situation due to
Do you mean to allow the scene:
* The checkpoint( thought invalid by old libvirt) produced by older libvirt
can be thought valid if restored by the patched newer libvirt?
But it will also allow a typo xml.
* Another scene may be live migration between old libvirt
and newer libvirt (pls correct me if err)
If so, I think these scene should be documented or commented here or
in the commit msg explicitly, otherwise this piece of code may confuse many
readers, because I don't think cur_balloon can exceed max_balloon
again after patched
(so if exceed must be typo xml).
> + * rounding; if the discrepancy is less than 1MiB, we silently
> + * round down, otherwise we flag the issue. */
> + if (VIR_DIV_UP(def->mem.cur_balloon, 1024) >
> + VIR_DIV_UP(def->mem.max_balloon, 1024)) {
> + virDomainReportError(VIR_ERR_XML_ERROR,
> + _("current memory '%lluk' exceeds "
> + "maximum '%lluk'"),
> + def->mem.cur_balloon, def->mem.max_balloon);
> + goto error;
> + } else {
> + VIR_DEBUG("Truncating current %lluk to maximum %lluk",
> + def->mem.cur_balloon, def->mem.max_balloon);
> + def->mem.cur_balloon = def->mem.max_balloon;
> + }
> } else if (def->mem.cur_balloon == 0) {
> def->mem.cur_balloon = def->mem.max_balloon;
> }
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 7093e1d..6d66587 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -2022,7 +2022,7 @@ int qemuMonitorJSONSetBalloon(qemuMonitorPtr mon,
> {
> int ret;
> virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("balloon",
> - "U:value", ((unsigned
> long long)newmem)*1024,
> + "U:value",
> newmem*1024ULL,
> NULL);
> virJSONValuePtr reply = NULL;
> if (!cmd)
> --
> 1.7.1
>
--
Zhou Peng
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 2/2] conf: allow fuzz in XML with cur balloon > max
Commit 1b1402b introduced a regression. Since older libvirt versions
would silently round memory up (until the previous patch), but populated
current memory based on querying the guest, it was possible to have
current > maximum by the amount of the rounding. Accept this fuzz
factor, and silently clamp current down to maximum in that case,
rather than failing to reparse XML for an existing VM.
* src/conf/domain_conf.c (virDomainDefParseXML): Don't reject
existing XML.
Based on a report by Zhou Peng.
---
src/conf/domain_conf.c | 19 +++
src/qemu/qemu_monitor_json.c |2 +-
2 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 4caef6f..7c95920 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7776,10 +7776,21 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr
caps,
goto error;
if (def->mem.cur_balloon > def->mem.max_balloon) {
-virDomainReportError(VIR_ERR_XML_ERROR,
- _("current memory '%lluk' exceeds maximum
'%lluk'"),
- def->mem.cur_balloon, def->mem.max_balloon);
-goto error;
+/* Older libvirt could get into this situation due to
+ * rounding; if the discrepancy is less than 1MiB, we silently
+ * round down, otherwise we flag the issue. */
+if (VIR_DIV_UP(def->mem.cur_balloon, 1024) >
+VIR_DIV_UP(def->mem.max_balloon, 1024)) {
+virDomainReportError(VIR_ERR_XML_ERROR,
+ _("current memory '%lluk' exceeds "
+ "maximum '%lluk'"),
+ def->mem.cur_balloon, def->mem.max_balloon);
+goto error;
+} else {
+VIR_DEBUG("Truncating current %lluk to maximum %lluk",
+ def->mem.cur_balloon, def->mem.max_balloon);
+def->mem.cur_balloon = def->mem.max_balloon;
+}
} else if (def->mem.cur_balloon == 0) {
def->mem.cur_balloon = def->mem.max_balloon;
}
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 7093e1d..6d66587 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2022,7 +2022,7 @@ int qemuMonitorJSONSetBalloon(qemuMonitorPtr mon,
{
int ret;
virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("balloon",
- "U:value", ((unsigned
long long)newmem)*1024,
+ "U:value", newmem*1024ULL,
NULL);
virJSONValuePtr reply = NULL;
if (!cmd)
--
1.7.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
