Re: [libvirt] [PATCH] Fix several memory leaks

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);

> 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.

> @@ -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.

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'.