On Thu, Apr 11, 2024 at 19:01:58 -0700, wucf@xxxxxxxxxxxxx wrote:
> From: Chun Feng Wu <wucf@xxxxxxxxxxxxx>
>
> * Add new cmds: throttlegroupset, throttlegrouplist, throttlegroupinfo, throttlegroupdel
> * Update "attach_disk" to support new option: throttle-groups to
> form filter chain in QEMU for specific disk
>
> Signed-off-by: Chun Feng Wu <wucf@xxxxxxxxxxxxx>
> ---
Please separate the new throttle-group modification APIs from the
'attach-disk' change.
> tools/virsh-completer-domain.c | 64 +++++
> tools/virsh-completer-domain.h | 10 +
> tools/virsh-domain.c | 471 +++++++++++++++++++++++++++++++++
> 3 files changed, 545 insertions(+)
>
> diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c
> index 2891d1399c..e4e5775c5c 100644
> --- a/tools/virsh-completer-domain.c
> +++ b/tools/virsh-completer-domain.c
[...]
> char **
> virshDomainUndefineStorageDisksCompleter(vshControl *ctl,
> const vshCmd *cmd,
> diff --git a/tools/virsh-completer-domain.h b/tools/virsh-completer-domain.h
> index 27cf963912..4ac29c4876 100644
> --- a/tools/virsh-completer-domain.h
> +++ b/tools/virsh-completer-domain.h
> @@ -41,6 +41,16 @@ virshDomainDiskTargetCompleter(vshControl *ctl,
> const vshCmd *cmd,
> unsigned int flags);
>
> +char **
> +virshDomainThrottleGroupCompleter(vshControl *ctl,
> + const vshCmd *cmd,
> + unsigned int flags);
This function is not used anywhere else so doesn't need to be exported.
> +
> +char **
> +virshDomainThrottleGroupsCompleter(vshControl *ctl,
> + const vshCmd *cmd,
> + unsigned int completeflags);
> +
> char **
> virshDomainInterfaceStateCompleter(vshControl *ctl,
> const vshCmd *cmd,
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 3d9c48629a..07a53a9a4c 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -522,6 +522,11 @@ static const vshCmdOptDef opts_attach_disk[] = {
> .type = VSH_OT_STRING,
> .help = N_("host socket for source of disk device")
> },
> + {.name = "throttle-groups",
> + .type = VSH_OT_STRING,
> + .completer = virshDomainThrottleGroupsCompleter,
> + .help = N_("comma separated list of throttle groups to be applied")
> + },
> VIRSH_COMMON_OPT_DOMAIN_PERSISTENT,
> VIRSH_COMMON_OPT_DOMAIN_CONFIG,
> VIRSH_COMMON_OPT_DOMAIN_LIVE,
> @@ -611,7 +616,10 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
> const char *host_name = NULL;
> const char *host_transport = NULL;
> const char *host_socket = NULL;
> + const char *throttle_groups = NULL;
'throttle_groups_str'
> + g_autofree char **groups = NULL;
'throttle_groups;
> int ret;
> + size_t i;
> unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
> const char *stype = NULL;
> int type = VIR_STORAGE_TYPE_NONE;
> @@ -622,6 +630,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
> g_auto(virBuffer) sourceAttrBuf = VIR_BUFFER_INITIALIZER;
> g_auto(virBuffer) sourceChildBuf = VIR_BUFFER_INIT_CHILD(&diskChildBuf);
> g_auto(virBuffer) hostAttrBuf = VIR_BUFFER_INITIALIZER;
> + g_auto(virBuffer) throttleChildBuf = VIR_BUFFER_INITIALIZER;
> g_autofree char *xml = NULL;
> struct stat st;
> bool current = vshCommandOptBool(cmd, "current");
> @@ -665,9 +674,14 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
> vshCommandOptStringReq(ctl, cmd, "source-protocol", &source_protocol) < 0 ||
> vshCommandOptStringReq(ctl, cmd, "source-host-name", &host_name) < 0 ||
> vshCommandOptStringReq(ctl, cmd, "source-host-transport", &host_transport) < 0 ||
> + vshCommandOptStringReq(ctl, cmd, "throttle-groups", &throttle_groups) < 0 ||
> vshCommandOptStringReq(ctl, cmd, "source-host-socket", &host_socket) < 0)
> return false;
>
> + if (throttle_groups) {
> + groups = g_strsplit(throttle_groups, ",", 0);
> + }
> +
> if (stype &&
> (type = virshAttachDiskSourceTypeFromString(stype)) < 0) {
> vshError(ctl, _("Unknown source type: '%1$s'"), stype);
> @@ -714,6 +728,15 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
>
> virXMLFormatElement(&diskChildBuf, "driver", &driverAttrBuf, NULL);
>
> + if (groups) {
> + for (i = 0; i < (g_strv_length(groups)); i++) {
Iterating g_strv which is a NULL-terminated array using g_strv_length is
a O(n^2) algorithm. Don't use numeric index and count, but check whether
the currently iterated element is non-NULL.
> + g_auto(virBuffer) throttleAttrBuf = VIR_BUFFER_INITIALIZER;
> + virBufferAsprintf(&throttleAttrBuf, " group='%s'", groups[i]);
> + virXMLFormatElement(&throttleChildBuf, "throttlefilter", &throttleAttrBuf, NULL);
> + }
> + virXMLFormatElement(&diskChildBuf, "throttlefilters", NULL, &throttleChildBuf);
> + }
> +
> switch ((enum virshAttachDiskSourceType) type) {
> case VIRSH_ATTACH_DISK_SOURCE_TYPE_FILE:
> virBufferEscapeString(&sourceAttrBuf, " file='%s'", source);
> @@ -1509,6 +1532,430 @@ 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)
> + 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);
> + if (vshTableRowAppend(table, name, NULL) < 0)
> + 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,
> + .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")
> + },
> + 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;
> +
> +
> +#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 (vshCommandOptStringReq(ctl, cmd, "group-name", &group_name) < 0) {
> + vshError(ctl, "%s", _("Unable to parse group-name parameter"));
This error doesn't make sense. If you declare the option for the command
properly it won't ever get reported as mandatory options are checked
before.
This code path would only ever hit on a programming error.
> + goto cleanup;
> + }
> +
> + if (group_name) {
> + 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;
> + 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,
> + .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 (vshCommandOptStringReq(ctl, cmd, "group-name", &group_name) < 0) {
> + vshError(ctl, "%s", _("Unable to parse group-name parameter"));
See above
> + 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 = virshCompleteEmpty,
> + .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;
> +
> + if (vshCommandOptStringReq(ctl, cmd, "group-name", &group_name) < 0) {
> + vshError(ctl, "%s", _("Unable to parse group-name parameter"));
...
> + goto cleanup;
> + }
> +
> + /* Get number of params */
> + if (nparams == 0) {
> + if (virDomainGetThrottleGroup(dom, NULL, NULL, &nparams, flags) != 0) {
> + vshError(ctl, "%s",
> + _("Unable to get number of throttle group parameters"));
> + goto cleanup;
> + }
> +
> + if (nparams == 0) {
> + ret = true;
> + goto cleanup;
> + }
> +
> + params = g_new0(virTypedParameter, nparams);
> + }
> +
> + /* Request with nparams */
> + if (virDomainGetThrottleGroup(dom, group_name, params, &nparams, flags) != 0) {
> + vshError(ctl, "%s",
> + _("Unable to get throttle group parameters"));
> + goto cleanup;
> + }
The code above nicely illustrates why we don't use this approach any
more.
> +
> + 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
> */
