[libvirt] [PATCH 0/4] Couple of <cpu/> fixes
*** BLURB HERE *** Michal Privoznik (4): util: Introduce virBufferAddBuffer virCPUDefFormatBufFull: Use our general error handling pattern cpu: Format <cpu/> properly qemu: Use correct flags for ABI stability check in SaveImageUpdateDef src/conf/cpu_conf.c | 31 ++++-- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 4 +- src/util/virbuffer.c | 33 +++++- src/util/virbuffer.h | 1 + .../qemuxml2argv-cpu-host-kvmclock.xml | 3 +- tests/virbuftest.c | 112 +++++++++++++++++++++ 7 files changed, 169 insertions(+), 16 deletions(-) -- 2.0.5
There are some places in our code base which follow the following pattern: char *s = virBufferContentAndReset(buf1); virBufferAdd(buf2, s, -1); How about introducing an API to join those two lines into one? Something like: virBufferAddBuffer(buf2, buf1); Signed-off-by: Michal Privoznik <mprivozn@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; 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! + * + * 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; + + needSize = buf->use + toadd->use; + if (virBufferGrow(buf, needSize - buf->use) < 0) + return; + + memcpy(&buf->content[buf->use], toadd->content, toadd->use); + buf->use += toadd->use; + buf->content[buf->use] = '\0'; + 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"); + + if (!virBufferUse(&buf1)) { + TEST_ERROR("Error adding to buf1"); + goto cleanup; + } + + if (!virBufferUse(&buf2)) { + TEST_ERROR("Error adding to buf2"); + goto cleanup; + } + + if (!virBufferUse(&buf3)) { + TEST_ERROR("Error adding to buf3"); + goto cleanup; + } + + virBufferAddBuffer(&buf2, &buf3); + + if (!virBufferUse(&buf2)) { + TEST_ERROR("buf2 cleared mistakenly"); + goto cleanup; + } + + if (virBufferUse(&buf3)) { + TEST_ERROR("buf3 is not clear even though it should be"); + goto cleanup; + } + + virBufferAddBuffer(&buf1, &buf2); + + if (!virBufferUse(&buf1)) { + TEST_ERROR("buf1 cleared mistakenly"); + goto cleanup; + } + + if (virBufferUse(&buf2)) { + TEST_ERROR("buf2 is not clear even though it should be"); + goto cleanup; + } + + result = virBufferContentAndReset(&buf1); + if (!result || STRNEQ(result, expected)) { + virtTestDifference(stderr, expected, result); + goto cleanup; + } + + ret = 0; + cleanup: + virBufferFreeAndReset(&buf1); + virBufferFreeAndReset(&buf2); + VIR_FREE(result); + return ret; +} + static int mymain(void) @@ -217,6 +328,7 @@ mymain(void) DO_TEST("VSprintf infinite loop", testBufInfiniteLoop, 0); DO_TEST("Auto-indentation", testBufAutoIndent, 0); DO_TEST("Trim", testBufTrim, 0); + DO_TEST("AddBuffer", testBufAddBuffer, 0); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.0.5
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@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
Well, so far there are no variables to free, no cleanup work needed on an error, so bare 'return -1;' after each error is just okay. But this will change in a while. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/cpu_conf.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index 4a367a1..9a430ef 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -555,6 +555,8 @@ virCPUDefFormatBufFull(virBufferPtr buf, virCPUDefPtr def, bool updateCPU) { + int ret = -1; + if (!def) return 0; @@ -566,7 +568,7 @@ virCPUDefFormatBufFull(virBufferPtr buf, if (!(tmp = virCPUModeTypeToString(def->mode))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unexpected CPU mode %d"), def->mode); - return -1; + goto cleanup; } virBufferAsprintf(buf, " mode='%s'", tmp); } @@ -578,7 +580,7 @@ virCPUDefFormatBufFull(virBufferPtr buf, virReportError(VIR_ERR_INTERNAL_ERROR, _("Unexpected CPU match policy %d"), def->match); - return -1; + goto cleanup; } virBufferAsprintf(buf, " match='%s'", tmp); } @@ -590,12 +592,14 @@ virCPUDefFormatBufFull(virBufferPtr buf, virBufferAsprintf(buf, "<arch>%s</arch>\n", virArchToString(def->arch)); if (virCPUDefFormatBuf(buf, def, updateCPU) < 0) - return -1; + goto cleanup; virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</cpu>\n"); - return 0; + ret = 0; + cleanup: + return ret; } int -- 2.0.5
On Thu, Feb 19, 2015 at 02:13:43PM +0100, Michal Privoznik wrote:
Well, so far there are no variables to free, no cleanup work needed on an error, so bare 'return -1;' after each error is just okay. But this will change in a while.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/cpu_conf.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
ACK, but you could've just squashed it in with the next one, I guess.
diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index 4a367a1..9a430ef 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -555,6 +555,8 @@ virCPUDefFormatBufFull(virBufferPtr buf, virCPUDefPtr def, bool updateCPU) { + int ret = -1; + if (!def) return 0;
@@ -566,7 +568,7 @@ virCPUDefFormatBufFull(virBufferPtr buf, if (!(tmp = virCPUModeTypeToString(def->mode))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unexpected CPU mode %d"), def->mode); - return -1; + goto cleanup; } virBufferAsprintf(buf, " mode='%s'", tmp); } @@ -578,7 +580,7 @@ virCPUDefFormatBufFull(virBufferPtr buf, virReportError(VIR_ERR_INTERNAL_ERROR, _("Unexpected CPU match policy %d"), def->match); - return -1; + goto cleanup; } virBufferAsprintf(buf, " match='%s'", tmp); } @@ -590,12 +592,14 @@ virCPUDefFormatBufFull(virBufferPtr buf, virBufferAsprintf(buf, "<arch>%s</arch>\n", virArchToString(def->arch)); if (virCPUDefFormatBuf(buf, def, updateCPU) < 0) - return -1; + goto cleanup; virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</cpu>\n");
- return 0; + ret = 0; + cleanup: + return ret; }
int -- 2.0.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Well, not that we are not formatting invalid XML, rather than not as beautiful as we can: <cpu mode='host-passthrough'> </cpu> If there are no children, let's use the singleton element. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/cpu_conf.c | 19 +++++++++++++------ .../qemuxml2argv-cpu-host-kvmclock.xml | 3 +-- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index 9a430ef..1a35185 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -556,6 +556,8 @@ virCPUDefFormatBufFull(virBufferPtr buf, bool updateCPU) { int ret = -1; + virBuffer childrenBuf = VIR_BUFFER_INITIALIZER; + int indent = virBufferGetIndent(buf, false); if (!def) return 0; @@ -585,20 +587,25 @@ virCPUDefFormatBufFull(virBufferPtr buf, virBufferAsprintf(buf, " match='%s'", tmp); } } - virBufferAddLit(buf, ">\n"); - virBufferAdjustIndent(buf, 2); + virBufferAdjustIndent(&childrenBuf, indent + 2); if (def->arch) - virBufferAsprintf(buf, "<arch>%s</arch>\n", + virBufferAsprintf(&childrenBuf, "<arch>%s</arch>\n", virArchToString(def->arch)); - if (virCPUDefFormatBuf(buf, def, updateCPU) < 0) + if (virCPUDefFormatBuf(&childrenBuf, def, updateCPU) < 0) goto cleanup; - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</cpu>\n"); + if (virBufferUse(&childrenBuf)) { + virBufferAddLit(buf, ">\n"); + virBufferAddBuffer(buf, &childrenBuf); + virBufferAddLit(buf, "</cpu>\n"); + } else { + virBufferAddLit(buf, "/>\n"); + } ret = 0; cleanup: + virBufferFreeAndReset(&childrenBuf); return ret; } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cpu-host-kvmclock.xml b/tests/qemuxml2argvdata/qemuxml2argv-cpu-host-kvmclock.xml index 89153a5..8954278 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-cpu-host-kvmclock.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-cpu-host-kvmclock.xml @@ -8,8 +8,7 @@ <type arch='x86_64' machine='pc'>hvm</type> <boot dev='network'/> </os> - <cpu mode='host-passthrough'> - </cpu> + <cpu mode='host-passthrough'/> <clock offset='utc'> <timer name='kvmclock' present='no'/> </clock> -- 2.0.5
On Thu, Feb 19, 2015 at 02:13:44PM +0100, Michal Privoznik wrote:
Well, not that we are not formatting invalid XML, rather than not as beautiful as we can:
<cpu mode='host-passthrough'> </cpu>
If there are no children, let's use the singleton element.
Is there any purpose behind this or is this just for the sake of looking good? ACK anyway (after the requisite 1/4 is fixed).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/cpu_conf.c | 19 +++++++++++++------ .../qemuxml2argv-cpu-host-kvmclock.xml | 3 +-- 2 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index 9a430ef..1a35185 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -556,6 +556,8 @@ virCPUDefFormatBufFull(virBufferPtr buf, bool updateCPU) { int ret = -1; + virBuffer childrenBuf = VIR_BUFFER_INITIALIZER; + int indent = virBufferGetIndent(buf, false);
if (!def) return 0; @@ -585,20 +587,25 @@ virCPUDefFormatBufFull(virBufferPtr buf, virBufferAsprintf(buf, " match='%s'", tmp); } } - virBufferAddLit(buf, ">\n"); - virBufferAdjustIndent(buf, 2);
+ virBufferAdjustIndent(&childrenBuf, indent + 2); if (def->arch) - virBufferAsprintf(buf, "<arch>%s</arch>\n", + virBufferAsprintf(&childrenBuf, "<arch>%s</arch>\n", virArchToString(def->arch)); - if (virCPUDefFormatBuf(buf, def, updateCPU) < 0) + if (virCPUDefFormatBuf(&childrenBuf, def, updateCPU) < 0) goto cleanup; - virBufferAdjustIndent(buf, -2);
- virBufferAddLit(buf, "</cpu>\n"); + if (virBufferUse(&childrenBuf)) { + virBufferAddLit(buf, ">\n"); + virBufferAddBuffer(buf, &childrenBuf); + virBufferAddLit(buf, "</cpu>\n"); + } else { + virBufferAddLit(buf, "/>\n"); + }
ret = 0; cleanup: + virBufferFreeAndReset(&childrenBuf); return ret; }
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cpu-host-kvmclock.xml b/tests/qemuxml2argvdata/qemuxml2argv-cpu-host-kvmclock.xml index 89153a5..8954278 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-cpu-host-kvmclock.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-cpu-host-kvmclock.xml @@ -8,8 +8,7 @@ <type arch='x86_64' machine='pc'>hvm</type> <boot dev='network'/> </os> - <cpu mode='host-passthrough'> - </cpu> + <cpu mode='host-passthrough'/> <clock offset='utc'> <timer name='kvmclock' present='no'/> </clock> -- 2.0.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
https://bugzilla.redhat.com/show_bug.cgi?id=1183869 Soo. you've successfully started yourself a domain. And since you want to use it on your host exclusively you are confident enough to passthrough the host CPU model, like this: <cpu mode='host-passthrough'/> Then, after a while, you want to save the domain into a file (e.g. virsh save dom dom.save). And here comes the trouble. The file consist of two parts: Libvirt header (containing domain XML among other things), and qemu migration data. Now, the domain XML in the header is formatted using special flags (VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_UPDATE_CPU | VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_MIGRATABLE). Then, on your way back from the bar, you think of changing something in the XML in the saved file (we have a command for it after all), say listen address for graphics console. So you successfully type in the command: virsh save-image-edit dom.save Change all the bits, and exit the editor. But instead of success you're left with sad error message: error: unsupported configuration: Target CPU model <null> does not match source Pentium Pro Sigh. Digging into the code you see lines, where we check for ABI stability. The new XML you've produced is compared with the old one from the saved file to see if qemu ABI will break or not. Wait, what? We are using different flags to parse the XML you've provided so we were just lucky it worked in some cases? Yep, that's right. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8f0cf2b..19dfce4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5734,8 +5734,8 @@ qemuDomainSaveImageUpdateDef(virQEMUDriverPtr driver, if (!(newdef_migr = qemuDomainDefCopy(driver, newdef, - VIR_DOMAIN_XML_MIGRATABLE | - VIR_DOMAIN_XML_SECURE))) + QEMU_DOMAIN_FORMAT_LIVE_FLAGS || + VIR_DOMAIN_XML_MIGRATABLE))) goto cleanup; if (!virDomainDefCheckABIStability(def, newdef_migr)) { -- 2.0.5
On Thu, Feb 19, 2015 at 02:13:45PM +0100, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1183869
Soo. you've successfully started yourself a domain. And since you want
s/you/You/ at least ;)
to use it on your host exclusively you are confident enough to passthrough the host CPU model, like this:
<cpu mode='host-passthrough'/>
Then, after a while, you want to save the domain into a file (e.g. virsh save dom dom.save). And here comes the trouble. The file consist of two parts: Libvirt header (containing domain XML among other things), and qemu migration data. Now, the domain XML in the header is formatted using special flags (VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_UPDATE_CPU | VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_MIGRATABLE).
Then, on your way back from the bar, you think of changing something in the XML in the saved file (we have a command for it after all), say listen address for graphics console. So you successfully type in the command:
virsh save-image-edit dom.save
Change all the bits, and exit the editor. But instead of success you're left with sad error message:
error: unsupported configuration: Target CPU model <null> does not match source Pentium Pro
Sigh. Digging into the code you see lines, where we check for ABI stability. The new XML you've produced is compared with the old one from the saved file to see if qemu ABI will break or not. Wait, what? We are using different flags to parse the XML you've provided so we were just lucky it worked in some cases? Yep, that's right.
Great story, is that yours? What bar was that? ;-)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8f0cf2b..19dfce4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5734,8 +5734,8 @@ qemuDomainSaveImageUpdateDef(virQEMUDriverPtr driver,
if (!(newdef_migr = qemuDomainDefCopy(driver, newdef, - VIR_DOMAIN_XML_MIGRATABLE | - VIR_DOMAIN_XML_SECURE))) + QEMU_DOMAIN_FORMAT_LIVE_FLAGS || + VIR_DOMAIN_XML_MIGRATABLE)))
Shouldn't there be also VIR_DOMAIN_XML_INACTIVE? Although any live data can't get there, can it? ACK
goto cleanup;
if (!virDomainDefCheckABIStability(def, newdef_migr)) { -- 2.0.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
This API joins the following two lines: char *s = virBufferContentAndReset(buf1); virBufferAdd(buf2, s, -1); into one: virBufferAddBuffer(buf2, buf1); With one exception: there's no re-indentation applied to @buf1. The idea is, that in general both can have different indentation (like the test I'm adding proves) Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- diff to v1: -Martin's suggestions worked in (hopefully) src/libvirt_private.syms | 1 + src/util/virbuffer.c | 40 ++++++++++++++++- src/util/virbuffer.h | 1 + tests/virbuftest.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 152 insertions(+), 2 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c156b40..ba05cc6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1081,6 +1081,7 @@ virBitmapToData; # util/virbuffer.h virBufferAdd; +virBufferAddBuffer; virBufferAddChar; virBufferAdjustIndent; virBufferAsprintf; diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index e94b35d..96a0f16 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; memset(&buf->content[buf->use], ' ', indent); @@ -173,6 +172,43 @@ 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 (intentionally) NOT applied! + * + * 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) + return; + + if (buf->error || toadd->error) { + if (!buf->error) + buf->error = toadd->error; + virBufferFreeAndReset(toadd); + return; + } + + needSize = buf->use + toadd->use; + if (virBufferGrow(buf, needSize - buf->use) < 0) + return; + + memcpy(&buf->content[buf->use], toadd->content, toadd->use); + buf->use += toadd->use; + buf->content[buf->use] = '\0'; + 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"); + + if (!virBufferUse(&buf1)) { + TEST_ERROR("Error adding to buf1"); + goto cleanup; + } + + if (!virBufferUse(&buf2)) { + TEST_ERROR("Error adding to buf2"); + goto cleanup; + } + + if (!virBufferUse(&buf3)) { + TEST_ERROR("Error adding to buf3"); + goto cleanup; + } + + virBufferAddBuffer(&buf2, &buf3); + + if (!virBufferUse(&buf2)) { + TEST_ERROR("buf2 cleared mistakenly"); + goto cleanup; + } + + if (virBufferUse(&buf3)) { + TEST_ERROR("buf3 is not clear even though it should be"); + goto cleanup; + } + + virBufferAddBuffer(&buf1, &buf2); + + if (!virBufferUse(&buf1)) { + TEST_ERROR("buf1 cleared mistakenly"); + goto cleanup; + } + + if (virBufferUse(&buf2)) { + TEST_ERROR("buf2 is not clear even though it should be"); + goto cleanup; + } + + result = virBufferContentAndReset(&buf1); + if (!result || STRNEQ(result, expected)) { + virtTestDifference(stderr, expected, result); + goto cleanup; + } + + ret = 0; + cleanup: + virBufferFreeAndReset(&buf1); + virBufferFreeAndReset(&buf2); + VIR_FREE(result); + return ret; +} + static int mymain(void) @@ -217,6 +328,7 @@ mymain(void) DO_TEST("VSprintf infinite loop", testBufInfiniteLoop, 0); DO_TEST("Auto-indentation", testBufAutoIndent, 0); DO_TEST("Trim", testBufTrim, 0); + DO_TEST("AddBuffer", testBufAddBuffer, 0); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.0.5
On Tue, Feb 24, 2015 at 04:14:37PM +0100, Michal Privoznik wrote:
This API joins the following two lines:
char *s = virBufferContentAndReset(buf1); virBufferAdd(buf2, s, -1);
into one:
virBufferAddBuffer(buf2, buf1);
With one exception: there's no re-indentation applied to @buf1. The idea is, that in general both can have different indentation (like the test I'm adding proves)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
diff to v1: -Martin's suggestions worked in (hopefully)
src/libvirt_private.syms | 1 + src/util/virbuffer.c | 40 ++++++++++++++++- src/util/virbuffer.h | 1 + tests/virbuftest.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 152 insertions(+), 2 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c156b40..ba05cc6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1081,6 +1081,7 @@ virBitmapToData;
# util/virbuffer.h virBufferAdd; +virBufferAddBuffer; virBufferAddChar; virBufferAdjustIndent; virBufferAsprintf; diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index e94b35d..96a0f16 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;
memset(&buf->content[buf->use], ' ', indent); @@ -173,6 +172,43 @@ 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 (intentionally) NOT applied! + * + * 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) + return; + + if (buf->error || toadd->error) { + if (!buf->error) + buf->error = toadd->error; + virBufferFreeAndReset(toadd); + return; + } + + needSize = buf->use + toadd->use; + if (virBufferGrow(buf, needSize - buf->use) < 0) + return; + + memcpy(&buf->content[buf->use], toadd->content, toadd->use); + buf->use += toadd->use; + buf->content[buf->use] = '\0'; + 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" \
s/war,/war./
+" 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");
Same here ;)
+ 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"); + + if (!virBufferUse(&buf1)) { + TEST_ERROR("Error adding to buf1"); + goto cleanup; + } + + if (!virBufferUse(&buf2)) { + TEST_ERROR("Error adding to buf2"); + goto cleanup; + } + + if (!virBufferUse(&buf3)) { + TEST_ERROR("Error adding to buf3"); + goto cleanup; + } + + virBufferAddBuffer(&buf2, &buf3); + + if (!virBufferUse(&buf2)) { + TEST_ERROR("buf2 cleared mistakenly"); + goto cleanup; + } + + if (virBufferUse(&buf3)) { + TEST_ERROR("buf3 is not clear even though it should be"); + goto cleanup; + } + + virBufferAddBuffer(&buf1, &buf2); + + if (!virBufferUse(&buf1)) { + TEST_ERROR("buf1 cleared mistakenly"); + goto cleanup; + } + + if (virBufferUse(&buf2)) { + TEST_ERROR("buf2 is not clear even though it should be"); + goto cleanup; + } + + result = virBufferContentAndReset(&buf1); + if (!result || STRNEQ(result, expected)) {
Just use STRNEQ_NULLABLE. ACK
+ virtTestDifference(stderr, expected, result); + goto cleanup; + } + + ret = 0; + cleanup: + virBufferFreeAndReset(&buf1); + virBufferFreeAndReset(&buf2); + VIR_FREE(result); + return ret; +} +
static int mymain(void) @@ -217,6 +328,7 @@ mymain(void) DO_TEST("VSprintf infinite loop", testBufInfiniteLoop, 0); DO_TEST("Auto-indentation", testBufAutoIndent, 0); DO_TEST("Trim", testBufTrim, 0); + DO_TEST("AddBuffer", testBufAddBuffer, 0);
return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.0.5
participants (2)
-
Martin Kletzander -
Michal Privoznik