Re: [PATCH 22/29] tools/libxl: Allow limiting amount copied by datacopier

[Ross Lagerwall <ross.lagerwall@citrix.com>]

On 09/12/2014 09:36 AM, Wen Congyang wrote:
> On 09/11/2014 01:11 AM, Andrew Cooper wrote:
>> From: Ross Lagerwall <ross.lagerwall@citrix.com>
>>
>> Add a parameter, maxread, to limit the amount of data read from the
>> source fd of a datacopier.
>>
>> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
>> ---
>>   tools/libxl/libxl_aoutils.c    |    9 +++++++--
>>   tools/libxl/libxl_bootloader.c |    2 ++
>>   tools/libxl/libxl_dom.c        |    1 +
>>   tools/libxl/libxl_internal.h   |    1 +
>>   4 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
>> index caba637..6502325 100644
>> --- a/tools/libxl/libxl_aoutils.c
>> +++ b/tools/libxl/libxl_aoutils.c
>> @@ -145,7 +145,7 @@ static void datacopier_check_state(libxl__egc *egc, libxl__datacopier_state *dc)
>>                   return;
>>               }
>>           }
>> -    } else if (!libxl__ev_fd_isregistered(&dc->toread)) {
>> +    } else if (!libxl__ev_fd_isregistered(&dc->toread) || dc->maxread == 0) {
>>           /* we have had eof */
>>           datacopier_callback(egc, dc, 0, 0);
>>           return;
>> @@ -233,7 +233,8 @@ static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
>>           }
>>           int r = read(ev->fd,
>>                        buf->buf + buf->used,
>> -                     sizeof(buf->buf) - buf->used);
>> +                     (sizeof(buf->buf) - buf->used) < dc->maxread ?
>> +                         (sizeof(buf->buf) - buf->used) : dc->maxread);
>>           if (r < 0) {
>>               if (errno == EINTR) continue;
>>               if (errno == EWOULDBLOCK) break;
>> @@ -258,7 +259,11 @@ static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
>>           }
>>           buf->used += r;
>>           dc->used += r;
>> +        dc->maxread -= r;
>>           assert(buf->used <= sizeof(buf->buf));
>> +        assert(dc->maxread >= 0);
>> +        if (dc->maxread == 0)
>> +            break;
>
> We can call libxl__ev_fd_deregister() here, and no need to touch datacopier_check_state().
> Otherwise, datacopier_readable() may be called again, and read() returns 0. And then
> libxl__ev_fd_deregister() is called.
>

Well datacopier_check_state() is called immediately after the break and 
that would deregister the fd so datacopier_readable() would not be 
called again. What you suggest is definitely better though.

Thanks
--
Ross Lagerwall