The -2 value is misleading because if 'qemuAgentFSFreeze' fails it
doesn't necessarily mean that the command was sent to the agent.

Since callers don't care about the -2 value specifically, remove it.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_snapshot.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index eacc8c6400..93b74b035a 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -119,9 +119,6 @@ qemuSnapshotCountExternal(void *payload,
 }


-/* Return -1 if request is not sent to agent due to misconfig, -2 if request
- * is sent but failed, and number of frozen filesystems on success. If -2 is
- * returned, FSThaw should be called revert the quiesced status. */
 int
 qemuSnapshotFSFreeze(virDomainObjPtr vm,
                      const char **mountpoints,
@@ -136,7 +133,7 @@ qemuSnapshotFSFreeze(virDomainObjPtr vm,
     agent = qemuDomainObjEnterAgent(vm);
     frozen = qemuAgentFSFreeze(agent, mountpoints, nmountpoints);
     qemuDomainObjExitAgent(vm, agent);
-    return frozen < 0 ? -2 : frozen;
+    return frozen;
 }


-- 
2.29.2

On Mon, Feb 15, 2021 at 06:27:51PM +0100, Peter Krempa wrote:
> The -2 value is misleading because if 'qemuAgentFSFreeze' fails it
> doesn't necessarily mean that the command was sent to the agent.
> 
> Since callers don't care about the -2 value specifically, remove it.

In addition this indirectly fixes virDomainFSFreeze public API where
we return result of qemuSnapshotFSFreeze directly. Now we comply with
the API description.

> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/qemu/qemu_snapshot.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)

Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

On Mon, Feb 15, 2021 at 19:20:25 +0100, Pavel Hrdina wrote:
> On Mon, Feb 15, 2021 at 06:27:51PM +0100, Peter Krempa wrote:
> > The -2 value is misleading because if 'qemuAgentFSFreeze' fails it
> > doesn't necessarily mean that the command was sent to the agent.
> > 
> > Since callers don't care about the -2 value specifically, remove it.
> 
> In addition this indirectly fixes virDomainFSFreeze public API where
> we return result of qemuSnapshotFSFreeze directly. Now we comply with
> the API description.

Luckily -2 happens only internally, so we never broke any public API
promise:

This is caused by virNetServerProgramDispatchCall treating any negative
value as error and transporting it over RPC and then
virNetClientProgramCall in the client returning -1 if the returned reply
is of VIR_NET_ERROR type.

Thread 1 "virsh" hit Breakpoint 1, virDomainFSFreeze (dom=dom@entry=0x55555569b550, mountpoints=0x0, nmountpoints=0, flags=flags@entry=0) at ../../../libvirt/src/libvirt-domain.c:11327
11327	{
(gdb) next
11328	    VIR_DOMAIN_DEBUG(dom, "mountpoints=%p, nmountpoints=%d, flags=0x%x",
(gdb)
11331	    virResetLastError();
(gdb)
11333	    virCheckDomainReturn(dom, -1);
(gdb)
11334	    virCheckReadOnlyGoto(dom->conn->flags, error);
(gdb)
11335	    virCheckNonNullArrayArgGoto(mountpoints, nmountpoints, error);
(gdb)
11337	    if (dom->conn->driver->domainFSFreeze) {
(gdb)
11338	        int ret = dom->conn->driver->domainFSFreeze(
(gdb)
11340	        if (ret < 0)
(gdb) p ret
$1 = -1
(gdb) c
Continuing.
error: Unable to freeze filesystems
error: internal error: unable to execute QEMU agent command 'guest-fsfreeze-freeze': The command guest-fsfreeze-freeze has been disabled for this instance

On Tue, Feb 16, 2021 at 09:46:42AM +0100, Peter Krempa wrote:
> On Mon, Feb 15, 2021 at 19:20:25 +0100, Pavel Hrdina wrote:
> > On Mon, Feb 15, 2021 at 06:27:51PM +0100, Peter Krempa wrote:
> > > The -2 value is misleading because if 'qemuAgentFSFreeze' fails it
> > > doesn't necessarily mean that the command was sent to the agent.
> > > 
> > > Since callers don't care about the -2 value specifically, remove it.
> > 
> > In addition this indirectly fixes virDomainFSFreeze public API where
> > we return result of qemuSnapshotFSFreeze directly. Now we comply with
> > the API description.
> 
> Luckily -2 happens only internally, so we never broke any public API
> promise:
> 
> This is caused by virNetServerProgramDispatchCall treating any negative
> value as error and transporting it over RPC and then
> virNetClientProgramCall in the client returning -1 if the returned reply
> is of VIR_NET_ERROR type.

Well, lucky for us :) I did not count RPC and the fact that even local
connection will use remote driver for QEMU.

Pavel