Re: [Qemu-devel] [PATCH v4 1/2] QEMUSizedBuffer based QEMUFile

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4499 bytes --]

On 09/17/2014 05:26 AM, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@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 <stefanb@linux.vnet.ibm.com>
> Signed-off-by: Joel Schopp <jschopp@linux.vnet.ibm.com>
> 
> For fixes/tweeks I've done:
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@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 <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]