[libvirt] [PATCH] qemu: Warn of restore with managed save being risky

[Daniel Henrique Barboza]

On 12/31/19 5:42 PM, Michael Weiser wrote:
> Hi Daniel,
> 
> On Tue, Dec 31, 2019 at 05:19:54PM -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 at gmx.de>
>>> Cc: Cole Robinson <crobinso at redhat.com>
>>> ---
>> 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 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.



Thanks,


DHB


> 
>> Reviewed-by: Daniel Henrique Barboza <danielhb413 at gmail.com>
> 
> Thanks!
>