[libvirt] [PATCH 0/5] phyp: Cleanups and memory leak fixes - Devel - Libvirt List Archives

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

December

[libvirt] [PATCH 0/5] phyp: Cleanups and memory leak fixes

[libvirt] [PATCH] Fix gcc 4.6...

[libvirt] [libvirt PATCH v3]...

Matthias Bolte

Saturday, 9 April 2011 Sat, 9 Apr '11

4:59 a.m.

This series covers several things I came across while improving the semantic of phypVolumeGetKey. Matthias

Reply

Show replies by date

Matthias Bolte

Saturday, 9 April Sat, 9 Apr

4:59 a.m.

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

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 */ -- 1.7.0.4

Reply

Daniel Veillard

Sunday, 10 April Sun, 10 Apr

9:30 p.m.

New subject: [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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

Reply

Matthias Bolte

Thursday, 14 April Thu, 14 Apr

5:56 a.m.

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

2011/4/11 Daniel Veillard <veillard(a)redhat.com>:

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(-) >

ACK, way nicer ! Daniel

Thanks, pushed. Matthias

Reply

Matthias Bolte

Saturday, 9 April Sat, 9 Apr

4:59 a.m.

New subject: [libvirt] [PATCH 2/5] phyp: Reduce code duplication in error and success paths

Also fix memory leaks along the way in phypCreateServerSCSIAdapter and phypAttachDevice. --- src/phyp/phyp_driver.c | 592 +++++++++++++++++++++++------------------------- 1 files changed, 289 insertions(+), 303 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index bd508fb..f441261 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -109,7 +109,7 @@ waitsocket(int socket_fd, LIBSSH2_SESSION * session) /* this function is the layer that manipulates the ssh channel itself * and executes the commands on the remote machine */ static char * -phypExec(LIBSSH2_SESSION * session, char *cmd, int *exit_status, +phypExec(LIBSSH2_SESSION *session, const char *cmd, int *exit_status, virConnectPtr conn) { LIBSSH2_CHANNEL *channel; @@ -249,19 +249,16 @@ phypGetVIOSPartitionID(virConnectPtr conn) ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; if (virStrToLong_i(ret, &char_ptr, 10, &id) == -1) - goto err; + goto cleanup; + cleanup: VIR_FREE(cmd); VIR_FREE(ret); - return id; - err: - VIR_FREE(cmd); - VIR_FREE(ret); - return -1; + return id; } static virCapsPtr @@ -324,7 +321,7 @@ phypNumDomainsGeneric(virConnectPtr conn, unsigned int type) LIBSSH2_SESSION *session = connection_data->session; int system_type = phyp_driver->system_type; int exit_status = 0; - int ndom = 0; + int ndom = -1; char *char_ptr; char *cmd = NULL; char *ret = NULL; @@ -358,19 +355,16 @@ phypNumDomainsGeneric(virConnectPtr conn, unsigned int type) ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; if (virStrToLong_i(ret, &char_ptr, 10, &ndom) == -1) - goto err; + goto cleanup; + cleanup: VIR_FREE(cmd); VIR_FREE(ret); - return ndom; - err: - VIR_FREE(cmd); - VIR_FREE(ret); - return -1; + return ndom; } /* This is a generic function that won't be used directly by @@ -417,7 +411,7 @@ phypListDomainsGeneric(virConnectPtr conn, int *ids, int nids, ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; /* I need to parse the textual return in order to get the ids */ line = ret; @@ -426,7 +420,7 @@ phypListDomainsGeneric(virConnectPtr conn, int *ids, int nids, if (virStrToLong_i(line, &next_line, 10, &ids[got]) == -1) { VIR_ERROR(_("Cannot parse number from '%s'"), line); got = -1; - goto err; + goto cleanup; } got++; line = next_line; @@ -434,9 +428,10 @@ phypListDomainsGeneric(virConnectPtr conn, int *ids, int nids, line++; /* skip \n */ } - err: + cleanup: VIR_FREE(cmd); VIR_FREE(ret); + return got; } @@ -1300,7 +1295,7 @@ phypGetLparID(LIBSSH2_SESSION * session, const char *managed_system, phyp_driverPtr phyp_driver = conn->privateData; int system_type = phyp_driver->system_type; int exit_status = 0; - int lpar_id = 0; + int lpar_id = -1; char *char_ptr; char *cmd = NULL; char *ret = NULL; @@ -1320,19 +1315,16 @@ phypGetLparID(LIBSSH2_SESSION * session, const char *managed_system, ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; if (virStrToLong_i(ret, &char_ptr, 10, &lpar_id) == -1) - goto err; + goto cleanup; + cleanup: VIR_FREE(cmd); VIR_FREE(ret); - return lpar_id; - err: - VIR_FREE(cmd); - VIR_FREE(ret); - return -1; + return lpar_id; } /* return the lpar name given a lpar_id and a managed system name */ @@ -1361,21 +1353,20 @@ phypGetLparNAME(LIBSSH2_SESSION * session, const char *managed_system, ret = phypExec(session, cmd, &exit_status, conn); - if (exit_status < 0 || ret == NULL) - goto err; + if (exit_status < 0 || ret == NULL) { + VIR_FREE(ret); + goto cleanup; + } char_ptr = strchr(ret, '\n'); if (char_ptr) *char_ptr = '\0'; + cleanup: VIR_FREE(cmd); - return ret; - err: - VIR_FREE(cmd); - VIR_FREE(ret); - return NULL; + return ret; } @@ -1442,7 +1433,7 @@ phypGetLparMem(virConnectPtr conn, const char *managed_system, int lpar_id, ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; char_ptr = strchr(ret, '\n'); @@ -1450,17 +1441,13 @@ phypGetLparMem(virConnectPtr conn, const char *managed_system, int lpar_id, *char_ptr = '\0'; if (virStrToLong_i(ret, &char_ptr, 10, &memory) == -1) - goto err; - - VIR_FREE(cmd); - VIR_FREE(ret); - return memory; + goto cleanup; - err: + cleanup: VIR_FREE(cmd); VIR_FREE(ret); - return 0; + return memory; } static unsigned long @@ -1494,7 +1481,7 @@ phypGetLparCPUGeneric(virConnectPtr conn, const char *managed_system, ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; char_ptr = strchr(ret, '\n'); @@ -1502,16 +1489,13 @@ phypGetLparCPUGeneric(virConnectPtr conn, const char *managed_system, *char_ptr = '\0'; if (virStrToLong_i(ret, &char_ptr, 10, &vcpus) == -1) - goto err; + goto cleanup; + cleanup: VIR_FREE(cmd); VIR_FREE(ret); - return (unsigned long) vcpus; - err: - VIR_FREE(cmd); - VIR_FREE(ret); - return 0; + return vcpus; } static unsigned long @@ -1552,7 +1536,7 @@ phypGetRemoteSlot(virConnectPtr conn, const char *managed_system, char *cmd = NULL; char *ret = NULL; char *char_ptr; - int remote_slot = 0; + int remote_slot = -1; int exit_status = 0; virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -1571,7 +1555,7 @@ phypGetRemoteSlot(virConnectPtr conn, const char *managed_system, ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; char_ptr = strchr(ret, '\n'); @@ -1579,16 +1563,13 @@ phypGetRemoteSlot(virConnectPtr conn, const char *managed_system, *char_ptr = '\0'; if (virStrToLong_i(ret, &char_ptr, 10, &remote_slot) == -1) - goto err; + goto cleanup; + cleanup: VIR_FREE(cmd); VIR_FREE(ret); - return remote_slot; - err: - VIR_FREE(cmd); - VIR_FREE(ret); - return -1; + return remote_slot; } /* XXX - is this needed? */ @@ -1629,7 +1610,7 @@ phypGetBackingDevice(virConnectPtr conn, const char *managed_system, ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; /* here is a little trick to deal returns of this kind: * @@ -1645,13 +1626,13 @@ phypGetBackingDevice(virConnectPtr conn, const char *managed_system, if (char_ptr[0] == '/') char_ptr++; else - goto err; + goto cleanup; backing_device = strdup(char_ptr); if (backing_device == NULL) { virReportOOMError(); - goto err; + goto cleanup; } } else { backing_device = ret; @@ -1663,14 +1644,11 @@ phypGetBackingDevice(virConnectPtr conn, const char *managed_system, if (char_ptr) *char_ptr = '\0'; + cleanup: VIR_FREE(cmd); VIR_FREE(ret); - return backing_device; - err: - VIR_FREE(cmd); - VIR_FREE(ret); - return NULL; + return backing_device; } static char * @@ -1702,21 +1680,20 @@ phypGetLparProfile(virConnectPtr conn, int lpar_id) ret = phypExec(session, cmd, &exit_status, conn); - if (exit_status < 0 || ret == NULL) - goto err; + if (exit_status < 0 || ret == NULL) { + VIR_FREE(ret); + goto cleanup; + } char_ptr = strchr(ret, '\n'); if (char_ptr) *char_ptr = '\0'; + cleanup: VIR_FREE(cmd); - return ret; - err: - VIR_FREE(cmd); - VIR_FREE(ret); - return NULL; + return ret; } static int @@ -1733,12 +1710,12 @@ phypGetVIOSNextSlotNumber(virConnectPtr conn) char *cmd = NULL; char *ret = NULL; char *profile = NULL; - int slot = 0; + int slot = -1; virBuffer buf = VIR_BUFFER_INITIALIZER; if (!(profile = phypGetLparProfile(conn, vios_id))) { VIR_ERROR0(_("Unable to get VIOS profile name.")); - goto err; + return -1; } virBufferAddLit(&buf, "lssyscfg"); @@ -1756,7 +1733,7 @@ phypGetVIOSNextSlotNumber(virConnectPtr conn) if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - return -1; + goto cleanup; } cmd = virBufferContentAndReset(&buf); @@ -1764,24 +1741,25 @@ phypGetVIOSNextSlotNumber(virConnectPtr conn) ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) - goto err; + goto cleanup; - VIR_FREE(cmd); - VIR_FREE(ret); - return slot + 1; + slot += 1; - err: + cleanup: + VIR_FREE(profile); VIR_FREE(cmd); VIR_FREE(ret); - return -1; + + return slot; } static int phypCreateServerSCSIAdapter(virConnectPtr conn) { + int result = -1; ConnectionData *connection_data = conn->networkPrivateData; phyp_driverPtr phyp_driver = conn->privateData; LIBSSH2_SESSION *session = connection_data->session; @@ -1800,17 +1778,17 @@ phypCreateServerSCSIAdapter(virConnectPtr conn) (vios_name = phypGetLparNAME(session, managed_system, vios_id, conn))) { VIR_ERROR0(_("Unable to get VIOS name")); - goto err; + goto cleanup; } if (!(profile = phypGetLparProfile(conn, vios_id))) { VIR_ERROR0(_("Unable to get VIOS profile name.")); - goto err; + goto cleanup; } if ((slot = phypGetVIOSNextSlotNumber(conn)) == -1) { VIR_ERROR0(_("Unable to get free slot number")); - goto err; + goto cleanup; } /* Listing all the virtual_scsi_adapter interfaces, the new adapter must @@ -1825,14 +1803,14 @@ phypCreateServerSCSIAdapter(virConnectPtr conn) if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - return -1; + goto cleanup; } cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; /* Here I change the VIOS configuration to append the new adapter * with the free slot I got with phypGetVIOSNextSlotNumber. @@ -1846,14 +1824,17 @@ phypCreateServerSCSIAdapter(virConnectPtr conn) if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - return -1; + goto cleanup; } - cmd = virBufferContentAndReset(&buf); + VIR_FREE(cmd); + VIR_FREE(ret); + + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; /* Finally I add the new scsi adapter to VIOS using the same slot * I used in the VIOS configuration. @@ -1867,27 +1848,27 @@ phypCreateServerSCSIAdapter(virConnectPtr conn) if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - return -1; + goto cleanup; } - cmd = virBufferContentAndReset(&buf); + VIR_FREE(cmd); + VIR_FREE(ret); + + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; - VIR_FREE(profile); - VIR_FREE(vios_name); - VIR_FREE(cmd); - VIR_FREE(ret); - return 0; + result = 0; - err: + cleanup: VIR_FREE(profile); VIR_FREE(vios_name); VIR_FREE(cmd); VIR_FREE(ret); - return -1; + + return result; } static char * @@ -1925,28 +1906,27 @@ phypGetVIOSFreeSCSIAdapter(virConnectPtr conn) ret = phypExec(session, cmd, &exit_status, conn); - if (exit_status < 0 || ret == NULL) - goto err; + if (exit_status < 0 || ret == NULL) { + VIR_FREE(ret); + goto cleanup; + } char_ptr = strchr(ret, '\n'); if (char_ptr) *char_ptr = '\0'; + cleanup: VIR_FREE(cmd); - return ret; - err: - VIR_FREE(cmd); - VIR_FREE(ret); - return NULL; + return ret; } static int phypAttachDevice(virDomainPtr domain, const char *xml) { - + int result = -1; virConnectPtr conn = domain->conn; ConnectionData *connection_data = domain->conn->networkPrivateData; phyp_driverPtr phyp_driver = domain->conn->privateData; @@ -1982,21 +1962,21 @@ phypAttachDevice(virDomainPtr domain, const char *xml) if (def->os.type == NULL) { virReportOOMError(); - goto err; + goto cleanup; } dev = virDomainDeviceDefParse(phyp_driver->caps, def, xml, VIR_DOMAIN_XML_INACTIVE); if (!dev) { virReportOOMError(); - goto err; + goto cleanup; } if (! (vios_name = phypGetLparNAME(session, managed_system, vios_id, conn))) { VIR_ERROR0(_("Unable to get VIOS name")); - goto err; + goto cleanup; } /* First, let's look for a free SCSI Adapter @@ -2006,11 +1986,11 @@ phypAttachDevice(virDomainPtr domain, const char *xml) * */ if (phypCreateServerSCSIAdapter(conn) == -1) { VIR_ERROR0(_("Unable to create new virtual adapter")); - goto err; + goto cleanup; } else { if (!(scsi_adapter = phypGetVIOSFreeSCSIAdapter(conn))) { VIR_ERROR0(_("Unable to create new virtual adapter")); - goto err; + goto cleanup; } } } @@ -2028,18 +2008,21 @@ phypAttachDevice(virDomainPtr domain, const char *xml) if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - return -1; + goto cleanup; } - cmd = virBufferContentAndReset(&buf); + VIR_FREE(cmd); + VIR_FREE(ret); + + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; if (!(profile = phypGetLparProfile(conn, domain->id))) { VIR_ERROR0(_("Unable to get VIOS profile name.")); - goto err; + goto cleanup; } /* Let's get the slot number for the adapter we just created @@ -2053,17 +2036,20 @@ phypAttachDevice(virDomainPtr domain, const char *xml) if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - return -1; + goto cleanup; } - cmd = virBufferContentAndReset(&buf); + VIR_FREE(cmd); + VIR_FREE(ret); + + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) - goto err; + goto cleanup; /* Listing all the virtual_scsi_adapter interfaces, the new adapter must * be appended to this list @@ -2078,14 +2064,17 @@ phypAttachDevice(virDomainPtr domain, const char *xml) if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - return -1; + goto cleanup; } - cmd = virBufferContentAndReset(&buf); + VIR_FREE(cmd); + VIR_FREE(ret); + + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; /* Here I change the LPAR configuration to append the new adapter * with the new slot we just created @@ -2101,14 +2090,17 @@ phypAttachDevice(virDomainPtr domain, const char *xml) if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - return -1; + goto cleanup; } - cmd = virBufferContentAndReset(&buf); + VIR_FREE(cmd); + VIR_FREE(ret); + + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) - goto err; + goto cleanup; /* Finally I add the new scsi adapter to VIOS using the same slot * I used in the VIOS configuration. @@ -2122,35 +2114,35 @@ phypAttachDevice(virDomainPtr domain, const char *xml) if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - return -1; + goto cleanup; } - cmd = virBufferContentAndReset(&buf); + VIR_FREE(cmd); + VIR_FREE(ret); + + cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) { VIR_ERROR0(_ ("Possibly you don't have IBM Tools installed in your LPAR." "Contact your support to enable this feature.")); - goto err; + goto cleanup; } - VIR_FREE(cmd); - VIR_FREE(ret); - VIR_FREE(def); - VIR_FREE(dev); - VIR_FREE(vios_name); - VIR_FREE(scsi_adapter); - return 0; + result = 0; - err: + cleanup: VIR_FREE(cmd); VIR_FREE(ret); - VIR_FREE(def); - VIR_FREE(dev); + virDomainDeviceDefFree(dev); + virDomainDefFree(def); VIR_FREE(vios_name); VIR_FREE(scsi_adapter); - return -1; + VIR_FREE(profile); + VIR_FREE(domain_name); + + return result; } static char * @@ -2188,21 +2180,20 @@ phypVolumeGetKey(virConnectPtr conn, const char *name) cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, conn); - if (exit_status < 0 || ret == NULL) - goto err; + if (exit_status < 0 || ret == NULL) { + VIR_FREE(ret); + goto cleanup; + } char_ptr = strchr(ret, '\n'); if (char_ptr) *char_ptr = '\0'; + cleanup: VIR_FREE(cmd); - return ret; - err: - VIR_FREE(cmd); - VIR_FREE(ret); - return NULL; + return ret; } static char * @@ -2240,21 +2231,20 @@ phypGetStoragePoolDevice(virConnectPtr conn, char *name) ret = phypExec(session, cmd, &exit_status, conn); - if (exit_status < 0 || ret == NULL) - goto err; + if (exit_status < 0 || ret == NULL) { + VIR_FREE(ret); + goto cleanup; + } char_ptr = strchr(ret, '\n'); if (char_ptr) *char_ptr = '\0'; + cleanup: VIR_FREE(cmd); - return ret; - err: - VIR_FREE(cmd); - VIR_FREE(ret); - return NULL; + return ret; } static unsigned long int @@ -2269,7 +2259,7 @@ phypGetStoragePoolSize(virConnectPtr conn, char *name) int vios_id = phyp_driver->vios_id; char *cmd = NULL; char *ret = NULL; - int sp_size = 0; + int sp_size = -1; char *char_ptr; virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -2294,19 +2284,16 @@ phypGetStoragePoolSize(virConnectPtr conn, char *name) ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; if (virStrToLong_i(ret, &char_ptr, 10, &sp_size) == -1) - goto err; + goto cleanup; + cleanup: VIR_FREE(cmd); VIR_FREE(ret); - return sp_size; - err: - VIR_FREE(cmd); - VIR_FREE(ret); - return -1; + return sp_size; } static char * @@ -2323,7 +2310,7 @@ phypBuildVolume(virConnectPtr conn, const char *lvname, const char *spname, char *ret = NULL; int exit_status = 0; virBuffer buf = VIR_BUFFER_INITIALIZER; - char *key; + char *key = NULL; if (system_type == HMC) virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", @@ -2345,22 +2332,19 @@ phypBuildVolume(virConnectPtr conn, const char *lvname, const char *spname, if (exit_status < 0) { VIR_ERROR(_("Unable to create Volume: %s"), ret); - goto err; + goto cleanup; } key = phypVolumeGetKey(conn, lvname); if (key == NULL) - goto err; + goto cleanup; + cleanup: VIR_FREE(cmd); VIR_FREE(ret); - return key; - err: - VIR_FREE(cmd); - VIR_FREE(ret); - return NULL; + return key; } static virStorageVolPtr @@ -2514,22 +2498,20 @@ phypVolumeGetPhysicalVolumeByStoragePool(virStorageVolPtr vol, char *sp) ret = phypExec(session, cmd, &exit_status, conn); - if (exit_status < 0 || ret == NULL) - goto err; + if (exit_status < 0 || ret == NULL) { + VIR_FREE(ret); + goto cleanup; + } char_ptr = strchr(ret, '\n'); if (char_ptr) *char_ptr = '\0'; + cleanup: VIR_FREE(cmd); - return ret; - - err: - VIR_FREE(cmd); - VIR_FREE(ret); - return NULL; + return ret; } static virStorageVolPtr @@ -2547,7 +2529,7 @@ phypVolumeLookupByPath(virConnectPtr conn, const char *volname) char *char_ptr; char *key = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; - virStorageVolPtr vol; + virStorageVolPtr vol = NULL; if (system_type == HMC) virBufferVSprintf(&buf, "viosvrcmd -m %s --id %d -c '", @@ -2570,7 +2552,7 @@ phypVolumeLookupByPath(virConnectPtr conn, const char *volname) spname = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || spname == NULL) - return NULL; + goto cleanup; char_ptr = strchr(spname, '\n'); @@ -2580,10 +2562,13 @@ phypVolumeLookupByPath(virConnectPtr conn, const char *volname) key = phypVolumeGetKey(conn, volname); if (key == NULL) - return NULL; + goto cleanup; vol = virGetStorageVol(conn, spname, volname, key); + cleanup: + VIR_FREE(cmd); + VIR_FREE(spname); VIR_FREE(key); return vol; @@ -2593,6 +2578,7 @@ static int phypGetStoragePoolUUID(virConnectPtr conn, unsigned char *uuid, const char *name) { + int result = -1; ConnectionData *connection_data = conn->networkPrivateData; phyp_driverPtr phyp_driver = conn->privateData; LIBSSH2_SESSION *session = connection_data->session; @@ -2625,19 +2611,18 @@ phypGetStoragePoolUUID(virConnectPtr conn, unsigned char *uuid, ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; if (memmove(uuid, ret, VIR_UUID_BUFLEN) == NULL) - goto err; + goto cleanup; - VIR_FREE(cmd); - VIR_FREE(ret); - return 0; + result = 0; - err: + cleanup: VIR_FREE(cmd); VIR_FREE(ret); - return -1; + + return result; } static virStoragePoolPtr @@ -2654,22 +2639,21 @@ phypStoragePoolLookupByName(virConnectPtr conn, const char *name) static char * phypVolumeGetXMLDesc(virStorageVolPtr vol, unsigned int flags) { + virStorageVolDef voldef; + virStoragePoolDef pool; + virStoragePoolPtr sp; char *xml; virCheckFlags(0, NULL); - virStorageVolDef voldef; memset(&voldef, 0, sizeof(virStorageVolDef)); + memset(&pool, 0, sizeof(virStoragePoolDef)); - virStoragePoolPtr sp = - phypStoragePoolLookupByName(vol->conn, vol->pool); + sp = phypStoragePoolLookupByName(vol->conn, vol->pool); if (!sp) goto err; - virStoragePoolDef pool; - memset(&pool, 0, sizeof(virStoragePoolDef)); - if (sp->name != NULL) { pool.name = sp->name; } else { @@ -2773,7 +2757,7 @@ phypVolumeGetPath(virStorageVolPtr vol) sp = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || sp == NULL) - goto err; + goto cleanup; char_ptr = strchr(sp, '\n'); @@ -2782,23 +2766,20 @@ phypVolumeGetPath(virStorageVolPtr vol) pv = phypVolumeGetPhysicalVolumeByStoragePool(vol, sp); - if (pv) { - if (virAsprintf(&path, "/%s/%s/%s", pv, sp, vol->name) < 0) { - virReportOOMError(); - goto err; - } - } else { - goto err; - } + if (!pv) + goto cleanup; - VIR_FREE(cmd); - return path; + if (virAsprintf(&path, "/%s/%s/%s", pv, sp, vol->name) < 0) { + virReportOOMError(); + goto cleanup; + } - err: + cleanup: VIR_FREE(cmd); VIR_FREE(sp); VIR_FREE(path); - return NULL; + + return path; } @@ -2806,6 +2787,7 @@ static int phypStoragePoolListVolumes(virStoragePoolPtr pool, char **const volumes, int nvolumes) { + bool success = false; virConnectPtr conn = pool->conn; ConnectionData *connection_data = conn->networkPrivateData; phyp_driverPtr phyp_driver = conn->privateData; @@ -2844,7 +2826,7 @@ phypStoragePoolListVolumes(virStoragePoolPtr pool, char **const volumes, /* I need to parse the textual return in order to get the volumes */ if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; else { volumes_list = ret; @@ -2855,7 +2837,7 @@ phypStoragePoolListVolumes(virStoragePoolPtr pool, char **const volumes, *char_ptr2 = '\0'; if ((volumes[got++] = strdup(volumes_list)) == NULL) { virReportOOMError(); - goto err; + goto cleanup; } char_ptr2++; volumes_list = char_ptr2; @@ -2864,16 +2846,20 @@ phypStoragePoolListVolumes(virStoragePoolPtr pool, char **const volumes, } } - VIR_FREE(cmd); - VIR_FREE(ret); - return got; + success = true; + + cleanup: + if (!success) { + for (i = 0; i < got; i++) + VIR_FREE(volumes[i]); + + got = -1; + } - err: - for (i = 0; i < got; i++) - VIR_FREE(volumes[i]); VIR_FREE(cmd); VIR_FREE(ret); - return -1; + + return got; } static int @@ -2885,7 +2871,7 @@ phypStoragePoolNumOfVolumes(virStoragePoolPtr pool) LIBSSH2_SESSION *session = connection_data->session; int system_type = phyp_driver->system_type; int exit_status = 0; - int nvolumes = 0; + int nvolumes = -1; char *cmd = NULL; char *ret = NULL; char *managed_system = phyp_driver->managed_system; @@ -2911,27 +2897,25 @@ phypStoragePoolNumOfVolumes(virStoragePoolPtr pool) ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; if (virStrToLong_i(ret, &char_ptr, 10, &nvolumes) == -1) - goto err; + goto cleanup; /* We need to remove 2 line from the header text output */ nvolumes -= 2; + cleanup: VIR_FREE(cmd); VIR_FREE(ret); - return nvolumes; - err: - VIR_FREE(cmd); - VIR_FREE(ret); - return -1; + return nvolumes; } static int phypDestroyStoragePool(virStoragePoolPtr pool) { + int result = -1; virConnectPtr conn = pool->conn; ConnectionData *connection_data = conn->networkPrivateData; phyp_driverPtr phyp_driver = conn->privateData; @@ -2965,29 +2949,29 @@ phypDestroyStoragePool(virStoragePoolPtr pool) "'rmsp %s'", managed_system, vios_id, pool->name) < 0) { virReportOOMError(); - goto err; + goto cleanup; } ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0) { VIR_ERROR(_("Unable to create Storage Pool: %s"), ret); - goto err; + goto cleanup; } - VIR_FREE(cmd); - VIR_FREE(ret); - return 0; + result = 0; - err: + cleanup: VIR_FREE(cmd); VIR_FREE(ret); - return -1; + + return result; } static int phypBuildStoragePool(virConnectPtr conn, virStoragePoolDefPtr def) { + int result = -1; ConnectionData *connection_data = conn->networkPrivateData; phyp_driverPtr phyp_driver = conn->privateData; LIBSSH2_SESSION *session = connection_data->session; @@ -3021,17 +3005,16 @@ phypBuildStoragePool(virConnectPtr conn, virStoragePoolDefPtr def) if (exit_status < 0) { VIR_ERROR(_("Unable to create Storage Pool: %s"), ret); - goto err; + goto cleanup; } - VIR_FREE(cmd); - VIR_FREE(ret); - return 0; + result = 0; - err: + cleanup: VIR_FREE(cmd); VIR_FREE(ret); - return -1; + + return result; } @@ -3043,7 +3026,7 @@ phypNumOfStoragePools(virConnectPtr conn) LIBSSH2_SESSION *session = connection_data->session; int system_type = phyp_driver->system_type; int exit_status = 0; - int nsp = 0; + int nsp = -1; char *cmd = NULL; char *ret = NULL; char *managed_system = phyp_driver->managed_system; @@ -3072,24 +3055,22 @@ phypNumOfStoragePools(virConnectPtr conn) ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; if (virStrToLong_i(ret, &char_ptr, 10, &nsp) == -1) - goto err; + goto cleanup; + cleanup: VIR_FREE(cmd); VIR_FREE(ret); - return nsp; - err: - VIR_FREE(cmd); - VIR_FREE(ret); - return -1; + return nsp; } static int phypListStoragePools(virConnectPtr conn, char **const pools, int npools) { + bool success = false; ConnectionData *connection_data = conn->networkPrivateData; phyp_driverPtr phyp_driver = conn->privateData; LIBSSH2_SESSION *session = connection_data->session; @@ -3125,7 +3106,7 @@ phypListStoragePools(virConnectPtr conn, char **const pools, int npools) /* I need to parse the textual return in order to get the storage pools */ if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; else { storage_pools = ret; @@ -3136,7 +3117,7 @@ phypListStoragePools(virConnectPtr conn, char **const pools, int npools) *char_ptr2 = '\0'; if ((pools[got++] = strdup(storage_pools)) == NULL) { virReportOOMError(); - goto err; + goto cleanup; } char_ptr2++; storage_pools = char_ptr2; @@ -3145,16 +3126,20 @@ phypListStoragePools(virConnectPtr conn, char **const pools, int npools) } } - VIR_FREE(cmd); - VIR_FREE(ret); - return got; + success = true; + + cleanup: + if (!success) { + for (i = 0; i < got; i++) + VIR_FREE(pools[i]); + + got = -1; + } - err: - for (i = 0; i < got; i++) - VIR_FREE(pools[i]); VIR_FREE(cmd); VIR_FREE(ret); - return -1; + + return got; } static virStoragePoolPtr @@ -3421,6 +3406,7 @@ phypListDomains(virConnectPtr conn, int *ids, int nids) static int phypListDefinedDomains(virConnectPtr conn, char **const names, int nnames) { + bool success = false; ConnectionData *connection_data = conn->networkPrivateData; phyp_driverPtr phyp_driver = conn->privateData; LIBSSH2_SESSION *session = connection_data->session; @@ -3451,7 +3437,7 @@ phypListDefinedDomains(virConnectPtr conn, char **const names, int nnames) /* I need to parse the textual return in order to get the domains */ if (exit_status < 0 || ret == NULL) - goto err; + goto cleanup; else { domains = ret; @@ -3462,7 +3448,7 @@ phypListDefinedDomains(virConnectPtr conn, char **const names, int nnames) *char_ptr2 = '\0'; if ((names[got++] = strdup(domains)) == NULL) { virReportOOMError(); - goto err; + goto cleanup; } char_ptr2++; domains = char_ptr2; @@ -3471,16 +3457,20 @@ phypListDefinedDomains(virConnectPtr conn, char **const names, int nnames) } } - VIR_FREE(cmd); - VIR_FREE(ret); - return got; + success = true; + + cleanup: + if (!success) { + for (i = 0; i < got; i++) + VIR_FREE(names[i]); + + got = -1; + } - err: - for (i = 0; i < got; i++) - VIR_FREE(names[i]); VIR_FREE(cmd); VIR_FREE(ret); - return -1; + + return got; } static virDomainPtr @@ -3524,22 +3514,20 @@ phypDomainLookupByID(virConnectPtr conn, int lpar_id) conn); if (phypGetLparUUID(lpar_uuid, lpar_id, conn) == -1) - goto err; + goto cleanup; if (exit_status < 0) - goto err; + goto cleanup; dom = virGetDomain(conn, lpar_name, lpar_uuid); if (dom) dom->id = lpar_id; + cleanup: VIR_FREE(lpar_name); - return dom; - err: - VIR_FREE(lpar_name); - return NULL; + return dom; } static char * @@ -3596,6 +3584,7 @@ phypDomainDumpXML(virDomainPtr dom, int flags) static int phypDomainResume(virDomainPtr dom) { + int result = -1; ConnectionData *connection_data = dom->conn->networkPrivateData; phyp_driverPtr phyp_driver = dom->conn->privateData; LIBSSH2_SESSION *session = connection_data->session; @@ -3621,21 +3610,21 @@ phypDomainResume(virDomainPtr dom) ret = phypExec(session, cmd, &exit_status, dom->conn); if (exit_status < 0) - goto err; + goto cleanup; - VIR_FREE(cmd); - VIR_FREE(ret); - return 0; + result = 0; - err: + cleanup: VIR_FREE(cmd); VIR_FREE(ret); - return -1; + + return result; } static int phypDomainShutdown(virDomainPtr dom) { + int result = -1; ConnectionData *connection_data = dom->conn->networkPrivateData; virConnectPtr conn = dom->conn; LIBSSH2_SESSION *session = connection_data->session; @@ -3654,23 +3643,22 @@ phypDomainShutdown(virDomainPtr dom) if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - return 0; + return -1; } cmd = virBufferContentAndReset(&buf); ret = phypExec(session, cmd, &exit_status, dom->conn); if (exit_status < 0) - goto err; + goto cleanup; - VIR_FREE(cmd); - VIR_FREE(ret); - return 0; + result = 0; - err: + cleanup: VIR_FREE(cmd); VIR_FREE(ret); - return 0; + + return result; } static int @@ -3699,6 +3687,7 @@ phypDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info) static int phypDomainDestroy(virDomainPtr dom) { + int result = -1; ConnectionData *connection_data = dom->conn->networkPrivateData; phyp_driverPtr phyp_driver = dom->conn->privateData; LIBSSH2_SESSION *session = connection_data->session; @@ -3723,27 +3712,25 @@ phypDomainDestroy(virDomainPtr dom) ret = phypExec(session, cmd, &exit_status, dom->conn); if (exit_status < 0) - goto err; + goto cleanup; if (phypUUIDTable_RemLpar(dom->conn, dom->id) == -1) - goto err; + goto cleanup; dom->id = -1; + result = 0; + cleanup: VIR_FREE(cmd); VIR_FREE(ret); - return 0; - - err: - VIR_FREE(cmd); - VIR_FREE(ret); - return -1; + return result; } static int phypBuildLpar(virConnectPtr conn, virDomainDefPtr def) { + int result = -1; ConnectionData *connection_data = conn->networkPrivateData; phyp_driverPtr phyp_driver = conn->privateData; LIBSSH2_SESSION *session = connection_data->session; @@ -3758,27 +3745,27 @@ phypBuildLpar(virConnectPtr conn, virDomainDefPtr def) PHYP_ERROR(VIR_ERR_XML_ERROR,"%s", _("Field \"<memory>\" on the domain XML file is missing or has " "invalid value.")); - goto err; + goto cleanup; } if (!def->mem.max_balloon) { PHYP_ERROR(VIR_ERR_XML_ERROR,"%s", _("Field \"<currentMemory>\" on the domain XML file is missing or" " has invalid value.")); - goto err; + goto cleanup; } if (def->ndisks < 1) { PHYP_ERROR(VIR_ERR_XML_ERROR, "%s", _("Domain XML must contain at least one \"<disk>\" element.")); - goto err; + goto cleanup; } if (!def->disks[0]->src) { PHYP_ERROR(VIR_ERR_XML_ERROR,"%s", _("Field \"<src>\" under \"<disk>\" on the domain XML file is " "missing.")); - goto err; + goto cleanup; } virBufferAddLit(&buf, "mksyscfg"); @@ -3792,7 +3779,7 @@ phypBuildLpar(virConnectPtr conn, virDomainDefPtr def) if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - return -1; + goto cleanup; } cmd = virBufferContentAndReset(&buf); @@ -3800,22 +3787,21 @@ phypBuildLpar(virConnectPtr conn, virDomainDefPtr def) if (exit_status < 0) { VIR_ERROR(_("Unable to create LPAR. Reason: '%s'"), ret); - goto err; + goto cleanup; } if (phypUUIDTable_AddLpar(conn, def->uuid, def->id) == -1) { VIR_ERROR0(_("Unable to add LPAR to the table")); - goto err; + goto cleanup; } - VIR_FREE(cmd); - VIR_FREE(ret); - return 0; + result = 0; - err: + cleanup: VIR_FREE(cmd); VIR_FREE(ret); - return -1; + + return result; } static virDomainPtr -- 1.7.0.4

