Although the compiler wasn't complaining (since it was the pointer, rather than what was being pointed to, that was actually const), it looks quite suspicious to call a function with an argument labeled const when the nature of the pointer (virBufferPtr) is hidden behind a typedef. Dropping const makes the function declarations easier to read. * src/util/buf.h: Drop const from all functions that modify buffer argument. * src/util/buf.c (virBufferSetError, virBufferAdd) (virBufferContentAndReset, virBufferFreeAndReset) (virBufferAsprintf, virBufferVasprintf, virBufferEscapeString) (virBufferEscapeSexpr): Fix fallout. --- src/util/buf.c | 28 ++++++++++++++-------------- src/util/buf.h | 22 ++++++++++++---------- 2 files changed, 26 insertions(+), 24 deletions(-) diff --git a/src/util/buf.c b/src/util/buf.c index 5002486..c737696 100644 --- a/src/util/buf.c +++ b/src/util/buf.c @@ -39,7 +39,7 @@ struct _virBuffer { * freeing the content and setting the error flag. */ static void -virBufferSetError(const virBufferPtr buf) +virBufferSetError(virBufferPtr buf) { VIR_FREE(buf->content); buf->size = 0; @@ -113,7 +113,7 @@ virBufferAdd(const virBufferPtr buf, const char *str, int len) /** * virBufferAddChar: - * @buf: the buffer to add to + * @buf: the buffer to append to * @c: the character to add * * Add a single character 'c' to a buffer. @@ -150,7 +150,7 @@ virBufferAddChar (virBufferPtr buf, char c) * Returns the buffer content or NULL in case of error. */ char * -virBufferContentAndReset(const virBufferPtr buf) +virBufferContentAndReset(virBufferPtr buf) { char *str; if (buf == NULL) @@ -172,7 +172,7 @@ virBufferContentAndReset(const virBufferPtr buf) * * Frees the buffer content and resets the buffer structure. */ -void virBufferFreeAndReset(const virBufferPtr buf) +void virBufferFreeAndReset(virBufferPtr buf) { char *str = virBufferContentAndReset(buf); @@ -214,14 +214,14 @@ virBufferUse(const virBufferPtr buf) /** * virBufferAsprintf: - * @buf: the buffer to dump + * @buf: the buffer to append to * @format: the format * @...: the variable list of arguments * * Do a formatted print to an XML buffer. */ void -virBufferAsprintf(const virBufferPtr buf, const char *format, ...) +virBufferAsprintf(virBufferPtr buf, const char *format, ...) { va_list argptr; va_start(argptr, format); @@ -238,7 +238,7 @@ virBufferAsprintf(const virBufferPtr buf, const char *format, ...) * Do a formatted print to an XML buffer. */ void -virBufferVasprintf(const virBufferPtr buf, const char *format, va_list argptr) +virBufferVasprintf(virBufferPtr buf, const char *format, va_list argptr) { int size, count, grow_size; va_list copy; @@ -285,7 +285,7 @@ virBufferVasprintf(const virBufferPtr buf, const char *format, va_list argptr) /** * virBufferEscapeString: - * @buf: the buffer to dump + * @buf: the buffer to append to * @format: a printf like format string but with only one %s parameter * @str: the string argument which need to be escaped * @@ -293,7 +293,7 @@ virBufferVasprintf(const virBufferPtr buf, const char *format, va_list argptr) * is escaped to avoid generating a not well-formed XML instance. */ void -virBufferEscapeString(const virBufferPtr buf, const char *format, const char *str) +virBufferEscapeString(virBufferPtr buf, const char *format, const char *str) { int len; char *escaped, *out; @@ -370,7 +370,7 @@ virBufferEscapeString(const virBufferPtr buf, const char *format, const char *st /** * virBufferEscapeSexpr: - * @buf: the buffer to dump + * @buf: the buffer to append to * @format: a printf like format string but with only one %s parameter * @str: the string argument which need to be escaped * @@ -379,7 +379,7 @@ virBufferEscapeString(const virBufferPtr buf, const char *format, const char *st * doesn't fully escape the sexpr, just enough for our code to work. */ void -virBufferEscapeSexpr(const virBufferPtr buf, +virBufferEscapeSexpr(virBufferPtr buf, const char *format, const char *str) { @@ -426,7 +426,7 @@ virBufferEscapeSexpr(const virBufferPtr buf, /** * virBufferURIEncodeString: - * @buf: the buffer to append to + * @buf: the buffer to append to * @str: the string argument which will be URI-encoded * * Append the string to the buffer. The string will be URI-encoded @@ -434,7 +434,7 @@ virBufferEscapeSexpr(const virBufferPtr buf, * with '%xx' hex sequences). */ void -virBufferURIEncodeString (virBufferPtr buf, const char *str) +virBufferURIEncodeString(virBufferPtr buf, const char *str) { int grow_size = 0; const char *p; @@ -473,7 +473,7 @@ virBufferURIEncodeString (virBufferPtr buf, const char *str) /** * virBufferStrcat: - * @buf: the buffer to dump + * @buf: the buffer to append to * @...: the variable list of strings, the last argument must be NULL * * Concatenate strings to an XML buffer. diff --git a/src/util/buf.h b/src/util/buf.h index 06d01ba..42a5044 100644 --- a/src/util/buf.h +++ b/src/util/buf.h @@ -36,21 +36,23 @@ struct _virBuffer { }; # endif -char *virBufferContentAndReset(const virBufferPtr buf); -void virBufferFreeAndReset(const virBufferPtr buf); +char *virBufferContentAndReset(virBufferPtr buf); +void virBufferFreeAndReset(virBufferPtr buf); int virBufferError(const virBufferPtr buf); unsigned int virBufferUse(const virBufferPtr buf); -void virBufferAdd(const virBufferPtr buf, const char *str, int len); -void virBufferAddChar(const virBufferPtr buf, char c); -void virBufferAsprintf(const virBufferPtr buf, const char *format, ...) +void virBufferAdd(virBufferPtr buf, const char *str, int len); +void virBufferAddChar(virBufferPtr buf, char c); +void virBufferAsprintf(virBufferPtr buf, const char *format, ...) ATTRIBUTE_FMT_PRINTF(2, 3); -void virBufferVasprintf(const virBufferPtr buf, const char *format, va_list ap) +void virBufferVasprintf(virBufferPtr buf, const char *format, va_list ap) ATTRIBUTE_FMT_PRINTF(2, 0); -void virBufferStrcat(const virBufferPtr buf, ...) +void virBufferStrcat(virBufferPtr buf, ...) ATTRIBUTE_SENTINEL; -void virBufferEscapeString(const virBufferPtr buf, const char *format, const char *str); -void virBufferEscapeSexpr(const virBufferPtr buf, const char *format, const char *str); -void virBufferURIEncodeString (const virBufferPtr buf, const char *str); +void virBufferEscapeString(virBufferPtr buf, const char *format, + const char *str); +void virBufferEscapeSexpr(virBufferPtr buf, const char *format, + const char *str); +void virBufferURIEncodeString(virBufferPtr buf, const char *str); # define virBufferAddLit(buf_, literal_string_) \ virBufferAdd (buf_, "" literal_string_ "", sizeof literal_string_ - 1) -- 1.7.4.4

On 10/17/2011 05:20 AM, Hai Dong Li wrote: No. Rather, this is an artifact of git diff. When showing the context line for a hunk, it uses the context from the pre-patch file (where the 'const' was still present), even though the hunk itself applies to the post-patch version without 'const'. Basically, I never worry about the @@ lines - they are there as hints for where to apply the patch, and not a definitive part of the patch itself. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

I had some temporary test failures while working on virbuf improvements in later patches, with output that looked like: Expected [<] Actual [ <] which is pretty hard to figure out. Adding an Offset designation made it much easier to find which particular '<' was at the wrong indentation, to fix the right part of the code. * tests/testutils.c (virtTestDifference): Make it easier to diagnose test failures. --- tests/testutils.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/tests/testutils.c b/tests/testutils.c index 08db732..7eb40f0 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -359,7 +359,7 @@ int virtTestDifference(FILE *stream, } /* Show the trimmed differences */ - fprintf(stream, "\nExpect ["); + fprintf(stream, "\nOffset %d\nExpect [", (int) (expectStart - expect)); if ((expectEnd - expectStart + 1) && fwrite(expectStart, (expectEnd-expectStart+1), 1, stream) != 1) return -1; -- 1.7.4.4

The next patch wants to add some sanity checking, which would be a different error than ENOMEM. Many existing callers blindly report OOM failure if virBuf reports an error, and this will be wrong in the (unlikely) case that they actually had a usage error instead; but since the most common error really is ENOMEM, I'm not going to fix all callers. Meanwhile, new discriminating callers can react differently depending on what failure happened. * src/util/buf.c (virBufferSetError): Add parameter. (virBufferGrow, virBufferVasprintf, virBufferEscapeString) (virBufferEscapeSexpr): Adjust callers. --- src/util/buf.c | 26 +++++++++++++------------- 1 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/util/buf.c b/src/util/buf.c index c737696..98bb681 100644 --- a/src/util/buf.c +++ b/src/util/buf.c @@ -27,24 +27,24 @@ struct _virBuffer { unsigned int size; unsigned int use; - unsigned int error; + unsigned int error; /* errno value, or -1 for usage error */ char *content; }; /** * virBufferFail * @buf: the buffer + * @error: which error occurred (errno value, or -1 for usage) * - * Mark the buffer has having failed a memory allocation, - * freeing the content and setting the error flag. + * Mark the buffer as failed, free the content and set the error flag. */ static void -virBufferSetError(virBufferPtr buf) +virBufferSetError(virBufferPtr buf, int error) { VIR_FREE(buf->content); buf->size = 0; buf->use = 0; - buf->error = 1; + buf->error = error; } /** @@ -70,7 +70,7 @@ virBufferGrow(virBufferPtr buf, unsigned int len) size = buf->use + len + 1000; if (VIR_REALLOC_N(buf->content, size) < 0) { - virBufferSetError(buf); + virBufferSetError(buf, errno); return -1; } buf->size = size; @@ -184,15 +184,15 @@ void virBufferFreeAndReset(virBufferPtr buf) * @buf: the buffer * * Check to see if the buffer is in an error state due - * to failed memory allocation + * to failed memory allocation or usage error * - * Return true if in error, 0 if normal + * Return positive errno value or -1 on usage error, 0 if normal */ int virBufferError(const virBufferPtr buf) { if (buf == NULL) - return 1; + return -1; return buf->error; } @@ -258,7 +258,7 @@ virBufferVasprintf(virBufferPtr buf, const char *format, va_list argptr) size = buf->size - buf->use; if ((count = vsnprintf(&buf->content[buf->use], size, format, copy)) < 0) { - virBufferSetError(buf); + virBufferSetError(buf, errno); va_end(copy); return; } @@ -276,7 +276,7 @@ virBufferVasprintf(virBufferPtr buf, const char *format, va_list argptr) size = buf->size - buf->use; if ((count = vsnprintf(&buf->content[buf->use], size, format, argptr)) < 0) { - virBufferSetError(buf); + virBufferSetError(buf, errno); return; } } @@ -313,7 +313,7 @@ virBufferEscapeString(virBufferPtr buf, const char *format, const char *str) if (xalloc_oversized(6, len) || VIR_ALLOC_N(escaped, 6 * len + 1) < 0) { - virBufferSetError(buf); + virBufferSetError(buf, errno); return; } @@ -401,7 +401,7 @@ virBufferEscapeSexpr(virBufferPtr buf, if (xalloc_oversized(2, len) || VIR_ALLOC_N(escaped, 2 * len + 1) < 0) { - virBufferSetError(buf); + virBufferSetError(buf, errno); return; } -- 1.7.4.4

Rather than having to adjust all callers in a chain to deal with indentation, it is nicer to have virBuffer do auto-indentation. * src/util/buf.h (_virBuffer): Increase size. (virBufferAdjustIndent, virBufferGetIndent): New prototypes. * src/libvirt_private.syms (buf.h): Export new functions. * src/util/buf.c (virBufferAdjustIndent, virBufferGetIndent): New functions. (virBufferSetError, virBufferAdd, virBufferAddChar) (virBufferVasprintf, virBufferStrcat, virBufferURIEncodeString): Implement auto-indentation. * tests/virbuftest.c (testBufAutoIndent): Test it. (testBufInfiniteLoop): Don't rely on internals. Idea by Daniel P. Berrange. --- src/libvirt_private.syms | 2 + src/util/buf.c | 138 ++++++++++++++++++++++++++++----------------- src/util/buf.h | 12 +++- tests/virbuftest.c | 85 +++++++++++++++++++++++++--- 4 files changed, 172 insertions(+), 65 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c2a3fab..695ff33 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -22,12 +22,14 @@ virBitmapString; # buf.h virBufferAdd; virBufferAddChar; +virBufferAdjustIndent; virBufferAsprintf; virBufferContentAndReset; virBufferError; virBufferEscapeSexpr; virBufferEscapeString; virBufferFreeAndReset; +virBufferGetIndent; virBufferStrcat; virBufferURIEncodeString; virBufferUse; diff --git a/src/util/buf.c b/src/util/buf.c index 98bb681..b409de4 100644 --- a/src/util/buf.c +++ b/src/util/buf.c @@ -28,6 +28,7 @@ struct _virBuffer { unsigned int size; unsigned int use; unsigned int error; /* errno value, or -1 for usage error */ + int indent; char *content; }; @@ -44,10 +45,54 @@ virBufferSetError(virBufferPtr buf, int error) VIR_FREE(buf->content); buf->size = 0; buf->use = 0; + buf->indent = 0; buf->error = error; } /** + * virBufferAdjustIndent: + * @buf: the buffer + * @indent: adjustment to make + * + * Alter the auto-indent value by adding indent (positive to increase, + * negative to decrease). Automatic indentation is performed by all + * additive functions when the existing buffer is empty or ends with a + * newline (however, note that no indentation is added if you append a + * string with an embedded newline). If @indent would cause overflow, + * the buffer error indicator is set. + */ +void +virBufferAdjustIndent(virBufferPtr buf, int indent) +{ + if (!buf || buf->error) + return; + if (indent > 0 ? INT_MAX - indent < buf->indent + : buf->indent < -indent) { + virBufferSetError(buf, -1); + return; + } + buf->indent += indent; +} + +/** + * virBufferGetIndent: + * @buf: the buffer + * @dynamic: if false, return set value; if true, return 0 unless next + * append would be affected by auto-indent + * + * Return the current auto-indent value, or -1 if there has been an error. + */ +int +virBufferGetIndent(const virBufferPtr buf, bool dynamic) +{ + if (!buf || buf->error) + return -1; + if (dynamic && buf->use && buf->content[buf->use - 1] != '\n') + return 0; + return buf->indent; +} + +/** * virBufferGrow: * @buf: the buffer * @len: the minimum free size to allocate on top of existing used space @@ -79,35 +124,39 @@ virBufferGrow(virBufferPtr buf, unsigned int len) /** * virBufferAdd: - * @buf: the buffer to add to - * @str: the string - * @len: the number of bytes to add + * @buf: the buffer to append to + * @str: the string + * @len: the number of bytes to add, or -1 * - * Add a string range to an XML buffer. if len == -1, the length of - * str is recomputed to the full string. + * Add a string range to an XML buffer. If @len == -1, the length of + * str is recomputed to the full string. Auto indentation may be applied. * */ void virBufferAdd(const virBufferPtr buf, const char *str, int len) { unsigned int needSize; + int indent; - if ((str == NULL) || (buf == NULL) || (len == 0)) + if (!str || !buf || (len == 0 && buf->indent == 0)) return; if (buf->error) return; + indent = virBufferGetIndent(buf, true); + if (len < 0) len = strlen(str); - needSize = buf->use + len + 2; + needSize = buf->use + indent + len + 2; if (needSize > buf->size && virBufferGrow(buf, needSize - buf->use) < 0) return; - memcpy (&buf->content[buf->use], str, len); - buf->use += len; + memset(&buf->content[buf->use], ' ', indent); + memcpy(&buf->content[buf->use + indent], str, len); + buf->use += indent + len; buf->content[buf->use] = '\0'; } @@ -116,27 +165,13 @@ virBufferAdd(const virBufferPtr buf, const char *str, int len) * @buf: the buffer to append to * @c: the character to add * - * Add a single character 'c' to a buffer. + * Add a single character 'c' to a buffer. Auto indentation may be applied. * */ void -virBufferAddChar (virBufferPtr buf, char c) +virBufferAddChar(virBufferPtr buf, char c) { - unsigned int needSize; - - if (buf == NULL) - return; - - if (buf->error) - return; - - needSize = buf->use + 2; - if (needSize > buf->size && - virBufferGrow (buf, needSize - buf->use) < 0) - return; - - buf->content[buf->use++] = c; - buf->content[buf->use] = '\0'; + virBufferAdd(buf, &c, 1); } /** @@ -218,7 +253,7 @@ virBufferUse(const virBufferPtr buf) * @format: the format * @...: the variable list of arguments * - * Do a formatted print to an XML buffer. + * Do a formatted print to an XML buffer. Auto indentation may be applied. */ void virBufferAsprintf(virBufferPtr buf, const char *format, ...) @@ -231,11 +266,11 @@ virBufferAsprintf(virBufferPtr buf, const char *format, ...) /** * virBufferVasprintf: - * @buf: the buffer to dump + * @buf: the buffer to append to * @format: the format * @argptr: the variable list of arguments * - * Do a formatted print to an XML buffer. + * Do a formatted print to an XML buffer. Auto indentation may be applied. */ void virBufferVasprintf(virBufferPtr buf, const char *format, va_list argptr) @@ -249,6 +284,9 @@ virBufferVasprintf(virBufferPtr buf, const char *format, va_list argptr) if (buf->error) return; + if (buf->indent) + virBufferAdd(buf, "", -1); + if (buf->size == 0 && virBufferGrow(buf, 100) < 0) return; @@ -287,10 +325,12 @@ virBufferVasprintf(virBufferPtr buf, const char *format, va_list argptr) * virBufferEscapeString: * @buf: the buffer to append to * @format: a printf like format string but with only one %s parameter - * @str: the string argument which need to be escaped + * @str: the string argument which needs to be escaped * - * Do a formatted print with a single string to an XML buffer. The string - * is escaped to avoid generating a not well-formed XML instance. + * Do a formatted print with a single string to an XML buffer. The + * string is escaped for use in XML. If @str is NULL, nothing is + * added (not even the rest of @format). Auto indentation may be + * applied. */ void virBufferEscapeString(virBufferPtr buf, const char *format, const char *str) @@ -372,11 +412,12 @@ virBufferEscapeString(virBufferPtr buf, const char *format, const char *str) * virBufferEscapeSexpr: * @buf: the buffer to append to * @format: a printf like format string but with only one %s parameter - * @str: the string argument which need to be escaped + * @str: the string argument which needs to be escaped * - * Do a formatted print with a single string to an sexpr buffer. The string - * is escaped to avoid generating a sexpr that xen will choke on. This - * doesn't fully escape the sexpr, just enough for our code to work. + * Do a formatted print with a single string to an sexpr buffer. The + * string is escaped to avoid generating a sexpr that xen will choke + * on. This doesn't fully escape the sexpr, just enough for our code + * to work. Auto indentation may be applied. */ void virBufferEscapeSexpr(virBufferPtr buf, @@ -431,7 +472,7 @@ virBufferEscapeSexpr(virBufferPtr buf, * * Append the string to the buffer. The string will be URI-encoded * during the append (ie any non alpha-numeric characters are replaced - * with '%xx' hex sequences). + * with '%xx' hex sequences). Auto indentation may be applied. */ void virBufferURIEncodeString(virBufferPtr buf, const char *str) @@ -447,6 +488,9 @@ virBufferURIEncodeString(virBufferPtr buf, const char *str) if (buf->error) return; + if (buf->indent) + virBufferAdd(buf, "", -1); + for (p = str; *p; ++p) { if (c_isalnum(*p)) grow_size++; @@ -454,7 +498,7 @@ virBufferURIEncodeString(virBufferPtr buf, const char *str) grow_size += 3; /* %ab */ } - if (virBufferGrow (buf, grow_size) < 0) + if (virBufferGrow(buf, grow_size) < 0) return; for (p = str; *p; ++p) { @@ -476,7 +520,8 @@ virBufferURIEncodeString(virBufferPtr buf, const char *str) * @buf: the buffer to append to * @...: the variable list of strings, the last argument must be NULL * - * Concatenate strings to an XML buffer. + * Concatenate strings to an XML buffer. Auto indentation may be applied + * after each string argument. */ void virBufferStrcat(virBufferPtr buf, ...) @@ -488,18 +533,7 @@ virBufferStrcat(virBufferPtr buf, ...) return; va_start(ap, buf); - - while ((str = va_arg(ap, char *)) != NULL) { - unsigned int len = strlen(str); - unsigned int needSize = buf->use + len + 2; - - if (needSize > buf->size) { - if (virBufferGrow(buf, needSize - buf->use) < 0) - break; - } - memcpy(&buf->content[buf->use], str, len); - buf->use += len; - buf->content[buf->use] = 0; - } + while ((str = va_arg(ap, char *)) != NULL) + virBufferAdd(buf, str, -1); va_end(ap); } diff --git a/src/util/buf.h b/src/util/buf.h index 42a5044..08cb727 100644 --- a/src/util/buf.h +++ b/src/util/buf.h @@ -24,15 +24,16 @@ typedef struct _virBuffer virBuffer; typedef virBuffer *virBufferPtr; # ifndef __VIR_BUFFER_C__ -# define VIR_BUFFER_INITIALIZER { 0, 0, 0, NULL } +# define VIR_BUFFER_INITIALIZER { 0, 0, 0, 0, NULL } -/* This struct must be kept in syn with the real struct +/* This struct must be kept in sync with the real struct in the buf.c impl file */ struct _virBuffer { unsigned int a; unsigned int b; unsigned int c; - char *d; + int d; + char *e; }; # endif @@ -55,6 +56,9 @@ void virBufferEscapeSexpr(virBufferPtr buf, const char *format, void virBufferURIEncodeString(virBufferPtr buf, const char *str); # define virBufferAddLit(buf_, literal_string_) \ - virBufferAdd (buf_, "" literal_string_ "", sizeof literal_string_ - 1) + virBufferAdd(buf_, "" literal_string_ "", sizeof literal_string_ - 1) + +void virBufferAdjustIndent(virBufferPtr buf, int indent); +int virBufferGetIndent(const virBufferPtr buf, bool dynamic); #endif /* __VIR_BUFFER_H__ */ diff --git a/tests/virbuftest.c b/tests/virbuftest.c index 01db313..2f94e1c 100644 --- a/tests/virbuftest.c +++ b/tests/virbuftest.c @@ -20,7 +20,7 @@ struct testInfo { int doEscape; }; -static int testBufInfiniteLoop(const void *data ATTRIBUTE_UNUSED) +static int testBufInfiniteLoop(const void *data) { virBuffer bufinit = VIR_BUFFER_INITIALIZER; virBufferPtr buf = &bufinit; @@ -28,18 +28,13 @@ static int testBufInfiniteLoop(const void *data ATTRIBUTE_UNUSED) int ret = -1; const struct testInfo *info = data; - /* This relies of virBuffer internals, so may break if things change - * in the future */ virBufferAddChar(buf, 'a'); - if (buf->a != 1002 || buf->b != 1) { - TEST_ERROR("Buf was not expected size, size=%d use=%d\n", - buf->a, buf->b); - goto out; - } /* - * Infinite loop triggers if: + * Infinite loop used to trigger if: * (strlen + 1 > 1000) && (strlen == buf-size - buf-use - 1) + * which was the case after the above addchar at the time of the bug. + * This test is a bit fragile, since it relies on virBuffer internals. */ if (virAsprintf(&addstr, "%*s", buf->a - buf->b - 1, "a") < 0) { goto out; @@ -63,6 +58,77 @@ out: return ret; } +static int testBufAutoIndent(const void *data ATTRIBUTE_UNUSED) +{ + virBuffer bufinit = VIR_BUFFER_INITIALIZER; + virBufferPtr buf = &bufinit; + const char expected[] = + " 1\n 2\n 3\n 4\n 5\n 6\n 7\n &amp;\n 8\n 9\n"; + char *result = NULL; + int ret = 0; + + if (virBufferGetIndent(buf, false) != 0 || + virBufferGetIndent(buf, true) != 0) { + TEST_ERROR("Wrong indentation"); + ret = -1; + } + virBufferAdjustIndent(buf, 3); + if (virBufferGetIndent(buf, false) != 3 || + virBufferGetIndent(buf, true) != 3 || + virBufferError(buf)) { + TEST_ERROR("Wrong indentation"); + ret = -1; + } + virBufferAdjustIndent(buf, -2); + if (virBufferGetIndent(buf, false) != 1 || + virBufferGetIndent(buf, true) != 1 || + virBufferError(buf)) { + TEST_ERROR("Wrong indentation"); + ret = -1; + } + virBufferAdjustIndent(buf, -2); + if (virBufferGetIndent(buf, false) != -1 || + virBufferGetIndent(buf, true) != -1 || + virBufferError(buf) != -1) { + TEST_ERROR("Usage error not flagged"); + ret = -1; + } + virBufferFreeAndReset(buf); + if (virBufferGetIndent(buf, false) != 0 || + virBufferGetIndent(buf, true) != 0 || + virBufferError(buf)) { + TEST_ERROR("Reset didn't clear indentation"); + ret = -1; + } + virBufferAdjustIndent(buf, 2); + virBufferAddLit(buf, "1"); + if (virBufferGetIndent(buf, false) != 2 || + virBufferGetIndent(buf, true) != 0) { + TEST_ERROR("Wrong indentation"); + ret = -1; + } + virBufferAddLit(buf, "\n"); + virBufferAdd(buf, "" "2\n", -1); /* Extra "" appeases syntax-check */ + virBufferAddChar(buf, '3'); + virBufferAddChar(buf, '\n'); + virBufferAsprintf(buf, "%d", 4); + virBufferAsprintf(buf, "%c", '\n'); + virBufferStrcat(buf, "5", "\n", "6\n", NULL); + virBufferEscapeString(buf, "%s\n", "7"); + virBufferEscapeString(buf, "%s\n", "&"); + virBufferEscapeSexpr(buf, "%s", "8\n"); + virBufferURIEncodeString(buf, "9"); + virBufferAddChar(buf, '\n'); + + result = virBufferContentAndReset(buf); + if (!result || STRNEQ(result, expected)) { + virtTestDifference(stderr, expected, result); + ret = -1; + } + VIR_FREE(result); + return ret; +} + static int mymain(void) { @@ -78,6 +144,7 @@ mymain(void) DO_TEST("EscapeString infinite loop", testBufInfiniteLoop, 1); DO_TEST("VSprintf infinite loop", testBufInfiniteLoop, 0); + DO_TEST("Auto-indentation", testBufAutoIndent, 0); return(ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); } -- 1.7.4.4

On 09/30/2011 12:22 AM, Eric Blake wrote: Let me express my appreciation towards this testBufAutoIndent() function, by the way. It's beautiful.

On 10/19/2011 11:51 AM, Peter Krempa wrote: Okay, I simplified these two lines down to one. Yet they missed out on the new stuff in the meantime. Here's what I squashed before pushing: diff --git i/src/util/buf.c w/src/util/buf.c index 3dd9a72..ac5360c 100644 --- i/src/util/buf.c +++ w/src/util/buf.c @@ -284,8 +284,7 @@ virBufferVasprintf(virBufferPtr buf, const char *format, va_list argptr) if (buf->error) return; - if (buf->indent) - virBufferAddLit(buf, ""); + virBufferAddLit(buf, ""); /* auto-indent */ if (buf->size == 0 && virBufferGrow(buf, 100) < 0) @@ -500,8 +499,7 @@ virBufferURIEncodeString(virBufferPtr buf, const char *str) if (buf->error) return; - if (buf->indent) - virBufferAddLit(buf, ""); + virBufferAddLit(buf, ""); /* auto-indent */ for (p = str; *p; ++p) { if (c_isalnum(*p)) diff --git i/tests/virbuftest.c w/tests/virbuftest.c index 2f94e1c..bc53959 100644 --- i/tests/virbuftest.c +++ w/tests/virbuftest.c @@ -63,7 +63,7 @@ static int testBufAutoIndent(const void *data ATTRIBUTE_UNUSED) virBuffer bufinit = VIR_BUFFER_INITIALIZER; virBufferPtr buf = &bufinit; const char expected[] = - " 1\n 2\n 3\n 4\n 5\n 6\n 7\n &amp;\n 8\n 9\n"; + " 1\n 2\n 3\n 4\n 5\n 6\n 7\n &amp;\n 8\n 9\n 10\n ' 11'\n"; char *result = NULL; int ret = 0; @@ -86,7 +86,7 @@ static int testBufAutoIndent(const void *data ATTRIBUTE_UNUSED) TEST_ERROR("Wrong indentation"); ret = -1; } - virBufferAdjustIndent(buf, -2); + virBufferAdjustIndent(buf, -3); if (virBufferGetIndent(buf, false) != -1 || virBufferGetIndent(buf, true) != -1 || virBufferError(buf) != -1) { @@ -119,6 +119,10 @@ static int testBufAutoIndent(const void *data ATTRIBUTE_UNUSED) virBufferEscapeSexpr(buf, "%s", "8\n"); virBufferURIEncodeString(buf, "9"); virBufferAddChar(buf, '\n'); + virBufferEscapeShell(buf, "10"); + virBufferAddChar(buf, '\n'); + virBufferEscapeShell(buf, " 11"); + virBufferAddChar(buf, '\n'); result = virBufferContentAndReset(buf); if (!result || STRNEQ(result, expected)) { -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

<domainsnapshot> is the first public instance of <domain> being used as a sub-element, although we have two other private uses (runtime state, and migration cookie). Although indentation has no effect on XML parsing, using it makes the output more consistent. This uses virBuffer auto-indentation to obtain the effect, for all but the portions of <domain> that are not generated a line at a time into the same virBuffer. Further patches will clean up the remaining problems. * src/conf/domain_conf.h (virDomainDefFormatInternal): New prototype. * src/conf/domain_conf.c (virDomainDefFormatInternal): Export. (virDomainObjFormat, virDomainSnapshotDefFormat): Update callers. * src/libvirt_private.syms (domain_conf.h): Add new export. * src/qemu/qemu_migration.c (qemuMigrationCookieXMLFormat): Use new function. (qemuMigrationCookieXMLFormatStr): Update caller. --- src/conf/domain_conf.c | 16 +++++++++++----- src/conf/domain_conf.h | 3 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_migration.c | 25 ++++++++++++++++--------- 4 files changed, 31 insertions(+), 14 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c141982..33482c0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10483,14 +10483,13 @@ verify(((VIR_DOMAIN_XML_INTERNAL_STATUS | /* This internal version can accept VIR_DOMAIN_XML_INTERNAL_*, * whereas the public version cannot. Also, it appends to an existing - * buffer, rather than flattening to string. Return -1 on failure. */ -static int + * buffer (possibly with auto-indent), rather than flattening to string. + * Return -1 on failure. */ +int virDomainDefFormatInternal(virDomainDefPtr def, unsigned int flags, virBufferPtr buf) { - /* XXX Also need to take an indentation parameter - either int or - * string prefix, so that snapshot xml gets uniform indentation. */ unsigned char *uuid; char uuidstr[VIR_UUID_STRING_BUFLEN]; const char *type = NULL; @@ -10980,8 +10979,10 @@ static char *virDomainObjFormat(virCapsPtr caps, ((caps->privateDataXMLFormat)(&buf, obj->privateData)) < 0) goto error; + virBufferAdjustIndent(&buf, 2); if (virDomainDefFormatInternal(obj->def, flags, &buf) < 0) goto error; + virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</domstatus>\n"); @@ -11996,7 +11997,12 @@ char *virDomainSnapshotDefFormat(char *domain_uuid, virBufferAddLit(&buf, " </disks>\n"); } if (def->dom) { - virDomainDefFormatInternal(def->dom, flags, &buf); + virBufferAdjustIndent(&buf, 2); + if (virDomainDefFormatInternal(def->dom, flags, &buf) < 0) { + virBufferFreeAndReset(&buf); + return NULL; + } + virBufferAdjustIndent(&buf, -2); } else { virBufferAddLit(&buf, " <domain>\n"); virBufferAsprintf(&buf, " <uuid>%s</uuid>\n", domain_uuid); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0bc0042..effc630 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1653,6 +1653,9 @@ int virDomainDefAddImplicitControllers(virDomainDefPtr def); char *virDomainDefFormat(virDomainDefPtr def, unsigned int flags); +int virDomainDefFormatInternal(virDomainDefPtr def, + unsigned int flags, + virBufferPtr buf); int virDomainCpuSetParse(const char **str, char sep, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 695ff33..89cd6da 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -265,6 +265,7 @@ virDomainDefCheckABIStability; virDomainDefClearDeviceAliases; virDomainDefClearPCIAddresses; virDomainDefFormat; +virDomainDefFormatInternal; virDomainDefFree; virDomainDefParseFile; virDomainDefParseNode; diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 1122dab..513ec37 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -378,12 +378,12 @@ static void qemuMigrationCookieGraphicsXMLFormat(virBufferPtr buf, } -static void qemuMigrationCookieXMLFormat(virBufferPtr buf, - qemuMigrationCookiePtr mig) +static int +qemuMigrationCookieXMLFormat(virBufferPtr buf, + qemuMigrationCookiePtr mig) { char uuidstr[VIR_UUID_STRING_BUFLEN]; char hostuuidstr[VIR_UUID_STRING_BUFLEN]; - char *domXML; int i; virUUIDFormat(mig->uuid, uuidstr); @@ -416,14 +416,17 @@ static void qemuMigrationCookieXMLFormat(virBufferPtr buf, if ((mig->flags & QEMU_MIGRATION_COOKIE_PERSISTENT) && mig->persistent) { - domXML = virDomainDefFormat(mig->persistent, - VIR_DOMAIN_XML_INACTIVE | - VIR_DOMAIN_XML_SECURE); - virBufferAdd(buf, domXML, -1); - VIR_FREE(domXML); + virBufferAdjustIndent(buf, 2); + if (virDomainDefFormatInternal(mig->persistent, + VIR_DOMAIN_XML_INACTIVE | + VIR_DOMAIN_XML_SECURE, + buf) < 0) + return -1; + virBufferAdjustIndent(buf, -2); } virBufferAddLit(buf, "</qemu-migration>\n"); + return 0; } @@ -431,10 +434,14 @@ static char *qemuMigrationCookieXMLFormatStr(qemuMigrationCookiePtr mig) { virBuffer buf = VIR_BUFFER_INITIALIZER; - qemuMigrationCookieXMLFormat(&buf, mig); + if (qemuMigrationCookieXMLFormat(&buf, mig) < 0) { + virBufferFreeAndReset(&buf); + return NULL; + } if (virBufferError(&buf)) { virReportOOMError(); + virBufferFreeAndReset(&buf); return NULL; } -- 1.7.4.4

On 10/20/2011 03:08 AM, Peter Krempa wrote: Yeah, hundreds of bytes at least. Maybe for sure. Relying on other function's error handling(especially when it is a deep calling line) is a little not that sure.

Add a test for the simple parts of my indentation changes, and fix the fallout. * tests/domainsnapshotxml2xmltest.c: New test. * tests/Makefile.am (domainsnapshotxml2xmltest_SOURCES): Build it. * src/conf/domain_conf.c (virDomainSnapshotDefFormat): Avoid NULL deref, match documented order. * src/conf/domain_conf.h (virDomainSnapshotDefFormat): Add const. * tests/domainsnapshotxml2xmlout/all_parameters.xml: Tweak output. * tests/domainsnapshotxml2xmlout/disk_snapshot.xml: Likewise. * tests/domainsnapshotxml2xmlout/full_domain.xml: Likewise. * .gitignore: Exempt new binary. --- .gitignore | 1 + src/conf/domain_conf.c | 10 +- src/conf/domain_conf.h | 2 +- tests/Makefile.am | 14 ++- tests/domainsnapshotxml2xmlout/all_parameters.xml | 2 +- tests/domainsnapshotxml2xmlout/disk_snapshot.xml | 102 ++++++++-------- tests/domainsnapshotxml2xmlout/full_domain.xml | 52 ++++---- tests/domainsnapshotxml2xmltest.c | 128 +++++++++++++++++++++ 8 files changed, 224 insertions(+), 87 deletions(-) create mode 100644 tests/domainsnapshotxml2xmltest.c diff --git a/.gitignore b/.gitignore index 41fa50f..bfe8e87 100644 --- a/.gitignore +++ b/.gitignore @@ -69,6 +69,7 @@ /src/util/virkeymaps.h /tests/*.log /tests/cputest +/tests/domainsnapshotxml2xmltest /tests/hashtest /tests/jsontest /tests/networkxml2argvtest diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 33482c0..0b82c3d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11942,7 +11942,7 @@ cleanup: return ret; } -char *virDomainSnapshotDefFormat(char *domain_uuid, +char *virDomainSnapshotDefFormat(const char *domain_uuid, virDomainSnapshotDefPtr def, unsigned int flags, int internal) @@ -11983,12 +11983,12 @@ char *virDomainSnapshotDefFormat(char *domain_uuid, virDomainDiskSnapshotTypeToString(disk->snapshot)); if (disk->file || disk->driverType) { virBufferAddLit(&buf, ">\n"); - if (disk->file) - virBufferEscapeString(&buf, " <source file='%s'/>\n", - disk->file); if (disk->driverType) virBufferEscapeString(&buf, " <driver type='%s'/>\n", disk->driverType); + if (disk->file) + virBufferEscapeString(&buf, " <source file='%s'/>\n", + disk->file); virBufferAddLit(&buf, " </disk>\n"); } else { virBufferAddLit(&buf, "/>\n"); @@ -12003,7 +12003,7 @@ char *virDomainSnapshotDefFormat(char *domain_uuid, return NULL; } virBufferAdjustIndent(&buf, -2); - } else { + } else if (domain_uuid) { virBufferAddLit(&buf, " <domain>\n"); virBufferAsprintf(&buf, " <uuid>%s</uuid>\n", domain_uuid); virBufferAddLit(&buf, " </domain>\n"); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index effc630..4126d58 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1472,7 +1472,7 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, unsigned int expectedVirtTypes, unsigned int flags); void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def); -char *virDomainSnapshotDefFormat(char *domain_uuid, +char *virDomainSnapshotDefFormat(const char *domain_uuid, virDomainSnapshotDefPtr def, unsigned int flags, int internal); diff --git a/tests/Makefile.am b/tests/Makefile.am index cbbbc6f..4769fa5 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -102,7 +102,8 @@ check_PROGRAMS += xml2sexprtest sexpr2xmltest \ xmconfigtest xencapstest statstest reconnect endif if WITH_QEMU -check_PROGRAMS += qemuxml2argvtest qemuxml2xmltest qemuargv2xmltest qemuhelptest +check_PROGRAMS += qemuxml2argvtest qemuxml2xmltest qemuargv2xmltest \ + qemuhelptest domainsnapshotxml2xmltest endif if WITH_OPENVZ @@ -228,7 +229,7 @@ endif if WITH_QEMU TESTS += qemuxml2argvtest qemuxml2xmltest qemuargv2xmltest qemuhelptest -TESTS += nwfilterxml2xmltest +TESTS += domainsnapshotxml2xmltest nwfilterxml2xmltest endif if WITH_OPENVZ @@ -350,8 +351,15 @@ qemuargv2xmltest_LDADD = $(qemu_LDADDS) $(LDADDS) qemuhelptest_SOURCES = qemuhelptest.c testutils.c testutils.h qemuhelptest_LDADD = $(qemu_LDADDS) $(LDADDS) + +domainsnapshotxml2xmltest_SOURCES = \ + domainsnapshotxml2xmltest.c testutilsqemu.c testutilsqemu.h \ + testutils.c testutils.h +domainsnapshotxml2xmltest_LDADD = $(qemu_LDADDS) $(LDADDS) else -EXTRA_DIST += qemuxml2argvtest.c qemuxml2xmltest.c qemuargv2xmltest.c qemuhelptest.c testutilsqemu.c testutilsqemu.h +EXTRA_DIST += qemuxml2argvtest.c qemuxml2xmltest.c qemuargv2xmltest.c \ + qemuhelptest.c domainsnapshotxml2xmltest.c \ + testutilsqemu.c testutilsqemu.h endif if WITH_OPENVZ diff --git a/tests/domainsnapshotxml2xmlout/all_parameters.xml b/tests/domainsnapshotxml2xmlout/all_parameters.xml index ed4a600..eb2ee85 100644 --- a/tests/domainsnapshotxml2xmlout/all_parameters.xml +++ b/tests/domainsnapshotxml2xmlout/all_parameters.xml @@ -1,10 +1,10 @@ <domainsnapshot> <name>my snap name</name> <description>!@#$%^</description> + <state>running</state> <parent> <name>earlier_snap</name> </parent> - <state>running</state> <creationTime>1272917631</creationTime> <domain> <uuid>9d37b878-a7cc-9f9a-b78f-49b3abad25a8</uuid> diff --git a/tests/domainsnapshotxml2xmlout/disk_snapshot.xml b/tests/domainsnapshotxml2xmlout/disk_snapshot.xml index e0414a1..a0a0965 100644 --- a/tests/domainsnapshotxml2xmlout/disk_snapshot.xml +++ b/tests/domainsnapshotxml2xmlout/disk_snapshot.xml @@ -1,10 +1,10 @@ <domainsnapshot> <name>my snap name</name> <description>!@#$%^</description> + <state>disk-snapshot</state> <parent> <name>earlier_snap</name> </parent> - <state>disk-snapshot</state> <creationTime>1272917631</creationTime> <disks> <disk name='hda' snapshot='no'/> @@ -23,55 +23,55 @@ <source file='/path/to/generated5'/> </disk> </disks> -<domain type='qemu'> - <name>QEMUGuest1</name> - <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <memory>219100</memory> - <currentMemory>219100</currentMemory> - <vcpu cpuset='1-4,8-20,525'>1</vcpu> - <os> - <type arch='i686' machine='pc'>hvm</type> - <boot dev='hd'/> - </os> - <clock offset='utc'/> - <on_poweroff>destroy</on_poweroff> - <on_reboot>restart</on_reboot> - <on_crash>destroy</on_crash> - <devices> - <emulator>/usr/bin/qemu</emulator> - <disk type='block' device='disk'> - <source dev='/dev/HostVG/QEMUGuest1'/> - <target dev='hda' bus='ide'/> - <address type='drive' controller='0' bus='0' unit='0'/> - </disk> - <disk type='block' device='disk'> - <source dev='/dev/HostVG/QEMUGuest2'/> - <target dev='hdb' bus='ide'/> - <address type='drive' controller='0' bus='1' unit='0'/> - </disk> - <disk type='block' device='disk'> - <source dev='/dev/HostVG/QEMUGuest3'/> - <target dev='hdc' bus='ide'/> - <address type='drive' controller='0' bus='2' unit='0'/> - </disk> - <disk type='block' device='disk'> - <source dev='/dev/HostVG/QEMUGuest4'/> - <target dev='hdd' bus='ide'/> - <address type='drive' controller='0' bus='3' unit='0'/> - </disk> - <disk type='block' device='disk'> - <source dev='/dev/HostVG/QEMUGuest5'/> - <target dev='hde' bus='ide'/> - <address type='drive' controller='0' bus='4' unit='0'/> - </disk> - <disk type='block' device='disk'> - <source dev='/dev/HostVG/QEMUGuest6'/> - <target dev='hdf' bus='ide'/> - <address type='drive' controller='0' bus='5' unit='0'/> - </disk> - <controller type='ide' index='0'/> - <memballoon model='virtio'/> - </devices> -</domain> + <domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219100</memory> + <currentMemory>219100</currentMemory> + <vcpu cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hdb' bus='ide'/> + <address type='drive' controller='0' bus='1' unit='0'/> + </disk> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest3'/> + <target dev='hdc' bus='ide'/> + <address type='drive' controller='0' bus='2' unit='0'/> + </disk> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest4'/> + <target dev='hdd' bus='ide'/> + <address type='drive' controller='0' bus='3' unit='0'/> + </disk> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest5'/> + <target dev='hde' bus='ide'/> + <address type='drive' controller='0' bus='4' unit='0'/> + </disk> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest6'/> + <target dev='hdf' bus='ide'/> + <address type='drive' controller='0' bus='5' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> + </domain> <active>1</active> </domainsnapshot> diff --git a/tests/domainsnapshotxml2xmlout/full_domain.xml b/tests/domainsnapshotxml2xmlout/full_domain.xml index 942bd7f..76f17e1 100644 --- a/tests/domainsnapshotxml2xmlout/full_domain.xml +++ b/tests/domainsnapshotxml2xmlout/full_domain.xml @@ -1,35 +1,35 @@ <domainsnapshot> <name>my snap name</name> <description>!@#$%^</description> + <state>running</state> <parent> <name>earlier_snap</name> </parent> - <state>running</state> <creationTime>1272917631</creationTime> -<domain type='qemu'> - <name>QEMUGuest1</name> - <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <memory>219100</memory> - <currentMemory>219100</currentMemory> - <vcpu cpuset='1-4,8-20,525'>1</vcpu> - <os> - <type arch='i686' machine='pc'>hvm</type> - <boot dev='hd'/> - </os> - <clock offset='utc'/> - <on_poweroff>destroy</on_poweroff> - <on_reboot>restart</on_reboot> - <on_crash>destroy</on_crash> - <devices> - <emulator>/usr/bin/qemu</emulator> - <disk type='block' device='disk'> - <source dev='/dev/HostVG/QEMUGuest1'/> - <target dev='hda' bus='ide'/> - <address type='drive' controller='0' bus='0' unit='0'/> - </disk> - <controller type='ide' index='0'/> - <memballoon model='virtio'/> - </devices> -</domain> + <domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219100</memory> + <currentMemory>219100</currentMemory> + <vcpu cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> + </domain> <active>1</active> </domainsnapshot> diff --git a/tests/domainsnapshotxml2xmltest.c b/tests/domainsnapshotxml2xmltest.c new file mode 100644 index 0000000..c2266f4 --- /dev/null +++ b/tests/domainsnapshotxml2xmltest.c @@ -0,0 +1,128 @@ +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <unistd.h> +#include <string.h> + +#include <sys/types.h> +#include <fcntl.h> + +#ifdef WITH_QEMU + +# include "internal.h" +# include "testutils.h" +# include "qemu/qemu_conf.h" +# include "qemu/qemu_domain.h" +# include "testutilsqemu.h" + +static struct qemud_driver driver; + +static int +testCompareXMLToXMLFiles(const char *inxml, const char *uuid, int internal) +{ + char *inXmlData = NULL; + char *actual = NULL; + int ret = -1; + virDomainSnapshotDefPtr def = NULL; + unsigned int flags = (VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE | + VIR_DOMAIN_SNAPSHOT_PARSE_DISKS); + + if (virtTestLoadFile(inxml, &inXmlData) < 0) + goto fail; + + if (internal) + flags |= VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL; + if (!(def = virDomainSnapshotDefParseString(inXmlData, driver.caps, + QEMU_EXPECTED_VIRT_TYPES, + flags))) + goto fail; + + if (!(actual = virDomainSnapshotDefFormat(uuid, def, + VIR_DOMAIN_XML_SECURE, + internal))) + goto fail; + + + if (STRNEQ(inXmlData, actual)) { + virtTestDifference(stderr, inXmlData, actual); + goto fail; + } + + ret = 0; + fail: + free(inXmlData); + free(actual); + virDomainSnapshotDefFree(def); + return ret; +} + +struct testInfo { + const char *name; + const char *uuid; + int internal; +}; + +static int +testCompareXMLToXMLHelper(const void *data) +{ + const struct testInfo *info = data; + char *xml_in = NULL; + int ret = -1; + + if (virAsprintf(&xml_in, "%s/domainsnapshotxml2xmlout/%s.xml", + abs_srcdir, info->name) < 0) + goto cleanup; + + ret = testCompareXMLToXMLFiles(xml_in, info->uuid, info->internal); + +cleanup: + free(xml_in); + return ret; +} + + +static int +mymain(void) +{ + int ret = 0; + + if ((driver.caps = testQemuCapsInit()) == NULL) + return (EXIT_FAILURE); + +# define DO_TEST(name, uuid, internal) \ + do { \ + const struct testInfo info = {name, uuid, internal}; \ + if (virtTestRun("SNAPSHOT XML-2-XML " name, \ + 1, testCompareXMLToXMLHelper, &info) < 0) \ + ret = -1; \ + } while (0) + + /* Unset or set all envvars here that are copied in qemudBuildCommandLine + * using ADD_ENV_COPY, otherwise these tests may fail due to unexpected + * values for these envvars */ + setenv("PATH", "/bin", 1); + + DO_TEST("all_parameters", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8", 1); + DO_TEST("disk_snapshot", "c7a5fdbd-edaf-9455-926a-d65c16db1809", 1); + DO_TEST("full_domain", "c7a5fdbd-edaf-9455-926a-d65c16db1809", 1); + DO_TEST("noparent_nodescription_noactive", NULL, 0); + DO_TEST("noparent_nodescription", NULL, 1); + DO_TEST("noparent", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8", 0); + + virCapabilitiesFree(driver.caps); + + return (ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); +} + +VIRT_TEST_MAIN(mymain) + +#else + +int +main(void) +{ + return EXIT_AM_SKIP; +} + +#endif /* WITH_QEMU */ -- 1.7.4.4

On 10/20/2011 01:23 AM, Hai Dong Li wrote: Previously, the test was useful only for schema validation of input, and our schema allows elements to be interleaved in any order. But now that we are adding a test of our XML generation matching expected output, then this file has to match what we generate (domain_conf.c virDomainSnapshotDefFormat). Besides, our documentation already mentioned <state> before <parent> (docs/formatsnapshot.html.in). On 10/20/2011 03:00 AM, Peter Krempa wrote: Pushed. I also had to rebase this one on top of recent test changes. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

The improvements to virBuffer, along with a paradigm shift to pass the original buffer through rather than creating a second buffer, allow us to shave off quite a few lines of code. * src/util/sysinfo.h (virSysinfoFormat): Alter signature. * src/util/sysinfo.c (virSysinfoFormat, virSysinfoBIOSFormat) (virSysinfoSystemFormat, virSysinfoProcessorFormat) (virSysinfoMemoryFormat): Change indentation parameter. * src/conf/domain_conf.c (virDomainSysinfoDefFormat): Adjust caller. * src/qemu/qemu_driver.c (qemuGetSysinfo): Likewise. --- src/conf/domain_conf.c | 12 +- src/qemu/qemu_driver.c | 9 +- src/util/sysinfo.c | 399 ++++++++++++++++-------------------------------- src/util/sysinfo.h | 3 +- 4 files changed, 147 insertions(+), 276 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0b82c3d..eb82fa6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9897,13 +9897,11 @@ static int virDomainSysinfoDefFormat(virBufferPtr buf, virSysinfoDefPtr def) { - char *format = virSysinfoFormat(def, " "); - - if (!format) - return -1; - virBufferAdd(buf, format, strlen(format)); - VIR_FREE(format); - return 0; + int ret; + virBufferAdjustIndent(buf, 2); + ret = virSysinfoFormat(buf, def); + virBufferAdjustIndent(buf, -2); + return ret; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0f18983..b459b73 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -963,6 +963,7 @@ static char * qemuGetSysinfo(virConnectPtr conn, unsigned int flags) { struct qemud_driver *driver = conn->privateData; + virBuffer buf = VIR_BUFFER_INITIALIZER; virCheckFlags(0, NULL); @@ -972,7 +973,13 @@ qemuGetSysinfo(virConnectPtr conn, unsigned int flags) return NULL; } - return virSysinfoFormat(driver->hostsysinfo, ""); + if (virSysinfoFormat(&buf, driver->hostsysinfo) < 0) + return NULL; + if (virBufferError(&buf)) { + virReportOOMError(); + return NULL; + } + return virBufferContentAndReset(&buf); } static int qemudGetMaxVCPUs(virConnectPtr conn ATTRIBUTE_UNUSED, const char *type) { diff --git a/src/util/sysinfo.c b/src/util/sysinfo.c index 6625cae..de3108a 100644 --- a/src/util/sysinfo.c +++ b/src/util/sysinfo.c @@ -515,324 +515,189 @@ no_memory: #endif /* !WIN32 && x86 */ static void -virSysinfoBIOSFormat(virSysinfoDefPtr def, const char *prefix, - virBufferPtr buf) +virSysinfoBIOSFormat(virBufferPtr buf, virSysinfoDefPtr def) { - int len = strlen(prefix); - - if ((def->bios_vendor != NULL) || (def->bios_version != NULL) || - (def->bios_date != NULL) || (def->bios_release != NULL)) { - virBufferAsprintf(buf, "%s <bios>\n", prefix); - if (def->bios_vendor != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='vendor'>%s</entry>\n", - def->bios_vendor); - } - if (def->bios_version != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='version'>%s</entry>\n", - def->bios_version); - } - if (def->bios_date != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='date'>%s</entry>\n", - def->bios_date); - } - if (def->bios_release != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='release'>%s</entry>\n", - def->bios_release); - } - virBufferAsprintf(buf, "%s </bios>\n", prefix); - } + if (!def->bios_vendor && !def->bios_version && + !def->bios_date && !def->bios_release) + return; - return; + virBufferAddLit(buf, " <bios>\n"); + virBufferEscapeString(buf, " <entry name='vendor'>%s</entry>\n", + def->bios_vendor); + virBufferEscapeString(buf, " <entry name='version'>%s</entry>\n", + def->bios_version); + virBufferEscapeString(buf, " <entry name='date'>%s</entry>\n", + def->bios_date); + virBufferEscapeString(buf, " <entry name='release'>%s</entry>\n", + def->bios_release); + virBufferAddLit(buf, " </bios>\n"); } static void -virSysinfoSystemFormat(virSysinfoDefPtr def, const char *prefix, - virBufferPtr buf) +virSysinfoSystemFormat(virBufferPtr buf, virSysinfoDefPtr def) { - int len = strlen(prefix); - - if ((def->system_manufacturer != NULL) || (def->system_product != NULL) || - (def->system_version != NULL) || (def->system_serial != NULL) || - (def->system_uuid != NULL) || (def->system_sku != NULL) || - (def->system_family != NULL)) { - virBufferAsprintf(buf, "%s <system>\n", prefix); - if (def->system_manufacturer != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='manufacturer'>%s</entry>\n", - def->system_manufacturer); - } - if (def->system_product != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='product'>%s</entry>\n", - def->system_product); - } - if (def->system_version != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='version'>%s</entry>\n", - def->system_version); - } - if (def->system_serial != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='serial'>%s</entry>\n", - def->system_serial); - } - if (def->system_uuid != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='uuid'>%s</entry>\n", - def->system_uuid); - } - if (def->system_sku != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='sku'>%s</entry>\n", - def->system_sku); - } - if (def->system_family != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='family'>%s</entry>\n", - def->system_family); - } - virBufferAsprintf(buf, "%s </system>\n", prefix); - } + if (!def->system_manufacturer && !def->system_product && + !def->system_version && !def->system_serial && + !def->system_uuid && !def->system_sku && !def->system_family) + return; - return; + virBufferAddLit(buf, " <system>\n"); + virBufferEscapeString(buf, " <entry name='manufacturer'>%s</entry>\n", + def->system_manufacturer); + virBufferEscapeString(buf, " <entry name='product'>%s</entry>\n", + def->system_product); + virBufferEscapeString(buf, " <entry name='version'>%s</entry>\n", + def->system_version); + virBufferEscapeString(buf, " <entry name='serial'>%s</entry>\n", + def->system_serial); + virBufferEscapeString(buf, " <entry name='uuid'>%s</entry>\n", + def->system_uuid); + virBufferEscapeString(buf, " <entry name='sku'>%s</entry>\n", + def->system_sku); + virBufferEscapeString(buf, " <entry name='family'>%s</entry>\n", + def->system_family); + virBufferAddLit(buf, " </system>\n"); } static void -virSysinfoProcessorFormat(virSysinfoDefPtr def, const char *prefix, - virBufferPtr buf) +virSysinfoProcessorFormat(virBufferPtr buf, virSysinfoDefPtr def) { int i; - int len = strlen(prefix); virSysinfoProcessorDefPtr processor; for (i = 0; i < def->nprocessor; i++) { processor = &def->processor[i]; - if ((processor->processor_socket_destination != NULL) || - (processor->processor_type != NULL) || - (processor->processor_family != NULL) || - (processor->processor_manufacturer != NULL) || - (processor->processor_signature != NULL) || - (processor->processor_version != NULL) || - (processor->processor_external_clock != NULL) || - (processor->processor_max_speed != NULL) || - (processor->processor_status != NULL) || - (processor->processor_serial_number != NULL) || - (processor->processor_part_number != NULL)) { - virBufferAsprintf(buf, "%s <processor>\n", prefix); - if (processor->processor_socket_destination != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='socket_destination'>%s</entry>\n", - processor->processor_socket_destination); - } - if (processor->processor_type != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='type'>%s</entry>\n", - processor->processor_type); - } - if (processor->processor_family != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='family'>%s</entry>\n", - processor->processor_family); - } - if (processor->processor_manufacturer != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='manufacturer'>%s</entry>\n", - processor->processor_manufacturer); - } - if (processor->processor_signature != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='signature'>%s</entry>\n", - processor->processor_signature); - } - if (processor->processor_version != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='version'>%s</entry>\n", - processor->processor_version); - } - if (processor->processor_external_clock != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='external_clock'>%s</entry>\n", - processor->processor_external_clock); - } - if (processor->processor_max_speed != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='max_speed'>%s</entry>\n", - processor->processor_max_speed); - } - if (processor->processor_status != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='status'>%s</entry>\n", - processor->processor_status); - } - if (processor->processor_serial_number != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='serial_number'>%s</entry>\n", - processor->processor_serial_number); - } - if (processor->processor_part_number != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='part_number'>%s</entry>\n", - processor->processor_part_number); - } - virBufferAsprintf(buf, "%s </processor>\n", prefix); - } + if (!processor->processor_socket_destination && + !processor->processor_type && + !processor->processor_family && + !processor->processor_manufacturer && + !processor->processor_signature && + !processor->processor_version && + !processor->processor_external_clock && + !processor->processor_max_speed && + !processor->processor_status && + !processor->processor_serial_number && + !processor->processor_part_number) + continue; + + virBufferAddLit(buf, " <processor>\n"); + virBufferAdjustIndent(buf, 4); + virBufferEscapeString(buf, + "<entry name='socket_destination'>%s</entry>\n", + processor->processor_socket_destination); + virBufferEscapeString(buf, "<entry name='type'>%s</entry>\n", + processor->processor_type); + virBufferEscapeString(buf, "<entry name='family'>%s</entry>\n", + processor->processor_family); + virBufferEscapeString(buf, "<entry name='manufacturer'>%s</entry>\n", + processor->processor_manufacturer); + virBufferEscapeString(buf, "<entry name='signature'>%s</entry>\n", + processor->processor_signature); + virBufferEscapeString(buf, "<entry name='version'>%s</entry>\n", + processor->processor_version); + virBufferEscapeString(buf, "<entry name='external_clock'>%s</entry>\n", + processor->processor_external_clock); + virBufferEscapeString(buf, "<entry name='max_speed'>%s</entry>\n", + processor->processor_max_speed); + virBufferEscapeString(buf, "<entry name='status'>%s</entry>\n", + processor->processor_status); + virBufferEscapeString(buf, "<entry name='serial_number'>%s</entry>\n", + processor->processor_serial_number); + virBufferEscapeString(buf, "<entry name='part_number'>%s</entry>\n", + processor->processor_part_number); + virBufferAdjustIndent(buf, -4); + virBufferAddLit(buf, " </processor>\n"); } - - return; } static void -virSysinfoMemoryFormat(virSysinfoDefPtr def, const char *prefix, - virBufferPtr buf) +virSysinfoMemoryFormat(virBufferPtr buf, virSysinfoDefPtr def) { int i; - int len = strlen(prefix); virSysinfoMemoryDefPtr memory; for (i = 0; i < def->nmemory; i++) { memory = &def->memory[i]; - if ((memory->memory_size != NULL) || - (memory->memory_form_factor != NULL) || - (memory->memory_locator != NULL) || - (memory->memory_bank_locator != NULL) || - (memory->memory_type != NULL) || - (memory->memory_type_detail != NULL) || - (memory->memory_speed != NULL) || - (memory->memory_manufacturer != NULL) || - (memory->memory_serial_number != NULL) || - (memory->memory_part_number != NULL)) { - virBufferAsprintf(buf, "%s <memory_device>\n", prefix); - if (memory->memory_size != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='size'>%s</entry>\n", - memory->memory_size); - } - if (memory->memory_form_factor != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='form_factor'>%s</entry>\n", - memory->memory_form_factor); - } - if (memory->memory_locator != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='locator'>%s</entry>\n", - memory->memory_locator); - } - if (memory->memory_bank_locator != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='bank_locator'>%s</entry>\n", - memory->memory_bank_locator); - } - if (memory->memory_type != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='type'>%s</entry>\n", - memory->memory_type); - } - if (memory->memory_type_detail != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='type_detail'>%s</entry>\n", - memory->memory_type_detail); - } - if (memory->memory_speed != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='speed'>%s</entry>\n", - memory->memory_speed); - } - if (memory->memory_manufacturer != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='manufacturer'>%s</entry>\n", - memory->memory_manufacturer); - } - if (memory->memory_serial_number != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='serial_number'>%s</entry>\n", - memory->memory_serial_number); - } - if (memory->memory_part_number != NULL) { - virBufferAdd(buf, prefix, len); - virBufferEscapeString(buf, - " <entry name='part_number'>%s</entry>\n", - memory->memory_part_number); - } - virBufferAsprintf(buf, "%s </memory_device>\n", prefix); - } + if (!memory->memory_size && + !memory->memory_form_factor && + !memory->memory_locator && + !memory->memory_bank_locator && + !memory->memory_type && + !memory->memory_type_detail && + !memory->memory_speed && + !memory->memory_manufacturer && + !memory->memory_serial_number && + !memory->memory_part_number) + continue; + + virBufferAddLit(buf, " <memory_device>\n"); + virBufferEscapeString(buf, " <entry name='size'>%s</entry>\n", + memory->memory_size); + virBufferEscapeString(buf, + " <entry name='form_factor'>%s</entry>\n", + memory->memory_form_factor); + virBufferEscapeString(buf, " <entry name='locator'>%s</entry>\n", + memory->memory_locator); + virBufferEscapeString(buf, + " <entry name='bank_locator'>%s</entry>\n", + memory->memory_bank_locator); + virBufferEscapeString(buf, " <entry name='type'>%s</entry>\n", + memory->memory_type); + virBufferEscapeString(buf, + " <entry name='type_detail'>%s</entry>\n", + memory->memory_type_detail); + virBufferEscapeString(buf, " <entry name='speed'>%s</entry>\n", + memory->memory_speed); + virBufferEscapeString(buf, + " <entry name='manufacturer'>%s</entry>\n", + memory->memory_manufacturer); + virBufferEscapeString(buf, + " <entry name='serial_number'>%s</entry>\n", + memory->memory_serial_number); + virBufferEscapeString(buf, + " <entry name='part_number'>%s</entry>\n", + memory->memory_part_number); + virBufferAddLit(buf, " </memory_device>\n"); } - - return; } /** * virSysinfoFormat: + * @buf: buffer to append output to (may use auto-indentation) * @def: structure to convert to xml string - * @prefix: string to prefix before each line of xml * - * This returns the XML description of the sysinfo, or NULL after - * generating an error message. + * Returns 0 on success, -1 on failure after generating an error message. */ -char * -virSysinfoFormat(virSysinfoDefPtr def, const char *prefix) +int +virSysinfoFormat(virBufferPtr buf, virSysinfoDefPtr def) { const char *type = virSysinfoTypeToString(def->type); - virBuffer buf = VIR_BUFFER_INITIALIZER; if (!type) { virSmbiosReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected sysinfo type model %d"), def->type); - return NULL; + virBufferFreeAndReset(buf); + return -1; } - virBufferAsprintf(&buf, "%s<sysinfo type='%s'>\n", prefix, type); + virBufferAsprintf(buf, "<sysinfo type='%s'>\n", type); - virSysinfoBIOSFormat(def, prefix, &buf); - virSysinfoSystemFormat(def, prefix, &buf); - virSysinfoProcessorFormat(def, prefix, &buf); - virSysinfoMemoryFormat(def, prefix, &buf); + virSysinfoBIOSFormat(buf, def); + virSysinfoSystemFormat(buf, def); + virSysinfoProcessorFormat(buf, def); + virSysinfoMemoryFormat(buf, def); - virBufferAsprintf(&buf, "%s</sysinfo>\n", prefix); + virBufferAddLit(buf, "</sysinfo>\n"); - if (virBufferError(&buf)) { + if (virBufferError(buf)) { virReportOOMError(); - return NULL; + return -1; } - return virBufferContentAndReset(&buf); + return 0; } bool virSysinfoIsEqual(virSysinfoDefPtr src, diff --git a/src/util/sysinfo.h b/src/util/sysinfo.h index 86fd20f..bee43ee 100644 --- a/src/util/sysinfo.h +++ b/src/util/sysinfo.h @@ -26,6 +26,7 @@ # include "internal.h" # include "util.h" +# include "buf.h" enum virSysinfoType { VIR_SYSINFO_SMBIOS, @@ -93,7 +94,7 @@ virSysinfoDefPtr virSysinfoRead(void); void virSysinfoDefFree(virSysinfoDefPtr def); -char *virSysinfoFormat(virSysinfoDefPtr def, const char *prefix) +int virSysinfoFormat(virBufferPtr buf, virSysinfoDefPtr def) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); bool virSysinfoIsEqual(virSysinfoDefPtr src, -- 1.7.4.4

On Thu, Oct 20, 2011 at 12:14:57PM +0200, Peter Krempa wrote: I'm not entirely convinced this is a good idea. This means that when looking at the code, it is no longer obvious what the nesting of XML elements is supposed to be - they are all the level. I see the value of the automatic indentation code, being to allow us to embed 1 XML document, inside another XML document. eg domain conf XML, inside QEMU state XML. I don't think we should use it to remove indentation in all our code. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 09/29/2011 06:22 PM, Eric Blake wrote: Oh well. I don't like this very much, but removing things like this would ultimately end in having a flat XML output structure and using indentation adjustment to have correct indentation across the xml, which is somewhat controversial. Well, it doesn't affect functionality, so it's not a show-stoping issue. Otherwise, this patch works correct, so ACK. Peter

More simplifications possible due to auto-indent. Also, <bandwidth> within <actual> was only using 6 instead of 8 spaces. * src/util/network.h (virVirtualPortProfileFormat) (virBandwidthDefFormat): Alter signature. * src/util/network.c (virVirtualPortProfileFormat) (virBandwidthDefFormat): Alter indentation. (virBandwidthChildDefFormat): Tweak to make use easier. * src/conf/network_conf.c (virPortGroupDefFormat) (virNetworkDefFormat): Adjust callers. * src/conf/domain_conf.c (virDomainNetDefFormat): Likewise. (virDomainActualNetDefFormat): Likewise, and fix bandwidth indentation. --- src/conf/domain_conf.c | 23 ++++++++++++++------- src/conf/network_conf.c | 14 +++++++++--- src/util/network.c | 49 ++++++++++++++++------------------------------ src/util/network.h | 11 ++++----- 4 files changed, 47 insertions(+), 50 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 40b06f3..70201af 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9415,15 +9415,18 @@ virDomainActualNetDefFormat(virBufferPtr buf, return ret; } virBufferAsprintf(buf, " mode='%s'/>\n", mode); - virVirtualPortProfileFormat(buf, def->data.direct.virtPortProfile, - " "); + virBufferAdjustIndent(buf, 8); + virVirtualPortProfileFormat(buf, def->data.direct.virtPortProfile); + virBufferAdjustIndent(buf, -8); break; default: break; } - if (virBandwidthDefFormat(buf, def->bandwidth, " ") < 0) + virBufferAdjustIndent(buf, 8); + if (virBandwidthDefFormat(buf, def->bandwidth) < 0) goto error; + virBufferAdjustIndent(buf, -8); virBufferAddLit(buf, " </actual>\n"); @@ -9462,8 +9465,9 @@ virDomainNetDefFormat(virBufferPtr buf, def->data.network.portgroup); } virBufferAddLit(buf, "/>\n"); - virVirtualPortProfileFormat(buf, def->data.network.virtPortProfile, - " "); + virBufferAdjustIndent(buf, 6); + virVirtualPortProfileFormat(buf, def->data.network.virtPortProfile); + virBufferAdjustIndent(buf, -6); if ((flags & VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET) && (virDomainActualNetDefFormat(buf, def->data.network.actual) < 0)) return -1; @@ -9514,8 +9518,9 @@ virDomainNetDefFormat(virBufferPtr buf, virBufferAsprintf(buf, " mode='%s'", virMacvtapModeTypeToString(def->data.direct.mode)); virBufferAddLit(buf, "/>\n"); - virVirtualPortProfileFormat(buf, def->data.direct.virtPortProfile, - " "); + virBufferAdjustIndent(buf, 6); + virVirtualPortProfileFormat(buf, def->data.direct.virtPortProfile); + virBufferAdjustIndent(buf, -6); break; case VIR_DOMAIN_NET_TYPE_USER: @@ -9580,8 +9585,10 @@ virDomainNetDefFormat(virBufferPtr buf, virBufferAsprintf(buf, " <link state='%s'/>\n", virDomainNetInterfaceLinkStateTypeToString(def->linkstate)); - if (virBandwidthDefFormat(buf, def->bandwidth, " ") < 0) + virBufferAdjustIndent(buf, 6); + if (virBandwidthDefFormat(buf, def->bandwidth) < 0) return -1; + virBufferAdjustIndent(buf, -6); if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) return -1; diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index b98ffad..7bc2e2c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1267,8 +1267,10 @@ virPortGroupDefFormat(virBufferPtr buf, virBufferAddLit(buf, " default='yes'"); } virBufferAddLit(buf, ">\n"); - virVirtualPortProfileFormat(buf, def->virtPortProfile, " "); - virBandwidthDefFormat(buf, def->bandwidth, " "); + virBufferAdjustIndent(buf, 4); + virVirtualPortProfileFormat(buf, def->virtPortProfile); + virBandwidthDefFormat(buf, def->bandwidth); + virBufferAdjustIndent(buf, -4); virBufferAddLit(buf, " </portgroup>\n"); } @@ -1341,15 +1343,19 @@ char *virNetworkDefFormat(const virNetworkDefPtr def) if (virNetworkDNSDefFormat(&buf, def->dns) < 0) goto error; - if (virBandwidthDefFormat(&buf, def->bandwidth, " ") < 0) + virBufferAdjustIndent(&buf, 2); + if (virBandwidthDefFormat(&buf, def->bandwidth) < 0) goto error; + virBufferAdjustIndent(&buf, -2); for (ii = 0; ii < def->nips; ii++) { if (virNetworkIpDefFormat(&buf, &def->ips[ii]) < 0) goto error; } - virVirtualPortProfileFormat(&buf, def->virtPortProfile, " "); + virBufferAdjustIndent(&buf, 2); + virVirtualPortProfileFormat(&buf, def->virtPortProfile); + virBufferAdjustIndent(&buf, -2); for (ii = 0; ii < def->nPortGroups; ii++) virPortGroupDefFormat(&buf, &def->portGroups[ii]); diff --git a/src/util/network.c b/src/util/network.c index ee69557..edf9c50 100644 --- a/src/util/network.c +++ b/src/util/network.c @@ -881,16 +881,14 @@ virVirtualPortProfileEqual(virVirtualPortProfileParamsPtr a, virVirtualPortProfi void virVirtualPortProfileFormat(virBufferPtr buf, - virVirtualPortProfileParamsPtr virtPort, - const char *indent) + virVirtualPortProfileParamsPtr virtPort) { char uuidstr[VIR_UUID_STRING_BUFLEN]; if (!virtPort || virtPort->virtPortType == VIR_VIRTUALPORT_NONE) return; - virBufferAsprintf(buf, "%s<virtualport type='%s'>\n", - indent, + virBufferAsprintf(buf, "<virtualport type='%s'>\n", virVirtualPortTypeToString(virtPort->virtPortType)); switch (virtPort->virtPortType) { @@ -902,9 +900,8 @@ virVirtualPortProfileFormat(virBufferPtr buf, virUUIDFormat(virtPort->u.virtPort8021Qbg.instanceID, uuidstr); virBufferAsprintf(buf, - "%s <parameters managerid='%d' typeid='%d' " + " <parameters managerid='%d' typeid='%d' " "typeidversion='%d' instanceid='%s'/>\n", - indent, virtPort->u.virtPort8021Qbg.managerID, virtPort->u.virtPort8021Qbg.typeID, virtPort->u.virtPort8021Qbg.typeIDVersion, @@ -913,13 +910,12 @@ virVirtualPortProfileFormat(virBufferPtr buf, case VIR_VIRTUALPORT_8021QBH: virBufferAsprintf(buf, - "%s <parameters profileid='%s'/>\n", - indent, + " <parameters profileid='%s'/>\n", virtPort->u.virtPort8021Qbh.profileID); break; } - virBufferAsprintf(buf, "%s</virtualport>\n", indent); + virBufferAddLit(buf, "</virtualport>\n"); } static int @@ -1074,11 +1070,14 @@ virBandwidthChildDefFormat(virBufferPtr buf, virRatePtr def, const char *elem_name) { - if (!buf || !def || !elem_name) + if (!buf || !elem_name) return -1; + if (!def) + return 0; if (def->average) { - virBufferAsprintf(buf, "<%s average='%llu'", elem_name, def->average); + virBufferAsprintf(buf, " <%s average='%llu'", elem_name, + def->average); if (def->peak) virBufferAsprintf(buf, " peak='%llu'", def->peak); @@ -1095,17 +1094,15 @@ virBandwidthChildDefFormat(virBufferPtr buf, * virBandwidthDefFormat: * @buf: Buffer to print to * @def: Data source - * @indent: prepend all lines printed with this * * Formats bandwidth and prepend each line with @indent. - * Passing NULL to @indent is equivalent passing "". + * @buf may use auto-indentation. * * Returns 0 on success, else -1. */ int virBandwidthDefFormat(virBufferPtr buf, - virBandwidthPtr def, - const char *indent) + virBandwidthPtr def) { int ret = -1; @@ -1117,23 +1114,11 @@ virBandwidthDefFormat(virBufferPtr buf, goto cleanup; } - if (!indent) - indent = ""; - - virBufferAsprintf(buf, "%s<bandwidth>\n", indent); - if (def->in) { - virBufferAsprintf(buf, "%s ", indent); - if (virBandwidthChildDefFormat(buf, def->in, "inbound") < 0) - goto cleanup; - } - - if (def->out) { - virBufferAsprintf(buf, "%s ", indent); - if (virBandwidthChildDefFormat(buf, def->out, "outbound") < 0) - goto cleanup; - } - - virBufferAsprintf(buf, "%s</bandwidth>\n", indent); + virBufferAddLit(buf, "<bandwidth>\n"); + if (virBandwidthChildDefFormat(buf, def->in, "inbound") < 0 || + virBandwidthChildDefFormat(buf, def->out, "outbound") < 0) + goto cleanup; + virBufferAddLit(buf, "</bandwidth>\n"); ret = 0; diff --git a/src/util/network.h b/src/util/network.h index 4d195af..552d5fd 100644 --- a/src/util/network.h +++ b/src/util/network.h @@ -1,7 +1,7 @@ /* * network.h: network helper APIs for libvirt * - * Copyright (C) 2009-2009 Red Hat, Inc. + * Copyright (C) 2009-2009, 2011 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -147,16 +147,15 @@ virVirtualPortProfileParseXML(xmlNodePtr node, virVirtualPortProfileParamsPtr *virtPort); void virVirtualPortProfileFormat(virBufferPtr buf, - virVirtualPortProfileParamsPtr virtPort, - const char *indent); + virVirtualPortProfileParamsPtr virtPort); -bool virVirtualPortProfileEqual(virVirtualPortProfileParamsPtr a, virVirtualPortProfileParamsPtr b); +bool virVirtualPortProfileEqual(virVirtualPortProfileParamsPtr a, + virVirtualPortProfileParamsPtr b); virBandwidthPtr virBandwidthDefParseNode(xmlNodePtr node); void virBandwidthDefFree(virBandwidthPtr def); int virBandwidthDefFormat(virBufferPtr buf, - virBandwidthPtr def, - const char *indent); + virBandwidthPtr def); int virBandwidthEnable(virBandwidthPtr bandwidth, const char *iface); int virBandwidthDisable(const char *iface, bool may_fail); -- 1.7.4.4

On 10/20/2011 07:00 AM, Peter Krempa wrote: I didn't even notice the typo in the pre-patch version - I have an emacs file save hook that auto-updates these lines. Fixed now. Pushed. Any review on 10-12 yet? -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

Dňa 29.9.2011 18:22, Eric Blake wrote / napísal(a): The offset of the <filterref element is 6 spaces and of NWFilter param attributes is 8 spaces here. Here you add a offset of 4 (the param attributes are indented 6 spaces). This probably should be set to 6 as other elements in this function are indented 6 spaces. (Look into the original mail please, if you are checking this, whitespace in this reply is mangled by thunderbird ...) ACK with that fixed. Peter

Use auto-indent in more places. * src/conf/storage_encryption_conf.h (virStorageEncryptionFormat): Drop parameter. * src/conf/storage_encryption_conf.c (virStorageEncryptionFormat) (virStorageEncryptionSecretFormat): Simplify with auto-indent. * src/conf/domain_conf.c (virDomainDiskDefFormat): Adjust caller. * src/conf/storage_conf.c (virStorageVolTargetDefFormat): Likewise. --- src/conf/domain_conf.c | 9 ++++++--- src/conf/storage_conf.c | 9 ++++++--- src/conf/storage_encryption_conf.c | 20 ++++++++------------ src/conf/storage_encryption_conf.h | 5 ++--- 4 files changed, 22 insertions(+), 21 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index efdf914..6ab73ec 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9221,9 +9221,12 @@ virDomainDiskDefFormat(virBufferPtr buf, if (def->serial) virBufferEscapeString(buf, " <serial>%s</serial>\n", def->serial); - if (def->encryption != NULL && - virStorageEncryptionFormat(buf, def->encryption, 6) < 0) - return -1; + if (def->encryption) { + virBufferAdjustIndent(buf, 6); + if (virStorageEncryptionFormat(buf, def->encryption) < 0) + return -1; + virBufferAdjustIndent(buf, -6); + } if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) return -1; diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index e893b2d..898db62 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1204,9 +1204,12 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, virBufferAddLit(buf," </permissions>\n"); - if (def->encryption != NULL && - virStorageEncryptionFormat(buf, def->encryption, 4) < 0) - return -1; + if (def->encryption) { + virBufferAdjustIndent(buf, 4); + if (virStorageEncryptionFormat(buf, def->encryption) < 0) + return -1; + virBufferAdjustIndent(buf, -4); + } virBufferAsprintf(buf, " </%s>\n", type); diff --git a/src/conf/storage_encryption_conf.c b/src/conf/storage_encryption_conf.c index 73e16ed..3208333 100644 --- a/src/conf/storage_encryption_conf.c +++ b/src/conf/storage_encryption_conf.c @@ -1,7 +1,7 @@ /* * storage_encryption_conf.c: volume encryption information * - * Copyright (C) 2009-2010 Red Hat, Inc. + * Copyright (C) 2009-2011 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -211,8 +211,7 @@ virStorageEncryptionParseNode(xmlDocPtr xml, xmlNodePtr root) static int virStorageEncryptionSecretFormat(virBufferPtr buf, - virStorageEncryptionSecretPtr secret, - int indent) + virStorageEncryptionSecretPtr secret) { const char *type; char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -225,15 +224,14 @@ virStorageEncryptionSecretFormat(virBufferPtr buf, } virUUIDFormat(secret->uuid, uuidstr); - virBufferAsprintf(buf, "%*s<secret type='%s' uuid='%s'/>\n", - indent, "", type, uuidstr); + virBufferAsprintf(buf, " <secret type='%s' uuid='%s'/>\n", + type, uuidstr); return 0; } int virStorageEncryptionFormat(virBufferPtr buf, - virStorageEncryptionPtr enc, - int indent) + virStorageEncryptionPtr enc) { const char *format; size_t i; @@ -244,16 +242,14 @@ virStorageEncryptionFormat(virBufferPtr buf, "%s", _("unexpected encryption format")); return -1; } - virBufferAsprintf(buf, "%*s<encryption format='%s'>\n", - indent, "", format); + virBufferAsprintf(buf, "<encryption format='%s'>\n", format); for (i = 0; i < enc->nsecrets; i++) { - if (virStorageEncryptionSecretFormat(buf, enc->secrets[i], - indent + 2) < 0) + if (virStorageEncryptionSecretFormat(buf, enc->secrets[i]) < 0) return -1; } - virBufferAsprintf(buf, "%*s</encryption>\n", indent, ""); + virBufferAddLit(buf, "</encryption>\n"); return 0; } diff --git a/src/conf/storage_encryption_conf.h b/src/conf/storage_encryption_conf.h index fa5f3cb..cfb088c 100644 --- a/src/conf/storage_encryption_conf.h +++ b/src/conf/storage_encryption_conf.h @@ -1,7 +1,7 @@ /* * storage_encryption_conf.h: volume encryption information * - * Copyright (C) 2009-2010 Red Hat, Inc. + * Copyright (C) 2009-2011 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -66,8 +66,7 @@ void virStorageEncryptionFree(virStorageEncryptionPtr enc); virStorageEncryptionPtr virStorageEncryptionParseNode(xmlDocPtr xml, xmlNodePtr root); int virStorageEncryptionFormat(virBufferPtr buf, - virStorageEncryptionPtr enc, - int indent); + virStorageEncryptionPtr enc); /* A helper for VIR_STORAGE_ENCRYPTION_FORMAT_QCOW */ enum { -- 1.7.4.4

Dňa 29.9.2011 18:22, Eric Blake wrote / napísal(a): This looks like it could be changed to virBufferEscapeString and drop the test. (or is the ip address being mangled by the escape function?) I'd probably break the first string on a new line too, to have them nicely lined up. They shoul have made terminals with more than 80 cols at the very beginning :( This looked a little bit confusing to me at first, interpreting the dot as a comma ... but, well, it works. ACK, you can safely ignore my comments to this patch, as they don't address any important issues :). I was looking for 13/13, but couldn't find one, so I suppose this is the last one. Peter