Re: [Qemu-devel] [PATCH v6 08/47] Add qemu_get_buffer_less_copy to avoid copies some of the time

[Amit Shah]

On (Tue) 14 Apr 2015 [18:03:34], Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <address@hidden>
> 
> qemu_get_buffer always copies the data it reads to a users buffer,
> however in many cases the file buffer inside qemu_file could be given
> back to the caller, avoiding the copy.  This isn't always possible
> depending on the size and alignment of the data.
> 
> Thus 'qemu_get_buffer_less_copy' either copies the data to a supplied
> buffer or updates a pointer to the internal buffer if convenient.
> 
> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> ---
>  include/migration/qemu-file.h |  2 ++
>  migration/qemu-file.c         | 45 
> +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
> index 3fe545e..4cac58f 100644
> --- a/include/migration/qemu-file.h
> +++ b/include/migration/qemu-file.h
> @@ -159,6 +159,8 @@ void qemu_put_be32(QEMUFile *f, unsigned int v);
>  void qemu_put_be64(QEMUFile *f, uint64_t v);
>  int qemu_peek_buffer(QEMUFile *f, uint8_t **buf, int size, size_t offset);
>  int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size);
> +int qemu_get_buffer_less_copy(QEMUFile *f, uint8_t **buf, int size);
> +
>  /*
>   * Note that you can only peek continuous bytes from where the current 
> pointer
>   * is; you aren't guaranteed to be able to peak to +n bytes unless you've
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 8dc5767..ec3a598 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -426,6 +426,51 @@ int qemu_get_buffer(QEMUFile *f, uint8_t *buf, int size)
>  }
>  
>  /*
> + * Read 'size' bytes of data from the file.
> + * 'size' can be larger than the internal buffer.
> + *
> + * The data:
> + *   may be held on an internal buffer (in which case *buf is updated
> + *     to point to it) that is valid until the next qemu_file operation.
> + * OR
> + *   will be copied to the *buf that was passed in.
> + *
> + * The code tries to avoid the copy if possible.

So is it expected that callers will store the originally-allocated
start location, so that g_free can be called on the correct location
in either case?  If that's a requirement, this text needs to be
updated.

If not (alternative idea below), text needs to be updated as well.

> + * It will return size bytes unless there was an error, in which case it will
> + * return as many as it managed to read (assuming blocking fd's which
> + * all current QEMUFile are)
> + */
> +int qemu_get_buffer_less_copy(QEMUFile *f, uint8_t **buf, int size)
> +{
> +    int pending = size;
> +    int done = 0;
> +    bool first = true;
> +
> +    while (pending > 0) {
> +        int res;
> +        uint8_t *src;
> +
> +        res = qemu_peek_buffer(f, &src, MIN(pending, IO_BUF_SIZE), 0);
> +        if (res == 0) {
> +            return done;
> +        }
> +        qemu_file_skip(f, res);
> +        done += res;
> +        pending -= res;
> +        if (first && res == size) {
> +            *buf = src;

So we've got to assume that buf was allocated by the calling function,
and since we're modifying the pointer (alternative idea to one above),
should we unallocate it here?  Can lead to a bad g_free later, or a
memleak.

> +            return done;

How about just 'break' instead of return?

> +        } else {
> +            first = false;
> +            memcpy(buf, src, res);
> +            buf += res;

In either case (break or return), the 'else' can be dropped..

                Amit