[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/5] phyp: Remove stack allocating a 4kb volume key and fix related memory leaks



On Sat, Apr 09, 2011 at 11:59:07AM +0200, Matthias Bolte wrote:
> Don't pre-allocate 4kb per key, make phypVolumeGetKey allocate the memory.
> 
> Make phypBuildVolume return the volume key instead of using pre-allocated
> memory to store it.
> 
> Also fix a memory leak in phypVolumeLookupByName when phypVolumeGetKey
> fails. Fix another memory leak in phypVolumeLookupByPath in the success
> path. Fix phypVolumeGetXMLDesc leaking voldef.key.
> ---
>  src/phyp/phyp_driver.c |   98 ++++++++++++++++++++++++++---------------------
>  src/phyp/phyp_driver.h |    1 -
>  2 files changed, 54 insertions(+), 45 deletions(-)
> 
> diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
> index ddbc103..bd508fb 100644
> --- a/src/phyp/phyp_driver.c
> +++ b/src/phyp/phyp_driver.c
> @@ -2153,8 +2153,8 @@ phypAttachDevice(virDomainPtr domain, const char *xml)
>      return -1;
>  }
>  
> -static int
> -phypVolumeGetKey(virConnectPtr conn, char *key, const char *name)
> +static char *
> +phypVolumeGetKey(virConnectPtr conn, const char *name)
>  {
>      ConnectionData *connection_data = conn->networkPrivateData;
>      phyp_driverPtr phyp_driver = conn->privateData;
> @@ -2182,10 +2182,10 @@ phypVolumeGetKey(virConnectPtr conn, char *key, const char *name)
>      if (virBufferError(&buf)) {
>          virBufferFreeAndReset(&buf);
>          virReportOOMError();
> -        return -1;
> +        return NULL;
>      }
> -    cmd = virBufferContentAndReset(&buf);
>  
> +    cmd = virBufferContentAndReset(&buf);
>      ret = phypExec(session, cmd, &exit_status, conn);
>  
>      if (exit_status < 0 || ret == NULL)
> @@ -2196,17 +2196,13 @@ phypVolumeGetKey(virConnectPtr conn, char *key, const char *name)
>      if (char_ptr)
>          *char_ptr = '\0';
>  
> -    if (memcpy(key, ret, MAX_KEY_SIZE) == NULL)
> -        goto err;
> -
>      VIR_FREE(cmd);
> -    VIR_FREE(ret);
> -    return 0;
> +    return ret;
>  
>    err:
>      VIR_FREE(cmd);
>      VIR_FREE(ret);
> -    return -1;
> +    return NULL;
>  }
>  
>  static char *
> @@ -2313,9 +2309,9 @@ phypGetStoragePoolSize(virConnectPtr conn, char *name)
>      return -1;
>  }
>  
> -static int
> +static char *
>  phypBuildVolume(virConnectPtr conn, const char *lvname, const char *spname,
> -                unsigned int capacity, char *key)
> +                unsigned int capacity)
>  {
>      ConnectionData *connection_data = conn->networkPrivateData;
>      phyp_driverPtr phyp_driver = conn->privateData;
> @@ -2327,6 +2323,7 @@ phypBuildVolume(virConnectPtr conn, const char *lvname, const char *spname,
>      char *ret = NULL;
>      int exit_status = 0;
>      virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    char *key;
>  
>      if (system_type == HMC)
>          virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '",
> @@ -2340,10 +2337,10 @@ phypBuildVolume(virConnectPtr conn, const char *lvname, const char *spname,
>      if (virBufferError(&buf)) {
>          virBufferFreeAndReset(&buf);
>          virReportOOMError();
> -        return -1;
> +        return NULL;
>      }
> -    cmd = virBufferContentAndReset(&buf);
>  
> +    cmd = virBufferContentAndReset(&buf);
>      ret = phypExec(session, cmd, &exit_status, conn);
>  
>      if (exit_status < 0) {
> @@ -2351,29 +2348,37 @@ phypBuildVolume(virConnectPtr conn, const char *lvname, const char *spname,
>          goto err;
>      }
>  
> -    if (phypVolumeGetKey(conn, key, lvname) == -1)
> -        goto err;;
> +    key = phypVolumeGetKey(conn, lvname);
> +
> +    if (key == NULL)
> +        goto err;
>  
>      VIR_FREE(cmd);
>      VIR_FREE(ret);
> -    return 0;
> +    return key;
>  
>    err:
>      VIR_FREE(cmd);
>      VIR_FREE(ret);
> -    return -1;
> +    return NULL;
>  }
>  
>  static virStorageVolPtr
>  phypVolumeLookupByName(virStoragePoolPtr pool, const char *volname)
>  {
> +    char *key;
> +    virStorageVolPtr vol;
>  
> -    char key[MAX_KEY_SIZE];
> +    key = phypVolumeGetKey(pool->conn, volname);
>  
> -    if (phypVolumeGetKey(pool->conn, key, volname) == -1)
> +    if (key == NULL)
>          return NULL;
>  
> -    return virGetStorageVol(pool->conn, pool->name, volname, key);
> +    vol = virGetStorageVol(pool->conn, pool->name, volname, key);
> +
> +    VIR_FREE(key);
> +
> +    return vol;
>  }
>  
>  static virStorageVolPtr
> @@ -2392,11 +2397,6 @@ phypStorageVolCreateXML(virStoragePoolPtr pool,
>          return NULL;
>      }
>  
> -    if (VIR_ALLOC_N(key, MAX_KEY_SIZE) < 0) {
> -        virReportOOMError();
> -        return NULL;
> -    }
> -
>      /* Filling spdef manually
>       * */
>      if (pool->name != NULL) {
> @@ -2454,9 +2454,10 @@ phypStorageVolCreateXML(virStoragePoolPtr pool,
>          goto err;
>      }
>  
> -    if (phypBuildVolume
> -        (pool->conn, voldef->name, spdef->name, voldef->capacity,
> -         key) == -1)
> +    key = phypBuildVolume(pool->conn, voldef->name, spdef->name,
> +                          voldef->capacity);
> +
> +    if (key == NULL)
>          goto err;
>  
>      if ((vol =
> @@ -2464,9 +2465,12 @@ phypStorageVolCreateXML(virStoragePoolPtr pool,
>                            key)) == NULL)
>          goto err;
>  
> +    VIR_FREE(key);
> +
>      return vol;
>  
>    err:
> +    VIR_FREE(key);
>      virStorageVolDefFree(voldef);
>      virStoragePoolDefFree(spdef);
>      if (vol)
> @@ -2540,8 +2544,10 @@ phypVolumeLookupByPath(virConnectPtr conn, const char *volname)
>      int exit_status = 0;
>      char *cmd = NULL;
>      char *spname = NULL;
> +    char *char_ptr;
>      char *key = NULL;
>      virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    virStorageVolPtr vol;
>  
>      if (system_type == HMC)
>          virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '",
> @@ -2566,20 +2572,21 @@ phypVolumeLookupByPath(virConnectPtr conn, const char *volname)
>      if (exit_status < 0 || spname == NULL)
>          return NULL;
>  
> -    char *char_ptr = strchr(spname, '\n');
> +    char_ptr = strchr(spname, '\n');
>  
>      if (char_ptr)
>          *char_ptr = '\0';
>  
> -    if (VIR_ALLOC_N(key, MAX_KEY_SIZE) < 0) {
> -        virReportOOMError();
> -        return NULL;
> -    }
> +    key = phypVolumeGetKey(conn, volname);
>  
> -    if (phypVolumeGetKey(conn, key, volname) == -1)
> +    if (key == NULL)
>          return NULL;
>  
> -    return virGetStorageVol(conn, spname, volname, key);
> +    vol = virGetStorageVol(conn, spname, volname, key);
> +
> +    VIR_FREE(key);
> +
> +    return vol;
>  }
>  
>  static int
> @@ -2647,6 +2654,8 @@ phypStoragePoolLookupByName(virConnectPtr conn, const char *name)
>  static char *
>  phypVolumeGetXMLDesc(virStorageVolPtr vol, unsigned int flags)
>  {
> +    char *xml;
> +
>      virCheckFlags(0, NULL);
>  
>      virStorageVolDef voldef;
> @@ -2661,11 +2670,6 @@ phypVolumeGetXMLDesc(virStorageVolPtr vol, unsigned int flags)
>      virStoragePoolDef pool;
>      memset(&pool, 0, sizeof(virStoragePoolDef));
>  
> -    if (VIR_ALLOC_N(voldef.key, MAX_KEY_SIZE) < 0) {
> -        virReportOOMError();
> -        return NULL;
> -    }
> -
>      if (sp->name != NULL) {
>          pool.name = sp->name;
>      } else {
> @@ -2702,14 +2706,20 @@ phypVolumeGetXMLDesc(virStorageVolPtr vol, unsigned int flags)
>          goto err;
>      }
>  
> -    if (memmove(voldef.key, vol->key, PATH_MAX) == NULL) {
> -        VIR_ERROR0(_("Unable to determine volume's key."));
> +    voldef.key = strdup(vol->key);
> +
> +    if (voldef.key == NULL) {
> +        virReportOOMError();
>          goto err;
>      }
>  
>      voldef.type = VIR_STORAGE_POOL_LOGICAL;
>  
> -    return virStorageVolDefFormat(&pool, &voldef);
> +    xml = virStorageVolDefFormat(&pool, &voldef);
> +
> +    VIR_FREE(voldef.key);
> +
> +    return xml;
>  
>    err:
>      return NULL;
> diff --git a/src/phyp/phyp_driver.h b/src/phyp/phyp_driver.h
> index bc8e003..a22156c 100644
> --- a/src/phyp/phyp_driver.h
> +++ b/src/phyp/phyp_driver.h
> @@ -30,7 +30,6 @@
>  # include <config.h>
>  # include <libssh2.h>
>  
> -# define MAX_KEY_SIZE (1024*4)
>  # define LPAR_EXEC_ERR -1
>  # define SSH_CONN_ERR -2         /* error while trying to connect to remote host */
>  # define SSH_CMD_ERR -3          /* error while trying to execute the remote cmd */

  ACK, way nicer !

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]