From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39869) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XIMSP-0001Qz-MF for qemu-devel@nongnu.org; Fri, 15 Aug 2014 14:42:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XIMSL-00069f-7J for qemu-devel@nongnu.org; Fri, 15 Aug 2014 14:42:33 -0400 Received: from mx1.redhat.com ([209.132.183.28]:3154) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XIMSK-00069b-Ul for qemu-devel@nongnu.org; Fri, 15 Aug 2014 14:42:29 -0400 Message-ID: <53EE548C.2070905@redhat.com> Date: Fri, 15 Aug 2014 12:42:20 -0600 From: Eric Blake MIME-Version: 1.0 References: <1407407085-22436-1-git-send-email-dgilbert@redhat.com> <1407407085-22436-2-git-send-email-dgilbert@redhat.com> In-Reply-To: <1407407085-22436-2-git-send-email-dgilbert@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="TIC8oTQaJRJxu8ASfWHq1UE2woDqUcC8U" Subject: Re: [Qemu-devel] [PATCH v2 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: joel.schopp@amd.com, stefanb@linux.vnet.ibm.com, quintela@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --TIC8oTQaJRJxu8ASfWHq1UE2woDqUcC8U Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 08/07/2014 04:24 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 >=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 minor tweeks/rebase I've done to it: > Signed-off-by: Dr. David Alan Gilbert > --- > include/migration/qemu-file.h | 28 +++ > include/qemu/typedefs.h | 1 + > qemu-file.c | 410 ++++++++++++++++++++++++++++++++++= ++++++++ > 3 files changed, 439 insertions(+) >=20 > =20 > +QEMUSizedBuffer *qsb_create(const uint8_t *buffer, size_t len); > +QEMUSizedBuffer *qsb_clone(const QEMUSizedBuffer *); > +void qsb_free(QEMUSizedBuffer *); > +size_t qsb_set_length(QEMUSizedBuffer *qsb, size_t length); Just on the prototype, I wonder if this should return ssize_t, to allow for the possibility of failure... > +/** > + * Create a QEMUSizedBuffer > + * This type of buffer uses scatter-gather lists internally and > + * can grow to any size. Any data array in the scatter-gather list > + * can hold different amount of bytes. > + * > + * @buffer: Optional buffer to copy into the QSB > + * @len: size of initial buffer; if @buffer is given, buffer must > + * hold at least len bytes > + * > + * Returns a pointer to a QEMUSizedBuffer > + */ > +QEMUSizedBuffer *qsb_create(const uint8_t *buffer, size_t len) > +{ > + QEMUSizedBuffer *qsb; > + size_t alloc_len, num_chunks, i, to_copy; > + size_t chunk_size =3D (len > QSB_MAX_CHUNK_SIZE) > + ? QSB_MAX_CHUNK_SIZE > + : QSB_CHUNK_SIZE; > + > + if (len =3D=3D 0) { > + /* we want to allocate at least one chunk */ > + len =3D QSB_CHUNK_SIZE; > + } So if I call qsb_create("", 0), len is now QSB_CHUNK_SIZE... > + > + num_chunks =3D DIV_ROUND_UP(len, chunk_size); =2E..num_chunks is now 1... > + alloc_len =3D num_chunks * chunk_size; > + > + qsb =3D g_new0(QEMUSizedBuffer, 1); > + qsb->iov =3D g_new0(struct iovec, num_chunks); > + qsb->n_iov =3D num_chunks; > + > + for (i =3D 0; i < num_chunks; i++) { > + qsb->iov[i].iov_base =3D g_malloc0(chunk_size); > + qsb->iov[i].iov_len =3D chunk_size; > + if (buffer) { =2E..we enter the loop, and have a buffer to copy... > + to_copy =3D (len - qsb->used) > chunk_size > + ? chunk_size : (len - qsb->used); > + memcpy(qsb->iov[i].iov_base, &buffer[qsb->used], to_copy);= =2E..and proceed to dereference QSB_CHUNK_SIZE bytes beyond the end of th= e empty string that we are converting to a buffer. Oops. Might be as simple as adding if (buffer) { assert(len); } before modifying len. > +/** > + * Set the length of the buffer; the primary usage of this > + * function is to truncate the number of used bytes in the buffer. > + * The size will not be extended beyond the current number of > + * allocated bytes in the QEMUSizedBuffer. > + * > + * @qsb: A QEMUSizedBuffer > + * @new_len: The new length of bytes in the buffer > + * > + * Returns the number of bytes the buffer was truncated or extended > + * to. Confusing; I'd suggest: Returns the resulting (possibly new) size of the buffer. Oh, and my earlier question appears to be answered - this function can't fail. > +ssize_t qsb_get_buffer(const QEMUSizedBuffer *qsb, off_t start, > + size_t count, uint8_t **buf) > +{ > + uint8_t *buffer; > + const struct iovec *iov; > + size_t to_copy, all_copy; > + ssize_t index; > + off_t s_off; > + off_t d_off =3D 0; > + char *s; > + > + if (start > qsb->used) { > + return 0; > + } It feels confusing that this can return 0 while leaving buf un-malloc'd. It's probably simpler to callers if a non-negative return value ALWAYS guarantees that buf is valid. > + > + index =3D qsb_get_iovec(qsb, start, &s_off); > + if (index < 0) { > + return 0; > + } Wait - how is a negative value possible? Since we already checked against qsb->used, can't this be assert(index >=3D 0)? > +static ssize_t qsb_grow(QEMUSizedBuffer *qsb, size_t new_size) > +{ > + size_t needed_chunks, i; > + size_t chunk_size =3D QSB_CHUNK_SIZE; > + > + if (qsb->size < new_size) { > + needed_chunks =3D DIV_ROUND_UP(new_size - qsb->size, > + chunk_size); > + Hmm. Original creation will use chunks larger than QSB_CHUNK_SIZE (up to the maximum chunk size), presumably for efficiency. Should this function likewise use larger chunks if the size to be grown is larger than the default chunk size? > + qsb->iov =3D g_realloc_n(qsb->iov, qsb->n_iov + needed_chunks,= > + sizeof(struct iovec)); > + if (qsb->iov =3D=3D NULL) { Wait. Can g_realloc_n fail? I know that g_malloc can't fail, and you have to use g_try_malloc instead. Do you need to try a different variant= ? > + return -ENOMEM; > + } > + > + for (i =3D qsb->n_iov; i < qsb->n_iov + needed_chunks; i++) { > + qsb->iov[i].iov_base =3D g_malloc0(chunk_size); Wait. Why do you care about realloc'ing the iov array failing (unlikely, as that's a relatively small allocation unlikely to be more than a megabyte) but completely ignore the possibility of abort'ing when g_malloc0 of a chunk size fails (much bigger, and therefore more likely to fail if memory pressure is high)? Either abort is always fine (no need to ever report -ENOMEM) or you should be using a different allocation function here. > +/** > + * Write into the QEMUSizedBuffer at a given position and a given > + * number of bytes. This function will automatically grow the > + * QEMUSizedBuffer. > + * > + * @qsb: A QEMUSizedBuffer > + * @source: A byte array to copy data from > + * @pos: The position withing the @qsb to write data to s/withing/within/ > + * @size: The number of bytes to copy into the @qsb > + * > + * Returns an error code in case of memory allocation failure, > + * @size otherwise. > + */ > +ssize_t qsb_write_at(QEMUSizedBuffer *qsb, const uint8_t *source, > + off_t pos, size_t count) > +{ > + ssize_t rc =3D qsb_grow(qsb, pos + count); > + size_t to_copy; > + size_t all_copy =3D count; > + const struct iovec *iov; > + ssize_t index; > + char *dest; > + off_t d_off, s_off =3D 0; > + > + if (rc < 0) { > + return rc; > + } > + > + if (pos + count > qsb->used) { > + qsb->used =3D pos + count; > + } > + > + index =3D qsb_get_iovec(qsb, pos, &d_off); > + if (index < 0) { Shouldn't this be an assert, because the prior qsb_grow() should have guaranteed that we are large enough? > +/** > + * Create an exact copy of the given QEMUSizedBuffer. s/exact // - it's definitely a copy representing the same contents, but might have different boundaries for internal iovs, so omitting the word will avoid confusion on whether that was intended. (If you wanted an exact copy, then I would manually allocate each iov to the right size, and memcpy contents over from the original, instead of using qsb_create and qsb_write_at). --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --TIC8oTQaJRJxu8ASfWHq1UE2woDqUcC8U 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 iQEcBAEBCAAGBQJT7lSNAAoJEKeha0olJ0NqzDcH/1PjCJzZNvSu+CYPfnCK4wt2 +oVunXAdzcaI3+SEaU3YiNOU5wezvDLsL3WHeg/agc6vetU34hXA0Wm8lRcn5SJI 4D6tWms20GdHIPM0y+tBVUhX5oqqjUQyJw7JFV3Mpnc8G+uNngB8v6BCDh2u6q6P jCI/Do+9zStcHTS7aOaaQJJHghQjluLJFzy9niTSf6VQg36jsGbO2vR/ZITjIKr+ h2I8HbBlveTQLv4JS0qHYirRxp5vLQ/PChJGVB+9ipsy9TbqpUugdy2fTt8C6jj2 tzzSEbY6fB6wqgjTm6Mp0ye8cewrnROKhzwCiAXeJCcDorJtr+yZG4rWvNxzGuM= =5Zm3 -----END PGP SIGNATURE----- --TIC8oTQaJRJxu8ASfWHq1UE2woDqUcC8U--