1. Patchew
  2. Libvirt
  3. View series

[PATCH] virsh: Simplify @flags handing in cmdSetmem() and cmdSetmaxmem()

Michal Privoznik posted 1 patch 3 years, 2 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/60d32c546036eddf58481c3161a49aa7135188ff.1611318505.git.mprivozn@redhat.com
tools/virsh-domain.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
[PATCH] virsh: Simplify @flags handing in cmdSetmem() and cmdSetmaxmem()
Posted by Michal Privoznik 3 years, 2 months ago 
What code tries to achieve is that if no flags were provided to
either 'setmem' or 'setmaxmem' commands then the old (no flags)
API is called to be able to communicate with older daemons.
Well, the code can be simplified a bit.

Note that with this change the old no flag version of APIs is
used more often. Previously if --current argument was given it
resulted in *Flags() version to be called even though it is not
necessary - VIR_DOMAIN_AFFECT_CURRENT is implied.

Therefore, this change in fact allows virsh to talk with broader
set of daemons.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 tools/virsh-domain.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 2bb136333f..9746117bdb 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -9018,9 +9018,6 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd)
         flags |= VIR_DOMAIN_AFFECT_CONFIG;
     if (live)
         flags |= VIR_DOMAIN_AFFECT_LIVE;
-    /* none of the options were specified */
-    if (!current && !live && !config)
-        flags = -1;
 
     if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
         return false;
@@ -9037,7 +9034,7 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd)
     }
     kibibytes = VIR_DIV_UP(bytes, 1024);
 
-    if (flags == -1) {
+    if (flags == 0) {
         if (virDomainSetMemory(dom, kibibytes) != 0)
             ret = false;
     } else {
@@ -9090,7 +9087,7 @@ cmdSetmaxmem(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 | VIR_DOMAIN_MEM_MAXIMUM;
+    unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
 
     VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
     VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
@@ -9099,9 +9096,6 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd)
         flags |= VIR_DOMAIN_AFFECT_CONFIG;
     if (live)
         flags |= VIR_DOMAIN_AFFECT_LIVE;
-    /* none of the options were specified */
-    if (!current && !live && !config)
-        flags = -1;
 
     if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
         return false;
@@ -9118,13 +9112,13 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd)
     }
     kibibytes = VIR_DIV_UP(bytes, 1024);
 
