On Wed, Jul 30, 2014 at 8:27 PM, Paolo Bonzini <pbonz...@redhat.com> wrote:
> Of the two functions portio_list_del and portio_list_destroy,
> the latter is just freeing a memory area. However, portio_list_del
> is the logical equivalent of memory_region_del_subregion so
> destruction of memory regions does not belong there.
>
So I found the original implementation made sense to me, in that _del
is the converse of _add and _destroy _init WRT to the MR ops.
Currently
_init = malloc array
_add = mr_init + mr_add_subregion
_del = mr_del_subregion + mr_destroy
_destory = free array
This delays mr_destory to _destroy breaking the symmetry.
With this change is is still possible to:
_init()
_add()
_del()
_add()
_del()
_destrory()
And not leak a ref, with the reinited memory region on second add?
Then again I may be misunderstanding, as apparently neither of these
API have any users so I'm having trouble getting a handle on intended
usage:
$ git grep portio_list_del
include/exec/ioport.h:void portio_list_del(PortioList *piolist);
ioport.c:void portio_list_del(PortioList *piolist)
$ git grep portio_list_destroy
include/exec/ioport.h:void portio_list_destroy(PortioList *piolist);
ioport.c:void portio_list_destroy(PortioList *piolist)
Regards,
Peter
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> ---
> ioport.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/ioport.c b/ioport.c
> index 3d91e79..dce94a3 100644
> --- a/ioport.c
> +++ b/ioport.c
> @@ -149,6 +149,14 @@ void portio_list_set_flush_coalesced(PortioList *piolist)
>
> void portio_list_destroy(PortioList *piolist)
> {
> + MemoryRegionPortioList *mrpio;
> + unsigned i;
> +
> + for (i = 0; i < piolist->nr; ++i) {
> + mrpio = container_of(piolist->regions[i], MemoryRegionPortioList,
> mr);
> + memory_region_destroy(&mrpio->mr);
> + g_free(mrpio);
> + }
> g_free(piolist->regions);
> }
>
> @@ -291,8 +299,5 @@ void portio_list_del(PortioList *piolist)
> for (i = 0; i < piolist->nr; ++i) {
> mrpio = container_of(piolist->regions[i], MemoryRegionPortioList,
> mr);
> memory_region_del_subregion(piolist->address_space, &mrpio->mr);
> - memory_region_destroy(&mrpio->mr);
> - g_free(mrpio);
> - piolist->regions[i] = NULL;
> }
> }
> --
> 1.8.3.1
>
>
>
