From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45332) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XXimU-0006u4-3h for qemu-devel@nongnu.org; Fri, 26 Sep 2014 23:34:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XXimP-0001hE-MH for qemu-devel@nongnu.org; Fri, 26 Sep 2014 23:34:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:5914) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XXimP-0001gl-Ej for qemu-devel@nongnu.org; Fri, 26 Sep 2014 23:34:41 -0400 Message-ID: <54263039.1080007@redhat.com> Date: Fri, 26 Sep 2014 21:34:17 -0600 From: Eric Blake MIME-Version: 1.0 References: <1410953167-9558-1-git-send-email-dgilbert@redhat.com> <1410953167-9558-2-git-send-email-dgilbert@redhat.com> In-Reply-To: <1410953167-9558-2-git-send-email-dgilbert@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="KD3hbgk7EUn9kQ5oE9ft47WVowbDd9jOL" Subject: Re: [Qemu-devel] [PATCH v4 1/2] QEMUSizedBuffer based QEMUFile List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert (git)" , qemu-devel@nongnu.org Cc: sanidhya.iiith@gmail.com, joel.schopp@amd.com, stefanb@linux.vnet.ibm.com, arei.gonglei@huawei.com, quintela@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --KD3hbgk7EUn9kQ5oE9ft47WVowbDd9jOL Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 09/17/2014 05:26 AM, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" >=20 > This is based on Stefan and Joel's patch that creates a QEMUFile that g= oes > to a memory buffer; from: >=20 > 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. >=20 > Using the QEMUFile interface, this patch adds support functions for > operating on in-memory sized buffers that can be written to or read fro= m. >=20 > Signed-off-by: Stefan Berger > Signed-off-by: Joel Schopp >=20 > For fixes/tweeks I've done: > Signed-off-by: Dr. David Alan Gilbert > --- > include/migration/qemu-file.h | 28 +++ > include/qemu/typedefs.h | 1 + > qemu-file.c | 457 ++++++++++++++++++++++++++++++++++= ++++++++ > 3 files changed, 486 insertions(+) >=20 > +QEMUSizedBuffer *qsb_create(const uint8_t *buffer, size_t len) > + for (i =3D 0; i < num_chunks; i++) { > + qsb->iov[i].iov_base =3D g_try_malloc0(chunk_size); > + if (!qsb->iov[i].iov_base) { > + size_t j; > + > + for (j =3D 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 =3D new_size - qsb->size; > + size_t chunk_size =3D (size_diff > QSB_MAX_CHUNK_SIZE) > + ? QSB_MAX_CHUNK_SIZE : QSB_CHUNK_SIZE; > + > + needed_chunks =3D DIV_ROUND_UP(size_diff, chunk_size); > + > + new_iov =3D 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 =3D (QEMUBuffer *)f->opaque; Cast is not necessary (this is C, after all, not C++). > + > + return p->qsb; > +} > + > +static const QEMUFileOps buf_read_ops =3D { > + .get_buffer =3D buf_get_buffer, > + .close =3D 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 =3D { > + .put_buffer =3D buf_put_buffer, > + .close =3D buf_close > +}; and again. > + > +QEMUFile *qemu_bufopen(const char *mode, QEMUSizedBuffer *input) > +{ > + QEMUBuffer *s; > + > + if (mode =3D=3D NULL || (mode[0] !=3D 'r' && mode[0] !=3D 'w') || = mode[1] !=3D 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 =3D g_malloc0(sizeof(QEMUBuffer)); > + if (mode[0] =3D=3D 'r') { > + s->qsb =3D input; > + } > + > + if (s->qsb =3D=3D NULL) { > + s->qsb =3D qsb_create(NULL, 0); > + } > + if (!s->qsb) { > + error_report("qemu_bufopen: qsb_create failed"); > + return NULL; Memory leak of s. > + } > + > + > + if (mode[0] =3D=3D 'r') { > + s->file =3D qemu_fopen_ops(s, &buf_read_ops); > + } else { > + s->file =3D qemu_fopen_ops(s, &buf_write_ops); > + } > + return s->file; > +} >=20 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 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --KD3hbgk7EUn9kQ5oE9ft47WVowbDd9jOL Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg iQEcBAEBCAAGBQJUJjA5AAoJEKeha0olJ0NqSYgIAJjcdZZwor6oIfanVmyK9/QC 3Ll1jlbzr1bD4h47atpXgb25u6udZ+3hEQXuIKqTfYIsV6r+dztDh++pkNju7Q/x nFyWsMaQg1SAssZlk9RPe4SUJCG9gAKlhykHpHQ3P9V15EaVaH2kSYe88htLlzDh +dT8ShJ2G6XEzOKUlDC1raiFXoWvYkRP9/BvLuY631U92vioS1jzTLR7w/VAJ9HJ 40wPBYd1n2yn9C9mtEJ7zolOpAF5mFh7OBCZ+reaY3Bky9optq9oh6hc3STxwpiu 6kGfJT7yTQInct4YjAK7SVTYbh31Nt3OudpuYtAxfY2nJtuFaPJ0T8xbg1biRRY= =Qf/u -----END PGP SIGNATURE----- --KD3hbgk7EUn9kQ5oE9ft47WVowbDd9jOL--