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@xxxxxxxxxxxxxxxxxx>
> >
> >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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list