[PATCH Libvirt 04/11] virsh: Introduce limit-dirty-page-rate api

[Peter Krempa]

On Mon, Aug 08, 2022 at 23:41:27 +0800, ~hyman wrote:
> From: Hyman Huang(黄勇) <yong.huang at smartx.com>
> 
> Introduce limit-dirty-page-rate virsh api to set dirty page
> rate upper limit for virtual CPUs:
> 
> The following is the usage:
> $ virsh limit-dirty-page-rate <domain> <rate> [--vcpu <number>]
> 
> Set the specified index of vcpu if 'vcpu' option is specified,
> set all virtual CPUs of a VM otherwise.
> 
> Signed-off-by: Hyman Huang(黄勇) <yong.huang at smartx.com>
> ---
>  tools/virsh-domain.c | 92 ++++++++++++++++++++++++++++++++++++++++++++

missing addition to docs/manpages/virsh.rst

>  1 file changed, 92 insertions(+)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index fb54562eb6..ba2309eb3c 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -13817,6 +13817,92 @@ cmdDomDirtyRateCalc(vshControl *ctl, const vshCmd *cmd)
>      return true;
>  }
>  
> +#define IGNORED_CPU_INDEX   -1

Too generic name. Also no need for the definition most likely.

> +
> +/*
> + * "limit-dirty-page-rate" command
> + */
> +static const vshCmdInfo info_limit_dirty_page_rate[] = {
> +    {.name = "help",
> +     .data = N_("Set dirty page rate upper limit")
> +    },
> +    {.name = "desc",
> +     .data = N_("Set dirty page rate upper limit, "
> +                "require dirty-ring size configured")

This limitation is not documented anywhere else.

> +    },
> +    {.name = NULL}
> +};
> +
> +static const vshCmdOptDef opts_limit_dirty_page_rate[] = {
> +    VIRSH_COMMON_OPT_DOMAIN_FULL(0),
> +    {.name = "rate",
> +     .type = VSH_OT_INT,
> +     .flags = VSH_OFLAG_REQ,
> +     .help = N_("Upper limit of dirty page rate (MB/s) for "
> +                "virtual CPUs")
> +    },
> +    {.name = "vcpu",
> +     .type = VSH_OT_INT,
> +     .help = N_("Index of a virtual CPU")
> +    },
> +    {.name = NULL}
> +};
> +
> +static bool
> +cmdLimitDirtyPageRate(vshControl *ctl, const vshCmd *cmd)
> +{
> +    g_autoptr(virshDomain) dom = NULL;
> +    int vcpu_idx = -1;
> +    unsigned long long rate = 0;
> +    bool vcpu = vshCommandOptBool(cmd, "vcpu");
> +    unsigned int flags = 0;
> +
> +    if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> +        return false;
> +
> +    if (vcpu) {
> +        if (vshCommandOptInt(ctl, cmd, "vcpu", &vcpu_idx) < 0)
> +            return false;
> +
> +        if (vcpu_idx < 0) {
> +            vshError(ctl, "%s", _("Invalid vcpu index, using --vcpu "
> +                                  "to specify cpu index"));
> +            return false;
> +        }
> +    }
> +
> +    if (vshCommandOptULongLong(ctl, cmd, "rate", &rate) < 0)
> +        return false;
> +
> +    if (!rate) {

Checks for 0 with integers that can have arbitrary numeric value should
be explicit "if (rate == 0)"...

> +        vshError(ctl, "%s", _("Invalid dirty page rate limit"));
> +        return false;
> +    }
> +
> +    if (vcpu) {
> +        /* set specified vcpu dirty page rate limit of vm */
> +        if (virDomainSetVcpuDirtyLimit(dom, vcpu_idx,
> +                rate, flags | VIR_DOMAIN_DIRTYLIMIT_VCPU) < 0)
> +            return false;
> +        g_autofree char *info =
> +            g_strdup_printf("Set vcpu[%d] dirty page rate upper "
> +                            "limit %lld(MB/s) successfully",
> +                            vcpu_idx, rate);

This is not translatable. Make sure to always construct the message
within vshPRintExtra's second argument which is wrapped with the gettext
macro.

> +        vshPrintExtra(ctl, _("%1$s\n"), info);
> +    } else {
> +        /* set all vcpu dirty page rate limit of vm */
> +        if (virDomainSetVcpuDirtyLimit(dom, IGNORED_CPU_INDEX,
> +                rate, flags | VIR_DOMAIN_DIRTYLIMIT_ALL) < 0)
> +            return false;
> +        g_autofree char *info =
> +            g_strdup_printf("Set dirty page rate limit %lld(MB/s) "
> +                            "on all virtual CPUs successfully",
> +                            rate);

This is not translatable.

> +        vshPrintExtra(ctl, _("%1$s\n"), info);
> +    }
> +
> +    return true;
> +}
>  
>  const vshCmdDef domManagementCmds[] = {
>      {.name = "attach-device",
> @@ -14481,5 +14567,11 @@ const vshCmdDef domManagementCmds[] = {
>       .info = info_dom_fd_associate,
>       .flags = 0
>      },
> +    {.name = "limit-dirty-page-rate",
> +     .handler = cmdLimitDirtyPageRate,
> +     .opts = opts_limit_dirty_page_rate,
> +     .info = info_limit_dirty_page_rate,
> +     .flags = 0
> +    },
>      {.name = NULL}
>  };
> -- 
> 2.38.5
>