From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41974) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XTuke-0004Kw-BJ for qemu-devel@nongnu.org; Tue, 16 Sep 2014 11:33:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XTukZ-0000oT-Bf for qemu-devel@nongnu.org; Tue, 16 Sep 2014 11:33:08 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42150) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XTukZ-0000nd-4p for qemu-devel@nongnu.org; Tue, 16 Sep 2014 11:33:03 -0400 Date: Tue, 16 Sep 2014 16:32:49 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20140916153247.GK2518@work-vm> References: <1407407085-22436-1-git-send-email-dgilbert@redhat.com> <1407407085-22436-2-git-send-email-dgilbert@redhat.com> <53EE548C.2070905@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <53EE548C.2070905@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 1/2] QEMUSizedBuffer based QEMUFile List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: joel.schopp@amd.com, quintela@redhat.com, qemu-devel@nongnu.org, stefanb@linux.vnet.ibm.com * Eric Blake (eblake@redhat.com) wrote: > On 08/07/2014 04:24 AM, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" > > > > This is based on Stefan and Joel's patch that creates a QEMUFile that goes > > to a memory buffer; from: Hi Eric, I think I agree with most of your points here; and believe I've nailed them in the v3 I've just posted. Dave > > > > http://lists.gnu.org/archive/html/qemu-devel/2013-03/msg05036.html > > > > 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 > > Signed-off-by: Joel Schopp > > > > 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(+) > > > > > > > +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 = (len > QSB_MAX_CHUNK_SIZE) > > + ? QSB_MAX_CHUNK_SIZE > > + : QSB_CHUNK_SIZE; > > + > > + if (len == 0) { > > + /* we want to allocate at least one chunk */ > > + len = QSB_CHUNK_SIZE; > > + } > > So if I call qsb_create("", 0), len is now QSB_CHUNK_SIZE... > > > + > > + num_chunks = DIV_ROUND_UP(len, chunk_size); > > ...num_chunks is now 1... > > > + alloc_len = num_chunks * chunk_size; > > + > > + qsb = g_new0(QEMUSizedBuffer, 1); > > + qsb->iov = g_new0(struct iovec, num_chunks); > > + qsb->n_iov = num_chunks; > > + > > + for (i = 0; i < num_chunks; i++) { > > + qsb->iov[i].iov_base = g_malloc0(chunk_size); > > + qsb->iov[i].iov_len = chunk_size; > > + if (buffer) { > > ...we enter the loop, and have a buffer to copy... > > > + to_copy = (len - qsb->used) > chunk_size > > + ? chunk_size : (len - qsb->used); > > + memcpy(qsb->iov[i].iov_base, &buffer[qsb->used], to_copy); > > ...and proceed to dereference QSB_CHUNK_SIZE bytes beyond the end of the > 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 = 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 = 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 >= 0)? > > > +static ssize_t qsb_grow(QEMUSizedBuffer *qsb, size_t new_size) > > +{ > > + size_t needed_chunks, i; > > + size_t chunk_size = QSB_CHUNK_SIZE; > > + > > + if (qsb->size < new_size) { > > + needed_chunks = 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 = g_realloc_n(qsb->iov, qsb->n_iov + needed_chunks, > > + sizeof(struct iovec)); > > + if (qsb->iov == 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 = qsb->n_iov; i < qsb->n_iov + needed_chunks; i++) { > > + qsb->iov[i].iov_base = 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 = qsb_grow(qsb, pos + count); > > + size_t to_copy; > > + size_t all_copy = count; > > + const struct iovec *iov; > > + ssize_t index; > > + char *dest; > > + off_t d_off, s_off = 0; > > + > > + if (rc < 0) { > > + return rc; > > + } > > + > > + if (pos + count > qsb->used) { > > + qsb->used = pos + count; > > + } > > + > > + index = 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). > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK