Re: [PATCH 23/29] tools/libxl: Extend datacopier to support reading into a buffer

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

On 09/12/2014 09:49 AM, Wen Congyang wrote:
> On 09/11/2014 01:11 AM, Andrew Cooper wrote:
>> From: Ross Lagerwall <ross.lagerwall@citrix.com>
>>
>> Implement a read-only mode for libxl__datacopier.  The mode is invoked
>> when readbuf is set and writefd is < 0.  On success, the callback passes
>> the number of bytes read.
>>
>> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
>> ---
>>   tools/libxl/libxl_aoutils.c  |   59 +++++++++++++++++++++++++-----------------
>>   tools/libxl/libxl_internal.h |    4 ++-
>>   2 files changed, 38 insertions(+), 25 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
>> index 6502325..9183716 100644
>> --- a/tools/libxl/libxl_aoutils.c
>> +++ b/tools/libxl/libxl_aoutils.c
>> @@ -134,7 +134,7 @@ static void datacopier_check_state(libxl__egc *egc, libxl__datacopier_state *dc)
>>       STATE_AO_GC(dc->ao);
>>       int rc;
>>
>> -    if (dc->used) {
>> +    if (dc->used && !dc->readbuf) {
>>           if (!libxl__ev_fd_isregistered(&dc->towrite)) {
>>               rc = libxl__ev_fd_register(gc, &dc->towrite, datacopier_writable,
>>                                          dc->writefd, POLLOUT);
>> @@ -147,7 +147,7 @@ static void datacopier_check_state(libxl__egc *egc, libxl__datacopier_state *dc)
>>           }
>>       } else if (!libxl__ev_fd_isregistered(&dc->toread) || dc->maxread == 0) {
>>           /* we have had eof */
>> -        datacopier_callback(egc, dc, 0, 0);
>> +        datacopier_callback(egc, dc, 0, dc->readbuf ? dc->used : 0);
>>           return;
>>       } else {
>>           /* nothing buffered, but still reading */
>> @@ -215,26 +215,31 @@ static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
>>       }
>>       assert(revents & POLLIN);
>>       for (;;) {
>> -        while (dc->used >= dc->maxsz) {
>> -            libxl__datacopier_buf *rm = LIBXL_TAILQ_FIRST(&dc->bufs);
>> -            dc->used -= rm->used;
>> -            assert(dc->used >= 0);
>> -            LIBXL_TAILQ_REMOVE(&dc->bufs, rm, entry);
>> -            free(rm);
>> -        }
>> +        libxl__datacopier_buf *buf = NULL;
>> +        int r;
>> +
>> +        if (dc->readbuf) {
>> +            r = read(ev->fd, dc->readbuf + dc->used, dc->maxread);
>> +        } else {
>> +            while (dc->used >= dc->maxsz) {
>> +                libxl__datacopier_buf *rm = LIBXL_TAILQ_FIRST(&dc->bufs);
>> +                dc->used -= rm->used;
>> +                assert(dc->used >= 0);
>> +                LIBXL_TAILQ_REMOVE(&dc->bufs, rm, entry);
>> +                free(rm);
>> +            }
>>
>> -        libxl__datacopier_buf *buf =
>> -            LIBXL_TAILQ_LAST(&dc->bufs, libxl__datacopier_bufs);
>> -        if (!buf || buf->used >= sizeof(buf->buf)) {
>> -            buf = malloc(sizeof(*buf));
>> -            if (!buf) libxl__alloc_failed(CTX, __func__, 1, sizeof(*buf));
>> -            buf->used = 0;
>> -            LIBXL_TAILQ_INSERT_TAIL(&dc->bufs, buf, entry);
>> -        }
>> -        int r = read(ev->fd,
>> -                     buf->buf + buf->used,
>> +            buf = LIBXL_TAILQ_LAST(&dc->bufs, libxl__datacopier_bufs);
>> +            if (!buf || buf->used >= sizeof(buf->buf)) {
>> +                buf = malloc(sizeof(*buf));
>> +                if (!buf) libxl__alloc_failed(CTX, __func__, 1, sizeof(*buf));
>> +                buf->used = 0;
>> +                LIBXL_TAILQ_INSERT_TAIL(&dc->bufs, buf, entry);
>> +            }
>> +            r = read(ev->fd, 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;
>> @@ -257,10 +262,12 @@ static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
>>                   return;
>>               }
>>           }
>> -        buf->used += r;
>> +        if (!dc->readbuf) {
>> +            buf->used += r;
>> +            assert(buf->used <= sizeof(buf->buf));
>> +        }
>>           dc->used += r;
>>           dc->maxread -= r;
>> -        assert(buf->used <= sizeof(buf->buf));
>>           assert(dc->maxread >= 0);
>>           if (dc->maxread == 0)
>>               break;
>> @@ -324,9 +331,13 @@ int libxl__datacopier_start(libxl__datacopier_state *dc)
>>           if (rc) goto out;
>>       }
>>
>> -    rc = libxl__ev_fd_register(gc, &dc->towrite, datacopier_writable,
>> -                               dc->writefd, POLLOUT);
>> -    if (rc) goto out;
>> +    if (dc->writefd >= 0) {
>> +        rc = libxl__ev_fd_register(gc, &dc->towrite, datacopier_writable,
>> +                                   dc->writefd, POLLOUT);
>> +        if (rc) goto out;
>> +    }
>> +
>> +    assert(dc->readfd >= 0 || dc->writefd >= 0);
>
> If readfd and writefd are valid, and readbuf is not NULL, what it the behavior?
>

I'd say that's an invalid use of the API. There should probably be an 
assert to check this.

Regards
-- 
Ross Lagerwall