On 09/17/2014 05:26 AM, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilb...@redhat.com> > > This is based on Stefan and Joel's patch that creates a QEMUFile that goes > to a memory buffer; from: > > http://lists.gnu.org/archive/html/qemu-devel/2013-03/msg05036.html
Sheesh - a year and a half ago, and still not ready to merge.
>
> Using the QEMUFile interface, this patch adds support functions for
> operating on in-memory sized buffers that can be written to or read from.
>
> Signed-off-by: Stefan Berger <stef...@linux.vnet.ibm.com>
> Signed-off-by: Joel Schopp <jsch...@linux.vnet.ibm.com>
>
> For fixes/tweeks I've done:
> Signed-off-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
> ---
> include/migration/qemu-file.h | 28 +++
> include/qemu/typedefs.h | 1 +
> qemu-file.c | 457
> ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 486 insertions(+)
>
> +QEMUSizedBuffer *qsb_create(const uint8_t *buffer, size_t len)
> + for (i = 0; i < num_chunks; i++) {
> + qsb->iov[i].iov_base = g_try_malloc0(chunk_size);
> + if (!qsb->iov[i].iov_base) {
> + size_t j;
> +
> + for (j = 0; j < i; j++) {
> + g_free(qsb->iov[j].iov_base);
> + }
> + g_free(qsb->iov);
> + g_free(qsb);
> + return NULL;
Rather than inlining all this cleanup, you could just call
qsb_free(qsb). But I can live with this version too.
> +/**
> + * Grow the QEMUSizedBuffer to the given size and allocated
> + * memory for it.
s/allocated/allocate/ ?
> + *
> + * @qsb: A QEMUSizedBuffer
> + * @new_size: The new size of the buffer
> + *
> + * Returns an error code in case of memory allocation failure
s/an error code/a negative error code/ ?
> + * or the new size of the buffer otherwise. The returned size
> + * may be greater or equal to @new_size.
> + */
> +static ssize_t qsb_grow(QEMUSizedBuffer *qsb, size_t new_size)
> +{
> + size_t needed_chunks, i;
> +
> + if (qsb->size < new_size) {
> + struct iovec *new_iov;
> + size_t size_diff = new_size - qsb->size;
> + size_t chunk_size = (size_diff > QSB_MAX_CHUNK_SIZE)
> + ? QSB_MAX_CHUNK_SIZE : QSB_CHUNK_SIZE;
> +
> + needed_chunks = DIV_ROUND_UP(size_diff, chunk_size);
> +
> + new_iov = g_try_malloc_n(qsb->n_iov + needed_chunks,
> + sizeof(struct iovec));
Indentation is off.
> +
> +const QEMUSizedBuffer *qemu_buf_get(QEMUFile *f)
> +{
> + QEMUBuffer *p;
> +
> + qemu_fflush(f);
> +
> + p = (QEMUBuffer *)f->opaque;
Cast is not necessary (this is C, after all, not C++).
> +
> + return p->qsb;
> +}
> +
> +static const QEMUFileOps buf_read_ops = {
> + .get_buffer = buf_get_buffer,
> + .close = buf_close
> +};
I think we prefer trailing commas, if only so that future additions
don't have to modify existing lines.
> +
> +static const QEMUFileOps buf_write_ops = {
> + .put_buffer = buf_put_buffer,
> + .close = buf_close
> +};
and again.
> +
> +QEMUFile *qemu_bufopen(const char *mode, QEMUSizedBuffer *input)
> +{
> + QEMUBuffer *s;
> +
> + if (mode == NULL || (mode[0] != 'r' && mode[0] != 'w') || mode[1] != 0) {
I prefer '\0' over 0 when comparing characters. Or shorthand such as
'|| *mode[1]'. But it works as is.
> + error_report("qemu_bufopen: Argument validity check failed");
> + return NULL;
> + }
> +
> + s = g_malloc0(sizeof(QEMUBuffer));
> + if (mode[0] == 'r') {
> + s->qsb = input;
> + }
> +
> + if (s->qsb == NULL) {
> + s->qsb = qsb_create(NULL, 0);
> + }
> + if (!s->qsb) {
> + error_report("qemu_bufopen: qsb_create failed");
> + return NULL;
Memory leak of s.
> + }
> +
> +
> + if (mode[0] == 'r') {
> + s->file = qemu_fopen_ops(s, &buf_read_ops);
> + } else {
> + s->file = qemu_fopen_ops(s, &buf_write_ops);
> + }
> + return s->file;
> +}
>
Closer; most of my findings are minor, but the memleak needs fixing. If
the only changes you make are in relation to my findings, then v5 can
start life with Reviewed-by: Eric Blake <ebl...@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
