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

[Dr. David Alan Gilbert]

* Eric Blake (address@hidden) wrote:
> On 09/17/2014 05:26 AM, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> > 
> > 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.

Yes; I'm dragging it screaming, hopefully over the line.

> > 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 <address@hidden>
> > Signed-off-by: Joel Schopp <address@hidden>
> > 
> > For fixes/tweeks I've done:
> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > ---
> >  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.

Done; I was wary at first, but I checked and since qsb_free
uses g_free, and g_free is defined to ignore NULL pointers, it's
OK to be used at this point.

> > +/**
> > + * Grow the QEMUSizedBuffer to the given size and allocated
> > + * memory for it.
> 
> s/allocated/allocate/ ?

Fixed.

> 
> > + *
> > + * @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/ ?

Done; reformatted that text to lay the 'or' out more clearly.

> > + * 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.

Done.

> > +
> > +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++).

Done.

> > +
> > +    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.

Done.

> 
> > +
> > +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.

Done as '\0'

> > +        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.

Fixed.

> > +    }
> > +
> > +
> > +    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 <address@hidden>

Thanks! I'll get that posted shortly, so please just check it over.

Dave

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


--
Dr. David Alan Gilbert / address@hidden / Manchester, UK