On 03/09/2011 01:13 AM, KAMEZAWA Hiroyuki wrote:
> +++ b/docs/schemas/domain.rng
> @@ -341,7 +341,7 @@
> </optional>
> <!-- Maximum swap area the VM can use -->
> <optional>
> - <element name="swap_hard_limit">
> + <element name="memswap_hard_limit">
> <ref name="memoryKB"/>
> </element>
> </optional>
This needs fixing. We need to support both old and new styles, since
we've had a release that used the old name. That means we need to add a
new test, rather than modifying an existing test, to show that we can
parse both styles when reading XML, but that we generate the new style
when writing.
Maybe danpb or DV will have some further comments on whether it is wise
to deprecate XML like this; it's unpleasant business to think about
backwards compatibility. We might be stuck with just documenting that
the choice of name was unfortunate to avoid the hassle of back-compat;
but I hope it doesn't come to that.
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index 618b350..d2600fa 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -735,13 +735,21 @@ typedef enum {
> #define VIR_DOMAIN_MEMORY_MIN_GUARANTEE "min_guarantee"
>
> /**
> - * VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT:
> + * VIR_DOMAIN_MEMORY_MEMSWAP_HARD_LIMIT:
> *
> - * Macro for the swap tunable swap_hard_limit: it represents the maximum swap
> - * the guest can use.
> + * Macro for the swap tunable memswap_hard_limit: it represents the maximum
> + * memory+swap the guest can use.
> */
>
> -#define VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT "swap_hard_limit"
> +#define VIR_DOMAIN_MEMORY_MEMSWAP_HARD_LIMIT "memswap_hard_limit"
> +
> +/**
> + * VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT
> + *
> + * macro for comaptibility with old version. please use
> + * VIR_DOMAIN_MEMORY_MEMSWAP_HARD_LIMIT
> + */
> +#define VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT "memswap_hard_limit"
Good, you didn't delete the old name. However, you no longer have any
code that recognizes the old string value of the name. Suppose that
virsh from 0.8.8 is talking to libvirtd from 0.9.0 - that means libvirtd
has to recognize the old name as well as the new name.
(The astute reader might complain: "But the #define for
memswap_hard_limit was already renamed once before, in commit 916f95b."
However, when you realize that the option was introduced in commit
bf1b76f, and that both commit 916f95b and bf1b76f were first released as
part of 0.8.5, therefore there was no release made with the intermediate
name, so that particular rename did not have any back-compat issues like
this proposed rename).
>
> /**
> * virDomainMemoryParameter:
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 16e1291..fdf37e1 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5192,9 +5192,17 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
> &def->mem.min_guarantee) < 0)
> def->mem.min_guarantee = 0;
>
> - if (virXPathULong("string(./memtune/swap_hard_limit[1])", ctxt,
> - &def->mem.swap_hard_limit) < 0)
> - def->mem.swap_hard_limit = 0;
> + if (virXPathULong("string(./memtune/memswap_hard_limit[1])", ctxt,
> + &def->mem.memswap_hard_limit) < 0)
> + def->mem.memswap_hard_limit = 0;
> + /* internal support for old style */
> + if (def->mem.memswap_hard_limit == 0 &&
> + virXPathULong("string(./memtune/swap_hard_limit[1])", ctxt,
> + &def->mem.memswap_hard_limit) < 0) {
> + def->mem.memswap_hard_limit = 0;
> + VIR_WARN("%s", _("<swap_hard_limit> is obsolete, please use"
> + "<memswap_hard_limit>"));
> + }
Good - this still parses the old XML, even though the .rng file didn't
mention it.
> +++ b/src/lxc/lxc_driver.c
> @@ -770,16 +770,16 @@ static int lxcDomainSetMemoryParameters(virDomainPtr dom,
> _("unable to set memory soft_limit tunable"));
> ret = -1;
> }
> - } else if (STREQ(param->field, VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT)) {
> + } else if (STREQ(param->field, VIR_DOMAIN_MEMORY_MEMSWAP_HARD_LIMIT)) {
Bad - here's where you forgot to also check STREQ against the old
spelling of the limit name.
> +++ b/src/qemu/qemu_driver.c
> @@ -4522,19 +4522,19 @@ static int qemuDomainSetMemoryParameters(virDomainPtr dom,
> _("unable to set memory soft_limit tunable"));
> ret = -1;
> }
> - } else if (STREQ(param->field, VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT)) {
> + } else if (STREQ(param->field, VIR_DOMAIN_MEMORY_MEMSWAP_HARD_LIMIT)) {
Likewise.
> --- a/tests/qemuxml2argvdata/qemuxml2argv-memtune.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-memtune.xml
> @@ -6,7 +6,7 @@
> <memtune>
> <hard_limit>512000</hard_limit>
> <soft_limit>128000</soft_limit>
> - <swap_hard_limit>1024000</swap_hard_limit>
> + <memswap_hard_limit>1024000</memswap_hard_limit>
> </memtune>
> <vcpu>1</vcpu>
> <os>
Back to my earlier complaint that you need to test both old and new naming.
> diff --git a/tools/virsh.c b/tools/virsh.c
> index 14c6d6b..b790248 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -3034,8 +3034,8 @@ static const vshCmdOptDef opts_memtune[] = {
> N_("Max memory in kilobytes")},
> {"soft-limit", VSH_OT_INT, VSH_OFLAG_NONE,
> N_("Memory during contention in kilobytes")},
> - {"swap-hard-limit", VSH_OT_INT, VSH_OFLAG_NONE,
> - N_("Max swap in kilobytes")},
> + {"memswap-hard-limit", VSH_OT_INT, VSH_OFLAG_NONE,
> + N_("Max memory+swap in kilobytes")},
This name change might break existing clients of virsh. Is there any
way to support the older name? Maybe we need to add some more framework
into virsh option parsing to allow option aliases that the parser
recognizes but which don't get documented, so that someone still doing
--swap-hard-limit will work?
I guess I need more convincing that this rename is worth the hassle, or
whether we are stuck with the smaller but less controversial patch of
documenting that the name isn't optimal for what it really represents.
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list