[libvirt] [PATCH 1/4] util: Introduce virBufferAddBuffer

[Martin Kletzander]

On Thu, Feb 19, 2015 at 02:13:42PM +0100, Michal Privoznik wrote:
>There are some places in our code base which follow the following
>pattern:
>
>char *s = virBufferContentAndReset(buf1);
>virBufferAdd(buf2, s, -1);
>

I haven't find any, but OK.  Also if there are some, why not using
the function to clean them up?

>How about introducing an API to join those two lines into one?
>Something like:
>
>virBufferAddBuffer(buf2, buf1);
>

It'd be nice, but I think that wasn't added due to two things I'll
mention below.

>Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>---
> src/libvirt_private.syms |   1 +
> src/util/virbuffer.c     |  33 +++++++++++++-
> src/util/virbuffer.h     |   1 +
> tests/virbuftest.c       | 112 +++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 145 insertions(+), 2 deletions(-)
>
>diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>index 46a1613..c145421 100644
>--- a/src/libvirt_private.syms
>+++ b/src/libvirt_private.syms
>@@ -1073,6 +1073,7 @@ virBitmapToData;
>
> # util/virbuffer.h
> virBufferAdd;
>+virBufferAddBuffer;
> virBufferAddChar;
> virBufferAdjustIndent;
> virBufferAsprintf;
>diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c
>index e94b35d..73c9dd3 100644
>--- a/src/util/virbuffer.c
>+++ b/src/util/virbuffer.c
>@@ -162,8 +162,7 @@ virBufferAdd(virBufferPtr buf, const char *str, int len)
>         len = strlen(str);
>
>     needSize = buf->use + indent + len + 2;
>-    if (needSize > buf->size &&
>-        virBufferGrow(buf, needSize - buf->use) < 0)
>+    if (virBufferGrow(buf, needSize - buf->use) < 0)
>         return;
>

Correct.

>     memset(&buf->content[buf->use], ' ', indent);
>@@ -173,6 +172,36 @@ virBufferAdd(virBufferPtr buf, const char *str, int len)
> }
>
> /**
>+ * virBufferAddBuffer:
>+ * @buf: the buffer to append to
>+ * @toadd: the buffer to append
>+ *
>+ * Add a buffer into another buffer without need to go through:
>+ * virBufferContentAndReset(), virBufferAdd(). Auto indentation
>+ * is NOT applied!
>+ *

Without this it doesn't help with the code that much, does it.
virBufferAdd(&buf, virBufferContentAndReset(&toadd), -1); would help
with one indent only, of course, and this is way faster, but not that
usable IMHO.

>+ * Moreover, be aware that @toadd is eaten with hair. IOW, the
>+ * @toadd buffer is reset after this.
>+ */
>+void
>+virBufferAddBuffer(virBufferPtr buf, virBufferPtr toadd)
>+{
>+    unsigned int needSize;
>+
>+    if (!buf || !toadd || buf->error || toadd->error)
>+        return;

This ^^ could be done better so it's more usable, too.  If @toadd has
an error, just set it on @buf also and free the @toadd so the caller
doesn't have to deal with it.  If you're saying @toadd is reset after
this without specifying any condition, make it so.  The caller can
then use it the same way as with virCommand.  (I know that resetting
the @toad unconditionally will safely work, but the comment doesn't
make sense like this).

>+
>+    needSize = buf->use + toadd->use;
>+    if (virBufferGrow(buf, needSize - buf->use) < 0)

This and ...

>+        return;
>+
>+    memcpy(&buf->content[buf->use], toadd->content, toadd->use);
>+    buf->use += toadd->use;
>+    buf->content[buf->use] = '\0';

... this line shivered up my spine, but they're actually correct if I
look at the code.  Somewhere we guard the needSize by magic number of
bytes and that feels safer.  Hopefully nobody will change the
internals of virBufferGrow() :)

