On Mon, Nov 18, 2024 at 19:24:11 +0530, Harikumar R wrote:
> From: Chun Feng Wu <danielwuwy@xxxxxxx>
>
> Introduce throttlegroup into domain and provide corresponding methods
>
> * Define new struct 'virDomainThrottleGroupDef' and corresponding destructor
> * Add operations(Add, Update, Del, ByName, Copy, Free) for 'virDomainThrottleGroupDef'
> * Update _virDomainDef to include virDomainThrottleGroupDef
> * Support new resource "Parse" and "Format" for operations between struct and DOM XML
> * Make sure "group_name" is defined in xml
>
> Signed-off-by: Chun Feng Wu <danielwuwy@xxxxxxx>
> ---
> src/conf/domain_conf.c | 293 ++++++++++++++++++++++++++++++++++++++++
> src/conf/domain_conf.h | 27 ++++
> src/conf/virconftypes.h | 2 +
> 3 files changed, 322 insertions(+)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 295707ec1f..ecad148783 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3790,6 +3790,32 @@ virDomainIOThreadIDDefArrayInit(virDomainDef *def,
> }
>
>
> +void
> +virDomainThrottleGroupDefFree(virDomainThrottleGroupDef *def)
> +{
> + if (!def)
> + return;
> + g_free(def->group_name);
> + g_free(def);
> +}
> +
> +
> +static void
> +virDomainThrottleGroupDefArrayFree(virDomainThrottleGroupDef **def,
> + int nthrottlegroups)
> +{
> + size_t i;
> +
> + if (!def)
> + return;
> +
> + for (i = 0; i < nthrottlegroups; i++)
> + virDomainThrottleGroupDefFree(def[i]);
> +
> + g_free(def);
> +}
> +
> +
> void
> virDomainResourceDefFree(virDomainResourceDef *resource)
> {
> @@ -4077,6 +4103,8 @@ void virDomainDefFree(virDomainDef *def)
>
> virDomainIOThreadIDDefArrayFree(def->iothreadids, def->niothreadids);
>
> + virDomainThrottleGroupDefArrayFree(def->throttlegroups, def->nthrottlegroups);
> +
> g_free(def->defaultIOThread);
>
> virBitmapFree(def->cputune.emulatorpin);
> @@ -7771,6 +7799,120 @@ virDomainDiskDefIotuneParse(virDomainDiskDef *def,
> #undef PARSE_IOTUNE
>
>
> +#define PARSE_THROTTLEGROUP(val) \
> + if (virXPathULongLong("string(./" #val ")", \
> + ctxt, &group->val) == -2) { \
> + virReportError(VIR_ERR_XML_ERROR, \
> + _("throttle group field '%1$s' must be an integer"), #val); \
> + return NULL; \
> + }
> +
> +
> +static virDomainThrottleGroupDef *
> +virDomainThrottleGroupDefParseXML(xmlNodePtr node,
> + xmlXPathContextPtr ctxt)
> +{
> + g_autoptr(virDomainThrottleGroupDef) group = g_new0(virDomainThrottleGroupDef, 1);
Since this is not the same code that parses/formats the <disk>
throttling definition add a comment to both places stating that anyone
adding fields must also change the other place as well.
> +
> + VIR_XPATH_NODE_AUTORESTORE(ctxt)
> + ctxt->node = node;
> +
> + PARSE_THROTTLEGROUP(total_bytes_sec);
> + PARSE_THROTTLEGROUP(read_bytes_sec);
> + PARSE_THROTTLEGROUP(write_bytes_sec);
> + PARSE_THROTTLEGROUP(total_iops_sec);
> + PARSE_THROTTLEGROUP(read_iops_sec);
> + PARSE_THROTTLEGROUP(write_iops_sec);
> +
> + PARSE_THROTTLEGROUP(total_bytes_sec_max);
> + PARSE_THROTTLEGROUP(read_bytes_sec_max);
> + PARSE_THROTTLEGROUP(write_bytes_sec_max);
> + PARSE_THROTTLEGROUP(total_iops_sec_max);
> + PARSE_THROTTLEGROUP(read_iops_sec_max);
> + PARSE_THROTTLEGROUP(write_iops_sec_max);
> +
> + PARSE_THROTTLEGROUP(size_iops_sec);
> +
> + PARSE_THROTTLEGROUP(total_bytes_sec_max_length);
> + PARSE_THROTTLEGROUP(read_bytes_sec_max_length);
> + PARSE_THROTTLEGROUP(write_bytes_sec_max_length);
> + PARSE_THROTTLEGROUP(total_iops_sec_max_length);
> + PARSE_THROTTLEGROUP(read_iops_sec_max_length);
> + PARSE_THROTTLEGROUP(write_iops_sec_max_length);
> +
> + /* group_name is required */
> + if (!(group->group_name = virXPathString("string(./group_name)", ctxt))) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("missing group name"));
> + return NULL;
> + }
> +
> + return g_steal_pointer(&group);
> +}
> +#undef PARSE_THROTTLEGROUP
> +
> +
> +/**
> + * virDomainThrottleGroupByName:
> + * @def: domain definition
> + * @name: throttle group name
> + *
> + * search throttle group within domain definition
> + * by @name
> + *
> + * Returns a pointer to throttle group found.
> + */
> +virDomainThrottleGroupDef *
> +virDomainThrottleGroupByName(const virDomainDef *def,
> + const char *name)
> +{
> + size_t i;
> +
> + if (!def->throttlegroups || def->nthrottlegroups == 0)
> + return NULL;
> +
> + for (i = 0; i < def->nthrottlegroups; i++) {
> + if (STREQ(def->throttlegroups[i]->group_name, name))
> + return def->throttlegroups[i];
> + }
> +
> + return NULL;
> +}
> +
> +
> +static int
> +virDomainDefThrottleGroupsParse(virDomainDef *def,
> + xmlXPathContextPtr ctxt)
> +{
> + size_t i;
> + int n = 0;
> + g_autofree xmlNodePtr *nodes = NULL;
> +
> + if ((n = virXPathNodeSet("./throttlegroups/throttlegroup", ctxt, &nodes)) < 0)
> + return -1;
> +
> + if (n)
We prefer explicit tests for (non-)zero values for anything that's
numeric (e.g. not a boolean or pointer).
> + def->throttlegroups = g_new0(virDomainThrottleGroupDef *, n);
Also there's no point to continue if 'n' is zero here.
> +
> + for (i = 0; i < n; i++) {
> + g_autoptr(virDomainThrottleGroupDef) group = NULL;
> +
> + if (!(group = virDomainThrottleGroupDefParseXML(nodes[i], ctxt))) {
> + return -1;
> + }
> +
> + if (virDomainThrottleGroupByName(def, group->group_name)) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("duplicate group name '%1$s' found"),
> + group->group_name);
> + return -1;
> + }
> + def->throttlegroups[def->nthrottlegroups++] = g_steal_pointer(&group);
> + }
> + return 0;
> +}
> +
> +
> static int
> virDomainDiskDefMirrorParse(virDomainDiskDef *def,
> xmlNodePtr cur,
> @@ -19112,6 +19254,9 @@ virDomainDefParseXML(xmlXPathContextPtr ctxt,
> if (virDomainDefParseBootOptions(def, ctxt, xmlopt, flags) < 0)
> return NULL;
>
> + if (virDomainDefThrottleGroupsParse(def, ctxt) < 0)
> + return NULL;
> +
> /* analysis of the disk devices */
> if ((n = virXPathNodeSet("./devices/disk", ctxt, &nodes)) < 0)
> return NULL;
> @@ -22387,6 +22532,95 @@ virDomainIOThreadIDDel(virDomainDef *def,
> }
>
>
> +/**
> + * virDomainThrottleGroupDefCopy:
> + * @src: throttle group to be copiped from
> + * @dst: throttle group to be copiped to
> + *
> + * copy throttle group content from @src to @dst
Usually copy functions also allocate memory; document that this one does
not.
> + */
> +void
> +virDomainThrottleGroupDefCopy(const virDomainThrottleGroupDef *src,
> + virDomainThrottleGroupDef *dst)
> +{
> + *dst = *src;
> + dst->group_name = g_strdup(src->group_name);
> +}
> +
> +
> +/**
> + * virDomainThrottleGroupAdd:
> + * @def: domain definition
> + * @throttle_group: throttle group definition within domain
> + *
> + * add new throttle group into @def
> + *
> + * return a pointer to throttle group added
> + */
> +virDomainThrottleGroupDef *
> +virDomainThrottleGroupAdd(virDomainDef *def,
> + virDomainThrottleGroupDef *throttle_group)
> +{
> + virDomainThrottleGroupDef * new_group = g_new0(virDomainThrottleGroupDef, 1);
> + virDomainThrottleGroupDefCopy(throttle_group, new_group);
> + VIR_APPEND_ELEMENT_COPY(def->throttlegroups, def->nthrottlegroups, new_group);
Since 'virDomainThrottleGroupUpdate' below has weird semantics of not
doing anything when given group doesn't exist consider merging these two
together. This will also prevent the situation when virDomainThrottleGroupAdd
is called on an existing definition which would add it without checking
that it already exists and none of the other functions consider that
scenario.
At the very least document that this does absolutely no checking.
> + return new_group;
> +}
> +
> +
> +/**
> + * virDomainThrottleGroupUpdate:
> + * @def: domain definition
> + * @info: throttle group definition within domain
> + *
> + * search existing throttle group within domain definition
> + * by group_name and then overwrite it with @info,
> + * it's to update existing throttle group
Replace by
Update corresponding throttle group in @def new config @info. If a
throttle group with given name doesn't exist this function does nothing.
> + */
> +void
> +virDomainThrottleGroupUpdate(virDomainDef *def,
> + virDomainThrottleGroupDef *info)
> +{
> + size_t i;
> +
> + if (!info->group_name)
> + return;
> +
> + for (i = 0; i < def->nthrottlegroups; i++) {
> + virDomainThrottleGroupDef *t = def->throttlegroups[i];
> +
> + if (STREQ_NULLABLE(t->group_name, info->group_name)) {
> + VIR_FREE(t->group_name);
> + virDomainThrottleGroupDefCopy(info, t);
> + }
> + }
> +}
> +
> +
> +/**
> + * virDomainThrottleGroupDel:
> + * @def: domain definition
> + * @name: throttle group name
> + *
> + * search existing throttle group within domain definition
> + * by group_name and then delete it with @name,
> + * it's to delete existing throttle group
Replace by:
Delete throttle group @name in @def.
> + */
> +void
> +virDomainThrottleGroupDel(virDomainDef *def,
> + const char *name)
> +{
> + size_t i;
> + for (i = 0; i < def->nthrottlegroups; i++) {
> + if (STREQ_NULLABLE(def->throttlegroups[i]->group_name, name)) {
> + virDomainThrottleGroupDefFree(def->throttlegroups[i]);
> + VIR_DELETE_ELEMENT(def->throttlegroups, i, def->nthrottlegroups);
> + return;
> + }
> + }
> +}
> +
> +
> static int
> virDomainEventActionDefFormat(virBuffer *buf,
> int type,
> @@ -27537,6 +27771,63 @@ virDomainDefIOThreadsFormat(virBuffer *buf,
> }
>
>
> +#define FORMAT_THROTTLE_GROUP(val) \
> + if (group->val > 0) { \
> + virBufferAsprintf(&childBuf, "<" #val ">%llu</" #val ">\n", \
> + group->val); \
> + }
> +
> +
Since this is not the same code that parses/formats the <disk>
throttling definition add a comment to both places stating that anyone
adding fields must also change the other place as well.
> +static void
> +virDomainThrottleGroupFormat(virBuffer *buf,
> + virDomainThrottleGroupDef *group)
> +{
> + g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
> +
> + FORMAT_THROTTLE_GROUP(total_bytes_sec);
> + FORMAT_THROTTLE_GROUP(read_bytes_sec);
> + FORMAT_THROTTLE_GROUP(write_bytes_sec);
> + FORMAT_THROTTLE_GROUP(total_iops_sec);
> + FORMAT_THROTTLE_GROUP(read_iops_sec);
> + FORMAT_THROTTLE_GROUP(write_iops_sec);
> +
> + FORMAT_THROTTLE_GROUP(total_bytes_sec_max);
> + FORMAT_THROTTLE_GROUP(read_bytes_sec_max);
> + FORMAT_THROTTLE_GROUP(write_bytes_sec_max);
> + FORMAT_THROTTLE_GROUP(total_iops_sec_max);
> + FORMAT_THROTTLE_GROUP(read_iops_sec_max);
> + FORMAT_THROTTLE_GROUP(write_iops_sec_max);
> +
> + FORMAT_THROTTLE_GROUP(total_bytes_sec_max_length);
> + FORMAT_THROTTLE_GROUP(read_bytes_sec_max_length);
> + FORMAT_THROTTLE_GROUP(write_bytes_sec_max_length);
> + FORMAT_THROTTLE_GROUP(total_iops_sec_max_length);
> + FORMAT_THROTTLE_GROUP(read_iops_sec_max_length);
> + FORMAT_THROTTLE_GROUP(write_iops_sec_max_length);
> +
> + virBufferEscapeString(&childBuf, "<group_name>%s</group_name>\n",
> + group->group_name);
> +
> + virXMLFormatElement(buf, "throttlegroup", NULL, &childBuf);
> +}
> +
> +#undef FORMAT_THROTTLE_GROUP
> +
> +static void
> +virDomainDefThrottleGroupsFormat(virBuffer *buf,
> + const virDomainDef *def)
> +{
> + g_auto(virBuffer) childrenBuf = VIR_BUFFER_INIT_CHILD(buf);
> + ssize_t n;
> +
> + for (n = 0; n < def->nthrottlegroups; n++) {
'nthrottlegroups' is 'size_t' (see below) so 'n' ought to be as well.
> + virDomainThrottleGroupFormat(&childrenBuf, def->throttlegroups[n]);
> + }
> +
> + virXMLFormatElement(buf, "throttlegroups", NULL, &childrenBuf);
> +}
> +
> +
> static void
> virDomainIOMMUDefFormat(virBuffer *buf,
> const virDomainIOMMUDef *iommu)
> @@ -28229,6 +28520,8 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def,
>
> virDomainDefIOThreadsFormat(buf, def);
>
> + virDomainDefThrottleGroupsFormat(buf, def);
> +
> if (virDomainCputuneDefFormat(buf, def, flags) < 0)
> return -1;
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index a187ab4083..2f40d304b4 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -3067,6 +3067,9 @@ struct _virDomainDef {
>
> virDomainDefaultIOThreadDef *defaultIOThread;
>
> + size_t nthrottlegroups;
[ ^^^^ ]
> + virDomainThrottleGroupDef **throttlegroups;
> +
> virDomainCputune cputune;
>
> virDomainResctrlDef **resctrls;
