[libvirt] [PATCHv2 0/2] Couple of NVRAM fixes
diff to v1: -patch 1/2 (2/2 in previous version) updated according to Laszlo's review Michal Privoznik (2): virDomainUndefineFlags: Allow NVRAM unlinking formatdomain: Update <loader/> example to match the rest docs/formatdomain.html.in | 2 +- include/libvirt/libvirt.h.in | 2 ++ src/qemu/qemu_driver.c | 20 +++++++++++++++++++- tools/virsh-domain.c | 20 ++++++++++++++++---- tools/virsh.pod | 6 +++++- 5 files changed, 43 insertions(+), 7 deletions(-) -- 1.8.5.5
When a domain is undefined, there are options to remove it's managed save state or snapshots. However, there's another file that libvirt creates per domain: the NVRAM variable store file. Make sure that the file is not left behind if the domain is undefined. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- include/libvirt/libvirt.h.in | 2 ++ src/qemu/qemu_driver.c | 20 +++++++++++++++++++- tools/virsh-domain.c | 20 ++++++++++++++++---- tools/virsh.pod | 6 +++++- 4 files changed, 42 insertions(+), 6 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 94b942c..3c2a51a 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2257,6 +2257,8 @@ typedef enum { VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA = (1 << 1), /* If last use of domain, then also remove any snapshot metadata */ + VIR_DOMAIN_UNDEFINE_NVRAM = (1 << 2), /* Also remove any + nvram file */ /* Future undefine control flags should come here. */ } virDomainUndefineFlagsValues; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 917b286..1807715 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6408,7 +6408,8 @@ qemuDomainUndefineFlags(virDomainPtr dom, virQEMUDriverConfigPtr cfg = NULL; virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_SAVE | - VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA, -1); + VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA | + VIR_DOMAIN_UNDEFINE_NVRAM, -1); if (!(vm = qemuDomObjFromDomain(dom))) return -1; @@ -6457,6 +6458,23 @@ qemuDomainUndefineFlags(virDomainPtr dom, } } + if (!virDomainObjIsActive(vm) && + vm->def->os.loader && vm->def->os.loader->nvram && + virFileExists(vm->def->os.loader->nvram)) { + if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot delete inactive domain with nvram")); + goto cleanup; + } + + if (unlink(vm->def->os.loader->nvram) < 0) { + virReportSystemError(errno, + _("failed to remove nvram: %s"), + vm->def->os.loader->nvram); + goto cleanup; + } + } + if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0) goto cleanup; diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 30b3fa9..ecd5283 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -3248,6 +3248,10 @@ static const vshCmdOptDef opts_undefine[] = { .type = VSH_OT_BOOL, .help = N_("remove all domain snapshot metadata, if inactive") }, + {.name = "nvram", + .type = VSH_OT_BOOL, + .help = N_("remove nvram file, if inactive") + }, {.name = NULL} }; @@ -3270,6 +3274,7 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) bool snapshots_metadata = vshCommandOptBool(cmd, "snapshots-metadata"); bool wipe_storage = vshCommandOptBool(cmd, "wipe-storage"); bool remove_all_storage = vshCommandOptBool(cmd, "remove-all-storage"); + bool nvram = vshCommandOptBool(cmd, "nvram"); /* Positive if these items exist. */ int has_managed_save = 0; int has_snapshots_metadata = 0; @@ -3313,6 +3318,9 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA; snapshots_safe = true; } + if (nvram) { + flags |= VIR_DOMAIN_UNDEFINE_NVRAM; + } if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) return false; @@ -3503,11 +3511,15 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) * VIR_DOMAIN_UNDEFINE_MANAGED_SAVE in 0.9.4, the * VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA flag was not present * until 0.9.5; skip to piecewise emulation if we couldn't prove - * above that the new API is safe. */ - if (managed_save_safe && snapshots_safe) { + * above that the new API is safe. + * Moreover, only the newer UndefineFlags() API understands + * the VIR_DOMAIN_UNDEFINE_NVRAM flag. So if user has + * specified --nvram we must use the Flags() API. */ + if ((managed_save_safe && snapshots_safe) || nvram) { rc = virDomainUndefineFlags(dom, flags); - if (rc == 0 || (last_error->code != VIR_ERR_NO_SUPPORT && - last_error->code != VIR_ERR_INVALID_ARG)) + if (rc == 0 || nvram || + (last_error->code != VIR_ERR_NO_SUPPORT && + last_error->code != VIR_ERR_INVALID_ARG)) goto out; vshResetLibvirtError(); } diff --git a/tools/virsh.pod b/tools/virsh.pod index 60ee515..5d4b12b 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2083,7 +2083,7 @@ Output the device used for the TTY console of the domain. If the information is not available the processes will provide an exit code of 1. =item B<undefine> I<domain> [I<--managed-save>] [I<--snapshots-metadata>] -[ {I<--storage> B<volumes> | I<--remove-all-storage>} I<--wipe-storage>] +[I<--nvram>] [ {I<--storage> B<volumes> | I<--remove-all-storage>} I<--wipe-storage>] Undefine a domain. If the domain is running, this converts it to a transient domain, without stopping it. If the domain is inactive, @@ -2099,6 +2099,10 @@ domain. Without the flag, attempts to undefine an inactive domain with snapshot metadata will fail. If the domain is active, this flag is ignored. +The I<--nvram> flag ensures no nvram (/domain/os/nvram/) file is +left behind. If the domain has an nvram file and the flag is +omitted, the undefine will fail. + The I<--storage> flag takes a parameter B<volumes>, which is a comma separated list of volume target names or source paths of storage volumes to be removed along with the undefined domain. Volumes can be undefined and thus removed only -- 1.8.5.5
On 09/12/14 13:33, Michal Privoznik wrote:
When a domain is undefined, there are options to remove it's managed save state or snapshots. However, there's another file that libvirt creates per domain: the NVRAM variable store file. Make sure that the file is not left behind if the domain is undefined.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- include/libvirt/libvirt.h.in | 2 ++ src/qemu/qemu_driver.c | 20 +++++++++++++++++++- tools/virsh-domain.c | 20 ++++++++++++++++---- tools/virsh.pod | 6 +++++- 4 files changed, 42 insertions(+), 6 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 94b942c..3c2a51a 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2257,6 +2257,8 @@ typedef enum { VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA = (1 << 1), /* If last use of domain, then also remove any snapshot metadata */ + VIR_DOMAIN_UNDEFINE_NVRAM = (1 << 2), /* Also remove any + nvram file */
/* Future undefine control flags should come here. */ } virDomainUndefineFlagsValues; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 917b286..1807715 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6408,7 +6408,8 @@ qemuDomainUndefineFlags(virDomainPtr dom, virQEMUDriverConfigPtr cfg = NULL;
virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_SAVE | - VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA, -1); + VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA | + VIR_DOMAIN_UNDEFINE_NVRAM, -1);
if (!(vm = qemuDomObjFromDomain(dom))) return -1; @@ -6457,6 +6458,23 @@ qemuDomainUndefineFlags(virDomainPtr dom, } }
+ if (!virDomainObjIsActive(vm) && + vm->def->os.loader && vm->def->os.loader->nvram && + virFileExists(vm->def->os.loader->nvram)) { + if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot delete inactive domain with nvram")); + goto cleanup; + } + + if (unlink(vm->def->os.loader->nvram) < 0) { + virReportSystemError(errno, + _("failed to remove nvram: %s"), + vm->def->os.loader->nvram); + goto cleanup; + } + } + if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0) goto cleanup;
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 30b3fa9..ecd5283 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -3248,6 +3248,10 @@ static const vshCmdOptDef opts_undefine[] = { .type = VSH_OT_BOOL, .help = N_("remove all domain snapshot metadata, if inactive") }, + {.name = "nvram", + .type = VSH_OT_BOOL, + .help = N_("remove nvram file, if inactive") + }, {.name = NULL} };
@@ -3270,6 +3274,7 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) bool snapshots_metadata = vshCommandOptBool(cmd, "snapshots-metadata"); bool wipe_storage = vshCommandOptBool(cmd, "wipe-storage"); bool remove_all_storage = vshCommandOptBool(cmd, "remove-all-storage"); + bool nvram = vshCommandOptBool(cmd, "nvram"); /* Positive if these items exist. */ int has_managed_save = 0; int has_snapshots_metadata = 0; @@ -3313,6 +3318,9 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA; snapshots_safe = true; } + if (nvram) { + flags |= VIR_DOMAIN_UNDEFINE_NVRAM; + }
if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) return false; @@ -3503,11 +3511,15 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) * VIR_DOMAIN_UNDEFINE_MANAGED_SAVE in 0.9.4, the * VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA flag was not present * until 0.9.5; skip to piecewise emulation if we couldn't prove - * above that the new API is safe. */ - if (managed_save_safe && snapshots_safe) { + * above that the new API is safe. + * Moreover, only the newer UndefineFlags() API understands + * the VIR_DOMAIN_UNDEFINE_NVRAM flag. So if user has + * specified --nvram we must use the Flags() API. */ + if ((managed_save_safe && snapshots_safe) || nvram) { rc = virDomainUndefineFlags(dom, flags); - if (rc == 0 || (last_error->code != VIR_ERR_NO_SUPPORT && - last_error->code != VIR_ERR_INVALID_ARG)) + if (rc == 0 || nvram || + (last_error->code != VIR_ERR_NO_SUPPORT && + last_error->code != VIR_ERR_INVALID_ARG)) goto out; vshResetLibvirtError(); } diff --git a/tools/virsh.pod b/tools/virsh.pod index 60ee515..5d4b12b 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2083,7 +2083,7 @@ Output the device used for the TTY console of the domain. If the information is not available the processes will provide an exit code of 1.
=item B<undefine> I<domain> [I<--managed-save>] [I<--snapshots-metadata>] -[ {I<--storage> B<volumes> | I<--remove-all-storage>} I<--wipe-storage>] +[I<--nvram>] [ {I<--storage> B<volumes> | I<--remove-all-storage>} I<--wipe-storage>]
Undefine a domain. If the domain is running, this converts it to a transient domain, without stopping it. If the domain is inactive, @@ -2099,6 +2099,10 @@ domain. Without the flag, attempts to undefine an inactive domain with snapshot metadata will fail. If the domain is active, this flag is ignored.
+The I<--nvram> flag ensures no nvram (/domain/os/nvram/) file is +left behind. If the domain has an nvram file and the flag is +omitted, the undefine will fail. + The I<--storage> flag takes a parameter B<volumes>, which is a comma separated list of volume target names or source paths of storage volumes to be removed along with the undefined domain. Volumes can be undefined and thus removed only
Acked-by: Laszlo Ersek <lersek@redhat.com> Thanks! Laszlo
At the beginning when I was inventing <loader/> attributes and <nvram/> I've introduced this @readonly attribute to the loader element. It accepted values 'on' and 'off'. However, later, during the review process, that has changed to 'yes' and 'no', but the example XML snippet wasn't updated, so while the description is correct, the example isn't. Reported-by: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.html.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index a2ea758..5081be3 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -102,7 +102,7 @@ ... <os> <type>hvm</type> - <loader readonly='on' type='rom'>/usr/lib/xen/boot/hvmloader</loader> + <loader readonly='yes' type='rom'>/usr/lib/xen/boot/hvmloader</loader> <nvram template='/usr/share/OVMF/OVMF_VARS.fd'>/var/lib/libvirt/nvram/guest_VARS.fd</nvram> <boot dev='hd'/> <boot dev='cdrom'/> -- 1.8.5.5
On 09/12/14 13:33, Michal Privoznik wrote:
At the beginning when I was inventing <loader/> attributes and <nvram/> I've introduced this @readonly attribute to the loader element. It accepted values 'on' and 'off'. However, later, during the review process, that has changed to 'yes' and 'no', but the example XML snippet wasn't updated, so while the description is correct, the example isn't.
Reported-by: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.html.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index a2ea758..5081be3 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -102,7 +102,7 @@ ... <os> <type>hvm</type> - <loader readonly='on' type='rom'>/usr/lib/xen/boot/hvmloader</loader> + <loader readonly='yes' type='rom'>/usr/lib/xen/boot/hvmloader</loader> <nvram template='/usr/share/OVMF/OVMF_VARS.fd'>/var/lib/libvirt/nvram/guest_VARS.fd</nvram> <boot dev='hd'/> <boot dev='cdrom'/>
Acked-by: Laszlo Ersek <lersek@redhat.com>
On 12.09.2014 13:33, Michal Privoznik wrote:
diff to v1: -patch 1/2 (2/2 in previous version) updated according to Laszlo's review
Michal Privoznik (2): virDomainUndefineFlags: Allow NVRAM unlinking formatdomain: Update <loader/> example to match the rest
docs/formatdomain.html.in | 2 +- include/libvirt/libvirt.h.in | 2 ++ src/qemu/qemu_driver.c | 20 +++++++++++++++++++- tools/virsh-domain.c | 20 ++++++++++++++++---- tools/virsh.pod | 6 +++++- 5 files changed, 43 insertions(+), 7 deletions(-)
I've pushed this. Thanks Laszlo. Michal
participants (2)
-
Laszlo Ersek -
Michal Privoznik