>+    virBufferFreeAndReset(toadd);
>+}
>+
>+/**
>  * virBufferAddChar:
>  * @buf: the buffer to append to
>  * @c: the character to add
>diff --git a/src/util/virbuffer.h b/src/util/virbuffer.h
>index 90e248d..24e81c7 100644
>--- a/src/util/virbuffer.h
>+++ b/src/util/virbuffer.h
>@@ -72,6 +72,7 @@ int virBufferCheckErrorInternal(const virBuffer *buf,
>     __LINE__)
> unsigned int virBufferUse(const virBuffer *buf);
> void virBufferAdd(virBufferPtr buf, const char *str, int len);
>+void virBufferAddBuffer(virBufferPtr buf, virBufferPtr toadd);
> void virBufferAddChar(virBufferPtr buf, char c);
> void virBufferAsprintf(virBufferPtr buf, const char *format, ...)
>   ATTRIBUTE_FMT_PRINTF(2, 3);
>diff --git a/tests/virbuftest.c b/tests/virbuftest.c
>index 554a8c0..884468c 100644
>--- a/tests/virbuftest.c
>+++ b/tests/virbuftest.c
>@@ -199,6 +199,117 @@ static int testBufTrim(const void *data ATTRIBUTE_UNUSED)
>     return ret;
> }
>
>+static int testBufAddBuffer(const void *data ATTRIBUTE_UNUSED)
>+{
>+    virBuffer buf1 = VIR_BUFFER_INITIALIZER;
>+    virBuffer buf2 = VIR_BUFFER_INITIALIZER;
>+    virBuffer buf3 = VIR_BUFFER_INITIALIZER;
>+    int ret = -1;
>+    char *result = NULL;
>+    const char *expected = \
>+"  A long time ago, in a galaxy far,\n" \
>+"  far away...\n"                       \
>+"    It is a period of civil war,\n"    \
>+"    Rebel spaceships, striking\n"      \
>+"    from a hidden base, have won\n"    \
>+"    their first victory against\n"     \
>+"    the evil Galactic Empire.\n"       \
>+"  During the battle, rebel\n"          \
>+"  spies managed to steal secret\n"     \
>+"  plans to the Empire's\n"             \
>+"  ultimate weapon, the DEATH\n"        \
>+"  STAR, an armored space\n"            \
>+"  station with enough power to\n"      \
>+"  destroy an entire planet.\n";
>+
>+    if (virBufferUse(&buf1)) {
>+        TEST_ERROR("buf1 already in use");
>+        goto cleanup;
>+    }
>+
>+    if (virBufferUse(&buf2)) {
>+        TEST_ERROR("buf2 already in use");
>+        goto cleanup;
>+    }
>+
>+    if (virBufferUse(&buf3)) {
>+        TEST_ERROR("buf3 already in use");
>+        goto cleanup;
>+    }
>+
>+    virBufferAdjustIndent(&buf1, 2);
>+    virBufferAddLit(&buf1, "A long time ago, in a galaxy far,\n");
>+    virBufferAddLit(&buf1, "far away...\n");
>+
>+    virBufferAdjustIndent(&buf2, 4);
>+    virBufferAddLit(&buf2, "It is a period of civil war,\n");
>+    virBufferAddLit(&buf2, "Rebel spaceships, striking\n");
>+    virBufferAddLit(&buf2, "from a hidden base, have won\n");
>+    virBufferAddLit(&buf2, "their first victory against\n");
>+    virBufferAddLit(&buf2, "the evil Galactic Empire.\n");
>+
>+    virBufferAdjustIndent(&buf3, 2);
>+    virBufferAddLit(&buf3, "During the battle, rebel\n");
>+    virBufferAddLit(&buf3, "spies managed to steal secret\n");
>+    virBufferAddLit(&buf3, "plans to the Empire's\n");
>+    virBufferAddLit(&buf3, "ultimate weapon, the DEATH\n");
>+    virBufferAddLit(&buf3, "STAR, an armored space\n");
>+    virBufferAddLit(&buf3, "station with enough power to\n");
>+    virBufferAddLit(&buf3, "destroy an entire planet.\n");
>+

Doesn't this break some authorship/copyright laws?  IANAL, so "just
sayin'", not that I head/read it anywhere before O:-) :D
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150224/26ba1244/attachment-0001.sig>