On Thu, Oct 17, 2024 at 05:02:00PM +0200, Peter Krempa wrote: > Normally when starting up a VM with a transient disk, if the file > libvirt would use as the temp overlay for the original disk image exists > libvirt will not touch it and fail startup. This is done in order to > avoid any potential removal of user files if they manage to create a > colliding filename. > > In case when the user doesn't want the above behaviour this patch' > introduces a 'overwriteTemp' attribute for the '<transient/>' element > which allows users to opt into simply removing the file before the next > start. > > Closes: https://gitlab.com/libvirt/libvirt/-/issues/684 > Signed-off-by: Peter Krempa <pkrempa@xxxxxxxxxx> > --- > docs/formatdomain.rst | 7 +++++++ > src/conf/domain_conf.c | 7 +++++++ > src/conf/domain_conf.h | 1 + > src/conf/schemas/domaincommon.rng | 5 +++++ > src/qemu/qemu_snapshot.c | 17 +++++++++++++---- > .../disk-transient.x86_64-latest.xml | 2 +- > tests/qemuxmlconfdata/disk-transient.xml | 2 +- > 7 files changed, 35 insertions(+), 6 deletions(-) > > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst > index e6f09a728f..ab2fccdd97 100644 > --- a/docs/formatdomain.rst > +++ b/docs/formatdomain.rst > @@ -3479,6 +3479,13 @@ paravirtualized driver is specified via the ``disk`` element. > ``shareBacking`` attribute should be set to ``yes``. Note that hypervisor > drivers may need to hotplug such disk and thus it works only with > configurations supporting hotplug. :since:`Since 7.4.0` > + > + Hypervisors may need to store a temporary file containing the data written > + by the domain while running, which may be stored in the same location > + as the original source of the disk. Note that in some cases the temporary > + file can't be cleaned up (e.g. when the domain is not shutdown before the host). > + An optional attribute ``overwriteTemp`` set to ``yes`` (:since:`Since 10.10`) > + indicates that the hypervisor may overwrite the file rather than fail startup. Is this really something we want to express in the guest XML ? The times at which you want to overwrite an existing file look to be tailored to the specific failure scenario hit. Users never expect to see a failure at first. If they don't add this attribute, and hit the failure, they must go back & edit the guest to add the attr, start the guest, and then possibly edit it again to remove the attr. If they add the attr unconditionally before seeing any failure, they'll never have an protection against mistakes. It feels like the same kind of situation that motivated flags VIR_DOMAIN_START_FORCE_BOOT and VIR_DOMAIN_START_RESET_NVRAM. IOW, suggesting we need VIR_DOMAIN_START_REPLACE_TRANSIENT ? > ``serial`` > If present, this specify serial number of virtual hard drive. For example, it > may look like ``<serial>WD-WMAP9A966149</serial>``. Not supported for > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 6d7dee7956..359591d8f7 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -8240,6 +8240,11 @@ virDomainDiskDefParseXML(virDomainXMLOption *xmlopt, > VIR_XML_PROP_NONE, > &def->transientShareBacking) < 0) > return NULL; > + > + if (virXMLPropTristateBool(transientNode, "overwriteTemp", > + VIR_XML_PROP_NONE, > + &def->transientOverwriteTemp) < 0) > + return NULL; > } > > if (virDomainDiskDefIotuneParse(def, ctxt) < 0) > @@ -23233,6 +23238,8 @@ virDomainDiskDefFormat(virBuffer *buf, > virBufferAddLit(&childBuf, "<transient"); > if (def->transientShareBacking == VIR_TRISTATE_BOOL_YES) > virBufferAddLit(&childBuf, " shareBacking='yes'"); > + if (def->transientOverwriteTemp == VIR_TRISTATE_BOOL_YES) > + virBufferAddLit(&childBuf, " overwriteTemp='yes'"); > virBufferAddLit(&childBuf, "/>\n"); > } > virBufferEscapeString(&childBuf, "<serial>%s</serial>\n", def->serial); > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index a15af4fae3..169c626584 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -569,6 +569,7 @@ struct _virDomainDiskDef { > virDomainStartupPolicy startupPolicy; > bool transient; > virTristateBool transientShareBacking; > + virTristateBool transientOverwriteTemp; > virDomainDeviceInfo info; > virTristateBool rawio; > virDomainDeviceSGIO sgio; > diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng > index efb5f00d77..cd87e83410 100644 > --- a/src/conf/schemas/domaincommon.rng > +++ b/src/conf/schemas/domaincommon.rng > @@ -1618,6 +1618,11 @@ > <ref name='virYesNo'/> > </attribute> > </optional> > + <optional> > + <attribute name="overwriteTemp"> > + <ref name='virYesNo'/> > + </attribute> > + </optional> > <empty/> > </element> > </optional> > diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c > index 1187ebf276..b8ca045900 100644 > --- a/src/qemu/qemu_snapshot.c > +++ b/src/qemu/qemu_snapshot.c > @@ -1287,10 +1287,19 @@ qemuSnapshotGetTransientDiskDef(virDomainDiskDef *domdisk, > domdisk->src->path, suffix); > > if (virFileExists(snapdisk->src->path)) { > - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, > - _("Overlay file '%1$s' for transient disk '%2$s' already exists"), > - snapdisk->src->path, domdisk->dst); > - return NULL; > + if (domdisk->transientOverwriteTemp == VIR_TRISTATE_BOOL_YES) { > + if (unlink(snapdisk->src->path) != 0) { > + virReportSystemError(errno, > + _("Failed to delete overlay file '%1$s' for transient disk '%2$s'"), > + snapdisk->src->path, domdisk->dst); > + return NULL; > + } > + } else { > + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, > + _("Overlay file '%1$s' for transient disk '%2$s' already exists"), > + snapdisk->src->path, domdisk->dst); > + return NULL; > + } > } > > return g_steal_pointer(&snapdisk); > diff --git a/tests/qemuxmlconfdata/disk-transient.x86_64-latest.xml b/tests/qemuxmlconfdata/disk-transient.x86_64-latest.xml > index aab5894447..5f8f15f714 100644 > --- a/tests/qemuxmlconfdata/disk-transient.x86_64-latest.xml > +++ b/tests/qemuxmlconfdata/disk-transient.x86_64-latest.xml > @@ -21,7 +21,7 @@ > <driver name='qemu' type='qcow2' cache='none'/> > <source file='/tmp/QEMUGuest2.img'/> > <target dev='vda' bus='virtio'/> > - <transient shareBacking='yes'/> > + <transient shareBacking='yes' overwriteTemp='yes'/> > <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> > </disk> > <disk type='file' device='disk'> > diff --git a/tests/qemuxmlconfdata/disk-transient.xml b/tests/qemuxmlconfdata/disk-transient.xml > index edd65a0da0..722f1f9a92 100644 > --- a/tests/qemuxmlconfdata/disk-transient.xml > +++ b/tests/qemuxmlconfdata/disk-transient.xml > @@ -18,7 +18,7 @@ > <driver name='qemu' type='qcow2' cache='none'/> > <source file='/tmp/QEMUGuest2.img'/> > <target dev='vda' bus='virtio'/> > - <transient shareBacking='yes'/> > + <transient shareBacking='yes' overwriteTemp='yes'/> > </disk> > <disk type='file' device='disk'> > <driver name='qemu' type='qcow2' cache='none'/> > -- > 2.47.0 > With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
