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

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-03-06 at 19:05 +0000, Andrew Cooper wrote:
> From: Ross Lagerwall <ross.lagerwall@citrix.com>
> 
> 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?

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).

> 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;
>  };
>