[libvirt] [PATCH v3] qemu: fix broken autostart symlink after renaming domain.

Julio Faracco posted 1 patch 5 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20180717035240.17920-1-jcfaracco@gmail.com
Test syntax-check passed
There is a newer version of this series
src/qemu/qemu_driver.c | 42 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)
[libvirt] [PATCH v3] qemu: fix broken autostart symlink after renaming domain.
Posted by Julio Faracco 5 years, 9 months ago
If a domain is configured to start on boot, it has a symlink to the
domain definition inside the autostart directory. If you rename this
domain, the definition is renamed too. The symlink need to be pointed to
this renamed file. This commit recreates the symlink after renaming the
XML file.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1594985

Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
---
 src/qemu/qemu_driver.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8fae46370e..6bbea324b9 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20914,6 +20914,8 @@ qemuDomainRenameCallback(virDomainObjPtr vm,
     char *old_dom_name = NULL;
     char *new_dom_cfg_file = NULL;
     char *old_dom_cfg_file = NULL;
+    char *new_dom_autostart_link = NULL;
+    char *old_dom_autostart_link = NULL;
 
     virCheckFlags(0, ret);
 
@@ -20934,6 +20936,22 @@ qemuDomainRenameCallback(virDomainObjPtr vm,
                                                  vm->def->name)))
         goto cleanup;
 
+    if (vm->autostart) {
+        if (!(new_dom_autostart_link = virDomainConfigFile(cfg->autostartDir,
+                                                          new_dom_name)) ||
+            !(old_dom_autostart_link = virDomainConfigFile(cfg->autostartDir,
+                                                          vm->def->name)))
+            goto cleanup;
+
+        if (virFileIsLink(old_dom_autostart_link) &&
+            unlink(old_dom_autostart_link) < 0) {
+            virReportSystemError(errno,
+                                 _("Failed to delete symlink '%s'"),
+                                 old_dom_autostart_link);
+            goto cleanup;
+        }
+    }
+
     event_old = virDomainEventLifecycleNewFromObj(vm,
                                             VIR_DOMAIN_EVENT_UNDEFINED,
                                             VIR_DOMAIN_EVENT_UNDEFINED_RENAMED);
