Although commit e3497f3f noted that the LIVE option doesn't
matter and removed the call to virDomainDefCompatibleDevice,
it didn't go quite far enough and change the order of the checks.

Since we don't have the possibility of LIVE succeeding and thus
the need for a delayed update in order to write the CONFIG change
let's just merge the saving of the config into one if statement.

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/lxc/lxc_driver.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index bde0ff6ad4..6afb36765a 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -4862,6 +4862,12 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom,
             goto endjob;
     }
 
+    if (flags & VIR_DOMAIN_AFFECT_LIVE) {
+        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+                       _("Unable to modify live devices"));
+        goto endjob;
+    }
+
     if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
         /* Make a copy for updated domain. */
         vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt);
@@ -4872,17 +4878,7 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom,
          * device we're going to update. */
         if ((ret = lxcDomainUpdateDeviceConfig(vmdef, dev)) < 0)
             goto endjob;
-    }
-
-    if (flags & VIR_DOMAIN_AFFECT_LIVE) {
-        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
-                       _("Unable to modify live devices"));
-
-        goto endjob;
-    }
 
-    /* Finally, if no error until here, we can save config. */
-    if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
         ret = virDomainSaveConfig(cfg->configDir, driver->caps, vmdef);
         if (!ret) {
             virDomainObjAssignDef(vm, vmdef, false, NULL);
-- 
2.14.4

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

On 06/26/2018 08:27 PM, John Ferlan wrote:
> Although commit e3497f3f noted that the LIVE option doesn't
> matter and removed the call to virDomainDefCompatibleDevice,
> it didn't go quite far enough and change the order of the checks.
> 
> Since we don't have the possibility of LIVE succeeding and thus
> the need for a delayed update in order to write the CONFIG change
> let's just merge the saving of the config into one if statement.
> 
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
>  src/lxc/lxc_driver.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index bde0ff6ad4..6afb36765a 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -4862,6 +4862,12 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom,
>              goto endjob;
>      }
>  
> +    if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +                       _("Unable to modify live devices"));
> +        goto endjob;
> +    }
> +
>      if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
>          /* Make a copy for updated domain. */
>          vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt);
> @@ -4872,17 +4878,7 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom,
>           * device we're going to update. */
>          if ((ret = lxcDomainUpdateDeviceConfig(vmdef, dev)) < 0)
>              goto endjob;
> -    }
> -
> -    if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> -        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> -                       _("Unable to modify live devices"));
> -
> -        goto endjob;
> -    }
>  
> -    /* Finally, if no error until here, we can save config. */
> -    if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
>          ret = virDomainSaveConfig(cfg->configDir, driver->caps, vmdef);
>          if (!ret) {
>              virDomainObjAssignDef(vm, vmdef, false, NULL);
> 

Well, in this case, @dev_copy is useless variable, and I guess the
@flags check should be moved right after
virDomainObjUpdateModificationImpact() call which simplifies the whole
function a bit.

Michal

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