| Message ID | 1443085502-596-5-git-send-email-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Sep 24, 2015 at 11:04:55AM +0200, Gerd Hoffmann wrote: > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> It's easy to see this is what the patch does. But why? Some explanation in the commit log about why it's done, as opposed to what is done, would be better. > --- > hw/display/virtio-gpu.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c > index a67d927..73bd9b6 100644 > --- a/hw/display/virtio-gpu.c > +++ b/hw/display/virtio-gpu.c > @@ -563,7 +563,6 @@ int virtio_gpu_create_mapping_iov(struct virtio_gpu_resource_attach_backing *ab, > __func__, ab->resource_id, i); > virtio_gpu_cleanup_mapping_iov(*iov, i); > g_free(ents); > - g_free(*iov); > *iov = NULL; > return -1; > } > @@ -580,12 +579,12 @@ void virtio_gpu_cleanup_mapping_iov(struct iovec *iov, uint32_t count) > cpu_physical_memory_unmap(iov[i].iov_base, iov[i].iov_len, 1, > iov[i].iov_len); > } > + g_free(iov); > } > > static void virtio_gpu_cleanup_mapping(struct virtio_gpu_simple_resource *res) > { > virtio_gpu_cleanup_mapping_iov(res->iov, res->iov_cnt); > - g_free(res->iov); > res->iov = NULL; > res->iov_cnt = 0; > } > -- > 1.8.3.1
On Do, 2015-09-24 at 13:16 +0300, Michael S. Tsirkin wrote: > On Thu, Sep 24, 2015 at 11:04:55AM +0200, Gerd Hoffmann wrote: > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > It's easy to see this is what the patch does. But why? Some > explanation in the commit log about why it's done, as opposed to what is > done, would be better. It's for symmetry reasons: virtio_gpu_create_mapping_iov() allocates it so virtio_gpu_cleanup_mapping_iov() should free it, otherwise it's easy to miss a free() needed and leak memory. cheers, Gerd
On 24.09.2015 11:04, Gerd Hoffmann wrote: > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > hw/display/virtio-gpu.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Reviewed-by: Max Reitz <mreitz@redhat.com> You could even make the parameter a struct iovec **, which is then set to NULL by virtio_gpu_cleanup_mapping_iov() itself (because both callers set their iovec pointer to NULL).
On Fri, Sep 25, 2015 at 03:14:59PM +0200, Gerd Hoffmann wrote: > On Do, 2015-09-24 at 13:16 +0300, Michael S. Tsirkin wrote: > > On Thu, Sep 24, 2015 at 11:04:55AM +0200, Gerd Hoffmann wrote: > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > > > It's easy to see this is what the patch does. But why? Some > > explanation in the commit log about why it's done, as opposed to what is > > done, would be better. > > It's for symmetry reasons: virtio_gpu_create_mapping_iov() allocates it > so virtio_gpu_cleanup_mapping_iov() should free it, otherwise it's easy > to miss a free() needed and leak memory. > > cheers, > Gerd > Makes sense. Pls include this info in the commit log.
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c index a67d927..73bd9b6 100644 --- a/hw/display/virtio-gpu.c +++ b/hw/display/virtio-gpu.c @@ -563,7 +563,6 @@ int virtio_gpu_create_mapping_iov(struct virtio_gpu_resource_attach_backing *ab, __func__, ab->resource_id, i); virtio_gpu_cleanup_mapping_iov(*iov, i); g_free(ents); - g_free(*iov); *iov = NULL; return -1; } @@ -580,12 +579,12 @@ void virtio_gpu_cleanup_mapping_iov(struct iovec *iov, uint32_t count) cpu_physical_memory_unmap(iov[i].iov_base, iov[i].iov_len, 1, iov[i].iov_len); } + g_free(iov); } static void virtio_gpu_cleanup_mapping(struct virtio_gpu_simple_resource *res) { virtio_gpu_cleanup_mapping_iov(res->iov, res->iov_cnt); - g_free(res->iov); res->iov = NULL; res->iov_cnt = 0; }
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/display/virtio-gpu.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)