Hi,
On Tue, Sep 1, 2009 at 7:49 PM, Daniel P. Berrange<berrange@xxxxxxxxxx> 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
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list