On 01/17/2013 06:12 AM, Peter Krempa wrote:

--- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

ACK.

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c4be130..d52cf24 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12354,7 +12354,7 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) goto cleanup;

- if (!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY)) { + if (!metadata_only) { if (!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) && virDomainSnapshotIsExternal(snap)) external++;

-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 01/21/13 11:45, Jiri Denemark wrote:

On Thu, Jan 17, 2013 at 14:12:03 +0100, Peter Krempa wrote:

...

virDomainSnapshotDropParent tried to dereference the parent even in case the snapshot didn't have a parent. This should not be possible as the snapshots use metaroot now, but bugs may induce this failure. --- src/conf/snapshot_conf.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 0c5b005..c3a8494 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -995,6 +995,9 @@ virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot) virDomainSnapshotObjPtr prev = NULL; virDomainSnapshotObjPtr curr = NULL;

+ if (!snapshot || !snapshot->parent) + return; + snapshot->parent->nchildren--; curr = snapshot->parent->first_child; while (curr != snapshot) {

I'm slightly against this patch as I prefer a crashing daemon to silent propagation of bug.

Hm, yeah. I agree. The snapshot metaroot patch that caused this problem expects that all snapshots except the metaroot have parents so this may disclose other potential bugs. Peter

Jirka

On Thu, Jan 17, 2013 at 14:12:04 +0100, Peter Krempa wrote:

When the snapshot definition can't be saved, the qemuDomainSnapshotCreate function succeeded without filling some of the fields in the internal definition.

This patch removes the snapshot and returns failure if the XML file cannot be written. --- src/qemu/qemu_driver.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d52cf24..b264fc8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11593,8 +11593,14 @@ cleanup: if (snapshot && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) { if (qemuDomainSnapshotWriteMetadata(vm, snap, driver->snapshotDir) < 0) { - VIR_WARN("unable to save metadata for snapshot %s", - snap->def->name); + /* if writing of metadata fails, error out rather than trying + * to silently carry on without completing the snapshot */ + virDomainSnapshotFree(snapshot); + snapshot = NULL; + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unable to save metadata for snapshot %s"), + snap->def->name); + virDomainSnapshotObjListRemove(vm->snapshots, snap); } else { if (update_current) vm->current_snapshot = snap;

ACK, I think this approach is better. We will still leave the snapshot data behind (which could be enhanced in the future when libvirt knows how to delete external snapshots) but at least we don't pretend everything was ok and surprise users when such snapshot would disappear after restarting libvirtd. Jirka

On Thu, Jan 17, 2013 at 14:12:05 +0100, Peter Krempa wrote:

The snapshot name is used to create path to the definition save file. When the name contains slashes the creation of the file fails. Reject such names. --- src/qemu/qemu_driver.c | 10 ++++++++++ 1 file changed, 10 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b264fc8..8aae803 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11342,6 +11342,16 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, parse_flags))) goto cleanup;

+ /* reject snapshot names containing slashes as snapshot definitions are + * saved in files containing the name */ + if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA) && + strchr(def->name, '/')) { + virReportError(VIR_ERR_XML_DETAIL, + _("invalid snapshot name '%s': name can't contain '/'"), + def->name); + goto cleanup; + } + /* reject the VIR_DOMAIN_SNAPSHOT_CREATE_LIVE flag where not supported */ if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_LIVE && (!virDomainObjIsActive(vm) ||

ACK we want this one to make the situation in 3/4 less likely to happen. Jirka