Reply

Daniel Veillard

Sunday, 10 April Sun, 10 Apr

10:01 p.m.

New subject: [libvirt] [PATCH 2/5] phyp: Reduce code duplication in error and success paths

On Sat, Apr 09, 2011 at 11:59:08AM +0200, Matthias Bolte wrote:

Also fix memory leaks along the way in phypCreateServerSCSIAdapter and phypAttachDevice. --- src/phyp/phyp_driver.c | 592 +++++++++++++++++++++++------------------------- 1 files changed, 289 insertions(+), 303 deletions(-)

Long, makes for simpler and more maintainable code, ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

Reply

Matthias Bolte

Thursday, 14 April Thu, 14 Apr

6:03 a.m.

New subject: [libvirt] [PATCH 2/5] phyp: Reduce code duplication in error and success paths

2011/4/11 Daniel Veillard <veillard(a)redhat.com>:

On Sat, Apr 09, 2011 at 11:59:08AM +0200, Matthias Bolte wrote: > Also fix memory leaks along the way in phypCreateServerSCSIAdapter and > phypAttachDevice. > --- > src/phyp/phyp_driver.c | 592 +++++++++++++++++++++++------------------------- > 1 files changed, 289 insertions(+), 303 deletions(-) Long, makes for simpler and more maintainable code, ACK, Daniel

