Re: [Qemu-devel] [PATCH v3 7/9] Add qemu_put_buffer_no_copy

[Juan Quintela]

Orit Wasserman <address@hidden> wrote:
> This allow us to add a buffer to the iovec to send without copying it
> into the static buffer.
>
> Signed-off-by: Orit Wasserman <address@hidden>
> ---
>  include/migration/qemu-file.h |  5 +++++
>  savevm.c                      | 37 ++++++++++++++++++++++++++++---------
>  2 files changed, 33 insertions(+), 9 deletions(-)
>
> diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
> index 8d3da9b..5168be2 100644
> --- a/include/migration/qemu-file.h
> +++ b/include/migration/qemu-file.h
> @@ -75,6 +75,11 @@ int qemu_fclose(QEMUFile *f);
>  int64_t qemu_ftell(QEMUFile *f);
>  void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size);
>  void qemu_put_byte(QEMUFile *f, int v);
> +/*
> + * put_buffer without copying the buffer.
> + * The buffer should be available till it is sent.
> + */
> +void qemu_put_buffer_no_copy(QEMUFile *f, const uint8_t *buf, int size);
>  
>  static inline void qemu_put_ubyte(QEMUFile *f, unsigned int v)
>  {
> diff --git a/savevm.c b/savevm.c
> index 83aa9e7..fbfb9e3 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -621,6 +621,30 @@ int qemu_fclose(QEMUFile *f)
>      return ret;
>  }
>  
> +
> +void qemu_put_buffer_no_copy(QEMUFile *f, const uint8_t *buf, int size)
> +{
> +    if (f->last_error) {
> +        return;
> +    }
> +
> +    if (f->is_write == 0 && f->buf_index > 0) {
> +        fprintf(stderr,
> +                "Attempted to write to buffer while read buffer is not 
> empty\n");
> +        abort();
> +    }

I don't understand this test at all (yes, I know that the test already
existed).

We shouldn't never arrived qemu_put_buffer() with a QEMUFile with
f->is_write == 0.

Change it for one assert()?

> +    f->iov[f->iovcnt].iov_base = f->buf + f->buf_index;
> +    f->iov[f->iovcnt++].iov_len = size;

This is clearly wrong, or I have completely missunderstood it (I will
give a 50% to each posiblitiy).

Here we should be using "buf" and "size" passed as paramenters, f->buf
and f->buf_index shouldn't be used, no?

> +
> +    f->is_write = 1;

is_write is completely redundant, and should just be a:

f->ops->put_buffer test (or now with writev).  We only set it up when
there is anything to be written?

But again, this is independent of this series.