On Thu, Apr 07, 2011 at 10:31:52AM +0800, Osier Yang wrote: > 1) Both "qemuDomainStartWithFlags" and "qemuAutostartDomain" try to > restore the domain from managedsave'ed image if it exists (by > invoking "qemuDomainObjRestore"), but it unlinks the image even > if restoring fails, which causes data loss. (This problem exists > for "virsh managedsave dom; virsh start dom"). > > The fix for is to unlink the managed state file only if restoring > succeeded. > > 2) For "virsh save dom; virsh restore dom;", it can cause data > corruption if one reuse the saved state file for restoring. Add > doc to tell user about it. > > 3) In "qemuDomainObjStart", if "managed_save" is NULL, we shouldn't > fallback to start the domain, skipping it to cleanup as a incidental > fix. Discovered by Eric. > > --- > src/qemu/qemu_driver.c | 12 +++++++----- > tools/virsh.pod | 6 +++++- > 2 files changed, 12 insertions(+), 6 deletions(-) > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 48fe266..a84780b 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -3423,18 +3423,20 @@ static int qemudDomainObjStart(virConnectPtr conn, > > /* > * If there is a managed saved state restore it instead of starting > - * from scratch. In any case the old state is removed. > + * from scratch. The old state is removed once the restoring succeeded. > */ > managed_save = qemuDomainManagedSavePath(driver, vm); > + > + if (!managed_save) > + goto cleanup; > + > if ((managed_save)&& (virFileExists(managed_save))) { > ret = qemuDomainObjRestore(conn, driver, vm, managed_save); > > - if (unlink(managed_save)< 0) { > + if ((ret == 0)&& (unlink(managed_save)< 0)) > VIR_WARN("Failed to remove the managed state %s", managed_save); > - } > > - if (ret == 0) > - goto cleanup; > + goto cleanup; > } > > ret = qemuProcessStart(conn, driver, vm, NULL, start_paused, -1, NULL, > diff --git a/tools/virsh.pod b/tools/virsh.pod > index f4bd294..6319373 100644 > --- a/tools/virsh.pod > +++ b/tools/virsh.pod > @@ -546,7 +546,11 @@ I<on_reboot> parameter in the domain's XML definition. > > =item B<restore> I<state-file> > > -Restores a domain from an B<virsh save> state file. See I<save> for more info. > +Restores a domain from an B<virsh save> state file. See I<save> for more info. > + > +B<Note>: To avoid corrupting file system contents within the domain, you > +should not reuse the saved state file to B<restore> unless you are convinced > +with reverting the domain to the previous state. > > =item B<save> I<domain-id> I<state-file> ACK, thanks ! Daniel

On 04/06/2011 08:31 PM, Osier Yang wrote: > managed_save = qemuDomainManagedSavePath(driver, vm); > + > + if (!managed_save) > + goto cleanup; > + > if ((managed_save)&& (virFileExists(managed_save))) { This second check for non-NULL managed_save is now redundant.

> +++ b/tools/virsh.pod > @@ -546,7 +546,11 @@ I<on_reboot> parameter in the domain's XML definition. > > =item B<restore> I<state-file> > > -Restores a domain from an B<virsh save> state file. See I<save> for more info. > +Restores a domain from an B<virsh save> state file. See I<save> for more info. Sorry for not noticing sooner, but we could do some grammar cleanups. Does this look like a reasonable followup? diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index a84780b..0734a76 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -3430,7 +3430,7 @@ static int qemudDomainObjStart(virConnectPtr conn, if (!managed_save) goto cleanup; - if ((managed_save)&& (virFileExists(managed_save))) { + if (virFileExists(managed_save)) { ret = qemuDomainObjRestore(conn, driver, vm, managed_save); if ((ret == 0)&& (unlink(managed_save)< 0)) diff --git i/tools/virsh.pod w/tools/virsh.pod index 6319373..9ce6905 100644 --- i/tools/virsh.pod +++ w/tools/virsh.pod @@ -546,11 +546,12 @@ I<on_reboot> parameter in the domain's XML definition. =item B<restore> I<state-file> -Restores a domain from an B<virsh save> state file. See I<save> for more info. +Restores a domain from a B<virsh save> state file. See I<save> for more info. B<Note>: To avoid corrupting file system contents within the domain, you -should not reuse the saved state file to B<restore> unless you are convinced -with reverting the domain to the previous state. +should not reuse the saved state file for a second B<restore> unless +you have also reverted all storage volumes back to the same contents +as when the state file was created.

=item B<save> I<domain-id> I<state-file>