Thanks, pushed. Matthias

Reply

Matthias Bolte

Saturday, 9 April Sat, 9 Apr

4:59 a.m.

New subject: [libvirt] [PATCH 3/5] phyp: Don't try to use a string from a failed virAsprintf

--- src/phyp/phyp_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index f441261..24165fb 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -210,7 +210,7 @@ phypGetSystemType(virConnectPtr conn) if (virAsprintf(&cmd, "lshmc -V") < 0) { virReportOOMError(); - exit_status = -1; + return -1; } ret = phypExec(session, cmd, &exit_status, conn); -- 1.7.0.4

Reply

Daniel Veillard

Sunday, 10 April Sun, 10 Apr

10:03 p.m.

New subject: [libvirt] [PATCH 3/5] phyp: Don't try to use a string from a failed virAsprintf

On Sat, Apr 09, 2011 at 11:59:09AM +0200, Matthias Bolte wrote:

--- src/phyp/phyp_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index f441261..24165fb 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -210,7 +210,7 @@ phypGetSystemType(virConnectPtr conn) if (virAsprintf(&cmd, "lshmc -V") < 0) { virReportOOMError(); - exit_status = -1; + return -1; } ret = phypExec(session, cmd, &exit_status, conn);

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

Reply

Matthias Bolte