@@ -20946,6 +20964,16 @@ qemuDomainRenameCallback(virDomainObjPtr vm,
     if (virDomainSaveConfig(cfg->configDir, driver->caps, vm->def) < 0)
         goto rollback;
 
+
+    if (vm->autostart) {
+        if (symlink(new_dom_cfg_file, new_dom_autostart_link) < 0) {
+            virReportSystemError(errno,
+                                 _("Failed to create symlink '%s to '%s'"),
+                                 new_dom_autostart_link, new_dom_cfg_file);
+            goto rollback;
+        }
+    }
+
     if (virFileExists(old_dom_cfg_file) &&
         unlink(old_dom_cfg_file) < 0) {
         virReportSystemError(errno,
@@ -20960,6 +20988,8 @@ qemuDomainRenameCallback(virDomainObjPtr vm,
     ret = 0;
 
  cleanup:
+    VIR_FREE(old_dom_autostart_link);
+    VIR_FREE(new_dom_autostart_link);
     VIR_FREE(old_dom_cfg_file);
     VIR_FREE(new_dom_cfg_file);
     VIR_FREE(old_dom_name);
@@ -20979,6 +21009,18 @@ qemuDomainRenameCallback(virDomainObjPtr vm,
     if (virFileExists(new_dom_cfg_file))
         unlink(new_dom_cfg_file);
 
+    if (vm->autostart) {
+        if (virFileExists(new_dom_autostart_link))
+            unlink(new_dom_autostart_link);
+
+        if (!virFileExists(old_dom_autostart_link) &&
+            symlink(old_dom_cfg_file, old_dom_autostart_link) < 0) {
+            virReportSystemError(errno,
+                                 _("Failed to create symlink '%s to '%s'"),
+                                 old_dom_autostart_link, old_dom_cfg_file);
+        }
+    }
+
     goto cleanup;
 }
 
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] qemu: fix broken autostart symlink after renaming domain.
Posted by Julio Faracco 5 years, 9 months ago
Changelog:
V1: The patch was automatically rejected due to some tabs
automatically added by my VIM editor.
V2: Tabs changed by spaces.
V3: Rebasing the patch considering the Erik's suggestions.

--
Julio Cesar Faracco
Em ter, 17 de jul de 2018 às 00:52, Julio Faracco
<jcfaracco@gmail.com> escreveu:
>
> If a domain is configured to start on boot, it has a symlink to the
> domain definition inside the autostart directory. If you rename this
> domain, the definition is renamed too. The symlink need to be pointed to
> this renamed file. This commit recreates the symlink after renaming the
> XML file.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1594985
>
> Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
> ---
>  src/qemu/qemu_driver.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 8fae46370e..6bbea324b9 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -20914,6 +20914,8 @@ qemuDomainRenameCallback(virDomainObjPtr vm,
>      char *old_dom_name = NULL;
>      char *new_dom_cfg_file = NULL;
>      char *old_dom_cfg_file = NULL;
> +    char *new_dom_autostart_link = NULL;
> +    char *old_dom_autostart_link = NULL;
>
>      virCheckFlags(0, ret);
>
> @@ -20934,6 +20936,22 @@ qemuDomainRenameCallback(virDomainObjPtr vm,
>                                                   vm->def->name)))
>          goto cleanup;
>
> +    if (vm->autostart) {
> +        if (!(new_dom_autostart_link = virDomainConfigFile(cfg->autostartDir,
> +                                                          new_dom_name)) ||
> +            !(old_dom_autostart_link = virDomainConfigFile(cfg->autostartDir,
> +                                                          vm->def->name)))
> +            goto cleanup;
> +
> +        if (virFileIsLink(old_dom_autostart_link) &&
> +            unlink(old_dom_autostart_link) < 0) {
> +            virReportSystemError(errno,
> +                                 _("Failed to delete symlink '%s'"),
> +                                 old_dom_autostart_link);
> +            goto cleanup;
> +        }
> +    }
> +
>      event_old = virDomainEventLifecycleNewFromObj(vm,
>                                              VIR_DOMAIN_EVENT_UNDEFINED,
>                                              VIR_DOMAIN_EVENT_UNDEFINED_RENAMED);
> @@ -20946,6 +20964,16 @@ qemuDomainRenameCallback(virDomainObjPtr vm,
>      if (virDomainSaveConfig(cfg->configDir, driver->caps, vm->def) < 0)
>          goto rollback;
>
> +
> +    if (vm->autostart) {
> +        if (symlink(new_dom_cfg_file, new_dom_autostart_link) < 0) {
> +            virReportSystemError(errno,
> +                                 _("Failed to create symlink '%s to '%s'"),
> +                                 new_dom_autostart_link, new_dom_cfg_file);
> +            goto rollback;
> +        }
> +    }
> +
>      if (virFileExists(old_dom_cfg_file) &&
>          unlink(old_dom_cfg_file) < 0) {
>          virReportSystemError(errno,
> @@ -20960,6 +20988,8 @@ qemuDomainRenameCallback(virDomainObjPtr vm,
>      ret = 0;
>
>   cleanup:
> +    VIR_FREE(old_dom_autostart_link);
> +    VIR_FREE(new_dom_autostart_link);
>      VIR_FREE(old_dom_cfg_file);
>      VIR_FREE(new_dom_cfg_file);
>      VIR_FREE(old_dom_name);
> @@ -20979,6 +21009,18 @@ qemuDomainRenameCallback(virDomainObjPtr vm,
>      if (virFileExists(new_dom_cfg_file))
>          unlink(new_dom_cfg_file);
>
> +    if (vm->autostart) {
> +        if (virFileExists(new_dom_autostart_link))
> +            unlink(new_dom_autostart_link);
> +
> +        if (!virFileExists(old_dom_autostart_link) &&
> +            symlink(old_dom_cfg_file, old_dom_autostart_link) < 0) {
> +            virReportSystemError(errno,
> +                                 _("Failed to create symlink '%s to '%s'"),
> +                                 old_dom_autostart_link, old_dom_cfg_file);
> +        }
> +    }
> +
>      goto cleanup;
>  }
>
> --
> 2.17.1
>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] qemu: fix broken autostart symlink after renaming domain.
Posted by Erik Skultety 5 years, 9 months ago
On Tue, Jul 17, 2018 at 12:52:40AM -0300, Julio Faracco wrote:
> If a domain is configured to start on boot, it has a symlink to the
> domain definition inside the autostart directory. If you rename this
> domain, the definition is renamed too. The symlink need to be pointed to
> this renamed file. This commit recreates the symlink after renaming the
> XML file.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1594985
>
> Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
> ---
>  src/qemu/qemu_driver.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 8fae46370e..6bbea324b9 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -20914,6 +20914,8 @@ qemuDomainRenameCallback(virDomainObjPtr vm,
>      char *old_dom_name = NULL;
>      char *new_dom_cfg_file = NULL;
>      char *old_dom_cfg_file = NULL;
> +    char *new_dom_autostart_link = NULL;
> +    char *old_dom_autostart_link = NULL;
>
>      virCheckFlags(0, ret);
>
> @@ -20934,6 +20936,22 @@ qemuDomainRenameCallback(virDomainObjPtr vm,
>                                                   vm->def->name)))
>          goto cleanup;
>
> +    if (vm->autostart) {
> +        if (!(new_dom_autostart_link = virDomainConfigFile(cfg->autostartDir,
> +                                                          new_dom_name)) ||
> +            !(old_dom_autostart_link = virDomainConfigFile(cfg->autostartDir,
> +                                                          vm->def->name)))
> +            goto cleanup;
> +
> +        if (virFileIsLink(old_dom_autostart_link) &&
> +            unlink(old_dom_autostart_link) < 0) {
> +            virReportSystemError(errno,
> +                                 _("Failed to delete symlink '%s'"),
> +                                 old_dom_autostart_link);
> +            goto cleanup;
> +        }


The patch can be further optimized...^this unlink could be done at very last
moment after the new config has been defined (if you put it right below the
old config unlink).

> +    }
> +
>      event_old = virDomainEventLifecycleNewFromObj(vm,
>                                              VIR_DOMAIN_EVENT_UNDEFINED,
>                                              VIR_DOMAIN_EVENT_UNDEFINED_RENAMED);
> @@ -20946,6 +20964,16 @@ qemuDomainRenameCallback(virDomainObjPtr vm,
>      if (virDomainSaveConfig(cfg->configDir, driver->caps, vm->def) < 0)
>          goto rollback;
>
> +
> +    if (vm->autostart) {
> +        if (symlink(new_dom_cfg_file, new_dom_autostart_link) < 0) {
> +            virReportSystemError(errno,
> +                                 _("Failed to create symlink '%s to '%s'"),
> +                                 new_dom_autostart_link, new_dom_cfg_file);
> +            goto rollback;
> +        }
> +    }

...you would then put ^this hunk in the if block above...

...

> +        if (!virFileExists(old_dom_autostart_link) &&
> +            symlink(old_dom_cfg_file, old_dom_autostart_link) < 0) {
> +            virReportSystemError(errno,
> +                                 _("Failed to create symlink '%s to '%s'"),
> +                                 old_dom_autostart_link, old_dom_cfg_file);
> +        }
> +    }
> +

And ^this bit could go away completely. The thing is that usually, you prepare
everything ahead and then perform the switch in 1 go to minimize all the places
where it can go wrong...same here, you create the new symlink and config and
then just simply unlink both the old config and the old symlink - if that
fails, you go to rollback where you just simply unlink all the new data that
has been created but to which we couldn't successfully switch. Patch looks fine
otherwise, so v4 should be pretty straightforward to get in.

Thanks,
Erik

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