[libvirt] [PATCH 1/5] phyp: Remove stack allocating a 4kb volume key and fix related memory leaks
Daniel Veillard
veillard at redhat.com
Mon Apr 11 02:30:26 UTC 2011
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 at veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
More information about the libvir-list
mailing list