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>
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>
---
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.
+ if (virXPathUInt("string(./@size)",
ctxt,
+ &def->dirty_ring_size) < 0) {
If you fetch the 'size' attribute via virXMLPropString there's no need
to re-parse it again using XPath. In fact you can avoid dragging xpath
into this function altogether.
+ virReportError(VIR_ERR_XML_ERROR,
"%s",
+ _("invalid number of dirty ring
size"));
+ return -1;
... [1] on this error path it's not fixed.
+ }
+
+ if ((def->dirty_ring_size & (def->dirty_ring_size - 1)) !=
0 ||
We have a function/macro for checking power-of-two.
+ def->dirty_ring_size < 1024 ||
+ def->dirty_ring_size > 65536) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("dirty ring must be power of 2 "
+ "and ranges [1024, 65536]"));
Also per coding style error messages are not to be broken up.
Additionally, is this a qemu-specific range?
+ return -1;
+ }
+ }
+ }
+ }
+
node = xmlNextElementSibling(node);
}
+ ctxt->node = tmp_node;
+
return 0;
}
[...]