On 03/06/2013 06:59 AM, Jiri Denemark wrote:

This also renames cleanup label as error since it is only used for error path rather then being common for both success and error paths. --- src/storage/storage_backend_logical.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index bb709df..e3b3de4 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -741,10 +741,10 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, virCommandAddArg(cmd, pool->def->source.name);

if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + goto error;

if ((fdret = virStorageBackendVolOpen(vol->target.path)) < 0) - goto cleanup; + goto error; fd = fdret;

/* We can only chown/grp if root */ @@ -753,21 +753,21 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, virReportSystemError(errno, _("cannot set file owner '%s'"), vol->target.path); - goto cleanup; + goto error; } } if (fchmod(fd, vol->target.perms.mode) < 0) { virReportSystemError(errno, _("cannot set file mode '%s'"), vol->target.path); - goto cleanup; + goto error; }

if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("cannot close file '%s'"), vol->target.path); - goto cleanup; + goto error; } fd = -1;

@@ -776,17 +776,18 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, virReportSystemError(errno, _("cannot find newly created volume '%s'"), vol->target.path); - goto cleanup; + goto error; }

return 0;

Would we be leaking cmd here?

- cleanup: + error: err = virSaveLastError(); VIR_FORCE_CLOSE(fd); virStorageBackendLogicalDeleteVol(conn, pool, vol, 0); virCommandFree(cmd); virSetError(err); + virFreeError(err);

Not that it matters here, but I did find at least one path where a virSaveLastError() didn't call virFreeError() (e.g in storage_backend_logical.c).

return -1; }

ACK otherwise. John