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

[Daniel Henrique Barboza <danielhb413@xxxxxxxxx>]

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@xxxxxxxxxx>
> ---
>   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;
>           }