On 05/03/2013 08:53 AM, Michal Privoznik wrote:
--- src/parallels/parallels_driver.c | 71 +++++++++++++++++---------------------- src/parallels/parallels_network.c | 23 +++++-------- src/parallels/parallels_storage.c | 62 +++++++++++----------------------- 3 files changed, 58 insertions(+), 98 deletions(-)
diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c @@ -209,8 +209,8 @@ parallelsGetSerialInfo(virDomainChrDefPtr chr, return -1; }
- if (!(chr->source.data.file.path = strdup(tmp))) - goto no_memory; + if (VIR_STRDUP(chr->source.data.file.path, tmp) < 0) + goto error; } else { parallelsParseError(); return -1; @@ -218,8 +218,7 @@ parallelsGetSerialInfo(virDomainChrDefPtr chr,
return 0;
- no_memory: - virReportOOMError(); +error: return -1;
It looks a bit odd that this function has inlined 'return -1' in some spots, and uses a label with no action other than 'return -1' in others. If you WANT to make it consistent, I don't care whether you use all inline or all goto; but I'm not going to insist on consistency.
@@ -771,13 +761,13 @@ parallelsLoadDomain(parallelsConnPtr privconn, virJSONValuePtr jobj) }
if (STREQ(tmp, "CT")) { - if (!(def->os.type = strdup("exe"))) - goto no_memory; - if (!(def->os.init = strdup("/sbin/init"))) - goto no_memory; + if (VIR_STRDUP(def->os.type, "exe") < 0) + goto cleanup; + if (VIR_STRDUP(def->os.init, "/sbin/init") < 0) + goto cleanup;
You could merge these into one 'if'.
+++ b/src/parallels/parallels_storage.c @@ -136,20 +136,9 @@ static char *parallelsMakePoolName(virConnectPtr conn, const char *path) bool found = false; int j;
- if (!(name = strdup(path))) { - virReportOOMError(); - return NULL; - } - - if (i == 0) - name = strdup(path);
Thanks for squashing this memory leak in the process :)
- else - ignore_value(virAsprintf(&name, "%s-%u", path, i)); - - if (!name) { - virReportOOMError(); + if ((!i && VIR_STRDUP(name, path) < 0) || + (i && virAsprintf(&name, "%s-%u", path, i) < 0)) return 0;
Pre-existing, but this should be 'return NULL'.
@@ -195,7 +184,8 @@ parallelsPoolCreateByPath(virConnectPtr conn, const char *path) }
def->type = VIR_STORAGE_POOL_DIR; - def->target.path = strdup(path); + if (VIR_STRDUP(def->target.path, path) < 0) + goto error;
silent->noisy, but looks like a good bug fix.
@@ -590,9 +579,9 @@ parallelsConnectListDefinedStoragePools(virConnectPtr conn, for (i = 0; i < privconn->pools.count && n < nnames; i++) { virStoragePoolObjLock(privconn->pools.objs[i]); if (!virStoragePoolObjIsActive(privconn->pools.objs[i]) && - !(names[n++] = strdup(privconn->pools.objs[i]->def->name))) { + VIR_STRDUP(names[n++], privconn->pools.objs[i]->def->name) < 0) { virStoragePoolObjUnlock(privconn->pools.objs[i]); - goto no_memory; + goto error; } virStoragePoolObjUnlock(privconn->pools.objs[i]); } @@ -600,7 +589,7 @@ parallelsConnectListDefinedStoragePools(virConnectPtr conn,
return n;
-no_memory: +error: virReportOOMError();
Drop this oom, since the only client of this label already reported oom. ACK with minor fixes. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org