Re: [Qemu-devel] [PATCH v2 1/2] QEMUSizedBuffer based QEMUFile

[Dr. David Alan Gilbert]

* Eric Blake (address@hidden) wrote:
> On 08/07/2014 04:24 AM, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> > 
> > 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 <address@hidden>
> > Signed-off-by: Joel Schopp <address@hidden>
> > 
> > For minor tweeks/rebase I've done to it:
> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > ---
> >  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 / address@hidden / Manchester, UK