9:05 a.m.
Originaly reported by Viktor, this changes a few bits I didn't like
on the original series.
Peter Krempa (2):
virsh: Move cmdConnect from virsh-host.c to virsh.c
virsh: Register and unregister the close callback also in cmdConnect
Viktor Mihajlovski (2):
libvirt: Increase connection reference count for callbacks
virsh: Unregister the connection close notifier upon termination
src/libvirt.c | 5 +++
tools/virsh-host.c | 67 ----------------------------------
tools/virsh.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++---
3 files changed, 103 insertions(+), 72 deletions(-)
--
1.8.1.5
9:05 a.m.
New subject: [libvirt] [PATCHv2 1/4] libvirt: Increase connection reference count for callbacks
From: Viktor Mihajlovski <mihajlov(a)linux.vnet.ibm.com>
By adjusting the reference count of the connection object we
prevent races between callback function and virConnectClose.
Signed-off-by: Viktor Mihajlovski <mihajlov(a)linux.vnet.ibm.com>
---
src/libvirt.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/src/libvirt.c b/src/libvirt.c
index 4b9ea75..e9aff8a 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -20186,6 +20186,8 @@ int virConnectRegisterCloseCallback(virConnectPtr conn,
return -1;
}
+ virObjectRef(conn);
+
virMutexLock(&conn->lock);
virCheckNonNullArgGoto(cb, error);
@@ -20206,6 +20208,7 @@ int virConnectRegisterCloseCallback(virConnectPtr conn,
error:
virMutexUnlock(&conn->lock);
+ virObjectUnref(conn);
virDispatchError(NULL);
return -1;
}
@@ -20255,6 +20258,8 @@ int virConnectUnregisterCloseCallback(virConnectPtr conn,
virMutexUnlock(&conn->lock);
+ virObjectUnref(conn);
+
return 0;
error:
--
1.8.1.5
9:05 a.m.
New subject: [libvirt] [PATCHv2 2/4] virsh: Unregister the connection close notifier upon termination
From: Viktor Mihajlovski <mihajlov(a)linux.vnet.ibm.com>
Before closing the connection we unregister the close callback
to prevent a reference leak.
Further, the messages on virConnectClose != 0 are a bit more specific
now.
Signed-off-by: Viktor Mihajlovski <mihajlov(a)linux.vnet.ibm.com>
---
tools/virsh.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index 7ff12ec..6588470 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -321,9 +321,18 @@ vshReconnect(vshControl *ctl)
{
bool connected = false;
- if (ctl->conn != NULL) {
+ if (ctl->conn) {
+ int ret;
+
connected = true;
- virConnectClose(ctl->conn);
+
+ virConnectUnregisterCloseCallback(ctl->conn, vshCatchDisconnect);
+ ret = virConnectClose(ctl->conn);
+ if (ret < 0)
+ vshError(ctl, "%s", _("Failed to disconnect from the
hypervisor"));
+ else if (ret > 0)
+ vshError(ctl, _("Leaked %d reference(s) after disconnect "
+ "from the hypervisor"), ret);
}
ctl->conn = virConnectOpenAuth(ctl->name,
@@ -2660,9 +2669,13 @@ vshDeinit(vshControl *ctl)
VIR_FREE(ctl->name);
if (ctl->conn) {
int ret;
- if ((ret = virConnectClose(ctl->conn)) != 0) {
- vshError(ctl, _("Failed to disconnect from the hypervisor, %d leaked
reference(s)"), ret);
- }
+ virConnectUnregisterCloseCallback(ctl->conn, vshCatchDisconnect);
+ ret = virConnectClose(ctl->conn);
+ if (ret < 0)
+ vshError(ctl, "%s", _("Failed to disconnect from the
hypervisor"));
+ else if (ret > 0)
+ vshError(ctl, _("Leaked %d reference(s) after disconnect "
+ "from the hypervisor"), ret);
}
virResetLastError();
--
1.8.1.5
9:05 a.m.
New subject: [libvirt] [PATCHv2 3/4] virsh: Move cmdConnect from virsh-host.c to virsh.c
The function is used to establish connection so it should be in the main
virsh file. This movement also enables further improvements done in next
patches.
---
tools/virsh-host.c | 67 ----------------------------------------------------
tools/virsh.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 69 insertions(+), 67 deletions(-)
diff --git a/tools/virsh-host.c b/tools/virsh-host.c
index 2d49296..a7e31da 100644
--- a/tools/virsh-host.c
+++ b/tools/virsh-host.c
@@ -68,67 +68,6 @@ cmdCapabilities(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)
}
/*
- * "connect" command
- */
-static const vshCmdInfo info_connect[] = {
- {.name = "help",
- .data = N_("(re)connect to hypervisor")
- },
- {.name = "desc",
- .data = N_("Connect to local hypervisor. This is built-in "
- "command after shell start up.")
- },
- {.name = NULL}
-};
-
-static const vshCmdOptDef opts_connect[] = {
- {.name = "name",
- .type = VSH_OT_DATA,
- .flags = VSH_OFLAG_EMPTY_OK,
- .help = N_("hypervisor connection URI")
- },
- {.name = "readonly",
- .type = VSH_OT_BOOL,
- .help = N_("read-only connection")
- },
- {.name = NULL}
-};
-
-static bool
-cmdConnect(vshControl *ctl, const vshCmd *cmd)
-{
- bool ro = vshCommandOptBool(cmd, "readonly");
- const char *name = NULL;
-
- if (ctl->conn) {
- int ret;
- if ((ret = virConnectClose(ctl->conn)) != 0) {
- vshError(ctl, _("Failed to disconnect from the hypervisor, %d leaked
reference(s)"), ret);
- return false;
- }
- ctl->conn = NULL;
- }
-
- VIR_FREE(ctl->name);
- if (vshCommandOptStringReq(ctl, cmd, "name", &name) < 0)
- return false;
-
- ctl->name = vshStrdup(ctl, name);
-
- ctl->useGetInfo = false;
- ctl->useSnapshotOld = false;
- ctl->readonly = ro;
-
- ctl->conn = virConnectOpenAuth(ctl->name, virConnectAuthPtrDefault,
- ctl->readonly ? VIR_CONNECT_RO : 0);
-
- if (!ctl->conn)
- vshError(ctl, "%s", _("Failed to connect to the
hypervisor"));
-
- return !!ctl->conn;
-}
-
-/*
* "freecell" command
*/
static const vshCmdInfo info_freecell[] = {
@@ -912,12 +851,6 @@ const vshCmdDef hostAndHypervisorCmds[] = {
.info = info_capabilities,
.flags = 0
},
- {.name = "connect",
- .handler = cmdConnect,
- .opts = opts_connect,
- .info = info_connect,
- .flags = VSH_CMD_FLAG_NOCONNECT
- },
{.name = "freecell",
.handler = cmdFreecell,
.opts = opts_freecell,
diff --git a/tools/virsh.c b/tools/virsh.c
index 6588470..0f9bb28 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -355,6 +355,69 @@ vshReconnect(vshControl *ctl)
ctl->useSnapshotOld = false;
}
+
+/*
+ * "connect" command
+ */
+static const vshCmdInfo info_connect[] = {
+ {.name = "help",
+ .data = N_("(re)connect to hypervisor")
+ },
+ {.name = "desc",
+ .data = N_("Connect to local hypervisor. This is built-in "
+ "command after shell start up.")
+ },
+ {.name = NULL}
+};
+
+static const vshCmdOptDef opts_connect[] = {
+ {.name = "name",
+ .type = VSH_OT_DATA,
+ .flags = VSH_OFLAG_EMPTY_OK,
+ .help = N_("hypervisor connection URI")
+ },
+ {.name = "readonly",
+ .type = VSH_OT_BOOL,
+ .help = N_("read-only connection")
+ },
+ {.name = NULL}
+};
+
+static bool
+cmdConnect(vshControl *ctl, const vshCmd *cmd)
+{
+ bool ro = vshCommandOptBool(cmd, "readonly");
+ const char *name = NULL;
+
+ if (ctl->conn) {
+ int ret;
+ if ((ret = virConnectClose(ctl->conn)) != 0) {
+ vshError(ctl, _("Failed to disconnect from the hypervisor, %d leaked
reference(s)"), ret);
+ return false;
+ }
+ ctl->conn = NULL;
+ }
+
+ VIR_FREE(ctl->name);
+ if (vshCommandOptStringReq(ctl, cmd, "name", &name) < 0)
+ return false;
+
+ ctl->name = vshStrdup(ctl, name);
+
+ ctl->useGetInfo = false;
+ ctl->useSnapshotOld = false;
+ ctl->readonly = ro;
+
+ ctl->conn = virConnectOpenAuth(ctl->name, virConnectAuthPtrDefault,
+ ctl->readonly ? VIR_CONNECT_RO : 0);
+
+ if (!ctl->conn)
+ vshError(ctl, "%s", _("Failed to connect to the
hypervisor"));
+
+ return !!ctl->conn;
+}
+
+
#ifndef WIN32
static void
vshPrintRaw(vshControl *ctl, ...)
@@ -3000,6 +3063,12 @@ vshParseArgv(vshControl *ctl, int argc, char **argv)
}
static const vshCmdDef virshCmds[] = {
+ {.name = "connect",
+ .handler = cmdConnect,
+ .opts = opts_connect,
+ .info = info_connect,
+ .flags = VSH_CMD_FLAG_NOCONNECT
+ },
{.name = "cd",
.handler = cmdCd,
.opts = opts_cd,
--
1.8.1.5
9:05 a.m.
New subject: [libvirt] [PATCHv2 4/4] virsh: Register and unregister the close callback also in cmdConnect
This patch improves the error message after disconnecting from the
hypervisor and adds the close callback operations required not to leak
the callback reference.
---
tools/virsh.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index 0f9bb28..9fb517f 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -391,10 +391,15 @@ cmdConnect(vshControl *ctl, const vshCmd *cmd)
if (ctl->conn) {
int ret;
- if ((ret = virConnectClose(ctl->conn)) != 0) {
- vshError(ctl, _("Failed to disconnect from the hypervisor, %d leaked
reference(s)"), ret);
- return false;
- }
+
+ virConnectUnregisterCloseCallback(ctl->conn, vshCatchDisconnect);
+ ret = virConnectClose(ctl->conn);
+ if (ret < 0)
+ vshError(ctl, "%s", _("Failed to disconnect from the
hypervisor"));
+ else if (ret > 0)
+ vshError(ctl, _("Leaked %d reference(s) after disconnect "
+ "from the hypervisor"), ret);
+
ctl->conn = NULL;
}
@@ -411,10 +416,16 @@ cmdConnect(vshControl *ctl, const vshCmd *cmd)
ctl->conn = virConnectOpenAuth(ctl->name, virConnectAuthPtrDefault,
ctl->readonly ? VIR_CONNECT_RO : 0);
- if (!ctl->conn)
+ if (!ctl->conn) {
vshError(ctl, "%s", _("Failed to connect to the
hypervisor"));
+ return false;
+ }
- return !!ctl->conn;
+ if (virConnectRegisterCloseCallback(ctl->conn, vshCatchDisconnect,
+ NULL, NULL) < 0)
+ vshError(ctl, "%s", _("Unable to register disconnect
callback"));
+
+ return true;
}
--
1.8.1.5
10 a.m.
New subject: [libvirt] [PATCHv2 4/4] virsh: Register and unregister the close callback also in cmdConnect
On 03/27/2013 03:05 PM, Peter Krempa wrote:
This patch improves the error message after disconnecting from the
hypervisor and adds the close callback operations required not to leak
the callback reference.
---
tools/virsh.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
Hi Peter, I see a bunch of errors running make check with this patch
applied (at least in conjunction with my patches).
Mostly:
error: Failed to disconnect from the hypervisor, 1 leaked reference(s)
--
Mit freundlichen Grüßen/Kind Regards
Viktor Mihajlovski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
10:05 a.m.
New subject: [libvirt] [PATCHv2 4/4] virsh: Register and unregister the close callback also in cmdConnect
On 03/27/13 16:00, Viktor Mihajlovski wrote:
On 03/27/2013 03:05 PM, Peter Krempa wrote:
> This patch improves the error message after disconnecting from the
> hypervisor and adds the close callback operations required not to leak
> the callback reference.
> ---
> tools/virsh.c | 23 +++++++++++++++++------
> 1 file changed, 17 insertions(+), 6 deletions(-)
>
Hi Peter, I see a bunch of errors running make check with this patch
applied (at least in conjunction with my patches).
Mostly:
error: Failed to disconnect from the hypervisor, 1 leaked reference(s)
Hmm, let me check. I might have missed some stuff without noticing :/
Peter
10:13 a.m.
New subject: [libvirt] [PATCHv2 4/4] virsh: Register and unregister the close callback also in cmdConnect
On 03/27/13 16:05, Peter Krempa wrote:
On 03/27/13 16:00, Viktor Mihajlovski wrote:
> On 03/27/2013 03:05 PM, Peter Krempa wrote:
>> This patch improves the error message after disconnecting from the
>> hypervisor and adds the close callback operations required not to leak
>> the callback reference.
>> ---
>> tools/virsh.c | 23 +++++++++++++++++------
>> 1 file changed, 17 insertions(+), 6 deletions(-)
>>
>
> Hi Peter, I see a bunch of errors running make check with this patch
> applied (at least in conjunction with my patches).
> Mostly:
> error: Failed to disconnect from the hypervisor, 1 leaked reference(s)
>
Hmm, let me check. I might have missed some stuff without noticing :/
Peter
I've re-run the test suite and I'm not getting any errors. I have the
current upstream (including your 2/3 patch from v1) and these four on top.
What tests are failing for you?
Peter
10:26 a.m.
New subject: [libvirt] [PATCHv2 4/4] virsh: Register and unregister the close callback also in cmdConnect
On 03/27/2013 04:13 PM, Peter Krempa wrote:
I've re-run the test suite and I'm not getting any errors. I have the
current upstream (including your 2/3 patch from v1) and these four on top.
What tests are failing for you?
Peter
After taking out 1/3 I don't get the errors, which is logical,
as the refcount isn't incremented in the register callback.
It seems that there are quite a lot situations where virsh
exits without unregistering the callback.
Please apply 1/3 (which is actually the essential patch
of the series) to see it happen.
--
Mit freundlichen Grüßen/Kind Regards
Viktor Mihajlovski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
10:32 a.m.
New subject: [libvirt] [PATCHv2 4/4] virsh: Register and unregister the close callback also in cmdConnect
On 03/27/13 16:26, Viktor Mihajlovski wrote:
On 03/27/2013 04:13 PM, Peter Krempa wrote:
>
> I've re-run the test suite and I'm not getting any errors. I have the
> current upstream (including your 2/3 patch from v1) and these four on
> top.
>
> What tests are failing for you?
>
> Peter
>
After taking out 1/3 I don't get the errors, which is logical,
as the refcount isn't incremented in the register callback.
It seems that there are quite a lot situations where virsh
exits without unregistering the callback.
Please apply 1/3 (which is actually the essential patch
of the series) to see it happen.
My tree actually contains 1/3 from your v1 series and I also reposted it
in my v2 and I don't seem to be getting errors when I have the 4 (5 with
the one pushed upstream) patches applied.
Peter
10:49 a.m.
New subject: [libvirt] [PATCHv2 4/4] virsh: Register and unregister the close callback also in cmdConnect
On 03/27/2013 04:32 PM, Peter Krempa wrote:
My tree actually contains 1/3 from your v1 series and I also reposted it
in my v2 and I don't seem to be getting errors when I have the 4 (5 with
the one pushed upstream) patches applied.
Peter
Never mind and sorry. It seems that I was tricked by our mail gateway
which delivered the 1/4 and 2/4 messages without the [libvirt] subject
prefix.
So I ended up seeing only 3/4 and 4/4 in my libvir-list folder. Argh...
With everything applied itlooks and works great. From my side a +1.
--
Mit freundlichen Grüßen/Kind Regards
Viktor Mihajlovski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
11:41 a.m.
On 03/27/13 15:05, Peter Krempa wrote:
Originaly reported by Viktor, this changes a few bits I didn't
like
on the original series.
Peter Krempa (2):
virsh: Move cmdConnect from virsh-host.c to virsh.c
virsh: Register and unregister the close callback also in cmdConnect
Viktor Mihajlovski (2):
libvirt: Increase connection reference count for callbacks
virsh: Unregister the connection close notifier upon termination
src/libvirt.c | 5 +++
tools/virsh-host.c | 67 ----------------------------------
tools/virsh.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++---
3 files changed, 103 insertions(+), 72 deletions(-)
This series is now obsolete. It did not fix all the possible paths where
the memory corruption could happen. The new version is at:
http://www.redhat.com/archives/libvir-list/2013-March/msg01682.html
Peter
4254
days inactive
4258
days old
11 comments
2 participants
participants (2)
-
Peter Krempa -
Viktor Mihajlovski