Hello Daniel, On Thu, Jan 02, 2020 at 09:58:19AM -0300, Daniel Henrique Barboza wrote: > > > > This patch marks the restore operation as risky at the libvirt level, > > > > requiring the user to remove the saved memory state first or force the > > > > operation. > > > > > > > > [1] https://www.redhat.com/archives/virt-tools-list/2019-November/msg00011.html > > > > [2] https://www.redhat.com/archives/virt-tools-list/2019-December/msg00049.html > > > > > > > > Signed-off-by: Michael Weiser <michael.weiser@xxxxxx> > > > > Cc: Cole Robinson <crobinso@xxxxxxxxxx> > > > > --- > > > As said in [1], I agree that the API needs a flag override to allow the user > > > to roll with it despite the risks. Given that this is somewhat a corner case, > > > I also believe that such override can go in a separated patch/series later > > > on. > > > > Do you mean a separate override flag beyond > > VIR_DOMAIN_SNAPSHOT_REVERT_FORCE? Or should I just update the commit > > message to make clear how the user can force the operation? > Ah, I overlooked the "or force the operation" part of the commit message after > reading the discussions in [1] and [2]. > Instead of updating the commit message, I think you can update the error message > to mention the '--force' option, e.g. : > + virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s", > + _("revert to snapshot while there is a managed " > + "saved state will cause corruption when run, " > + "remove saved state first or use the " > + "--force option")); I'd rather not reference virsh command options in the error message as it would be highly confusing in any other context. For example, python clients get the error message wrapped in an exception, augmented already by a prefix telling them they need to force the operation: Traceback (most recent call last): File "/usr/share/virt-manager/virtManager/asyncjob.py", line 75, in cb_wrapper callback(asyncjob, *args, **kwargs) File "/usr/share/virt-manager/virtManager/asyncjob.py", line 111, in tmpcb callback(*args, **kwargs) File "/usr/share/virt-manager/virtManager/object/libvirtobject.py", line 66, in newfn ret = fn(self, *args, **kwargs) File "/usr/share/virt-manager/virtManager/object/domain.py", line 1055, in revert_to_snapshot self._backend.revertToSnapshot(snap.get_backend()) File "/usr/lib64/python3.6/site-packages/libvirt.py", line 2088, in revertToSnapshot if ret == -1: raise libvirtError ('virDomainRevertToSnapshot() failed', dom=self) libvirt.libvirtError: revert requires force: revert to snapshot while there is a managed saved state will cause corruption when run, remove saved state first The same is actually the case for virsh already: virsh # snapshot-revert debian --snapshotname snapshot1 error: revert requires force: revert to snapshot while there is a managed saved state will cause corruption when run, remove saved state first virsh # We could of course reword to better take context and prefix into account, e.g.: error: revert requires force: Removal of existing managed saved state strongly recommended to avoid corruption Any ideas? > I also noticed that the man page for 'snapshot-revert' mentions two cases where > the use of --force is required. One of them talks about snapshots created using old > Libvirt versions. The other goes as follows: > ---- > (docs/manpages/virsh.rst) > The other is the case of reverting from a running domain to an active state > where a new hypervisor has to be created rather than reusing the existing > hypervisor, because it implies drawbacks such as breaking any existing > VNC or Spice connections; this condition happens with an active > snapshot that uses a provably incompatible configuration, as well as > with an inactive snapshot that is combined with the --start or > --pause flag. > ---- > I am almost certain that scenario you're handling here is not covered by this > excerpt. In this case, since you're adding a new '--force' condition, I believe a > second patch adding this new '--force' condition in the documentation is a > nice touch. Will do. v2 coming as soon as we've agreed on where to go with the error message. -- Thanks, Michael -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
