Re: [PATCH v6 3/4] conf: introduce dirty_ring_size field

On Sat, Nov 20, 2021 at 03:20:47 -0500, huangy81(a)chinatelecom.cn wrote: > From: Hyman Huang(黄勇) <huangy81(a)chinatelecom.cn&gt; > > introduce dirty_ring_size in struct "_virDomainDef" to hold > the ring size configured by user, and pass dirty_ring_size > when building qemu commandline if dirty ring feature enabled. > > Signed-off-by: Hyman Huang(黄勇) <huangy81(a)chinatelecom.cn&gt; > --- > src/conf/domain_conf.c | 76 ++++++++++++++++++++++++++++++++++++++++- > src/conf/domain_conf.h | 4 +++ > src/qemu/qemu_command.c | 3 ++ > 3 files changed, 82 insertions(+), 1 deletion(-) [...] > @@ -4826,6 +4827,18 @@ virDomainDefPostParseMemtune(virDomainDef *def) > } > > > +static void > +virDomainDefPostParseFeatures(virDomainDef *def) > +{ > + if (def->features[VIR_DOMAIN_FEATURE_KVM] == VIR_TRISTATE_SWITCH_ON && > + def->kvm_features[VIR_DOMAIN_KVM_DIRTY_RING] == VIR_TRISTATE_SWITCH_ON && > + def->dirty_ring_size == 0) { > + /* set 4096 as default size if dirty ring size not congfigured */ Instead of this quite obvious thing I'd prefer a justification of why this setting is done in the global post-parse function rather than the qemu specific one. If there is no good reason to have it globally, please move it to the qemu-specific one.

> + def->dirty_ring_size = 4096; > + } > +} [...] > @@ -17566,8 +17581,10 @@ virDomainFeaturesHyperVDefParse(virDomainDef *def, > > static int > virDomainFeaturesKVMDefParse(virDomainDef *def, > + xmlXPathContextPtr ctxt, > xmlNodePtr node) > { > + xmlNodePtr tmp_node = ctxt->node; We have an established approach for resetting the XPath context. Please use it, as ...[1].

> def->features[VIR_DOMAIN_FEATURE_KVM] = VIR_TRISTATE_SWITCH_ON; > > node = xmlFirstElementChild(node); > @@ -17589,9 +17606,37 @@ virDomainFeaturesKVMDefParse(virDomainDef *def, > > def->kvm_features[feature] = value; > > + /* dirty ring feature should parse size property */ > + if ((virDomainKVM) feature == VIR_DOMAIN_KVM_DIRTY_RING) { > + if (((virDomainKVM) feature) == VIR_DOMAIN_KVM_DIRTY_RING && > + value == VIR_TRISTATE_SWITCH_ON) { > + ctxt->node = node; > + > + if (virXMLPropString(node, "size")) { virXMLPropString returns an allocated string, so this is leaking memory