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

[Orit Wasserman <owasserm@redhat.com>]

On 03/21/2013 07:34 PM, Juan Quintela wrote:
> Orit Wasserman <owasserm@redhat.com> 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 <owasserm@redhat.com>
>> ---
>>  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?
you are right, it somehow moved to the next patch :(
I will send a fixed v4 after you finish review the series ..
> 
>> +
>> +    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.
> 
I agree, maybe we need a cleanup series and remove it.
It will certainly simplify this code

Orit