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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list