Re: [Xen-devel] [PATCH v2 16/27] tools/libxl: Infrastructure for reading a libxl migration v2 stream

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Andrew Cooper writes ("Re: [PATCH v2 16/27] tools/libxl: Infrastructure for 
reading a libxl migration v2 stream"):
...
> > This function has one call site.  As an alternative to abolishing
> > stream_failed, maybe stream_failed should be renamed to
> > `stream_complete' and then the call site for stream_success could
> > simply say `stream_complete(egc, stream, 0)' ?
> 
> I have to admit that I find that paragdim specifically confusing to
> read, which is why I deliberately split the _success() and _failed()
> cases. 

I'm not sure why it's confusing.  It means you have a unified exit
path.

> >> +static void stream_failed(libxl__egc *egc,
> >> +                          libxl__stream_read_state *stream, int rc)
> >> +{
...
> >> +    if (stream->running) {
> >> +        stream_done(egc, stream);
> > Is the check for running necessary here ?
> 
> Yes.  An _abort() call from outside can happen at any arbitrary time. 
> We must not deliver the callback twice.

Obviously.  But it is surely a serious bug if stream_failed is called
when the stream is not running, because stream_failed is then in
flight in circumstances where the caller might reuse the
libxl__stream_read_state.


> >> +    sprintf(path, XC_DEVICE_MODEL_RESTORE_FILE".%u", dcs->guest_domid);
> >> +
> >> +    libxl__carefd_begin();
> >> +    writefd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0600);
> > Does this need to be a carefd ?  This is going to be a small amount of
> > data, and the worst that happens is some other process inherits an fd
> > onto a file which might otherwise be deleted sooner.
> 
> Who says the file will be deleted?  That will be down to the exact
> semantics of the device model.

If the file is not going to be deleted then it is even less necessary.

> (I was asked to switch from a plain open() to a carefd as part of review
> from v1).

I tried to find this and all I found was Ian C saying:

  Also, should consider whether this fd needs to be subject to the
  carefd machinery.

I think it doesn't.  But overuse of the carefd machinery is just
pointless and not actually incorrect.  So:

> > If it does, then please use libxl__carefd_opened.  (Best would be to
> > libxl__carefd_opened to save and restore errno and then you can call
> > it unconditionally after the open.)
> 
> I will go down this route.

... if you do this it won't be a blocker.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel