On Thu, Apr 11, 2024 at 19:01:49 -0700, wucf@xxxxxxxxxxxxx wrote:
> From: Chun Feng Wu <wucf@xxxxxxxxxxxxx>
>
> * Define new structs 'virDomainThrottleGroupDef' and 'virDomainThrottleFilterDef'
> * Update _virDomainDef to include virDomainThrottleGroupDef
> * Update _virDomainDiskDef to include virDomainThrottleFilterDef
> * Support new resource operations for DOM XML and structs, corresponding "Parse" and "Format" methods are provided
>
> Signed-off-by: Chun Feng Wu <wucf@xxxxxxxxxxxxx>
> ---
> src/conf/domain_conf.c | 386 ++++++++++++++++++++++++++++++++++++++++
> src/conf/domain_conf.h | 54 ++++++
> src/conf/virconftypes.h | 4 +
> 3 files changed, 444 insertions(+)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 48c5d546da..134a787dda 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3747,6 +3747,54 @@ virDomainIOThreadIDDefArrayInit(virDomainDef *def,
> }
>
>
> +static virDomainThrottleFilterDef *
> +virDomainThrottleFilterDefNew(void)
Having an constructor can be handy if you need to set non zero values in
it.
> +{
> + virDomainThrottleFilterDef *def = g_new0(virDomainThrottleFilterDef, 1);
'g_new0' explicitly zeroes out the allocated memory.
Also virDomainThrottleFilterDef is declared with a 'virObject' member,
but this is not the correct way to instantiate 'virObjects' there's
plenty more boilerplate code needed to do that.
Do you even need reference counting for the struct? Otherwise you don't
probably need to use a virObject
> +
> + def->group_name = NULL;
So this is not needed and we avoid it.
> +
> + return def;
> +}
> +
> +
> +void
> +virDomainThrottleFilterDefFree(virDomainThrottleFilterDef *def)
> +{
> + if (!def)
> + return;
> + VIR_FREE(def->group_name);
> + VIR_FREE(def->nodename);
Preferrably new code should not use VIR_FREE bug g_free instead in
destructors (if the variable won't be around any more thus the value
doesn't matter.
> + virObjectUnref(def);
See note about proper virObject usage.
> +}
> +
> +
> +void
> +virDomainThrottleGroupDefFree(virDomainThrottleGroupDef *def)
> +{
> + if (!def)
> + return;
> + g_free(def->group_name);
> + g_free(def);
See, you do it here, please be consistent.
> +}
> +
> +
> +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)
> {
> @@ -4026,6 +4074,8 @@ void virDomainDefFree(virDomainDef *def)
>
> virDomainIOThreadIDDefArrayFree(def->iothreadids, def->niothreadids);
>
> + virDomainThrottleGroupDefArrayFree(def->throttlegroups, def->nthrottlegroups);
> +
> g_free(def->defaultIOThread);
>
> virBitmapFree(def->cputune.emulatorpin);
> @@ -7694,6 +7744,165 @@ virDomainDiskDefIotuneParse(virDomainDiskDef *def,
> #undef PARSE_IOTUNE
>
>
> +static virDomainThrottleFilterDef *
> +virDomainDiskThrottleFilterDefParse(xmlNodePtr node,
> + xmlXPathContextPtr ctxt)
Okay so as long as you are touching the XML parser this patch needs to
be also adding the XML docs and schema, or the schema and docs need to
be added *before* this patch. And the summary (first line) of the commit
message should then also state that it's adding XML parsing stuff as
that's way more improtant information
Also this patch doesn't really need to add both the '*Filter*' and
'*Group*' stuff at once, which would make the patch easier to go
through.
> +{
> + g_autoptr(virDomainThrottleFilterDef) filter = virDomainThrottleFilterDefNew();
> +
> + VIR_XPATH_NODE_AUTORESTORE(ctxt)
> + ctxt->node = node;
> +
> + filter->group_name = virXMLPropString(ctxt->node, "group");
Umm, this is not actually using the XPath context, you don't even need
to touch it and can use 'node' here directly.
> +
> + return g_steal_pointer(&filter);
> +}
> +
> +
> +static int
> +virDomainDiskDefThrottleFilterChainParse(virDomainDiskDef *def,
> + xmlXPathContextPtr ctxt)
> +{
> + size_t i;
> + int n = 0;
> + g_autofree xmlNodePtr *nodes = NULL;
> +
> + if ((n = virXPathNodeSet("./throttlefilters/throttlefilter", ctxt, &nodes)) < 0)
> + return -1;
> +
> + if (n)
> + def->throttlefilters = g_new0(virDomainThrottleFilterDef *, n);
> +
> + for (i = 0; i < n; i++) {
> + g_autoptr(virDomainThrottleFilterDef) filter = NULL;
> +
> + if (!(filter = virDomainDiskThrottleFilterDefParse(nodes[i], ctxt))) {
> + return -1;
This function currently can't fail so this error reporting makes no
sense.
> + }
> +
> + if (virDomainThrottleFilterFind(def, filter->group_name)) {
Note that 'virDomainDiskThrottleFilterDefParse' doesn't parse the
'group' field as required so it may be NULL here if the user omits it.
virDomainThrottleFilterFind will crash if the second argument is NULL
> + virReportError(VIR_ERR_XML_ERROR,
> + _("duplicate filter name '%1$s' found"),
> + filter->group_name);
> + return -1;
> + }
> + def->throttlefilters[def->nthrottlefilters++] = g_steal_pointer(&filter);
> + }
> + return 0;
> +}
> +
> +
> +#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);
> +
> + 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->group_name = virXPathString("string(./group_name)", ctxt);
> +
> + return g_steal_pointer(&group);
> +}
> +#undef PARSE_THROTTLEGROUP
> +
> +
> +int
> +virDomainThrottleGroupIndexByName(virDomainDef *def,
> + const char *name)
At the end of this series this function is still used exactly only in
one place and that is virDomainThrottleGroupByName. Since it's not
needed apparently please remove it in favor of +virDomainThrottleGroupByName
doing everything.
> +{
> + virDomainThrottleGroupDef *tgroup;
> + size_t i;
> + int candidate = -1;
> +
> + for (i = 0; i < def->nthrottlegroups; i++) {
> + tgroup = def->throttlegroups[i];
> + if (STREQ(tgroup->group_name, name))
> + return i;
> + }
> + return candidate;
> +}
> +
> +
> +virDomainThrottleGroupDef *
> +virDomainThrottleGroupByName(virDomainDef *def,
> + const char *name)
> +{
> + int idx = virDomainThrottleGroupIndexByName(def, name);
> +
> + 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,
> @@ -8184,6 +8393,9 @@ virDomainDiskDefParseXML(virDomainXMLOption *xmlopt,
> if (virDomainDiskDefIotuneParse(def, ctxt) < 0)
> return NULL;
>
> + if (virDomainDiskDefThrottleFilterChainParse(def, ctxt) < 0)
> + return NULL;
> +
> def->domain_name = virXPathString("string(./backenddomain/@name)", ctxt);
> def->serial = virXPathString("string(./serial)", ctxt);
> def->wwn = virXPathString("string(./wwn)", ctxt);
> @@ -18857,6 +19069,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;
> @@ -22066,6 +22281,88 @@ virDomainIOThreadIDDel(virDomainDef *def,
> }
>
>
> +virDomainThrottleGroupDef *
> +virDomainThrottleGroupFind(const virDomainDef *def,
> + const char *name)
> +{
> + size_t i;
> +
> + if (!def->throttlegroups || !def->nthrottlegroups)
as noted before 'nthrottlegroups' is a number, thus you should use
explicit checks against 0. There are more instances below.
> + return NULL;
> +
> + for (i = 0; i < def->nthrottlegroups; i++) {
> + if (STREQ(name, def->throttlegroups[i]->group_name))
> + return def->throttlegroups[i];
> + }
> +
> + return NULL;
> +}
> +
> +
> +virDomainThrottleFilterDef *
> +virDomainThrottleFilterFind(const virDomainDiskDef *def,
> + const char *name)
> +{
> + size_t i;
> +
> + if (!def->throttlefilters || !def->nthrottlefilters)
> + return NULL;
> +
> + for (i = 0; i < def->nthrottlefilters; i++) {
> + if (STREQ(name, def->throttlefilters[i]->group_name))
> + return def->throttlefilters[i];
> + }
> +
> + return NULL;
> +}
> +
> +
> +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;
> +}
> +
> +
> +void
> +virDomainThrottleGroupUpdate(virDomainDef *def,
> + virDomainThrottleGroupDef *info)
Please document this function. Its not clear at the first clance what it
does.
> +{
> + 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);
> + }
> + }
> +}
> +
> +
> +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,
> @@ -22682,6 +22979,21 @@ virDomainDiskDefFormatIotune(virBuffer *buf,
> #undef FORMAT_IOTUNE
>
>
> +static void
> +virDomainDiskDefFormatThrottleFilterChain(virBuffer *buf,
> + virDomainDiskDef *disk)
> +{
> + size_t i;
> + g_auto(virBuffer) throttleChildBuf = VIR_BUFFER_INIT_CHILD(buf);
> + for (i = 0; i < disk->nthrottlefilters; i++) {
> + g_auto(virBuffer) throttleAttrBuf = VIR_BUFFER_INITIALIZER;
> + virBufferAsprintf(&throttleAttrBuf, " group='%s'", disk->throttlefilters[i]->group_name);
The group name is user-provided string thus you must use
virBufferEscapeString to format it so that proper XML escaping is
enforced.
> + virXMLFormatElement(&throttleChildBuf, "throttlefilter", &throttleAttrBuf, NULL);
> + }
> + virXMLFormatElement(buf, "throttlefilters", NULL, &throttleChildBuf);
> +}
> +
> +
> static void
> virDomainDiskDefFormatDriver(virBuffer *buf,
> virDomainDiskDef *disk)
> @@ -22968,6 +23280,8 @@ virDomainDiskDefFormat(virBuffer *buf,
>
> virDomainDiskDefFormatIotune(&childBuf, def);
>
> + virDomainDiskDefFormatThrottleFilterChain(&childBuf, def);
> +
> if (def->src->readonly)
> virBufferAddLit(&childBuf, "<readonly/>\n");
> if (def->src->shared)
> @@ -27137,6 +27451,67 @@ virDomainDefIOThreadsFormat(virBuffer *buf,
> }
>
>
> +#define FORMAT_THROTTLE_GROUP(val) \
> + if (group->val) { \
as 'val' is a number not a pointer or boolean, coding style dictates to
use explicit comparison with '0'
if (group->val != 0) or 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);
> +
> + if (group->group_name) {
> + virBufferEscapeString(&childBuf, "<group_name>%s</group_name>\n",
> + group->group_name);
virBufferEscapeString has integrated NULL-check for the third argument
so the if-wrapper is not needed.
> + }
> +
> + 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)
> @@ -27801,6 +28176,8 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def,
>
> virDomainDefIOThreadsFormat(buf, def);
>
> + virDomainDefThrottleGroupsFormat(buf, def);
> +
> if (virDomainCputuneDefFormat(buf, def, flags) < 0)
> return -1;
>
> @@ -31034,6 +31411,15 @@ virDomainBlockIoTuneInfoCopy(const virDomainBlockIoTuneInfo *src,
> }
>
>
> +void
> +virDomainThrottleGroupDefCopy(const virDomainThrottleGroupDef *src,
> + virDomainThrottleGroupDef *dst)
> +{
> + *dst = *src;
> + dst->group_name = g_strdup(src->group_name);
> +}
> +
> +
> bool
> virDomainBlockIoTuneInfoEqual(const virDomainBlockIoTuneInfo *a,
> const virDomainBlockIoTuneInfo *b)
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 5925faaf1a..08e2a22aeb 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -464,6 +464,14 @@ struct _virDomainBlockIoTuneInfo {
> * virDomainBlockIoTuneInfoEqual. */
> };
>
> +/* Stores information related to a ThrottleFilter resource. */
> +struct _virDomainThrottleFilterDef {
> + virObject parent;
> +
> + unsigned int id; /* throttle filter identifier, 0 is unset */
> + char *group_name;
> + char *nodename; /* node name of throttle filter object */
node name (and thus most likely also the id) is an implementation
odetail of the qemu driver. As we do have precedents putting node-names
into structs that (can be) are used by other hyeprvisors it's okay to
do, but add a comment stating that those below are internal fields used
for at runtime so that they don't get confused with user config and move
them to the end of the struct.
> +};
>
> typedef enum {
> VIR_DOMAIN_DISK_MIRROR_STATE_NONE = 0, /* No job, or job still not synced */
> @@ -550,6 +558,9 @@ struct _virDomainDiskDef {
>
> virDomainBlockIoTuneInfo blkdeviotune;
>
> + size_t nthrottlefilters;
> + virDomainThrottleFilterDef **throttlefilters;
> +
> char *driverName;
>
> char *serial;
> @@ -2992,6 +3003,9 @@ struct _virDomainDef {
>
> virDomainDefaultIOThreadDef *defaultIOThread;
>
> + size_t nthrottlegroups;
> + virDomainThrottleGroupDef **throttlegroups;
> +
> virDomainCputune cputune;
>
> virDomainResctrlDef **resctrls;
> @@ -4504,3 +4518,43 @@ virDomainObjGetMessages(virDomainObj *vm,
>
> bool
> virDomainDefHasSpiceGraphics(const virDomainDef *def);
> +
> +void
> +virDomainThrottleGroupDefFree(virDomainThrottleGroupDef *def);
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainThrottleGroupDef, virDomainThrottleGroupDefFree);
> +
> +void
> +virDomainThrottleFilterDefFree(virDomainThrottleFilterDef *def);
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainThrottleFilterDef, virDomainThrottleFilterDefFree);
> +
> +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);
> +
> +virDomainThrottleFilterDef *
> +virDomainThrottleFilterFind(const virDomainDiskDef *def,
> + const char *name);
> +
> +int
> +virDomainThrottleGroupIndexByName(virDomainDef *def,
> + const char *name);
> +
> +virDomainThrottleGroupDef *
> +virDomainThrottleGroupByName(virDomainDef *def,
> + const char *name);
> +
> +void
> +virDomainThrottleGroupDefCopy(const virDomainThrottleGroupDef *src,
> + virDomainThrottleGroupDef *dst);
Preferrably add a new conf module (src/conf/virdomainthrottle.c/h) and
put all of this into that module.
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
