Re: [libvirt] [PATCH] esx: Fix memory leak when looking up an non-existing domain by name

2010-08-15 Thread Matthias Bolte 
2010/8/10 Eric Blake ebl...@redhat.com:
 On 08/08/2010 02:56 PM, Matthias Bolte wrote:
 In case an optional object cannot be found the lookup function is
 left early and the cleanup code is not executed. Add a success label
 and goto instead of an early return.

 This pattern occurs in some other functions too.
 ---
  src/esx/esx_vi.c |   14 --
  1 files changed, 8 insertions(+), 6 deletions(-)

 Definitely a leak to plug, but:


 diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
 index c6b2b63..b064b11 100644
 --- a/src/esx/esx_vi.c
 +++ b/src/esx/esx_vi.c
 @@ -2254,7 +2254,7 @@ esxVI_LookupVirtualMachineByUuid(esxVI_Context *ctx, 
 const unsigned char *uuid,

      if (managedObjectReference == NULL) {
          if (occurrence == esxVI_Occurrence_OptionalItem) {
 -            return 0;
 +            goto success;

 Why not use the idiom:

 result = 0;
 goto cleanup;

          } else {
              ESX_VI_ERROR(VIR_ERR_NO_DOMAIN,
                           _(Could not find domain with UUID '%s'),
 @@ -2270,6 +2270,7 @@ esxVI_LookupVirtualMachineByUuid(esxVI_Context *ctx, 
 const unsigned char *uuid,
          goto cleanup;
      }

 +  success:
      result = 0;

    cleanup:

 such that you only have to have one label, instead of adding a new
 label?  All three instances follow the same pattern.


Here's v2 attached. I also removed two already existing success labels.

Matthias
From e98dd282cbe6d92accffb6cde6c9be973dfe10a6 Mon Sep 17 00:00:00 2001
From: Matthias Bolte matthias.bo...@googlemail.com
Date: Sun, 8 Aug 2010 21:32:29 +0200
Subject: [PATCH] esx: Fix memory leak when looking up an non-existing domain by name

In case an optional object cannot be found the lookup function is
left early and the cleanup code is not executed.

This pattern occurs in some other functions too.
---
 src/esx/esx_vi.c |   24 
 1 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
index 55c5246..3773a5f 100644
--- a/src/esx/esx_vi.c
+++ b/src/esx/esx_vi.c
@@ -2254,7 +2254,9 @@ esxVI_LookupVirtualMachineByUuid(esxVI_Context *ctx, const unsigned char *uuid,
 
 if (managedObjectReference == NULL) {
 if (occurrence == esxVI_Occurrence_OptionalItem) {
-return 0;
+result = 0;
+
+goto cleanup;
 } else {
 ESX_VI_ERROR(VIR_ERR_NO_DOMAIN,
  _(Could not find domain with UUID '%s'),
@@ -2327,7 +2329,9 @@ esxVI_LookupVirtualMachineByName(esxVI_Context *ctx, const char *name,
 
 if (*virtualMachine == NULL) {
 if (occurrence == esxVI_Occurrence_OptionalItem) {
-return 0;
+result = 0;
+
+goto cleanup;
 } else {
 ESX_VI_ERROR(VIR_ERR_NO_DOMAIN,
  _(Could not find domain with name '%s'), name);
@@ -2454,8 +2458,10 @@ esxVI_LookupDatastoreByName(esxVI_Context *ctx, const char *name,
 goto cleanup;
 }
 
-// Found datastore with matching name
-goto success;
+/* Found datastore with matching name */
+result = 0;
+
+goto cleanup;
 }
 }
 
@@ -2465,7 +2471,6 @@ esxVI_LookupDatastoreByName(esxVI_Context *ctx, const char *name,
 goto cleanup;
 }
 
-  success:
 result = 0;
 
   cleanup:
@@ -2540,7 +2545,9 @@ esxVI_LookupDatastoreByAbsolutePath(esxVI_Context *ctx,
 }
 
 /* Found datastore with matching mount path */
-goto success;
+result = 0;
+
+goto cleanup;
 }
 }
 }
@@ -2552,7 +2559,6 @@ esxVI_LookupDatastoreByAbsolutePath(esxVI_Context *ctx,
 goto cleanup;
 }
 
-  success:
 result = 0;
 
   cleanup:
@@ -2890,7 +2896,9 @@ esxVI_LookupCurrentSnapshotTree
 
 if (currentSnapshot == NULL) {
 if (occurrence == esxVI_Occurrence_OptionalItem) {
-return 0;
+result = 0;
+
+goto cleanup;
 } else {
 ESX_VI_ERROR(VIR_ERR_NO_DOMAIN_SNAPSHOT, %s,
  _(Domain has no current snapshot));
-- 
1.7.0.4

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

Re: [libvirt] [PATCH 3/3] Remove wrong check for uml monitor response size

2010-08-15 Thread Soren Hansen 
On Sat, Aug 14, 2010 at 11:11:40AM -0600, Eric Blake wrote:
 First of all, for me, the call to recvfrom returns 0, even though the
 buffer actually is populated with the response from the monitor.
 I'm not sure about this one.  The recvfrom is called in a loop, and
 POSIX says that recvfrom shall return the size if a message was read, or
 0 if no messages were available but the other end of the connection did
 an orderly shutdown.

Yes, it does. That's part of why it took me so long to figure out. I really
didn't expect data to come back when recvfrom returned 0 :)

 I think we need a bit more understanding of why you are getting a 0 return,
 and whether it only happens after a successful iteration of the loop (in
 which case the contents of res are leftover from the prior iteration).

Unless multiple threads are reading from the fd (?), I'm quite confident
in my data. I dumped the whole monitor_response buffer right before and
right after the recvfrom call along with the return value of recvfrom.
I'll rerun the tests tomorrow and show you the results.

If my results are accurate and it is indeed a kernel bug (well, or
eglibc or whatnot), I believe it's in the spirit of libvirt to accept
that one of the other parts of the equation is doing something silly and
work around it. :) I'm happy to provide more evidence, though.

I'm not sure if I pointed this out in my initial e-mail, but even if I
didn't have this recvfrom problem, the check seems odd to my eye. Is the
monitor really going to send a max sized datagram each time? I would
have expected it to only send a datagram the size of the header + the
length of the data.

-- 
Soren Hansen so...@linux2go.dk
Systems Architect, The Rackspace Cloud
Ubuntu Developer

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