From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54533) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XGv5b-0002Qp-Ay for qemu-devel@nongnu.org; Mon, 11 Aug 2014 15:17:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XGv5U-0008VJ-I3 for qemu-devel@nongnu.org; Mon, 11 Aug 2014 15:17:03 -0400 Received: from e9.ny.us.ibm.com ([32.97.182.139]:40728) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XGv5U-0008V3-DS for qemu-devel@nongnu.org; Mon, 11 Aug 2014 15:16:56 -0400 Received: from /spool/local by e9.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 11 Aug 2014 15:16:54 -0400 Received: from b01cxnp23033.gho.pok.ibm.com (b01cxnp23033.gho.pok.ibm.com [9.57.198.28]) by d01dlp02.pok.ibm.com (Postfix) with ESMTP id C5FF86E8047 for ; Mon, 11 Aug 2014 15:16:39 -0400 (EDT) Received: from d01av03.pok.ibm.com (d01av03.pok.ibm.com [9.56.224.217]) by b01cxnp23033.gho.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id s7BJGow82163064 for ; Mon, 11 Aug 2014 19:16:50 GMT Received: from d01av03.pok.ibm.com (localhost [127.0.0.1]) by d01av03.pok.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s7BJGoZc027849 for ; Mon, 11 Aug 2014 15:16:50 -0400 Received: from [9.2.140.31] (dhcp-9-2-140-31.watson.ibm.com [9.2.140.31]) by d01av03.pok.ibm.com (8.14.4/8.14.4/NCO v10.0 AVin) with ESMTP id s7BJGn8t027820 for ; Mon, 11 Aug 2014 15:16:50 -0400 Message-ID: <53E916A1.6070301@linux.vnet.ibm.com> Date: Mon, 11 Aug 2014 15:16:49 -0400 From: Stefan Berger MIME-Version: 1.0 References: <1407565595-18861-1-git-send-email-sanidhya.iiith@gmail.com> <1407565595-18861-2-git-send-email-sanidhya.iiith@gmail.com> <33183CC9F5247A488A2544077AF1902086C29A46@SZXEMA503-MBS.china.huawei.com> <20140811184714.GJ2368@work-vm> In-Reply-To: <20140811184714.GJ2368@work-vm> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH v3 1/6] QEMUSizedBuffer/QEMUFile List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org On 08/11/2014 02:47 PM, Dr. David Alan Gilbert wrote: > * Gonglei (Arei) (arei.gonglei@huawei.com) wrote: > > > >>> +/** >>> + * Grow the QEMUSizedBuffer to the given size and allocated >>> + * memory for it. >>> + * >>> + * @qsb: A QEMUSizedBuffer >>> + * @new_size: The new size of the buffer >>> + * >>> + * Returns an error code in case of memory allocation failure >>> + * 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; >>> + size_t chunk_size = QSB_CHUNK_SIZE; >>> + >>> + if (qsb->size < new_size) { >>> + needed_chunks = DIV_ROUND_UP(new_size - qsb->size, >>> + chunk_size); >>> + >>> + qsb->iov = g_realloc_n(qsb->iov, qsb->n_iov + needed_chunks, >>> + sizeof(struct iovec)); >>> + if (qsb->iov == NULL) { >>> + return -ENOMEM; >>> + } >> OK... > Is it? https://developer.gnome.org/glib/stable/glib-Memory-Allocation.html > says that g_realloc_n is 'similar to g_realloc()' except for overflow protection, > g_realloc() doesn't say what it's behaviour on OOM is, but g_try_realloc says > that 'Contrast with g_realloc(), which aborts the program on failure' > So the only case iov will return NULL there is if the size is 0 which it can't > be. So should that be a g_try_realloc_n ? > >>> + >>> + for (i = qsb->n_iov; i < qsb->n_iov + needed_chunks; i++) { >>> + qsb->iov[i].iov_base = g_malloc0(chunk_size); >>> + qsb->iov[i].iov_len = chunk_size; >>> + } >>> + >>> + qsb->n_iov += needed_chunks; >>> + qsb->size += (needed_chunks * chunk_size); >>> + } >>> + >>> + return qsb->size; >>> +} >>> + >>> +/** >>> + * 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 >>> + * @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; >>> + } >>> + >> OK... >> >>> + if (pos + count > qsb->used) { >>> + qsb->used = pos + count; >>> + } >>> + >>> + index = qsb_get_iovec(qsb, pos, &d_off); >>> + if (index < 0) { >>> + return 0; >>> + } >>> + >>> + while (all_copy > 0) { >>> + iov = &qsb->iov[index]; >>> + >>> + dest = iov->iov_base; >>> + >>> + to_copy = iov->iov_len - d_off; >>> + if (to_copy > all_copy) { >>> + to_copy = all_copy; >>> + } >>> + >>> + memcpy(&dest[d_off], &source[s_off], to_copy); >>> + >>> + s_off += to_copy; >>> + all_copy -= to_copy; >>> + >>> + d_off = 0; >>> + index++; >>> + } >>> + >>> + return count; >>> +} >>> + >>> +/** >>> + * Create an exact copy of the given QEMUSizedBuffer. >>> + * >>> + * @qsb : A QEMUSizedBuffer >>> + * >>> + * Returns a clone of @qsb >>> + */ >>> +QEMUSizedBuffer *qsb_clone(const QEMUSizedBuffer *qsb) >>> +{ >>> + QEMUSizedBuffer *out = qsb_create(NULL, qsb_get_length(qsb)); >>> + size_t i; >>> + off_t pos = 0; >>> + >>> + for (i = 0; i < qsb->n_iov; i++) { >>> + pos += qsb_write_at(out, qsb->iov[i].iov_base, >>> + pos, qsb->iov[i].iov_len); >> If qsb_write_at() return -ENOMEM, just simply add it to pos ? > qsb_create uses g_new0 so it will abort on out of memory; > what should qsb_clone do if qsb_write_at returns -ENOMEM? > (Admittedly anything is better than getting the position wrong). > I guess the choice is to allow it to return NULL, tidying up > and offering the chance sometime in the future of tidying up > the other allocators. I remember looking at all the allocation API as well. It seems QEMU would terminate upon OOM since g_new0 is used all over the place. Stefan