* Amit Shah (amit.s...@redhat.com) wrote:
> On (Tue) 14 Apr 2015 [18:03:34], Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilb...@redhat.com>
> >
> > 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 <dgilb...@redhat.com>
> > ---
> > 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.
see reply below.
>
> > + * 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.
My use tends to involve a buffer allocated once:
uint8_t *mybuffer = g_malloc(...)
while (aloop) {
uint8_t *ourdata = mybuffer;
if (qemu_get_buffer_less_copy(f, &ourdata, size)...) {
do something with *ourdata
}
}
g_free(mybuffer);
The pointer that's passed into qemu_get_buffer_less_copy is only a copy
of the allocation pointer, and thus you're not losing anything when it
changes it.
I've added the following text, does this make it clearer?
* Note: Since **buf may get changed, the caller should take care to
* keep a pointer to the original buffer if it needs to deallocate it.
> > + return done;
>
> How about just 'break' instead of return?
Changed.
>
> > + } else {
> > + first = false;
> > + memcpy(buf, src, res);
> > + buf += res;
>
> In either case (break or return), the 'else' can be dropped..
Changed.
Thanks,
>
> Amit
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
