Re: [Qemu-devel] [PATCH 6/9] ioport: split deletion and destruction

[Peter Crosthwaite]

On Wed, Jul 30, 2014 at 8:27 PM, Paolo Bonzini <address@hidden> 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 <address@hidden>
> ---
>  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
>
>
>