Re: [dpdk-dev] [PATCH v2 1/1] vhost: fix leak of fds and mmaps

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

Hey Rich,

Sorry for the long delay; I barely forgot it :(

On Tue, Jan 19, 2016 at 10:13:23AM -0800, Rich Lane wrote:
> On Sun, Jan 17, 2016 at 11:58 PM, Yuanhan Liu <yuanhan.liu@linux.intel.com>
> wrote:
> 
>     On Sun, Jan 17, 2016 at 11:57:18AM -0800, Rich Lane wrote:
>     > +/*
>     > + * Implementation-specific cleanup. Defined by vhost-cuse and
>     vhost-user.
>     > + */
>     > +void vhost_impl_cleanup(struct virtio_net *dev);
> 
>     TBH, I am not quite like "_impl_"; maybe "_backend_" is better?
> 
> 
> If you have a strong preference I will change it. Let me know.

"backend" is just a more common word to me, as well as to QEMU.
So, I would suggest you to do such change, and if so, you could 
add my ACK:

Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

> 
>     OTOH, what I thought of has slight difference than yours: not
>     necessary to export a function, but instead, call the vhost
>     backend specific unmap function inside the backend itself. Say,
>     call vhost_user_unmap() on RESET_OWNER and connection close.
>     What do you think of that?
> 
> 
> The munmap must be done after the notify_ops->destroy_device callback. That
> means
> the backend can't call it before reset_owner() or destroy_device().

Well, you could:

	case VHOST_USER_RESET_OWNER:
		ops->reset_owner();
		vhost_user_unmap();
		break;

Anyway, it's not a big deal. Let's go with your solution first.

	--yliu

> The munmap
> could
> be done afterwards, but that requires saving dev->mem in the caller in the case
> of
> destroy_device. The cleanest solution is for the vhost common code to ask the
> backend to clean up at the correct time.