Re: [Xen-devel] [PATCH v2 5/6] tools/libxl: Extend datacopier to support reading into a buffer

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

On 03/11/2015 12:19 PM, Ian Campbell wrote:
> On Fri, 2015-03-06 at 19:05 +0000, Andrew Cooper wrote:
> > From: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> >
> > Currently a datacopier may source its data from an fd or local buffer, but its
> > destination must be an fd.  For migration v2, libxl needs to read from the
> > migration stream into a local buffer.
> >
> > Implement a "read into local buffer" mode, invoked when readbuf is set and
> > writefd is -1.  On success, the callback passes the number of bytes read.
> > +        if (dc->readbuf) {
> > +            r = read(ev->fd, dc->readbuf + dc->used, dc->maxread);
> Should the final parameter not be maxread - used?
No, because maxread is decreased after every read. This makes more sense 
if it is renamed to bytes_to_read (as it is in V3).
> I assume the caller is expected to make maxread and the readbuf size
> consistent (which could BTW be mentioned in an update to the comments
> relating to this interface).
Yes, done in V3.

> > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > index 22921c7..3e1d78a 100644
> > --- a/tools/libxl/libxl_internal.h
> > +++ b/tools/libxl/libxl_internal.h
> > @@ -2524,7 +2524,8 @@ _hidden void 
> > libxl__device_disk_local_initiate_detach(libxl__egc *egc,
> >
> >   /* onwrite==1 means failure happened when writing, logged, errnoval is valid
> >    * onwrite==0 means failure happened when reading
> > - *     errnoval==0 means we got eof and all data was written
> > + *     errnoval>=0 means we got eof and all data was written or number of bytes
> > + *                 written when in read mode
> >    *     errnoval!=0 means we had a read error, logged
> Nit: the last should be errnoval < 0.
>
> I'm not sure how I feel about using a field called errnoval to store
> something other than an errno though. I suppose I can live with it.
>
> >    * onwrite==-1 means some other internal failure, errnoval not valid, logged
> >    * If we get POLLHUP, we call callback_pollhup(..., onwrite, -1);
> > @@ -2553,6 +2554,7 @@ struct libxl__datacopier_state {
> >       /* remaining fields are private to datacopier */
> >       libxl__ev_fd toread, towrite;
> >       ssize_t used;
> > +    void *readbuf;
> >       LIBXL_TAILQ_HEAD(libxl__datacopier_bufs, libxl__datacopier_buf) bufs;
> >   };

--
Ross Lagerwall

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