Re: [Xen-devel] [PATCH v5 15/15] libxl: Re-implement domain_suspend_device_model using libxl__ev_qmp

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Fri, Nov 09, 2018 at 05:11:05PM +0000, Ian Jackson wrote:
> Anthony PERARD writes ("Re: [PATCH v5 15/15] libxl: Re-implement 
> domain_suspend_device_model using libxl__ev_qmp"):
> > On Tue, Oct 16, 2018 at 04:28:55PM +0100, Ian Jackson wrote:
> > > Does this statefile fd really need to be a carefd ?  Is it a pipe or a
> > > file ?  If it is a file, is it of nontrivial size ?
> > 
> > Yes, it needs to be caredfd, because that's what the libxl__ev_qmp API
> > wants. I don't know yet if it is a good idee to have the _ev_qmp API
> > only takes fd. Do you think it's fine to have libxl__ev_qmp API takes a
> > simple fd and let callers handle the fd the way they want?
> > 
> > (In previous version of the patch series, libxl__ev_qmp used to close
> > the carefd. That's not the case anymore, and that carefd is only read,
> > so I don't think it matter anymore which kind it is between int and
> > carefd.)
> 
> Ah.  In OOP terms I think the API should take the narrowest class that
> has all the required methods.  In this case that means that since you
> don't need to close the fd you shouldn't prejudge whether it's a
> carefd or not - so you should make it an int.
> 
> Regardless of whether it's an fd or a carefd, I think the qmp API
> caller needs to keep the thing open until the qmp async operation is
> done, right ?

Yes, there is "they must all remain valid" in the struct libxl__ev_qmp
comments.

> Feel free to try to talk me out of this view.  I was asking the
> question to provoke thought, not necessarily to push a particular
> opinion.

I don't like much that the possibility exist to leak stuff, but on the
other end with the API change, I would need to store the carefd
somewhere else. I think in this case I downgrade to int as it is easier,
and as you said, it would be a small file with a low probability of been
leaked, so it doesn't matter too much.

Thanks,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel