[libvirt] Connection release is not correct in libvirt and libvrt java
Hi, The following is the current code to release connection in libvirt. int virConnectClose(virConnectPtr conn) { ... if (!VIR_IS_CONNECT(conn)) { virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); goto error; } ... error: virDispatchError(NULL); return ret; } Now if the cable is unplugged and the application call virConnectClose to release connection, the code will enter into the error procedure, the connection Can't be released. I have changed the following two parts to fix this issue. Please give your comments: Changed Code1: int virConnectClose(virConnectPtr conn) { ... + if(NULL == conn) { + return 0; + } ... - if (!VIR_IS_CONNECT(conn)) { - virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); - goto error; - } ... error: virDispatchError(NULL); return ret; } Changed Code2: int virUnrefConnect(virConnectPtr conn) { ... + if(NULL == conn) { + return 0; + } - if ((!VIR_IS_CONNECT(conn))) { - virLibConnError(VIR_ERR_INVALID_ARG, _("no connection")); - return -1; - } ... } For libvirt java, there are similar issue. I have changed code as following in Collect.java. Please also give your comments. public int close() throws LibvirtException { int success = 0; if (VCP != null) { + try { success = libvirt.virConnectClose(VCP); processError(); + } + finally { // If leave an invalid pointer dangling around JVM crashes and burns // if someone tries to call a method on us // We rely on the underlying libvirt error handling to detect that // it's called with a null virConnectPointer VCP = null; + } } return success; } B.R. Benjamin Wang
On 12/18/2012 08:14 AM, Benjamin Wang (gendwang) wrote:
Now if the cable is unplugged and the application call virConnectClose to release connection, the code will enter into the error procedure, the connection Can't be released. I have changed the following two parts to fix this issue. Please give your comments: Changed Code1: int virConnectClose(virConnectPtr conn) { ...
+ if(NULL == conn) { + return 0; + }
...
- if (!VIR_IS_CONNECT(conn)) { - virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); - goto error; - } ...
It's much easier to read patches that are produced by 'git diff' than by open-coding the differences into your email. In particular, missing context makes it hard to see where you are intending your patch to apply. However, I will NACK this patch. NULL is never a valid connection, and passing NULL must make the API return failure, not success. The existing 'if (!VIR_IS_CONNECT(conn))' is sufficient to check for an invalid passing of a NULL pointer.
For libvirt java, there are similar issue. I have changed code as following in Collect.java. Please also give your comments. public int close() throws LibvirtException { int success = 0; if (VCP != null) { + try { success = libvirt.virConnectClose(VCP); processError(); + } + finally { // If leave an invalid pointer dangling around JVM crashes and burns // if someone tries to call a method on us // We rely on the underlying libvirt error handling to detect that // it's called with a null virConnectPointer VCP = null; + }
Unfortunately, I'm not familiar enough with libvirt-java to know if this patch makes sense. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Hi, sorry for replying so late. At Wed, 19 Dec 2012 15:31:54 -0700, Eric Blake wrote:
On 12/18/2012 08:14 AM, Benjamin Wang (gendwang) wrote:
For libvirt java, there are similar issue. I have changed code as following in Collect.java. Please also give your comments. public int close() throws LibvirtException { int success = 0; if (VCP != null) { + try { success = libvirt.virConnectClose(VCP); processError(); + } + finally { // If leave an invalid pointer dangling around JVM crashes and burns // if someone tries to call a method on us // We rely on the underlying libvirt error handling to detect that // it's called with a null virConnectPointer VCP = null; + }
Unfortunately, I'm not familiar enough with libvirt-java to know if this patch makes sense.
Basically, when applying this patch it gives you only a one-chance opportunity to call virConnectClose. If an exception is thrown, it will set VCP to null, no matter what. Might be that there was only a temporary problem, and you could retry... ? Or do some other stuff if the VCP is still valid. But that depends on the error. So, is it ensured, that when virConnectClose returns an error, the virConnectPtr is "wasted" and should be invalidated? Claudio -- AV-Test GmbH, Henricistraße 20, 04155 Leipzig, Germany Phone: +49 341 265 310 19 Web:<http://www.av-test.org> Eingetragen am / Registered at: Amtsgericht Stendal (HRB 114076) Geschaeftsfuehrer (CEO): Andreas Marx, Guido Habicht, Maik Morgenstern
participants (3)
-
Benjamin Wang (gendwang) -
Claudio Bley -
Eric Blake