Re: [Qemu-devel] [RFC PATCH v3 1/6] QEMUSizedBuffer/QEMUFile

[Stefan Berger <stefanb@linux.vnet.ibm.com>]

On 08/11/2014 02:47 PM, Dr. David Alan Gilbert wrote:
> * Gonglei (Arei) (arei.gonglei@huawei.com) wrote:
>
> <snip>
>
>>> +/**
>>> + * 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