From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ross Lagerwall <ross.lagerwall@citrix.com>
Subject: Re: [PATCH v2 5/6] tools/libxl: Extend datacopier to
 support reading into a buffer
Date: Mon, 16 Mar 2015 13:31:02 +0000
Message-ID: <5506DB16.7010601@citrix.com>
References: <1425668741-28003-1-git-send-email-andrew.cooper3@citrix.com>
	<1425668741-28003-6-git-send-email-andrew.cooper3@citrix.com>
	<1426076398.21353.220.camel@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1426076398.21353.220.camel@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Ian Campbell <ian.campbell@citrix.com>, Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>, Ian Jackson <Ian.Jackson@eu.citrix.com>, Xen-devel <xen-devel@lists.xen.org>
List-Id: xen-devel@lists.xenproject.org

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@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?

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