On Sat, Nov 20, 2021 at 03:20:47 -0500, huangy81@xxxxxxxxxxxxxxx wrote:
> From: Hyman Huang(黄勇) <huangy81@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
> ---
> 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;
> }
[...]
