From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:42749) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UIjNr-0007Nj-3g for qemu-devel@nongnu.org; Thu, 21 Mar 2013 13:34:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UIjNp-0006WU-QV for qemu-devel@nongnu.org; Thu, 21 Mar 2013 13:34:35 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56507) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UIjNp-0006WP-HS for qemu-devel@nongnu.org; Thu, 21 Mar 2013 13:34:33 -0400 From: Juan Quintela In-Reply-To: <1363881940-27505-8-git-send-email-owasserm@redhat.com> (Orit Wasserman's message of "Thu, 21 Mar 2013 18:05:38 +0200") References: <1363881940-27505-1-git-send-email-owasserm@redhat.com> <1363881940-27505-8-git-send-email-owasserm@redhat.com> Date: Thu, 21 Mar 2013 18:34:38 +0100 Message-ID: <87r4j8psgh.fsf@elfo.elfo> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v3 7/9] Add qemu_put_buffer_no_copy Reply-To: quintela@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Orit Wasserman Cc: pbonzini@redhat.com, chegu_vinod@hp.com, qemu-devel@nongnu.org, mst@redhat.com Orit Wasserman 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 > --- > 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.