On Wed, Jun 12, 2024 at 03:02:23 -0700, wucf@xxxxxxxxxxxxx wrote:
> From: Chun Feng Wu <wucf@xxxxxxxxxxxxx>
>
> * Add new cmds: throttlegroupset, throttlegrouplist, throttlegroupinfo, throttlegroupdel
>
> Signed-off-by: Chun Feng Wu <wucf@xxxxxxxxxxxxx>
> ---
> tools/virsh-domain.c | 428 +++++++++++++++++++++++++++++++++++++++++++
This patch is missing addition to the manpage documenting the commands
in docs/manpages/virsh.rst.
> 1 file changed, 428 insertions(+)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 50e80689a2..f84a65451a 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -1509,6 +1509,410 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd)
> goto cleanup;
> }
>
> +
> +/*
> + * "throttlegrouplist" command
> + */
> +static const vshCmdInfo info_throttlegrouplist = {
> + .help = N_("list all domain throttlegroups."),
> + .desc = N_("Get the summary of throttle groups for a domain."),
> +};
> +
> +
> +static const vshCmdOptDef opts_throttlegrouplist[] = {
> + VIRSH_COMMON_OPT_DOMAIN_FULL(0),
> + {.name = "inactive",
> + .type = VSH_OT_BOOL,
> + .help = N_("get inactive rather than running configuration")
> + },
> + {.name = NULL}
> +};
> +
> +
> +static bool
> +cmdThrottleGroupList(vshControl *ctl,
> + const vshCmd *cmd)
> +{
> + unsigned int flags = 0;
> + size_t i;
> + g_autoptr(xmlDoc) xml = NULL;
> + g_autoptr(xmlXPathContext) ctxt = NULL;
> + g_autofree xmlNodePtr *groups = NULL;
> + ssize_t ngroups;
> + g_autoptr(vshTable) table = NULL;
> +
> + if (vshCommandOptBool(cmd, "inactive"))
> + flags |= VIR_DOMAIN_XML_INACTIVE;
> +
> + if (virshDomainGetXML(ctl, cmd, flags, &xml, &ctxt) < 0)
> + return false;
> +
> + ngroups = virXPathNodeSet("./throttlegroups/throttlegroup", ctxt, &groups);
> + if (ngroups < 0)
You need to report an error here, but on the other hand I don't think
that having 0 groups is an error.
> + return false;
> +
> + table = vshTableNew(_("Name"), NULL);
> +
> + if (!table)
> + return false;
> +
> + for (i = 0; i < ngroups; i++) {
> + g_autofree char *name = NULL;
> + ctxt->node = groups[i];
> + name = virXPathString("string(./group_name)", ctxt);
Here you don't handle NULL
> + if (vshTableRowAppend(table, name, NULL) < 0)
... which would make this fail use NULLSTR_EMPTY
> + return false;
> + }
> +
> + vshTablePrintToStdout(table, ctl);
> +
> + return true;
> +}
> +
> +
> +/*
> + * "throttlegroupset" command
> + */
> +static const vshCmdInfo info_throttlegroupset = {
> + .help = N_("Add or update a throttling group."),
> + .desc = N_("Add or updte a throttling group."),
> +};
> +
> +
> +static const vshCmdOptDef opts_throttlegroupset[] = {
> + VIRSH_COMMON_OPT_DOMAIN_FULL(0),
> + {.name = "group-name",
> + .type = VSH_OT_STRING,
> + .positional = true,
> + .required = true,
> + .completer = virshCompleteEmpty,
It will be possible to auto-complete this argument so virshCompleteEmpty
is not appropriate. That annotation must be used exclusively for fields
which don't make sense tot be complted.
> + .help = N_("throttle group name")
> + },
> + {.name = "total-bytes-sec",
> + .type = VSH_OT_INT,
> + .help = N_("total throughput limit, as scaled integer (default bytes)")
> + },
> + {.name = "read-bytes-sec",
> + .type = VSH_OT_INT,
> + .help = N_("read throughput limit, as scaled integer (default bytes)")
> + },
> + {.name = "write-bytes-sec",
> + .type = VSH_OT_INT,
> + .help = N_("write throughput limit, as scaled integer (default bytes)")
> + },
> + {.name = "total-iops-sec",
> + .type = VSH_OT_INT,
> + .help = N_("total I/O operations limit per second")
> + },
> + {.name = "read-iops-sec",
> + .type = VSH_OT_INT,
> + .help = N_("read I/O operations limit per second")
> + },
> + {.name = "write-iops-sec",
> + .type = VSH_OT_INT,
> + .help = N_("write I/O operations limit per second")
> + },
> + {.name = "total-bytes-sec-max",
> + .type = VSH_OT_INT,
> + .help = N_("total max, as scaled integer (default bytes)")
> + },
> + {.name = "read-bytes-sec-max",
> + .type = VSH_OT_INT,
> + .help = N_("read max, as scaled integer (default bytes)")
> + },
> + {.name = "write-bytes-sec-max",
> + .type = VSH_OT_INT,
> + .help = N_("write max, as scaled integer (default bytes)")
> + },
> + {.name = "total-iops-sec-max",
> + .type = VSH_OT_INT,
> + .help = N_("total I/O operations max")
> + },
> + {.name = "read-iops-sec-max",
> + .type = VSH_OT_INT,
> + .help = N_("read I/O operations max")
> + },
> + {.name = "write-iops-sec-max",
> + .type = VSH_OT_INT,
> + .help = N_("write I/O operations max")
> + },
> + {.name = "size-iops-sec",
> + .type = VSH_OT_INT,
> + .help = N_("I/O size in bytes")
> + },
> + {.name = "total-bytes-sec-max-length",
> + .type = VSH_OT_INT,
> + .help = N_("duration in seconds to allow total max bytes")
> + },
> + {.name = "read-bytes-sec-max-length",
> + .type = VSH_OT_INT,
> + .help = N_("duration in seconds to allow read max bytes")
> + },
> + {.name = "write-bytes-sec-max-length",
> + .type = VSH_OT_INT,
> + .help = N_("duration in seconds to allow write max bytes")
> + },
> + {.name = "total-iops-sec-max-length",
> + .type = VSH_OT_INT,
> + .help = N_("duration in seconds to allow total I/O operations max")
> + },
> + {.name = "read-iops-sec-max-length",
> + .type = VSH_OT_INT,
> + .help = N_("duration in seconds to allow read I/O operations max")
> + },
> + {.name = "write-iops-sec-max-length",
> + .type = VSH_OT_INT,
> + .help = N_("duration in seconds to allow write I/O operations max")
> + },
I guess that once we have two of the commands using these it'd make
sense to have these as a macro to avoid having to update two places the
next time another tunable is added.
> + VIRSH_COMMON_OPT_DOMAIN_CONFIG,
> + VIRSH_COMMON_OPT_DOMAIN_LIVE,
> + VIRSH_COMMON_OPT_DOMAIN_CURRENT,
> + {.name = NULL}
> +};
> +
> +
> +static bool
> +cmdThrottleGroupSet(vshControl *ctl,
> + const vshCmd *cmd)
> +{
> + g_autoptr(virshDomain) dom = NULL;
> + const char *name;
> + const char *group_name = NULL;
> + unsigned long long value;
> + int nparams = 0;
> + int maxparams = 0;
> + virTypedParameterPtr params = NULL;
> + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
> + int rv = 0;
> + bool current = vshCommandOptBool(cmd, "current");
> + bool config = vshCommandOptBool(cmd, "config");
> + bool live = vshCommandOptBool(cmd, "live");
> + bool ret = false;
> +
> + VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
> + VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
> +
> + if (config)
> + flags |= VIR_DOMAIN_AFFECT_CONFIG;
> + if (live)
> + flags |= VIR_DOMAIN_AFFECT_LIVE;
> +
> + if (!(dom = virshCommandOptDomain(ctl, cmd, &name)))
> + goto cleanup;
This function doesn't use 'name' any more so there's no point in
fetching it.
> +
> +
> +#define VSH_SET_THROTTLE_GROUP_SCALED(PARAM, CONST) \
> + if ((rv = vshCommandOptScaledInt(ctl, cmd, #PARAM, &value, \
> + 1, ULLONG_MAX)) < 0) { \
> + goto interror; \
> + } else if (rv > 0) { \
> + if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams, \
> + VIR_DOMAIN_BLOCK_IOTUNE_##CONST, \
> + value) < 0) \
> + goto save_error; \
> + }
> +
> + VSH_SET_THROTTLE_GROUP_SCALED(total-bytes-sec, TOTAL_BYTES_SEC);
> + VSH_SET_THROTTLE_GROUP_SCALED(read-bytes-sec, READ_BYTES_SEC);
> + VSH_SET_THROTTLE_GROUP_SCALED(write-bytes-sec, WRITE_BYTES_SEC);
> + VSH_SET_THROTTLE_GROUP_SCALED(total-bytes-sec-max, TOTAL_BYTES_SEC_MAX);
> + VSH_SET_THROTTLE_GROUP_SCALED(read-bytes-sec-max, READ_BYTES_SEC_MAX);
> + VSH_SET_THROTTLE_GROUP_SCALED(write-bytes-sec-max, WRITE_BYTES_SEC_MAX);
> +#undef VSH_SET_THROTTLE_GROUP_SCALED
> +
> +#define VSH_SET_THROTTLE_GROUP(PARAM, CONST) \
> + if ((rv = vshCommandOptULongLong(ctl, cmd, #PARAM, &value)) < 0) { \
> + goto interror; \
> + } else if (rv > 0) { \
> + if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams, \
> + VIR_DOMAIN_BLOCK_IOTUNE_##CONST, \
> + value) < 0) \
> + goto save_error; \
> + }
> +
> + VSH_SET_THROTTLE_GROUP(total-iops-sec, TOTAL_IOPS_SEC);
> + VSH_SET_THROTTLE_GROUP(read-iops-sec, READ_IOPS_SEC);
> + VSH_SET_THROTTLE_GROUP(write-iops-sec, WRITE_IOPS_SEC);
> + VSH_SET_THROTTLE_GROUP(total-iops-sec-max, TOTAL_IOPS_SEC_MAX);
> + VSH_SET_THROTTLE_GROUP(read-iops-sec-max, READ_IOPS_SEC_MAX);
> + VSH_SET_THROTTLE_GROUP(write-iops-sec-max, WRITE_IOPS_SEC_MAX);
> + VSH_SET_THROTTLE_GROUP(size-iops-sec, SIZE_IOPS_SEC);
> +
> + VSH_SET_THROTTLE_GROUP(total-bytes-sec-max-length, TOTAL_BYTES_SEC_MAX_LENGTH);
> + VSH_SET_THROTTLE_GROUP(read-bytes-sec-max-length, READ_BYTES_SEC_MAX_LENGTH);
> + VSH_SET_THROTTLE_GROUP(write-bytes-sec-max-length, WRITE_BYTES_SEC_MAX_LENGTH);
> + VSH_SET_THROTTLE_GROUP(total-iops-sec-max-length, TOTAL_IOPS_SEC_MAX_LENGTH);
> + VSH_SET_THROTTLE_GROUP(read-iops-sec-max-length, READ_IOPS_SEC_MAX_LENGTH);
> + VSH_SET_THROTTLE_GROUP(write-iops-sec-max-length, WRITE_IOPS_SEC_MAX_LENGTH);
> +#undef VSH_SET_THROTTLE_GROUP
> +
> + if (vshCommandOptString(ctl, cmd, "group-name", &group_name) < 0) {
> + goto cleanup;
> + }
> +
> + if (group_name) {
gropu name is guaranteed to exist ...
> + if (virTypedParamsAddString(¶ms, &nparams, &maxparams,
> + VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME,
> + group_name) < 0)
> + goto save_error;
> + }
> +
> + if (virDomainSetThrottleGroup(dom, group_name, params, nparams, flags) < 0)
> + goto error;
otherwis this'd fail anyways.
> + vshPrintExtra(ctl, "%s", _("Throttle group set successfully\n"));
> +
> + ret = true;
> +
> + cleanup:
> + virTypedParamsFree(params, nparams);
> + return ret;
> +
> + save_error:
> + vshSaveLibvirtError();
> + error:
> + vshError(ctl, "%s", _("Unable to change throttle group"));
> + goto cleanup;
> +
> + interror:
> + vshError(ctl, "%s", _("Unable to parse integer parameter"));
> + goto cleanup;
> +}
> +
> +
> +/*
> + * "throttlegroupdel" command
> + */
> +static const vshCmdInfo info_throttlegroupdel = {
> + .help = N_("Delete a throttling group."),
> + .desc = N_("Delete a throttling group."),
> +};
> +
> +
> +static const vshCmdOptDef opts_throttlegroupdel[] = {
> + VIRSH_COMMON_OPT_DOMAIN_FULL(0),
> + {.name = "group-name",
> + .type = VSH_OT_STRING,
> + .positional = true,
> + .required = true,
> + .completer = virshCompleteEmpty,
Same as above regarding the annotation.
> + .help = N_("throttle group name")
> + },
> + VIRSH_COMMON_OPT_DOMAIN_CONFIG,
> + VIRSH_COMMON_OPT_DOMAIN_LIVE,
> + VIRSH_COMMON_OPT_DOMAIN_CURRENT,
> + {.name = NULL}
> +};
> +
> +
> +static bool
> +cmdThrottleGroupDel(vshControl *ctl,
> + const vshCmd *cmd)
> +{
> + g_autoptr(virshDomain) dom = NULL;
> + const char *group_name = NULL;
> + bool config = vshCommandOptBool(cmd, "config");
> + bool live = vshCommandOptBool(cmd, "live");
> + bool current = vshCommandOptBool(cmd, "current");
> + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
> +
> + VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
> + VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
> +
> + if (config)
> + flags |= VIR_DOMAIN_AFFECT_CONFIG;
> + if (live)
> + flags |= VIR_DOMAIN_AFFECT_LIVE;
> +
> + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> + return false;
> +
> + if (vshCommandOptString(ctl, cmd, "group-name", &group_name) < 0) {
> + return false;
> + }
> +
> + if (virDomainDelThrottleGroup(dom, group_name, flags) < 0)
> + return false;
> + vshPrintExtra(ctl, "%s", _("Throttle group deleted successfully\n"));
> +
> + return true;
> +}
> +
> +
> +/*
> + * "throttlegroupinfo" command
> + */
> +static const vshCmdInfo info_throttlegroupinfo = {
> + .help = N_("Get a throttling group."),
> + .desc = N_("Get a throttling group."),
> +};
> +
> +
> +static const vshCmdOptDef opts_throttlegroupinfo[] = {
> + VIRSH_COMMON_OPT_DOMAIN_FULL(0),
> + {.name = "group-name",
> + .type = VSH_OT_STRING,
> + .positional = true,
> + .required = true,
> + .completer = virshCompleteEmptya
Same as above.
,
> + .help = N_("throttle group name")
> + },
> + VIRSH_COMMON_OPT_DOMAIN_CONFIG,
> + VIRSH_COMMON_OPT_DOMAIN_LIVE,
> + VIRSH_COMMON_OPT_DOMAIN_CURRENT,
> + {.name = NULL}
> +};
> +
> +
> +static bool
> +cmdThrottleGroupInfo(vshControl *ctl,
> + const vshCmd *cmd)
> +{
> + g_autoptr(virshDomain) dom = NULL;
> + const char *name;
> + const char *group_name = NULL;
> + int nparams = 0;
> + virTypedParameterPtr params = NULL;
> + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
> + size_t i;
> + bool current = vshCommandOptBool(cmd, "current");
> + bool config = vshCommandOptBool(cmd, "config");
> + bool live = vshCommandOptBool(cmd, "live");
> + bool ret = false;
> +
> + VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
> + VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
> +
> + if (config)
> + flags |= VIR_DOMAIN_AFFECT_CONFIG;
> + if (live)
> + flags |= VIR_DOMAIN_AFFECT_LIVE;
> +
> + if (!(dom = virshCommandOptDomain(ctl, cmd, &name)))
> + goto cleanup;
Name is not used.
> +
> + if (vshCommandOptString(ctl, cmd, "group-name", &group_name) < 0) {
> + goto cleanup;
> + }
> +
> + if (virDomainGetThrottleGroup(dom, group_name, ¶ms, &nparams, flags) != 0) {
> + vshError(ctl, "%s",
> + _("Unable to get throttle group parameters"));
Misaligned.
> + goto cleanup;
> + }
> +
> + for (i = 0; i < nparams; i++) {
> + g_autofree char *str = vshGetTypedParamValue(ctl, ¶ms[i]);
> + vshPrint(ctl, "%-15s: %s\n", params[i].field, str);
> + }
> +
> + ret = true;
> +
> + cleanup:
> + virTypedParamsFree(params, nparams);
> + return ret;
> +}
> +
> +
> /*
> * "blkiotune" command
> */
> @@ -13378,6 +13782,30 @@ const vshCmdDef domManagementCmds[] = {
> .info = &info_blkdeviotune,
> .flags = 0
> },
> + {.name = "throttlegroupset",
> + .handler = cmdThrottleGroupSet,
> + .opts = opts_throttlegroupset,
> + .info = &info_throttlegroupset,
> + .flags = 0
> + },
> + {.name = "throttlegroupdel",
> + .handler = cmdThrottleGroupDel,
> + .opts = opts_throttlegroupdel,
> + .info = &info_throttlegroupdel,
> + .flags = 0
> + },
> + {.name = "throttlegroupinfo",
> + .handler = cmdThrottleGroupInfo,
> + .opts = opts_throttlegroupinfo,
> + .info = &info_throttlegroupinfo,
> + .flags = 0
> + },
> + {.name = "throttlegrouplist",
> + .handler = cmdThrottleGroupList,
> + .opts = opts_throttlegrouplist,
> + .info = &info_throttlegrouplist,
> + .flags = 0
> + },
I suggest using the 'dom' prefix as all of the objects are per-domain.
> {.name = "blkiotune",
> .handler = cmdBlkiotune,
> .opts = opts_blkiotune,
> --
> 2.34.1
>
