[libvirt] [PATCH] Fix a reference leak for node devices.
There was one major, and a few minor bugs having to do with the reference counting of node devices in daemon/remote.c The major bug was that remoteDispatchNodeDeviceListCaps() was completely failing to unreference node devices; this would lead to many open file descriptors, which would eventually fail. The minor bugs were along the same lines, but were in rarely used error paths. Still, they should be corrected. Signed-off-by: Chris Lalancette <clalance@redhat.com> --- daemon/remote.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 91faa9a..ec5f85b 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -5012,11 +5012,13 @@ remoteDispatchNodeDeviceGetParent (struct qemud_server *server ATTRIBUTE_UNUSED, /* remoteDispatchClientRequest will free this. */ char **parent_p; if (VIR_ALLOC(parent_p) < 0) { + virNodeDeviceFree(dev); remoteDispatchOOMError(rerr); return -1; } *parent_p = strdup(parent); if (*parent_p == NULL) { + virNodeDeviceFree(dev); remoteDispatchOOMError(rerr); return -1; } @@ -5048,6 +5050,7 @@ remoteDispatchNodeDeviceNumOfCaps (struct qemud_server *server ATTRIBUTE_UNUSED, ret->num = virNodeDeviceNumOfCaps(dev); if (ret->num < 0) { + virNodeDeviceFree(dev); remoteDispatchConnError(rerr, conn); return -1; } @@ -5076,6 +5079,7 @@ remoteDispatchNodeDeviceListCaps (struct qemud_server *server ATTRIBUTE_UNUSED, } if (args->maxnames > REMOTE_NODE_DEVICE_NAME_LIST_MAX) { + virNodeDeviceFree(dev); remoteDispatchFormatError(rerr, "%s", _("maxnames > REMOTE_NODE_DEVICE_NAME_LIST_MAX")); return -1; @@ -5083,6 +5087,7 @@ remoteDispatchNodeDeviceListCaps (struct qemud_server *server ATTRIBUTE_UNUSED, /* Allocate return buffer. */ if (VIR_ALLOC_N(ret->names.names_val, args->maxnames) < 0) { + virNodeDeviceFree(dev); remoteDispatchOOMError(rerr); return -1; } @@ -5091,11 +5096,13 @@ remoteDispatchNodeDeviceListCaps (struct qemud_server *server ATTRIBUTE_UNUSED, virNodeDeviceListCaps (dev, ret->names.names_val, args->maxnames); if (ret->names.names_len == -1) { + virNodeDeviceFree(dev); remoteDispatchConnError(rerr, conn); VIR_FREE(ret->names.names_val); return -1; } + virNodeDeviceFree(dev); return 0; } -- 1.6.6.1
On 06/25/2010 03:22 PM, Chris Lalancette wrote:
There was one major, and a few minor bugs having to do with the reference counting of node devices in daemon/remote.c The major bug was that remoteDispatchNodeDeviceListCaps() was completely failing to unreference node devices; this would lead to many open file descriptors, which would eventually fail.
The minor bugs were along the same lines, but were in rarely used error paths. Still, they should be corrected.
Signed-off-by: Chris Lalancette <clalance@redhat.com> --- daemon/remote.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c index 91faa9a..ec5f85b 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -5012,11 +5012,13 @@ remoteDispatchNodeDeviceGetParent (struct qemud_server *server ATTRIBUTE_UNUSED, /* remoteDispatchClientRequest will free this. */ char **parent_p; if (VIR_ALLOC(parent_p) < 0) { + virNodeDeviceFree(dev); remoteDispatchOOMError(rerr); return -1; }
ACK. But given the number of places where you had to duplicate this line insertion, would it be better to change all the return -1 into goto cleanup, where the cleanup: label guarantees this cleanup for all possible exit paths? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Jun 25, 2010 at 03:36:39PM -0600, Eric Blake wrote:
On 06/25/2010 03:22 PM, Chris Lalancette wrote:
There was one major, and a few minor bugs having to do with the reference counting of node devices in daemon/remote.c The major bug was that remoteDispatchNodeDeviceListCaps() was completely failing to unreference node devices; this would lead to many open file descriptors, which would eventually fail.
The minor bugs were along the same lines, but were in rarely used error paths. Still, they should be corrected.
Signed-off-by: Chris Lalancette <clalance@redhat.com> --- daemon/remote.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c index 91faa9a..ec5f85b 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -5012,11 +5012,13 @@ remoteDispatchNodeDeviceGetParent (struct qemud_server *server ATTRIBUTE_UNUSED, /* remoteDispatchClientRequest will free this. */ char **parent_p; if (VIR_ALLOC(parent_p) < 0) { + virNodeDeviceFree(dev); remoteDispatchOOMError(rerr); return -1; }
ACK. But given the number of places where you had to duplicate this line insertion, would it be better to change all the return -1 into goto cleanup, where the cleanup: label guarantees this cleanup for all possible exit paths?
+1 Dave
On 06/25/10 - 03:36:39PM, Eric Blake wrote:
On 06/25/2010 03:22 PM, Chris Lalancette wrote:
There was one major, and a few minor bugs having to do with the reference counting of node devices in daemon/remote.c The major bug was that remoteDispatchNodeDeviceListCaps() was completely failing to unreference node devices; this would lead to many open file descriptors, which would eventually fail.
The minor bugs were along the same lines, but were in rarely used error paths. Still, they should be corrected.
Signed-off-by: Chris Lalancette <clalance@redhat.com> --- daemon/remote.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c index 91faa9a..ec5f85b 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -5012,11 +5012,13 @@ remoteDispatchNodeDeviceGetParent (struct qemud_server *server ATTRIBUTE_UNUSED, /* remoteDispatchClientRequest will free this. */ char **parent_p; if (VIR_ALLOC(parent_p) < 0) { + virNodeDeviceFree(dev); remoteDispatchOOMError(rerr); return -1; }
ACK. But given the number of places where you had to duplicate this line insertion, would it be better to change all the return -1 into goto cleanup, where the cleanup: label guarantees this cleanup for all possible exit paths?
Yeah, I considered it. I'm going to push this patch as-is since it's pretty important, and then I'll do a follow-up with the cleanup paths. -- Chris Lalancette
2010/6/25 Chris Lalancette <clalance@redhat.com>:
There was one major, and a few minor bugs having to do with the reference counting of node devices in daemon/remote.c The major bug was that remoteDispatchNodeDeviceListCaps() was completely failing to unreference node devices; this would lead to many open file descriptors, which would eventually fail.
The minor bugs were along the same lines, but were in rarely used error paths. Still, they should be corrected.
Signed-off-by: Chris Lalancette <clalance@redhat.com> ---
ACK. Matthias
On 06/25/2010 05:22 PM, Chris Lalancette wrote:
There was one major, and a few minor bugs having to do with the reference counting of node devices in daemon/remote.c The major bug was that remoteDispatchNodeDeviceListCaps() was completely failing to unreference node devices; this would lead to many open file descriptors, which would eventually fail.
The minor bugs were along the same lines, but were in rarely used error paths. Still, they should be corrected.
Signed-off-by: Chris Lalancette<clalance@redhat.com> --- daemon/remote.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c index 91faa9a..ec5f85b 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -5012,11 +5012,13 @@ remoteDispatchNodeDeviceGetParent (struct qemud_server *server ATTRIBUTE_UNUSED, /* remoteDispatchClientRequest will free this. */ char **parent_p; if (VIR_ALLOC(parent_p)< 0) { + virNodeDeviceFree(dev); remoteDispatchOOMError(rerr); return -1; } *parent_p = strdup(parent); if (*parent_p == NULL) { + virNodeDeviceFree(dev); remoteDispatchOOMError(rerr); return -1; } @@ -5048,6 +5050,7 @@ remoteDispatchNodeDeviceNumOfCaps (struct qemud_server *server ATTRIBUTE_UNUSED,
ret->num = virNodeDeviceNumOfCaps(dev); if (ret->num< 0) { + virNodeDeviceFree(dev); remoteDispatchConnError(rerr, conn); return -1; } @@ -5076,6 +5079,7 @@ remoteDispatchNodeDeviceListCaps (struct qemud_server *server ATTRIBUTE_UNUSED, }
if (args->maxnames> REMOTE_NODE_DEVICE_NAME_LIST_MAX) { + virNodeDeviceFree(dev); remoteDispatchFormatError(rerr, "%s", _("maxnames> REMOTE_NODE_DEVICE_NAME_LIST_MAX")); return -1; @@ -5083,6 +5087,7 @@ remoteDispatchNodeDeviceListCaps (struct qemud_server *server ATTRIBUTE_UNUSED,
/* Allocate return buffer. */ if (VIR_ALLOC_N(ret->names.names_val, args->maxnames)< 0) { + virNodeDeviceFree(dev); remoteDispatchOOMError(rerr); return -1; } @@ -5091,11 +5096,13 @@ remoteDispatchNodeDeviceListCaps (struct qemud_server *server ATTRIBUTE_UNUSED, virNodeDeviceListCaps (dev, ret->names.names_val, args->maxnames); if (ret->names.names_len == -1) { + virNodeDeviceFree(dev); remoteDispatchConnError(rerr, conn); VIR_FREE(ret->names.names_val); return -1; }
+ virNodeDeviceFree(dev); return 0; }
ACK for the ones that are here. But what about the other NodeDevice functions: remoteDispatchNodeDeviceDettach remoteDispatchNodeDeviceReAttach remoteDispatchNodeDeviceReset remoteDispatchNodeDeviceDestroy Don't those need the same treatment?
2010/6/25 Laine Stump <laine@laine.org>:
On 06/25/2010 05:22 PM, Chris Lalancette wrote:
There was one major, and a few minor bugs having to do with the reference counting of node devices in daemon/remote.c The major bug was that remoteDispatchNodeDeviceListCaps() was completely failing to unreference node devices; this would lead to many open file descriptors, which would eventually fail.
The minor bugs were along the same lines, but were in rarely used error paths. Still, they should be corrected.
Signed-off-by: Chris Lalancette<clalance@redhat.com> ---
ACK for the ones that are here. But what about the other NodeDevice functions:
remoteDispatchNodeDeviceDettach remoteDispatchNodeDeviceReAttach remoteDispatchNodeDeviceReset remoteDispatchNodeDeviceDestroy
Don't those need the same treatment?
Yes. I squashed in my additional ACKed patch for this, tweak the commit message a bit to reflect this and pushed the result. Matthias
participants (5)
-
Chris Lalancette -
Dave Allan -
Eric Blake -
Laine Stump -
Matthias Bolte