2:30 p.m.
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(a)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..bd9325a1d8 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 (flags == VIR_DOMAIN_AFFECT_LIVE) {
if (virDomainSetMemory(dom, kibibytes) != 0)
ret = false;
} else {
--
2.26.3
2:42 p.m.
On Tue, May 18, 2021 at 02:30:06PM +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(a)redhat.com>
---
tools/virsh-domain.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
I would add a comment in the code as well to help other developers
understand the logic without looking up the commit message.
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
3:02 p.m.
On a Tuesday in 2021, 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
before that commit, if the user did not specify any of:
config, live, current
we used the old API.
After this change, the new API will be used for those cases.
Jano
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1961118
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tools/virsh-domain.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
3:05 p.m.
On 5/18/21 3:02 PM, Ján Tomko wrote:
On a Tuesday in 2021, 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
before that commit, if the user did not specify any of:
config, live, current
we used the old API.
After this change, the new API will be used for those cases.
Will it? I think that with just --live @flags will be
VIR_DOMAIN_AFFECT_CURRENT | VIR_DOMAIN_AFFECT_LIVE, which is effectively
just VIR_DOMAIN_AFFECT_LIVE which means that the old API is used.
Am I missing something?
Michal
3:12 p.m.
On Tue, May 18, 2021 at 03:02:55PM +0200, Ján Tomko wrote:
On a Tuesday in 2021, 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
before that commit, if the user did not specify any of:
config, live, current
we used the old API.
After this change, the new API will be used for those cases.
The question is if we support using upstream virsh with old libvirt
where only non-flags API is available. If not we should drop the
non-flags API usage from virsh completely.
Pavel
3:17 p.m.
On 5/18/21 3:12 PM, Pavel Hrdina wrote:
On Tue, May 18, 2021 at 03:02:55PM +0200, Ján Tomko wrote:
> On a Tuesday in 2021, 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
>
> before that commit, if the user did not specify any of:
> config, live, current
> we used the old API.
>
> After this change, the new API will be used for those cases.
The question is if we support using upstream virsh with old libvirt
where only non-flags API is available. If not we should drop the
non-flags API usage from virsh completely.
I think in general we do. In this specific case,
virDomainSetMemoryFlags() API was introduced in v0.9.0 release (which is
10 years ago). And since we dropped RHEL-7 support recently and the
oldest QEMU we support is from late 2017 our attempt to be that
backwards compatible is questionable IMO.
But anyway - with this proposed patch the older API is called in more
cases than it was before I touched the code => compatibility++.
Michal
3:55 p.m.
On Tue, May 18, 2021 at 03:17:56PM +0200, Michal Prívozník wrote:
On 5/18/21 3:12 PM, Pavel Hrdina wrote:
> On Tue, May 18, 2021 at 03:02:55PM +0200, Ján Tomko wrote:
>> On a Tuesday in 2021, 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
>>
>> before that commit, if the user did not specify any of:
>> config, live, current
>> we used the old API.
>>
>> After this change, the new API will be used for those cases.
>
> The question is if we support using upstream virsh with old libvirt
> where only non-flags API is available. If not we should drop the
> non-flags API usage from virsh completely.
I think in general we do. In this specific case,
virDomainSetMemoryFlags() API was introduced in v0.9.0 release (which is
10 years ago). And since we dropped RHEL-7 support recently and the
oldest QEMU we support is from late 2017 our attempt to be that
backwards compatible is questionable IMO.
Exactly my question. We have some support matrix where libvirt should be
able to compile and run but there is no explicit support policy for
using virsh with old libvirt or for obsolete APIs that should not be
used. I would vote to use the same matrix as we already have.
That would allow us to remove probably all non-flags APIs from virsh
code.
Pavel
But anyway - with this proposed patch the older API is called in
more
cases than it was before I touched the code => compatibility++.
Michal
10:07 a.m.
On Tue, May 18, 2021 at 15:55:45 +0200, Pavel Hrdina wrote:
On Tue, May 18, 2021 at 03:17:56PM +0200, Michal Prívozník wrote:
> On 5/18/21 3:12 PM, Pavel Hrdina wrote:
> > On Tue, May 18, 2021 at 03:02:55PM +0200, Ján Tomko wrote:
> >> On a Tuesday in 2021, 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
> >>
> >> before that commit, if the user did not specify any of:
> >> config, live, current
> >> we used the old API.
> >>
> >> After this change, the new API will be used for those cases.
> >
> > The question is if we support using upstream virsh with old libvirt
> > where only non-flags API is available. If not we should drop the
> > non-flags API usage from virsh completely.
>
> I think in general we do. In this specific case,
> virDomainSetMemoryFlags() API was introduced in v0.9.0 release (which is
> 10 years ago). And since we dropped RHEL-7 support recently and the
> oldest QEMU we support is from late 2017 our attempt to be that
> backwards compatible is questionable IMO.
Exactly my question. We have some support matrix where libvirt should be
able to compile and run but there is no explicit support policy for
using virsh with old libvirt or for obsolete APIs that should not be
used. I would vote to use the same matrix as we already have.
That would allow us to remove probably all non-flags APIs from virsh
code.
This fix, well a variation of it removing the old API would be the only
acceptable variant here.
The original code in virsh prior to the change would call the old API
also when no flag was used.
This patch is not really fixing the old behaviour though, because with
no flag you get the behavior of '--current' rather than the pre-existing
behavior of implying '--live'.
Thus we either declare that we do want to change the behaviour in virsh
and use it as an excuse to delete the use of the old api,
or
this patch needs to be fixed more to restore the logic,
or
just revert the original commit.
But this patch as-is is wrong and is not really fixing the behaviour
without adding more regression.
1221
days inactive
1222
days old
7 comments
5 participants
participants (5)
-
Ján Tomko -
Michal Privoznik -
Michal Prívozník
-
Pavel Hrdina -
Peter Krempa