* Gonglei (Arei) (arei.gong...@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.
Dave
>
> Best regards,
> -Gonglei
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