Thursday, 14 April Thu, 14 Apr

6:07 a.m.

New subject: [libvirt] [PATCH 3/5] phyp: Don't try to use a string from a failed virAsprintf

2011/4/11 Daniel Veillard <veillard(a)redhat.com>:

On Sat, Apr 09, 2011 at 11:59:09AM +0200, Matthias Bolte wrote: > --- > src/phyp/phyp_driver.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c > index f441261..24165fb 100644 > --- a/src/phyp/phyp_driver.c > +++ b/src/phyp/phyp_driver.c > @@ -210,7 +210,7 @@ phypGetSystemType(virConnectPtr conn) > > if (virAsprintf(&cmd, "lshmc -V") < 0) { > virReportOOMError(); > - exit_status = -1; > + return -1; > } > ret = phypExec(session, cmd, &exit_status, conn); > ACK, Daniel

Thanks, pushed. Matthias

Reply

Matthias Bolte

Saturday, 9 April Sat, 9 Apr

4:59 a.m.

New subject: [libvirt] [PATCH 4/5] phyp: Fix too small buffer allocation in phypAttachDevice

sizeof(domain->name) is the wrong thing. Instead of using strdup here rewrite escape_specialcharacters to allocate the buffer itself. Add a contains_specialcharacters to be used in phypOpen, as phypOpen is not interested in the escaped version. --- src/phyp/phyp_driver.c | 90 ++++++++++++++++++++++++++++++++--------------- 1 files changed, 61 insertions(+), 29 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 24165fb..226946d 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -891,14 +891,63 @@ phypUUIDTable_Free(uuid_tablePtr uuid_table) VIR_FREE(uuid_table); } -static int -escape_specialcharacters(char *src, char *dst, size_t dstlen) +static bool +contains_specialcharacters(const char *src) { size_t len = strlen(src); - char temp_buffer[len]; - unsigned int i = 0, j = 0; + size_t i = 0; + if (len == 0) - return -1; + return false; + + for (i = 0; i < len; i++) { + switch (src[i]) { + case '&': + case ';': + case '`': + case '@': + case '"': + case '|': + case '*': + case '?': + case '~': + case '<': + case '>': + case '^': + case '(': + case ')': + case '[': + case ']': + case '{': + case '}': + case '$': + case '%': + case '#': + case '\\': + case '\n': + case '\r': + case '\t': + return true; + } + } + + return false; +} + +static char * +escape_specialcharacters(const char *src) +{ + size_t len = strlen(src); + size_t i = 0, j = 0; + char *dst; + + if (len == 0) + return NULL; + + if (VIR_ALLOC_N(dst, len + 1) < 0) { + virReportOOMError(); + return NULL; + } for (i = 0; i < len; i++) { switch (src[i]) { @@ -929,16 +978,14 @@ escape_specialcharacters(char *src, char *dst, size_t dstlen) case '\t': continue; default: - temp_buffer[j] = src[i]; + dst[j] = src[i]; j++; } } - temp_buffer[j] = '\0'; - if (virStrcpy(dst, temp_buffer, dstlen) == NULL) - return -1; + dst[j] = '\0'; - return 0; + return dst; } static LIBSSH2_SESSION * @@ -1121,8 +1168,6 @@ phypOpen(virConnectPtr conn, { LIBSSH2_SESSION *session = NULL; ConnectionData *connection_data = NULL; - char *string = NULL; - size_t len = 0; int internal_socket; uuid_tablePtr uuid_table = NULL; phyp_driverPtr phyp_driver = NULL; @@ -1157,13 +1202,6 @@ phypOpen(virConnectPtr conn, } if (conn->uri->path) { - len = strlen(conn->uri->path) + 1; - - if (VIR_ALLOC_N(string, len) < 0) { - virReportOOMError(); - goto failure; - } - /* need to shift one byte in order to remove the first "/" of URI component */ if (conn->uri->path[0] == '/') managed_system = strdup(conn->uri->path + 1); @@ -1183,7 +1221,7 @@ phypOpen(virConnectPtr conn, if (char_ptr) *char_ptr = '\0'; - if (escape_specialcharacters(conn->uri->path, string, len) == -1) { + if (contains_specialcharacters(conn->uri->path)) { PHYP_ERROR(VIR_ERR_INTERNAL_ERROR, "%s", _("Error parsing 'path'. Invalid characters.")); @@ -1242,7 +1280,6 @@ phypOpen(virConnectPtr conn, } VIR_FREE(connection_data); - VIR_FREE(string); return VIR_DRV_OPEN_ERROR; } @@ -1947,15 +1984,10 @@ phypAttachDevice(virDomainPtr domain, const char *xml) virBuffer buf = VIR_BUFFER_INITIALIZER; char *domain_name = NULL; - if (VIR_ALLOC_N(domain_name, sizeof(domain->name)) < 0) { - virReportOOMError(); - goto err; - } + domain_name = escape_specialcharacters(domain->name); - if (escape_specialcharacters - (domain->name, domain_name, strlen(domain->name)) == -1) { - virReportOOMError(); - goto err; + if (domain_name == NULL) { + goto cleanup; } def->os.type = strdup("aix"); -- 1.7.0.4

Reply

Daniel Veillard

Sunday, 10 April Sun, 10 Apr

10:06 p.m.

New subject: [libvirt] [PATCH 4/5] phyp: Fix too small buffer allocation in phypAttachDevice

On Sat, Apr 09, 2011 at 11:59:10AM +0200, Matthias Bolte wrote:

sizeof(domain->name) is the wrong thing. Instead of using strdup here rewrite escape_specialcharacters to allocate the buffer itself. Add a contains_specialcharacters to be used in phypOpen, as phypOpen is not interested in the escaped version. --- src/phyp/phyp_driver.c | 90 ++++++++++++++++++++++++++++++++--------------- 1 files changed, 61 insertions(+), 29 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 24165fb..226946d 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -891,14 +891,63 @@ phypUUIDTable_Free(uuid_tablePtr uuid_table) VIR_FREE(uuid_table); } -static int -escape_specialcharacters(char *src, char *dst, size_t dstlen) +static bool +contains_specialcharacters(const char *src) { size_t len = strlen(src); - char temp_buffer[len]; - unsigned int i = 0, j = 0; + size_t i = 0; + if (len == 0) - return -1; + return false; + + for (i = 0; i < len; i++) { + switch (src[i]) { + case '&': + case ';': + case '`': + case '@': + case '"': + case '|': + case '*': + case '?': + case '~': + case '<': + case '>': + case '^': + case '(': + case ')': + case '[': + case ']': + case '{': + case '}': + case '$': + case '%': + case '#': + case '\\': + case '\n': + case '\r': + case '\t': + return true; + } + } + + return false; +} + +static char * +escape_specialcharacters(const char *src) +{ + size_t len = strlen(src); + size_t i = 0, j = 0; + char *dst; + + if (len == 0) + return NULL; + + if (VIR_ALLOC_N(dst, len + 1) < 0) { + virReportOOMError(); + return NULL; + } for (i = 0; i < len; i++) { switch (src[i]) { @@ -929,16 +978,14 @@ escape_specialcharacters(char *src, char *dst, size_t dstlen) case '\t': continue; default: - temp_buffer[j] = src[i]; + dst[j] = src[i]; j++; } } - temp_buffer[j] = '\0'; - if (virStrcpy(dst, temp_buffer, dstlen) == NULL) - return -1; + dst[j] = '\0'; - return 0; + return dst; } static LIBSSH2_SESSION * @@ -1121,8 +1168,6 @@ phypOpen(virConnectPtr conn, { LIBSSH2_SESSION *session = NULL; ConnectionData *connection_data = NULL; - char *string = NULL; - size_t len = 0; int internal_socket; uuid_tablePtr uuid_table = NULL; phyp_driverPtr phyp_driver = NULL; @@ -1157,13 +1202,6 @@ phypOpen(virConnectPtr conn, } if (conn->uri->path) { - len = strlen(conn->uri->path) + 1; - - if (VIR_ALLOC_N(string, len) < 0) { - virReportOOMError(); - goto failure; - } - /* need to shift one byte in order to remove the first "/" of URI component */ if (conn->uri->path[0] == '/') managed_system = strdup(conn->uri->path + 1); @@ -1183,7 +1221,7 @@ phypOpen(virConnectPtr conn, if (char_ptr) *char_ptr = '\0'; - if (escape_specialcharacters(conn->uri->path, string, len) == -1) { + if (contains_specialcharacters(conn->uri->path)) { PHYP_ERROR(VIR_ERR_INTERNAL_ERROR, "%s", _("Error parsing 'path'. Invalid characters.")); @@ -1242,7 +1280,6 @@ phypOpen(virConnectPtr conn, } VIR_FREE(connection_data); - VIR_FREE(string); return VIR_DRV_OPEN_ERROR; } @@ -1947,15 +1984,10 @@ phypAttachDevice(virDomainPtr domain, const char *xml) virBuffer buf = VIR_BUFFER_INITIALIZER; char *domain_name = NULL; - if (VIR_ALLOC_N(domain_name, sizeof(domain->name)) < 0) { - virReportOOMError(); - goto err; - } + domain_name = escape_specialcharacters(domain->name); - if (escape_specialcharacters - (domain->name, domain_name, strlen(domain->name)) == -1) { - virReportOOMError(); - goto err; + if (domain_name == NULL) { + goto cleanup; } def->os.type = strdup("aix");

ACK, we just need to make sure contains_specialcharacters() and escape_specialcharacters() don't diverge on the charater set. Maybe add a comment in escape_specialcharacters() to this effect. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

Reply

Matthias Bolte

Thursday, 14 April Thu, 14 Apr

7 a.m.

New subject: [libvirt] [PATCH 4/5] phyp: Fix too small buffer allocation in phypAttachDevice

2011/4/11 Daniel Veillard <veillard(a)redhat.com>:

On Sat, Apr 09, 2011 at 11:59:10AM +0200, Matthias Bolte wrote: > sizeof(domain->name) is the wrong thing. Instead of using strdup here > rewrite escape_specialcharacters to allocate the buffer itself. > > Add a contains_specialcharacters to be used in phypOpen, as phypOpen is > not interested in the escaped version. > --- > src/phyp/phyp_driver.c | 90 ++++++++++++++++++++++++++++++++--------------- > 1 files changed, 61 insertions(+), 29 deletions(-) >

ACK, we just need to make sure contains_specialcharacters() and escape_specialcharacters() don't diverge on the charater set. Maybe add a comment in escape_specialcharacters() to this effect. Daniel

I just moved the character set to a define, like this and pushed the result. diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 2eb8317..3862c9c 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -894,6 +894,12 @@ phypUUIDTable_Free(uuid_tablePtr uuid_table) VIR_FREE(uuid_table); } +#define SPECIALCHARACTER_CASES \ + case '&': case ';': case '`': case '@': case '"': case '|': case '*': \ + case '?': case '~': case '<': case '>': case '^': case '(': case ')': \ + case '[': case ']': case '{': case '}': case '$': case '%': case '#': \ + case '\\': case '\n': case '\r': case '\t': + static bool contains_specialcharacters(const char *src) { @@ -905,32 +911,10 @@ contains_specialcharacters(const char *src) for (i = 0; i < len; i++) { switch (src[i]) { - case '&': - case ';': - case '`': - case '@': - case '"': - case '|': - case '*': - case '?': - case '~': - case '<': - case '>': - case '^': - case '(': - case ')': - case '[': - case ']': - case '{': - case '}': - case '$': - case '%': - case '#': - case '\\': - case '\n': - case '\r': - case '\t': - return true; + SPECIALCHARACTER_CASES + return true; + default: + continue; } } @@ -954,35 +938,11 @@ escape_specialcharacters(const char *src) for (i = 0; i < len; i++) { switch (src[i]) { - case '&': - case ';': - case '`': - case '@': - case '"': - case '|': - case '*': - case '?': - case '~': - case '<': - case '>': - case '^': - case '(': - case ')': - case '[': - case ']': - case '{': - case '}': - case '$': - case '%': - case '#': - case '\\': - case '\n': - case '\r': - case '\t': - continue; - default: - dst[j] = src[i]; - j++; + SPECIALCHARACTER_CASES + continue; + default: + dst[j] = src[i]; + j++; } }

Reply

Eric Blake

11:49 a.m.

New subject: [libvirt] [PATCH 4/5] phyp: Fix too small buffer allocation in phypAttachDevice

On 04/14/2011 06:00 AM, Matthias Bolte wrote:

> ACK, we just need to make sure contains_specialcharacters() and > escape_specialcharacters() don't diverge on the charater set. Maybe > add a comment in escape_specialcharacters() to this effect. > > Daniel > I just moved the character set to a define, like this and pushed the result. @@ -905,32 +911,10 @@ contains_specialcharacters(const char *src) for (i = 0; i < len; i++) { switch (src[i]) { + SPECIALCHARACTER_CASES + return true; + default: + continue; }

Fair enough - continue the loop until we find a special character.

} @@ -954,35 +938,11 @@ escape_specialcharacters(const char *src) for (i = 0; i < len; i++) { switch (src[i]) { + SPECIALCHARACTER_CASES + continue; + default: + dst[j] = src[i]; + j++;

Huh? That is not escaping characters, but omitting them! This code seems rather broken; was the intent to add \ escaping before special characters? I _really_ want to add something to src/util/util.h that outputs shell-escaped strings automatically (virCommandToString needs to use it, and the current 'virsh echo --shell' could share it), so that all clients that are doing shell escaping use a single entry point rather than re-coding ad-hoc methods. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

Reply

Matthias Bolte

12:06 p.m.

New subject: [libvirt] [PATCH 4/5] phyp: Fix too small buffer allocation in phypAttachDevice

2011/4/14 Eric Blake <eblake(a)redhat.com>:

On 04/14/2011 06:00 AM, Matthias Bolte wrote: >> ACK, we just need to make sure contains_specialcharacters() and >> escape_specialcharacters() don't diverge on the charater set. Maybe >> add a comment in escape_specialcharacters() to this effect. >> >> Daniel >> > > I just moved the character set to a define, like this and pushed the result. > > @@ -905,32 +911,10 @@ contains_specialcharacters(const char *src) > > for (i = 0; i < len; i++) { > switch (src[i]) { > + SPECIALCHARACTER_CASES > + return true; > + default: > + continue; > } Fair enough - continue the loop until we find a special character. > } > > @@ -954,35 +938,11 @@ escape_specialcharacters(const char *src) > > for (i = 0; i < len; i++) { > switch (src[i]) { > + SPECIALCHARACTER_CASES > + continue; > + default: > + dst[j] = src[i]; > + j++; Huh? That is not escaping characters, but omitting them! This code seems rather broken; was the intent to add \ escaping before special characters?

Well, The code was this way from the beginning, we'll have to ask Eduardo about its intention here. Matthias

Reply

Matthias Bolte

Saturday, 9 April Sat, 9 Apr

4:59 a.m.

New subject: [libvirt] [PATCH 5/5] phyp: Don't overwrite error from virDomainDeviceDefParse by OOM error

--- src/phyp/phyp_driver.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 226946d..f36b357 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -2000,7 +2000,6 @@ phypAttachDevice(virDomainPtr domain, const char *xml) dev = virDomainDeviceDefParse(phyp_driver->caps, def, xml, VIR_DOMAIN_XML_INACTIVE); if (!dev) { - virReportOOMError(); goto cleanup; } -- 1.7.0.4

Reply

Daniel Veillard

Sunday, 10 April Sun, 10 Apr

10:06 p.m.

New subject: [libvirt] [PATCH 5/5] phyp: Don't overwrite error from virDomainDeviceDefParse by OOM error

On Sat, Apr 09, 2011 at 11:59:11AM +0200, Matthias Bolte wrote:

--- src/phyp/phyp_driver.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 226946d..f36b357 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -2000,7 +2000,6 @@ phypAttachDevice(virDomainPtr domain, const char *xml) dev = virDomainDeviceDefParse(phyp_driver->caps, def, xml, VIR_DOMAIN_XML_INACTIVE); if (!dev) { - virReportOOMError(); goto cleanup; }

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

Reply

Matthias Bolte

Thursday, 14 April Thu, 14 Apr

6:09 a.m.

New subject: [libvirt] [PATCH 5/5] phyp: Don't overwrite error from virDomainDeviceDefParse by OOM error

2011/4/11 Daniel Veillard <veillard(a)redhat.com>:

On Sat, Apr 09, 2011 at 11:59:11AM +0200, Matthias Bolte wrote: > --- > src/phyp/phyp_driver.c | 1 - > 1 files changed, 0 insertions(+), 1 deletions(-) > > diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c > index 226946d..f36b357 100644 > --- a/src/phyp/phyp_driver.c > +++ b/src/phyp/phyp_driver.c > @@ -2000,7 +2000,6 @@ phypAttachDevice(virDomainPtr domain, const char *xml) > dev = virDomainDeviceDefParse(phyp_driver->caps, def, xml, > VIR_DOMAIN_XML_INACTIVE); > if (!dev) { > - virReportOOMError(); > goto cleanup; > } > ACK, Daniel

Thanks, pushed. Matthias

Reply

5007

days inactive

5012

days old

devel@lists.libvirt.org

Manage subscription

17 comments

3 participants

tags (0)

participants (3)

Daniel Veillard
Eric Blake
Matthias Bolte