[libvirt] [PATCH 2/3] Routine to truncate virBuffer

[Bharata B Rao]

On Mon, Nov 07, 2011 at 11:12:07AM -0700, Eric Blake wrote:
> On 11/06/2011 06:58 AM, Bharata B Rao wrote:
> >Routine to truncate virBuffer
> >
> >From: Bharata B Rao<bharata at linux.vnet.ibm.com>
> >
> >Add a helper to truncate virBuffer.
> >
> >/**
> >  * virBufferTruncate:
> 
> >+++ b/src/util/buf.c
> >@@ -123,6 +123,31 @@ virBufferGrow(virBufferPtr buf, unsigned int len)
> >  }
> >
> >  /**
> >+ * virBufferTruncate:
> >+ * @buf:  the buffer
> >+ * @len:  number of bytes by which the buffer is truncated
> >+ *
> >+ * Truncate the buffer by @len bytes.
> >+ *
> >+ * Returns zero on success or -1 on error
> >+ */
> >+int
> >+virBufferTruncate(virBufferPtr buf, unsigned int len)
> 
> What good is returning an error, given that callers already have to
> use virBufferError for detecting other errors, and given that you
> aren't marking the function as ATTRIBUTE_RETURN_CHECK to require
> callers to respect that error?  One of the points of the virBuffer
> API is that most functions can return void (for example, see
> virBufferAdjustIndent), because the user doesn't care about error
> checking until the end.

Ok, point taken.

> 
> >+{
> >+    if (buf->error)
> >+        return -1;
> >+
> >+    if ((signed)(buf->use - len)<  0) {
> 
> I tend to write the cast as (int), not (signed), if a cast is even
> needed.  You don't catch all possible overflows, though - if
> buf->use is 2, and len is 4294967295 (aka (unsigned)-1), then you've
> proceeded to expand(!) buf->use to 3, skipping an uninitialized
> byte.  A better filter would be:
> 
> if (len > buf->use)
> 
> >+        virBufferSetError(buf, -1);
> >+        return -1;
> >+    }
> >+
> >+    buf->use -= len;
> >+    buf->content[buf->use] = '\0';
> 
> This looks correct, once you filter out invalid len first.

Agreed.

> 
> >+++ b/tests/virbuftest.c
> >@@ -123,7 +123,8 @@ static int testBufAutoIndent(const void *data ATTRIBUTE_UNUSED)
> >      virBufferAddChar(buf, '\n');
> >      virBufferEscapeShell(buf, " 11");
> >      virBufferAddChar(buf, '\n');
> >-
> >+    virBufferAsprintf(buf, "%d", 12);
> >+    virBufferTruncate(buf, 4);
> 
> That gives no change in the expected output.  I'd almost rather see:
> 
> virBufferAsprintf(buf, "%d", 123);
> virBufferTruncate(buf, 1);
> 
> as well as an update to the expected output to see output ending in "12".

The idea was to test with minimal change and hence resorted to this.

In any case, I am dropping this patch for the reason explained in another
thread.

Regards,
Bharata.