Re: [PATCH v3 2/4] qemu: introduce qemuSetSchedulerParametersFlags

[Eric Blake <eblake@xxxxxxxxxx>]

On 05/17/2011 12:20 AM, Hu Tao wrote:
> Support for virDomainSetSchedulerParametersFlags of qemu driver.
> ---
>  src/qemu/qemu_driver.c |   94 ++++++++++++++++++++++++++++++++++++------------
>  1 files changed, 71 insertions(+), 23 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index ccaae66..d3b832d 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4835,7 +4835,7 @@ static int qemuDomainSetMemoryParameters(virDomainPtr dom,
>          goto cleanup;
>      }
>  
> -    if (!virDomainObjIsActive(vm)) {
> +    if (!virDomainObjIsActive(vm) && (flags & VIR_DOMAIN_SCHEDPARAM_LIVE)) {

This hunk doesn't belong.  SCHEDPARAM flags should not affect
MemoryParameters.

> +    bool isActive;
> +
> +    virCheckFlags(VIR_DOMAIN_SCHEDPARAM_LIVE |
> +                  VIR_DOMAIN_SCHEDPARAM_CONFIG |
> +                  VIR_DOMAIN_SCHEDPARAM_CURRENT, -1);

Given that _CURRENT is 0, it is redundant in this check.

> +    if (!isActive && (flags & VIR_DOMAIN_SCHEDPARAM_LIVE)) {
>          qemuReportError(VIR_ERR_OPERATION_INVALID,
>                          "%s", _("domain is not running"));
>          goto cleanup;
>      }
>  

Missing a check that a transient domain can't use _CONFIG.

>  
> -            vm->def->cputune.shares = params[i].value.ul;
> +            if (flags & VIR_DOMAIN_SCHEDPARAM_CONFIG) {
> +                persistentDef = virDomainObjGetPersistentDef(driver->caps, vm);
> +                if (!persistentDef) {
> +                    qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                                    _("can't get persistentDef"));
> +                    goto cleanup;
> +                }
> +                persistentDef->cputune.shares = params[i].value.ul;
> +                rc = virDomainSaveConfig(driver->configDir, persistentDef);

Hmm, this is fetching and saving persistentDef every time through the
loop.  Then again, qemu only goes through the loop at most once, so it's
not wasteful.  But if we ever add another parameter, it would be worth
refactoring things to grab persistentDef before the loop, and call
virDomainSaveConfig only after the loop completes, rather than repeating
it each iteration.

> @@ -5143,9 +5190,8 @@ static int qemuGetSchedulerParameters(virDomainPtr dom,
>      }
>  
>      if (!virDomainObjIsActive(vm)) {
> -        qemuReportError(VIR_ERR_OPERATION_INVALID,
> -                        "%s", _("domain is not running"));
> -        goto cleanup;
> +        val = vm->def->cputune.shares;
> +        goto out;

Hmm, this really argues that we need qemuGetSchedulerParametersFlags as
a counterpart to qemuSetSchedulerParametersFlags.  But that can be a
separate patch.

ACK with this squashed in, so I'm pushing:

diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index d3b832d..fdb3b30 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -4835,7 +4835,7 @@ static int
qemuDomainSetMemoryParameters(virDomainPtr dom,
         goto cleanup;
     }

-    if (!virDomainObjIsActive(vm) && (flags &
VIR_DOMAIN_SCHEDPARAM_LIVE)) {
+    if (!virDomainObjIsActive(vm)) {
         qemuReportError(VIR_ERR_OPERATION_INVALID,
                         "%s", _("domain is not running"));
         goto cleanup;
@@ -5052,8 +5052,7 @@ static int
qemuSetSchedulerParametersFlags(virDomainPtr dom,
     bool isActive;

     virCheckFlags(VIR_DOMAIN_SCHEDPARAM_LIVE |
-                  VIR_DOMAIN_SCHEDPARAM_CONFIG |
-                  VIR_DOMAIN_SCHEDPARAM_CURRENT, -1);
+                  VIR_DOMAIN_SCHEDPARAM_CONFIG, -1);

     qemuDriverLock(driver);

@@ -5074,13 +5073,19 @@ static int
qemuSetSchedulerParametersFlags(virDomainPtr dom,
             flags = VIR_DOMAIN_SCHEDPARAM_CONFIG;
     }

-    if (!isActive && (flags & VIR_DOMAIN_SCHEDPARAM_LIVE)) {
-        qemuReportError(VIR_ERR_OPERATION_INVALID,
-                        "%s", _("domain is not running"));
+    if ((flags & VIR_DOMAIN_MEM_CONFIG) && !vm->persistent) {
+        qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
+                        _("cannot change persistent config of a
transient domain"));
         goto cleanup;
     }

     if (flags & VIR_DOMAIN_SCHEDPARAM_LIVE) {
+        if (!isActive) {
+            qemuReportError(VIR_ERR_OPERATION_INVALID,
+                            "%s", _("domain is not running"));
+            goto cleanup;
+        }
+
         if (!qemuCgroupControllerActive(driver,
VIR_CGROUP_CONTROLLER_CPU)) {
             qemuReportError(VIR_ERR_OPERATION_INVALID,
                             "%s", _("cgroup CPU controller is not
mounted"));
@@ -5088,7 +5093,8 @@ static int
qemuSetSchedulerParametersFlags(virDomainPtr dom,
         }
         if (virCgroupForDomain(driver->cgroup, vm->def->name, &group,
0) != 0) {
             qemuReportError(VIR_ERR_INTERNAL_ERROR,
-                            _("cannot find cgroup for domain %s"),
vm->def->name);
+                            _("cannot find cgroup for domain %s"),
+                            vm->def->name);
             goto cleanup;
         }
     }


-- 
Eric Blake   eblake@xxxxxxxxxx    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list