[PATCH v2 0/3] virsh: Fix logic wrt to --current flag in cmdSetmem
v2 of: https://listman.redhat.com/archives/libvir-list/2021-May/msg00484.html diff to v1: - Work in Jano's and Peter's review suggestions in 1/3. - Patches 2/3 and 3/3 are new. I've done 2/3 before 3/3 because it's still worth merging even if 3/3 is disliked. Michal Prívozník (3): virsh: Fix logic wrt to --current flag in cmdSetmem virsh-domain: Fix @ret handling in cmdSetmem and cmdSetmaxmem virsh-domain: Drop support for old APIs in cmdSetmem and cmdSetmaxmem tools/virsh-domain.c | 48 +++++++++++++++++--------------------------- 1 file changed, 18 insertions(+), 30 deletions(-) -- 2.26.3
In my commit of v7.1.0-rc1~376 I've simplified the logic of handling @flags. My assumption back then was that calling virDomainSetMemory() is equivalent to virDomainSetMemoryFlags(flags = 0). But that is not the case, because it is equivalent to virDomainSetMemoryFlags(flags = VIR_DOMAIN_AFFECT_LIVE). Fix the condition that calls the old API. Fixes: b5e267e8c59a257652f88d034cb1e0ce1ed4b58a Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1961118 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 0825f82522..9316aae3fd 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9025,7 +9025,7 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd) } kibibytes = VIR_DIV_UP(bytes, 1024); - if (flags == 0) { + if (!current && !live && !config) { if (virDomainSetMemory(dom, kibibytes) != 0) ret = false; } else { -- 2.26.3
On Wed, May 19, 2021 at 12:16:55 +0200, Michal Privoznik wrote:
In my commit of v7.1.0-rc1~376 I've simplified the logic of handling @flags. My assumption back then was that calling virDomainSetMemory() is equivalent to virDomainSetMemoryFlags(flags = 0). But that is not the case, because it is equivalent to virDomainSetMemoryFlags(flags = VIR_DOMAIN_AFFECT_LIVE). Fix the condition that calls the old API.
Fixes: b5e267e8c59a257652f88d034cb1e0ce1ed4b58a Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1961118 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
These functions initialize @ret to true and only after something fails either they call cleanup code (which consists only from virshDomainFree()) and return false, or they set ret = false and carry on (when the failure occurred close to cleanup code). Switch them to the usual pattern in which ret is initialized to failure, goto cleanup is used and ret is set to true only after everything succeeded. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 9316aae3fd..5a5215ab4c 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8996,7 +8996,7 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd) unsigned long long bytes = 0; unsigned long long max; unsigned long kibibytes = 0; - bool ret = true; + bool ret = false; bool config = vshCommandOptBool(cmd, "config"); bool live = vshCommandOptBool(cmd, "live"); bool current = vshCommandOptBool(cmd, "current"); @@ -9019,20 +9019,20 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd) max = 1024ull * ULONG_MAX; else max = ULONG_MAX; - if (vshCommandOptScaledInt(ctl, cmd, "size", &bytes, 1024, max) < 0) { - virshDomainFree(dom); - return false; - } + if (vshCommandOptScaledInt(ctl, cmd, "size", &bytes, 1024, max) < 0) + goto cleanup; kibibytes = VIR_DIV_UP(bytes, 1024); if (!current && !live && !config) { if (virDomainSetMemory(dom, kibibytes) != 0) - ret = false; + goto cleanup; } else { if (virDomainSetMemoryFlags(dom, kibibytes, flags) < 0) - ret = false; + goto cleanup; } + ret = true; + cleanup: virshDomainFree(dom); return ret; } @@ -9074,7 +9074,7 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd) unsigned long long bytes = 0; unsigned long long max; unsigned long kibibytes = 0; - bool ret = true; + bool ret = false; bool config = vshCommandOptBool(cmd, "config"); bool live = vshCommandOptBool(cmd, "live"); bool current = vshCommandOptBool(cmd, "current"); @@ -9097,24 +9097,24 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd) max = 1024ull * ULONG_MAX; else max = ULONG_MAX; - if (vshCommandOptScaledInt(ctl, cmd, "size", &bytes, 1024, max) < 0) { - virshDomainFree(dom); - return false; - } + if (vshCommandOptScaledInt(ctl, cmd, "size", &bytes, 1024, max) < 0) + goto cleanup; kibibytes = VIR_DIV_UP(bytes, 1024); if (flags == 0) { if (virDomainSetMaxMemory(dom, kibibytes) != 0) { vshError(ctl, "%s", _("Unable to change MaxMemorySize")); - ret = false; + goto cleanup; } } else { if (virDomainSetMemoryFlags(dom, kibibytes, flags | VIR_DOMAIN_MEM_MAXIMUM) < 0) { vshError(ctl, "%s", _("Unable to change MaxMemorySize")); - ret = false; + goto cleanup; } } + ret = true; + cleanup: virshDomainFree(dom); return ret; } -- 2.26.3
On Wed, May 19, 2021 at 12:16:56 +0200, Michal Privoznik wrote:
These functions initialize @ret to true and only after something fails either they call cleanup code (which consists only from virshDomainFree()) and return false, or they set ret = false and carry on (when the failure occurred close to cleanup code).
Switch them to the usual pattern in which ret is initialized to failure, goto cleanup is used and ret is set to true only after everything succeeded.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Some of our really old APIs are missing @flags argument. We introduced their variants with "Flags" suffix and wired some logic into virsh to call the new variant only if necessary. This enables virsh to talk to older daemon which may be lacking new APIs. However, in case of cmdSetmem() we are talking about v0.1.1 (virDomainSetMemory()) vs. v0.9.0 (virDomainSetMemoryFlags()) and in case of cmdSetmaxmem() we are talking about v0.0.3 (virDomainSetMaxMemory()) vs v0.9.0 (virDomainSetMemoryFlags()). Libvirt v0.9.0 was released more than 10 years ago and recently we dropped support for RHEL-7 which has v4.5.0 (released ~3 years ago). Thus it is not really necessary to have support in virsh for such old daemons. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain.c | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 5a5215ab4c..81357d23aa 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9000,15 +9000,15 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd) bool config = vshCommandOptBool(cmd, "config"); bool live = vshCommandOptBool(cmd, "live"); bool current = vshCommandOptBool(cmd, "current"); - unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + unsigned int flags = VIR_DOMAIN_AFFECT_LIVE; VSH_EXCLUSIVE_OPTIONS_VAR(current, live); VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + if (current) + flags = VIR_DOMAIN_AFFECT_CURRENT; if (config) flags |= VIR_DOMAIN_AFFECT_CONFIG; - if (live) - flags |= VIR_DOMAIN_AFFECT_LIVE; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -9023,13 +9023,8 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd) goto cleanup; kibibytes = VIR_DIV_UP(bytes, 1024); - if (!current && !live && !config) { - if (virDomainSetMemory(dom, kibibytes) != 0) - goto cleanup; - } else { - if (virDomainSetMemoryFlags(dom, kibibytes, flags) < 0) - goto cleanup; - } + if (virDomainSetMemoryFlags(dom, kibibytes, flags) < 0) + goto cleanup; ret = true; cleanup: @@ -9101,16 +9096,9 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd) goto cleanup; kibibytes = VIR_DIV_UP(bytes, 1024); - if (flags == 0) { - if (virDomainSetMaxMemory(dom, kibibytes) != 0) { - vshError(ctl, "%s", _("Unable to change MaxMemorySize")); - goto cleanup; - } - } else { - if (virDomainSetMemoryFlags(dom, kibibytes, flags | VIR_DOMAIN_MEM_MAXIMUM) < 0) { - vshError(ctl, "%s", _("Unable to change MaxMemorySize")); - goto cleanup; - } + if (virDomainSetMemoryFlags(dom, kibibytes, flags | VIR_DOMAIN_MEM_MAXIMUM) < 0) { + vshError(ctl, "%s", _("Unable to change MaxMemorySize")); + goto cleanup; } ret = true; -- 2.26.3
On Wed, May 19, 2021 at 12:16:57 +0200, Michal Privoznik wrote:
Some of our really old APIs are missing @flags argument. We introduced their variants with "Flags" suffix and wired some logic into virsh to call the new variant only if necessary. This enables virsh to talk to older daemon which may be lacking new APIs.
However, in case of cmdSetmem() we are talking about v0.1.1 (virDomainSetMemory()) vs. v0.9.0 (virDomainSetMemoryFlags()) and in case of cmdSetmaxmem() we are talking about v0.0.3 (virDomainSetMaxMemory()) vs v0.9.0 (virDomainSetMemoryFlags()).
Libvirt v0.9.0 was released more than 10 years ago and recently we dropped support for RHEL-7 which has v4.5.0 (released ~3 years ago). Thus it is not really necessary to have support in virsh for such old daemons.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain.c | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com> While I agree with the premise, please hold off pushing this patch to let others chime in.
On 5/19/21 3:03 PM, Peter Krempa wrote:
On Wed, May 19, 2021 at 12:16:57 +0200, Michal Privoznik wrote:
Some of our really old APIs are missing @flags argument. We introduced their variants with "Flags" suffix and wired some logic into virsh to call the new variant only if necessary. This enables virsh to talk to older daemon which may be lacking new APIs.
However, in case of cmdSetmem() we are talking about v0.1.1 (virDomainSetMemory()) vs. v0.9.0 (virDomainSetMemoryFlags()) and in case of cmdSetmaxmem() we are talking about v0.0.3 (virDomainSetMaxMemory()) vs v0.9.0 (virDomainSetMemoryFlags()).
Libvirt v0.9.0 was released more than 10 years ago and recently we dropped support for RHEL-7 which has v4.5.0 (released ~3 years ago). Thus it is not really necessary to have support in virsh for such old daemons.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain.c | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
While I agree with the premise, please hold off pushing this patch to let others chime in.
Since nobody objected, I've pushed it. Michal
participants (3)
-
Michal Privoznik -
Michal Prívozník
-
Peter Krempa