* Eric Blake (ebl...@redhat.com) wrote:
> 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.
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 <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.
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 <ebl...@redhat.com>
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 / dgilb...@redhat.com / Manchester, UK
