On Mon, Nov 18, 2024 at 19:24:12 +0530, Harikumar R wrote:
> From: Chun Feng Wu <danielwuwy@xxxxxxx>
>
> Introduce throttle filter along with corresponding operations.
>
> * Define new struct 'virDomainThrottleFilterDef' and corresponding destructor
> * Update _virDomainDiskDef to include virDomainThrottleFilterDef
> * Support throttle filter "Parse" and "Format" for operations between DOM XML
> and structs. Note, this commit just contains parse/format of group name for
> throttle filter in domain_conf.c, there is other commit to handle throttle
> filter nodename parse/format between throttlefilter and diskPrivateData for
> statusxml in qemu_domain.c when processing qemuDomainDiskPrivate and
> qemuDomainDiskPrivate
>
> Signed-off-by: Chun Feng Wu <danielwuwy@xxxxxxx>
> ---
> src/conf/domain_conf.c | 105 ++++++++++++++++++++++++++++++++++++++++
> src/conf/domain_conf.h | 18 +++++++
> src/conf/virconftypes.h | 2 +
> 3 files changed, 125 insertions(+)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index ecad148783..8841425adb 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3816,6 +3816,16 @@ virDomainThrottleGroupDefArrayFree(virDomainThrottleGroupDef **def,
> }
>
>
> +void
> +virDomainThrottleFilterDefFree(virDomainThrottleFilterDef *def)
> +{
> + if (!def)
> + return;
> + g_free(def->group_name);
> + g_free(def->nodename);
This doesn't free 'def' itself. If it's just meant to free the contents
please use the 'Clear' naming instead of 'Free'
> +}
> +
> +
> void
> virDomainResourceDefFree(virDomainResourceDef *resource)
> {
> @@ -7913,6 +7923,53 @@ virDomainDefThrottleGroupsParse(virDomainDef *def,
> }
>
>
> +static virDomainThrottleFilterDef *
> +virDomainDiskThrottleFilterDefParse(xmlNodePtr node)
> +{
> + g_autoptr(virDomainThrottleFilterDef) filter = g_new0(virDomainThrottleFilterDef, 1);
> +
> + filter->group_name = virXMLPropString(node, "group");
This doesn't report error ...
> +
> + if (!filter->group_name)
> + return NULL;
And you break out here ..
> +
> + return g_steal_pointer(&filter);
> +}
> +
> +
> +static int
> +virDomainDiskDefThrottleFiltersParse(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]))) {
> + return -1;
... and here too so this function will not report an error. Use
virXMLPropStringRequired instead if the function field is required.
> + }
> +
> + if (virDomainThrottleFilterFind(def, filter->group_name)) {
> + 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;
> +}
> +
> +
> static int
> virDomainDiskDefMirrorParse(virDomainDiskDef *def,
> xmlNodePtr cur,
> @@ -8403,6 +8460,9 @@ virDomainDiskDefParseXML(virDomainXMLOption *xmlopt,
> if (virDomainDiskDefIotuneParse(def, ctxt) < 0)
> return NULL;
>
> + if (virDomainDiskDefThrottleFiltersParse(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);
> @@ -22621,6 +22681,34 @@ virDomainThrottleGroupDel(virDomainDef *def,
> }
>
>
> +/**
> + * virDomainThrottleFilterFind:
> + * @def: domain disk definition
> + * @name: throttle group name
> + *
> + * Search domain disk to find throttle filter referencing
> + * throttle group with name @name.
> + *
> + * Return a pointer to throttle filter found
> + */
> +virDomainThrottleFilterDef *
> +virDomainThrottleFilterFind(const virDomainDiskDef *def,
> + const char *name)
> +{
> + size_t i;
> +
> + if (!def->throttlefilters || def->nthrottlefilters == 0)
> + return NULL;
virDomainDiskDefFormatThrottleFilters below checks only the
'nthrottlefilters'. In fact all of the above check is not needed as the
loop simply won't find anything in a 0 sized array and return NULL at
the end.
> +
> + for (i = 0; i < def->nthrottlefilters; i++) {
> + if (STREQ(name, def->throttlefilters[i]->group_name))
> + return def->throttlefilters[i];
> + }
> +
> + return NULL;
> +}
> +
> +
> static int
> virDomainEventActionDefFormat(virBuffer *buf,
> int type,
> @@ -23235,6 +23323,21 @@ virDomainDiskDefFormatIotune(virBuffer *buf,
> #undef FORMAT_IOTUNE
>
>
> +static void
> +virDomainDiskDefFormatThrottleFilters(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;
> + virBufferEscapeString(&throttleAttrBuf, " group='%s'", disk->throttlefilters[i]->group_name);
> + virXMLFormatElement(&throttleChildBuf, "throttlefilter", &throttleAttrBuf, NULL);
> + }
> + virXMLFormatElement(buf, "throttlefilters", NULL, &throttleChildBuf);
> +}
> +
> +
> static void
> virDomainDiskDefFormatDriver(virBuffer *buf,
> virDomainDiskDef *disk)
> @@ -23521,6 +23624,8 @@ virDomainDiskDefFormat(virBuffer *buf,
>
> virDomainDiskDefFormatIotune(&childBuf, def);
>
> + virDomainDiskDefFormatThrottleFilters(&childBuf, def);
> +
> if (def->src->readonly)
> virBufferAddLit(&childBuf, "<readonly/>\n");
> if (def->src->shared)
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 2f40d304b4..f1e85ab42f 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -520,6 +520,13 @@ void virDomainDiskIothreadDefFree(virDomainDiskIothreadDef *def);
> G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainDiskIothreadDef, virDomainDiskIothreadDefFree);
>
>
> +/* Stores information related to a ThrottleFilter resource. */
> +struct _virDomainThrottleFilterDef {
> + char *group_name;
Add a separator comment here here as node name is a qemu driver implementation
detail and not a configuration field.
> + char *nodename; /* node name of throttle filter object */
This will also later have to be serialized into the qemu driver's disk
private data (I didn't check if it is).
Rest looks okay to me.
