Hi, On Tue, Sep 1, 2009 at 7:49 PM, Daniel P. Berrange<berrange@redhat.com> wrote:
On Tue, Sep 01, 2009 at 01:50:50AM +0900, Ryota Ozaki wrote:
index df275e6..17ba44a 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -1733,7 +1733,7 @@ readmore:
/* Possibly need to create another receive buffer */ if ((client->nrequests < max_client_requests && - VIR_ALLOC(client->rx) < 0)) { + !client->rx && VIR_ALLOC(client->rx) < 0)) { qemudDispatchClientFailure(client); } else { if (client->rx)
This is not required. Although client->rx looks like a list, it will only ever have a single entry in it. Since we have just called qemudClientMessageQueueServe() a few lines earlier, we know that client->rx is NULL at this point. So there is no leak by always calling VIR_ALLOC(client->rx);
As I said in the previous mail, this fix seems wrong because I cannot reproduce the leak. I'll drop it. (snip)
diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c index ca6d329..222e591 100644 --- a/src/storage_backend_fs.c +++ b/src/storage_backend_fs.c @@ -856,6 +856,8 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn,
vol->type = VIR_STORAGE_VOL_FILE; vol->target.format = VIR_STORAGE_VOL_FILE_RAW; /* Real value is filled in during probe */ + if (vol->target.path) + VIR_FREE(vol->target.path); if (virAsprintf(&vol->target.path, "%s/%s", pool->def->target.path, vol->name) == -1)
This doesn't make any sense - we just allocated 'vol' a few lines earlier, so it can't possibly be non-NULL.
Yes, I'm wrong and will drop it.
@@ -1022,6 +1024,9 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn,
vol->type = VIR_STORAGE_VOL_FILE;
+ if (vol->target.path) + VIR_FREE(vol->target.path); + if (virAsprintf(&vol->target.path, "%s/%s", pool->def->target.path, vol->name) == -1) {
THis one is possibly correct, though we would need the same for vol->key too.
Hmm, valgrind didn't pointed out about vol->key. vol->key is set only on refreshing a volume and is NULL when coming here. Nonetheless, I think adding VIR_FREE is good idea for preventing the possible leak in the future. So I'll add it.
Also there is no need to have 'if (xx)' before a call to VIR_FREE - it accepts NULLs happily. 'make syntax-check' would complain about this additional 'if'.
Oops, I did it, but did like 'make check; make syntax-check; make -C tests valgrind' and then lost the warning ;< Thanks anyway and will send revised patch soon! ozaki-r