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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list