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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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;
>  }

[...]




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux