[libvirt] [PATCH 03/20] virConnect(Un)RegisterCloseCallback: Throw an error in case the API is not supported

Marc Hartmayer posted 20 patches 7 years, 9 months ago
[libvirt] [PATCH 03/20] virConnect(Un)RegisterCloseCallback: Throw an error in case the API is not supported
Posted by Marc Hartmayer 7 years, 9 months ago
Report an error in case the driver does not support
connect(Un)registerCloseCallback. The commit 'close callback: move it
to driver' (88f09b75eb99) moved the responsibility for the close
callback to the driver. But if the driver doesn't support the
connectRegisterCloseCallback API this function does nothing, even no
unsupported error report. The only case where an error is reported is
when the API is supported but the call fails. The same behavior
applies to virConnectUnregisterCloseCallback.

This behavior is not intended as there are many use cases of this API
where the state of for example allocations depends on the result of
these functions.

To keep the behavior of virsh as before it must silently ignore
unsupported error for virConnectRegisterCloseCallback. For the remote
driver this change wouldn't be needed, but for the byhve driver, for
example. Otherwise the user would see the error message that virsh was
unable to register a disconnect callback.

Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
---
 src/libvirt-host.c | 24 ++++++++++++++----------
 tools/virsh.c      | 11 +++++++++--
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/src/libvirt-host.c b/src/libvirt-host.c
index 7ff7407a0874..bfa104b39918 100644
--- a/src/libvirt-host.c
+++ b/src/libvirt-host.c
@@ -1221,12 +1221,14 @@ virConnectRegisterCloseCallback(virConnectPtr conn,
     virCheckConnectReturn(conn, -1);
     virCheckNonNullArgGoto(cb, error);
 
-    if (conn->driver->connectRegisterCloseCallback &&
-        conn->driver->connectRegisterCloseCallback(conn, cb, opaque, freecb) < 0)
-        goto error;
-
-    return 0;
+    if (conn->driver->connectRegisterCloseCallback) {
+        int ret = conn->driver->connectRegisterCloseCallback(conn, cb, opaque, freecb);
+        if (ret < 0)
+            goto error;
+        return ret;
+    }
 
+    virReportUnsupportedError();
  error:
     virDispatchError(conn);
     return -1;
@@ -1256,12 +1258,14 @@ virConnectUnregisterCloseCallback(virConnectPtr conn,
     virCheckConnectReturn(conn, -1);
     virCheckNonNullArgGoto(cb, error);
 
-    if (conn->driver->connectUnregisterCloseCallback &&
-        conn->driver->connectUnregisterCloseCallback(conn, cb) < 0)
-        goto error;
-
-    return 0;
+    if (conn->driver->connectUnregisterCloseCallback) {
+        int ret = conn->driver->connectUnregisterCloseCallback(conn, cb);
+        if (ret < 0)
+            goto error;
+        return ret;
+    }
 
+    virReportUnsupportedError();
  error:
     virDispatchError(conn);
     return -1;
diff --git a/tools/virsh.c b/tools/virsh.c
index 5f8352e861d3..2df1197252b3 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -256,8 +256,15 @@ virshReconnect(vshControl *ctl, const char *name, bool readonly, bool force)
         priv->readonly = readonly;
 
         if (virConnectRegisterCloseCallback(priv->conn, virshCatchDisconnect,
-                                            ctl, NULL) < 0)
-            vshError(ctl, "%s", _("Unable to register disconnect callback"));
+                                            ctl, NULL) < 0) {
+            virErrorPtr error = virGetLastError();
+            if (error && error->code == VIR_ERR_NO_SUPPORT) {
+                /* silently ignore the unsupported error */
+                virResetLastError();
+            } else {
+                vshError(ctl, "%s", _("Unable to register disconnect callback"));
+            }
+        }
         if (connected && !force)
             vshError(ctl, "%s", _("Reconnected to the hypervisor"));
     }
-- 
2.13.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/20] virConnect(Un)RegisterCloseCallback: Throw an error in case the API is not supported
Posted by John Ferlan 7 years, 9 months ago

On 03/08/2018 07:20 AM, Marc Hartmayer wrote:
> Report an error in case the driver does not support
> connect(Un)registerCloseCallback. The commit 'close callback: move it
> to driver' (88f09b75eb99) moved the responsibility for the close
> callback to the driver. But if the driver doesn't support the
> connectRegisterCloseCallback API this function does nothing, even no
> unsupported error report. The only case where an error is reported is
> when the API is supported but the call fails. The same behavior
> applies to virConnectUnregisterCloseCallback.
> 
> This behavior is not intended as there are many use cases of this API
> where the state of for example allocations depends on the result of
> these functions.
> 
> To keep the behavior of virsh as before it must silently ignore
> unsupported error for virConnectRegisterCloseCallback. For the remote
> driver this change wouldn't be needed, but for the byhve driver, for
> example. Otherwise the user would see the error message that virsh was
  ^^^^^^^
I think you forgot to end your sentence for example what about bhyve?

> unable to register a disconnect callback.

So the reason to keep the virsh behavior is because failure to do so
could be problematic if connecting to an older server that doesn't
support virConnectRegisterCloseCallback?  I almost wonder if we should
blat a vshDebug type message to "inform" the consumer that it's not
possible to catch disconnect...

Since the code path doesn't return -1 anyway (even if "Unable to
register..." is printed), it may not hurt.  Your call though - it's not
that important other than the oh, OK so there's something newer in the
newer server that might be useful.


Reviewed-by: John Ferlan <jferlan@redhat.com>

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/20] virConnect(Un)RegisterCloseCallback: Throw an error in case the API is not supported
Posted by Nikolay Shirokovskiy 7 years, 9 months ago

On 08.03.2018 15:20, Marc Hartmayer wrote:
> Report an error in case the driver does not support
> connect(Un)registerCloseCallback. The commit 'close callback: move it
> to driver' (88f09b75eb99) moved the responsibility for the close
> callback to the driver. But if the driver doesn't support the
> connectRegisterCloseCallback API this function does nothing, even no
> unsupported error report. The only case where an error is reported is
> when the API is supported but the call fails. The same behavior
> applies to virConnectUnregisterCloseCallback.
> 
> This behavior is not intended as there are many use cases of this API
> where the state of for example allocations depends on the result of
> these functions.
> 
> To keep the behavior of virsh as before it must silently ignore
> unsupported error for virConnectRegisterCloseCallback. For the remote
> driver this change wouldn't be needed, but for the byhve driver, for
> example. Otherwise the user would see the error message that virsh was
> unable to register a disconnect callback.
> 
> Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
> ---
>  src/libvirt-host.c | 24 ++++++++++++++----------
>  tools/virsh.c      | 11 +++++++++--
>  2 files changed, 23 insertions(+), 12 deletions(-)
> 

We can't change public API. As to patch 18 I suggest either to:

- don't refcount client object, this works though it is dangerous. Or
- check if driver supports VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK and throw
  an error if it does not (looks better to me)

Nikolay

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/20] virConnect(Un)RegisterCloseCallback: Throw an error in case the API is not supported
Posted by Nikolay Shirokovskiy 7 years, 9 months ago

On 16.03.2018 12:39, Nikolay Shirokovskiy wrote:
> 
> 
> On 08.03.2018 15:20, Marc Hartmayer wrote:
>> Report an error in case the driver does not support
>> connect(Un)registerCloseCallback. The commit 'close callback: move it
>> to driver' (88f09b75eb99) moved the responsibility for the close
>> callback to the driver. But if the driver doesn't support the
>> connectRegisterCloseCallback API this function does nothing, even no
>> unsupported error report. The only case where an error is reported is
>> when the API is supported but the call fails. The same behavior
>> applies to virConnectUnregisterCloseCallback.
>>
>> This behavior is not intended as there are many use cases of this API
>> where the state of for example allocations depends on the result of
>> these functions.
>>
>> To keep the behavior of virsh as before it must silently ignore
>> unsupported error for virConnectRegisterCloseCallback. For the remote
>> driver this change wouldn't be needed, but for the byhve driver, for
>> example. Otherwise the user would see the error message that virsh was
>> unable to register a disconnect callback.
>>
>> Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
>> ---
>>  src/libvirt-host.c | 24 ++++++++++++++----------
>>  tools/virsh.c      | 11 +++++++++--
>>  2 files changed, 23 insertions(+), 12 deletions(-)
>>
> 
> We can't change public API. As to patch 18 I suggest either to:
> 
> - don't refcount client object, this works though it is dangerous. Or
> - check if driver supports VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK and throw
>   an error if it does not (looks better to me)
> 

Still other clients (other than daemon) can have opaque leaks as they
don't check for VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK before call to 
virConnectRegisterCloseCallback. This is introduced in [1]

[1] 88f09b75e
Author: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Date:   Tue Mar 1 14:17:38 2016 +0000

    close callback: move it to driver
    
    Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>

To fix this we can fallback to previous scheme (using closeCallback in
virConnectPtr) if connectRegisterCloseCallback is not defined for
the driver. Then we can refcount client object in patch 18 without
any additional checks.

Nikolay

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/20] virConnect(Un)RegisterCloseCallback: Throw an error in case the API is not supported
Posted by Daniel P. Berrangé 7 years, 9 months ago
On Thu, Mar 08, 2018 at 01:20:26PM +0100, Marc Hartmayer wrote:
> Report an error in case the driver does not support
> connect(Un)registerCloseCallback. The commit 'close callback: move it
> to driver' (88f09b75eb99) moved the responsibility for the close
> callback to the driver. But if the driver doesn't support the
> connectRegisterCloseCallback API this function does nothing, even no
> unsupported error report. The only case where an error is reported is
> when the API is supported but the call fails. The same behavior
> applies to virConnectUnregisterCloseCallback.
> 
> This behavior is not intended as there are many use cases of this API
> where the state of for example allocations depends on the result of
> these functions.
> 
> To keep the behavior of virsh as before it must silently ignore
> unsupported error for virConnectRegisterCloseCallback. For the remote
> driver this change wouldn't be needed, but for the byhve driver, for
> example. Otherwise the user would see the error message that virsh was
> unable to register a disconnect callback.

NACK, you cann't change the API semantics in this way.

Fixing virsh is no where near sufficient, becasue this change will
still potentially break every other application using this libvirt
API that expect it to always succeeed.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list