-    if (flags == -1) {
+    if (flags == 0) {
         if (virDomainSetMaxMemory(dom, kibibytes) != 0) {
             vshError(ctl, "%s", _("Unable to change MaxMemorySize"));
             ret = false;
         }
     } else {
-        if (virDomainSetMemoryFlags(dom, kibibytes, flags) < 0) {
+        if (virDomainSetMemoryFlags(dom, kibibytes, flags | VIR_DOMAIN_MEM_MAXIMUM) < 0) {
             vshError(ctl, "%s", _("Unable to change MaxMemorySize"));
             ret = false;
         }
-- 
2.26.2
Re: [PATCH] virsh: Simplify @flags handing in cmdSetmem() and cmdSetmaxmem()
Posted by Daniel Henrique Barboza 3 years, 2 months ago 
Michal,


I see that this patch was sent standalone in the ML, and the cover-letter
of the series doesn't mention it. Is this an ooopsie? Should I review this
patch together with the rest of the series?


Thanks,


DHB


On 1/22/21 9:50 AM, Michal Privoznik wrote:
> What code tries to achieve is that if no flags were provided to
> either 'setmem' or 'setmaxmem' commands then the old (no flags)
> API is called to be able to communicate with older daemons.
> Well, the code can be simplified a bit.
> 
> Note that with this change the old no flag version of APIs is
> used more often. Previously if --current argument was given it
> resulted in *Flags() version to be called even though it is not
> necessary - VIR_DOMAIN_AFFECT_CURRENT is implied.
> 
> Therefore, this change in fact allows virsh to talk with broader
> set of daemons.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>   tools/virsh-domain.c | 14 ++++----------
>   1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 2bb136333f..9746117bdb 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -9018,9 +9018,6 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd)
>           flags |= VIR_DOMAIN_AFFECT_CONFIG;
>       if (live)
>           flags |= VIR_DOMAIN_AFFECT_LIVE;
> -    /* none of the options were specified */
> -    if (!current && !live && !config)
> -        flags = -1;
>   
>       if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
>           return false;
> @@ -9037,7 +9034,7 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd)
>       }
>       kibibytes = VIR_DIV_UP(bytes, 1024);
>   
> -    if (flags == -1) {
> +    if (flags == 0) {
>           if (virDomainSetMemory(dom, kibibytes) != 0)
>               ret = false;
>       } else {
> @@ -9090,7 +9087,7 @@ cmdSetmaxmem(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 | VIR_DOMAIN_MEM_MAXIMUM;
> +    unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
>   
>       VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
>       VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
> @@ -9099,9 +9096,6 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd)
>           flags |= VIR_DOMAIN_AFFECT_CONFIG;
>       if (live)
>           flags |= VIR_DOMAIN_AFFECT_LIVE;
> -    /* none of the options were specified */
> -    if (!current && !live && !config)
> -        flags = -1;
>   
>       if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
>           return false;
> @@ -9118,13 +9112,13 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd)
>       }
>       kibibytes = VIR_DIV_UP(bytes, 1024);
>   
> -    if (flags == -1) {
> +    if (flags == 0) {
>           if (virDomainSetMaxMemory(dom, kibibytes) != 0) {
>               vshError(ctl, "%s", _("Unable to change MaxMemorySize"));
>               ret = false;
>           }
>       } else {
> -        if (virDomainSetMemoryFlags(dom, kibibytes, flags) < 0) {
> +        if (virDomainSetMemoryFlags(dom, kibibytes, flags | VIR_DOMAIN_MEM_MAXIMUM) < 0) {
>               vshError(ctl, "%s", _("Unable to change MaxMemorySize"));
>               ret = false;
>           }
>
Re: [PATCH] virsh: Simplify @flags handing in cmdSetmem() and cmdSetmaxmem()
Posted by Michal Privoznik 3 years, 2 months ago 
On 1/22/21 6:40 PM, Daniel Henrique Barboza wrote:
> Michal,
> 
> 
> I see that this patch was sent standalone in the ML, and the cover-letter
> of the series doesn't mention it. Is this an ooopsie? Should I review this
> patch together with the rest of the series?

Yeah, that's what I get for not removing old .patch files when 
formatting new series. This patch does not really belong in this series, 
so review the stand alone patch please.

Michal
Re: [PATCH] virsh: Simplify @flags handing in cmdSetmem() and cmdSetmaxmem()
Posted by Daniel Henrique Barboza 3 years, 2 months ago 

On 1/22/21 9:28 AM, Michal Privoznik wrote:
> What code tries to achieve is that if no flags were provided to
> either 'setmem' or 'setmaxmem' commands then the old (no flags)
> API is called to be able to communicate with older daemons.
> Well, the code can be simplified a bit.
> 
> Note that with this change the old no flag version of APIs is
> used more often. Previously if --current argument was given it
> resulted in *Flags() version to be called even though it is not
> necessary - VIR_DOMAIN_AFFECT_CURRENT is implied.
> 
> Therefore, this change in fact allows virsh to talk with broader
> set of daemons.

I suggest adding in the end of the commit message something in the lines
of "No user visible changes were made", making it clear that you didn't
change the behavior of setmem and setmaxmem flags.



Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>


> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>   tools/virsh-domain.c | 14 ++++----------
>   1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 2bb136333f..9746117bdb 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -9018,9 +9018,6 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd)
>           flags |= VIR_DOMAIN_AFFECT_CONFIG;
>       if (live)
>           flags |= VIR_DOMAIN_AFFECT_LIVE;
> -    /* none of the options were specified */
> -    if (!current && !live && !config)
> -        flags = -1;
>   
>       if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
>           return false;
> @@ -9037,7 +9034,7 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd)
>       }
>       kibibytes = VIR_DIV_UP(bytes, 1024);
>   
> -    if (flags == -1) {
> +    if (flags == 0) {
>           if (virDomainSetMemory(dom, kibibytes) != 0)
>               ret = false;
>       } else {
> @@ -9090,7 +9087,7 @@ cmdSetmaxmem(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 | VIR_DOMAIN_MEM_MAXIMUM;
> +    unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
>   
>       VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
>       VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
> @@ -9099,9 +9096,6 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd)
>           flags |= VIR_DOMAIN_AFFECT_CONFIG;
>       if (live)
>           flags |= VIR_DOMAIN_AFFECT_LIVE;
> -    /* none of the options were specified */
> -    if (!current && !live && !config)
> -        flags = -1;
>   
>       if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
>           return false;
> @@ -9118,13 +9112,13 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd)
>       }
>       kibibytes = VIR_DIV_UP(bytes, 1024);
>   
> -    if (flags == -1) {
> +    if (flags == 0) {
>           if (virDomainSetMaxMemory(dom, kibibytes) != 0) {
>               vshError(ctl, "%s", _("Unable to change MaxMemorySize"));
>               ret = false;
>           }
>       } else {
> -        if (virDomainSetMemoryFlags(dom, kibibytes, flags) < 0) {
> +        if (virDomainSetMemoryFlags(dom, kibibytes, flags | VIR_DOMAIN_MEM_MAXIMUM) < 0) {
>               vshError(ctl, "%s", _("Unable to change MaxMemorySize"));
>               ret = false;
>           }
>
Re: [PATCH] virsh: Simplify @flags handing in cmdSetmem() and cmdSetmaxmem()
Posted by Michal Privoznik 3 years, 2 months ago 
On 2/1/21 12:29 PM, Daniel Henrique Barboza wrote:
> 
> 
> On 1/22/21 9:28 AM, Michal Privoznik wrote:
>> What code tries to achieve is that if no flags were provided to
>> either 'setmem' or 'setmaxmem' commands then the old (no flags)
>> API is called to be able to communicate with older daemons.
>> Well, the code can be simplified a bit.
>>
>> Note that with this change the old no flag version of APIs is
>> used more often. Previously if --current argument was given it
>> resulted in *Flags() version to be called even though it is not
>> necessary - VIR_DOMAIN_AFFECT_CURRENT is implied.
>>
>> Therefore, this change in fact allows virsh to talk with broader
>> set of daemons.
> 
> I suggest adding in the end of the commit message something in the lines
> of "No user visible changes were made", making it clear that you didn't
> change the behavior of setmem and setmaxmem flags.
> 
> 
> 
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Pushed, thanks.

Michal

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.