On Wed, Jun 12, 2024 at 03:02:11 -0700, wucf@xxxxxxxxxxxxx wrote:
> From: Chun Feng Wu <wucf@xxxxxxxxxxxxx>
>
> * Define new struct 'virDomainThrottleGroupDef' and corresponding destructor
> * Add operations(Add, Update, Del, Find, 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 <wucf@xxxxxxxxxxxxx>
> ---
> src/conf/domain_conf.c | 281 ++++++++++++++++++++++++++++++++++++++++
> src/conf/domain_conf.h | 31 +++++
> src/conf/virconftypes.h | 2 +
> 3 files changed, 314 insertions(+)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index fde594f811..05d6f7ad3a 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3750,6 +3750,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)
> {
> @@ -4029,6 +4055,8 @@ void virDomainDefFree(virDomainDef *def)
>
> virDomainIOThreadIDDefArrayFree(def->iothreadids, def->niothreadids);
>
> + virDomainThrottleGroupDefArrayFree(def->throttlegroups, def->nthrottlegroups);
> +
> g_free(def->defaultIOThread);
>
> virBitmapFree(def->cputune.emulatorpin);
> @@ -7697,6 +7725,113 @@ virDomainDiskDefIotuneParse(virDomainDiskDef *def,
> #undef PARSE_IOTUNE
>
>
> +#define PARSE_THROTTLEGROUP(val) \
> + if (virXPathULongLong("string(./" #val ")", \
> + ctxt, &group->val) == -2) { \
This can also return -1, in which case you also want to at least return
NULL, but for simplicity reporting the error as below is okay.
> + 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);
> +
> + 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);
I'd prefer if we had only one copy of the code parsing this and the
equivalent disk throttling data so that the code doesn't need to be
duplicated. The cleanup can be done as a followup though.
> +
> + /* 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
> +
> +
> +virDomainThrottleGroupDef *
> +virDomainThrottleGroupByName(virDomainDef *def,
> + const char *name)
> +{
> + virDomainThrottleGroupDef *tgroup;
> + size_t i;
> + int idx = -1;
> +
> + for (i = 0; i < def->nthrottlegroups; i++) {
> + tgroup = def->throttlegroups[i];
> + if (STREQ(tgroup->group_name, name))
> + idx = i;
> + }
> +
> + if (idx < 0)
> + return NULL;
> +
> + return def->throttlegroups[idx];
> +}
> +
> +
> +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)
> + def->throttlegroups = g_new0(virDomainThrottleGroupDef *, n);
> +
> + for (i = 0; i < n; i++) {
> + g_autoptr(virDomainThrottleGroupDef) group = NULL;
> +
> + if (!(group = virDomainThrottleGroupDefParseXML(nodes[i], ctxt))) {
> + return -1;
> + }
> +
> + if (virDomainThrottleGroupFind(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,
> @@ -18880,6 +19015,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;
> @@ -22090,6 +22228,88 @@ virDomainIOThreadIDDel(virDomainDef *def,
> }
>
>
> +virDomainThrottleGroupDef *
> +virDomainThrottleGroupFind(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(name, def->throttlegroups[i]->group_name))
> + return def->throttlegroups[i];
> + }
> +
> + return NULL;
> +}
> +
> +
> +void
> +virDomainThrottleGroupDefCopy(const virDomainThrottleGroupDef *src,
> + virDomainThrottleGroupDef *dst)
> +{
> + *dst = *src;
> + dst->group_name = g_strdup(src->group_name);
> +}
> +
> +
> +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);
> + 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
> + */
> +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);
> + }
> + }
> +}
> +
> +
Please add docs for any function that is being exported.
> +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,
> @@ -27174,6 +27394,65 @@ virDomainDefIOThreadsFormat(virBuffer *buf,
> }
>
>
> +#define FORMAT_THROTTLE_GROUP(val) \
> + if (group->val > 0) { \
> + virBufferAsprintf(&childBuf, "<" #val ">%llu</" #val ">\n", \
> + group->val); \
> + }
> +
> +
> +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(size_iops_sec);
> +
> + virBufferEscapeString(&childBuf, "<group_name>%s</group_name>\n",
> + group->group_name);
> +
> + 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);
> +
> + 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++) {
> + virDomainThrottleGroupFormat(&childrenBuf, def->throttlegroups[n]);
> + }
> +
> + virXMLFormatElement(buf, "throttlegroups", NULL, &childrenBuf);
> +}
> +
> +
> static void
> virDomainIOMMUDefFormat(virBuffer *buf,
> const virDomainIOMMUDef *iommu)
> @@ -27839,6 +28118,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 a06f015444..c9e3fcd924 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -3000,6 +3000,9 @@ struct _virDomainDef {
>
> virDomainDefaultIOThreadDef *defaultIOThread;
>
> + size_t nthrottlegroups;
> + virDomainThrottleGroupDef **throttlegroups;
> +
> virDomainCputune cputune;
>
> virDomainResctrlDef **resctrls;
> @@ -4512,3 +4515,31 @@ virDomainObjGetMessages(virDomainObj *vm,
>
> bool
> virDomainDefHasSpiceGraphics(const virDomainDef *def);
> +
> +void
> +virDomainThrottleGroupDefFree(virDomainThrottleGroupDef *def);
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainThrottleGroupDef, virDomainThrottleGroupDefFree);
> +
> +virDomainThrottleGroupDef *
> +virDomainThrottleGroupAdd(virDomainDef *def,
> + virDomainThrottleGroupDef *throttle_group);
> +
> +void
> +virDomainThrottleGroupUpdate(virDomainDef *def,
> + virDomainThrottleGroupDef *info);
> +
> +virDomainThrottleGroupDef *
> +virDomainThrottleGroupFind(const virDomainDef *def,
> + const char *name);
> +
> +void
> +virDomainThrottleGroupDel(virDomainDef *def,
> + const char *name);
> +
> +virDomainThrottleGroupDef *
> +virDomainThrottleGroupByName(virDomainDef *def,
> + const char *name);
> +
> +void
> +virDomainThrottleGroupDefCopy(const virDomainThrottleGroupDef *src,
> + virDomainThrottleGroupDef *dst);
> diff --git a/src/conf/virconftypes.h b/src/conf/virconftypes.h
> index 0779bc224b..d51e8f5f40 100644
> --- a/src/conf/virconftypes.h
> +++ b/src/conf/virconftypes.h
> @@ -80,6 +80,8 @@ typedef struct _virDomainBlkiotune virDomainBlkiotune;
>
> typedef struct _virDomainBlockIoTuneInfo virDomainBlockIoTuneInfo;
>
> +typedef struct _virDomainBlockIoTuneInfo virDomainThrottleGroupDef;
> +
> typedef struct _virDomainCheckpointDef virDomainCheckpointDef;
>
> typedef struct _virDomainCheckpointObj virDomainCheckpointObj;
> --
> 2.34.1
>
