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@xxxxxxxxxx>
---
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
Attachment:
pgpiG8Ja8aVIu.pgp
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list