[PATCH 0/5] domain_capabilities: Use virXMLFormatElement() to format XML
*** BLURB HERE *** Michal Prívozník (5): domain_capabilities: Move indentation adjustment out of virDomainCapsCPUCustomFormat() domain_capabilities: Rework virDomainCapsCPUCustomFormat() domain_capabilities: Rework virDomainCapsCPUFormat() domain_capabilities: Check NULL in FORMAT_PROLOGUE domain_capabilities: Use virXMLFormatElement() in FORMAT_PROLOGUE and FORMAT_EPILOGUE macros src/conf/domain_capabilities.c | 135 ++++++++++---------- tests/domaincapsdata/bhyve_basic.x86_64.xml | 3 +- tests/domaincapsdata/bhyve_fbuf.x86_64.xml | 3 +- tests/domaincapsdata/bhyve_uefi.x86_64.xml | 3 +- 4 files changed, 68 insertions(+), 76 deletions(-) -- 2.51.0
From: Michal Privoznik <mprivozn@redhat.com> The aim of virDomainCapsCPUCustomFormat() is to format CPU models into given buffer. But it starts by adjusting indentation. Move this one level up into the caller so that another buffer can be used. This also makes the pattern match in the caller (virDomainCapsCPUFormat()) with the rest of CPU related domcaps formatting. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_capabilities.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 422b68c085..5a94edf9bc 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -446,8 +446,6 @@ virDomainCapsCPUCustomFormat(virBuffer *buf, { size_t i; - virBufferAdjustIndent(buf, 2); - for (i = 0; i < custom->nmodels; i++) { virDomainCapsCPUModel *model = custom->models + i; @@ -480,8 +478,6 @@ virDomainCapsCPUCustomFormat(virBuffer *buf, virBufferAddLit(buf, "</blockers>\n"); } } - - virBufferAdjustIndent(buf, -2); } static void @@ -539,7 +535,9 @@ virDomainCapsCPUFormat(virBuffer *buf, virCPUModeTypeToString(VIR_CPU_MODE_CUSTOM)); if (cpu->custom && cpu->custom->nmodels) { virBufferAddLit(buf, "supported='yes'>\n"); + virBufferAdjustIndent(buf, 2); virDomainCapsCPUCustomFormat(buf, cpu->custom); + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</mode>\n"); } else { virBufferAddLit(buf, "supported='no'/>\n"); -- 2.51.0
On Mon, Nov 10, 2025 at 09:39:46 +0100, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
The aim of virDomainCapsCPUCustomFormat() is to format CPU models into given buffer. But it starts by adjusting indentation. Move this one level up into the caller so that another buffer can be used. This also makes the pattern match in the caller (virDomainCapsCPUFormat()) with the rest of CPU related domcaps formatting.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_capabilities.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
From: Michal Privoznik <mprivozn@redhat.com> Make the virDomainCapsCPUCustomFormat() function use virXMLFormatElement() family of functions. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_capabilities.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 5a94edf9bc..443e6dcd8e 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -447,35 +447,39 @@ virDomainCapsCPUCustomFormat(virBuffer *buf, size_t i; for (i = 0; i < custom->nmodels; i++) { + g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) childBuf = VIR_BUFFER_INITIALIZER; virDomainCapsCPUModel *model = custom->models + i; - virBufferAsprintf(buf, "<model usable='%s'", + virBufferAsprintf(&attrBuf, " usable='%s'", virDomainCapsCPUUsableTypeToString(model->usable)); if (model->deprecated) - virBufferAddLit(buf, " deprecated='yes'"); + virBufferAddLit(&attrBuf, " deprecated='yes'"); if (model->vendor) - virBufferAsprintf(buf, " vendor='%s'", model->vendor); + virBufferAsprintf(&attrBuf, " vendor='%s'", model->vendor); else - virBufferAddLit(buf, " vendor='unknown'"); + virBufferAddLit(&attrBuf, " vendor='unknown'"); if (model->canonical) - virBufferAsprintf(buf, " canonical='%s'", model->canonical); + virBufferAsprintf(&attrBuf, " canonical='%s'", model->canonical); - virBufferAsprintf(buf, ">%s</model>\n", model->name); + virBufferAddStr(&childBuf, model->name); + + virXMLFormatElementDirect(buf, "model", &attrBuf, &childBuf); if (model->blockers) { + g_auto(virBuffer) blockerAttrBuf = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) blockerChildBuf = VIR_BUFFER_INIT_CHILD(buf); char **blocker; - virBufferAsprintf(buf, "<blockers model='%s'>\n", model->name); - virBufferAdjustIndent(buf, 2); + virBufferAsprintf(&blockerAttrBuf, " model='%s'", model->name); for (blocker = model->blockers; *blocker; blocker++) - virBufferAsprintf(buf, "<feature name='%s'/>\n", *blocker); + virBufferAsprintf(&blockerChildBuf, "<feature name='%s'/>\n", *blocker); - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</blockers>\n"); + virXMLFormatElement(buf, "blockers", &blockerAttrBuf, &blockerChildBuf); } } } -- 2.51.0
On Mon, Nov 10, 2025 at 09:39:47 +0100, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
Make the virDomainCapsCPUCustomFormat() function use virXMLFormatElement() family of functions.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_capabilities.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
From: Michal Privoznik <mprivozn@redhat.com> Make the virDomainCapsCPUFormat() function use virXMLFormatElement() family of functions. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_capabilities.c | 77 ++++++++++++++++------------------ 1 file changed, 37 insertions(+), 40 deletions(-) diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 443e6dcd8e..1c69a05685 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -488,67 +488,64 @@ static void virDomainCapsCPUFormat(virBuffer *buf, const virDomainCapsCPU *cpu) { - virBufferAddLit(buf, "<cpu>\n"); - virBufferAdjustIndent(buf, 2); + g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); + g_auto(virBuffer) hostPassModeChildBuf = VIR_BUFFER_INIT_CHILD(&childBuf); + g_auto(virBuffer) hostPassModeAttrBuf = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) maxModeChildBuf = VIR_BUFFER_INIT_CHILD(&childBuf); + g_auto(virBuffer) maxModeAttrBuf = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) hostModeChildBuf = VIR_BUFFER_INIT_CHILD(&childBuf); + g_auto(virBuffer) hostModeAttrBuf = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) customModeChildBuf = VIR_BUFFER_INIT_CHILD(&childBuf); + g_auto(virBuffer) customModeAttrBuf = VIR_BUFFER_INITIALIZER; - virBufferAsprintf(buf, "<mode name='%s' supported='%s'", + virBufferAsprintf(&hostPassModeAttrBuf, " name='%s' supported='%s'", virCPUModeTypeToString(VIR_CPU_MODE_HOST_PASSTHROUGH), cpu->hostPassthrough ? "yes" : "no"); if (cpu->hostPassthrough && cpu->hostPassthroughMigratable.report) { - virBufferAddLit(buf, ">\n"); - virBufferAdjustIndent(buf, 2); - ENUM_PROCESS(cpu, hostPassthroughMigratable, - virTristateSwitchTypeToString); - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</mode>\n"); - } else { - virBufferAddLit(buf, "/>\n"); + virDomainCapsEnumFormat(&hostPassModeChildBuf, + &cpu->hostPassthroughMigratable, + "hostPassthroughMigratable", + virTristateSwitchTypeToString); } - virBufferAsprintf(buf, "<mode name='%s' supported='%s'", + virXMLFormatElement(&childBuf, "mode", &hostPassModeAttrBuf, &hostPassModeChildBuf); + + virBufferAsprintf(&maxModeAttrBuf, " name='%s' supported='%s'", virCPUModeTypeToString(VIR_CPU_MODE_MAXIMUM), cpu->maximum ? "yes" : "no"); if (cpu->maximum && cpu->maximumMigratable.report) { - virBufferAddLit(buf, ">\n"); - virBufferAdjustIndent(buf, 2); - ENUM_PROCESS(cpu, maximumMigratable, - virTristateSwitchTypeToString); - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</mode>\n"); - } else { - virBufferAddLit(buf, "/>\n"); + virDomainCapsEnumFormat(&maxModeChildBuf, + &cpu->maximumMigratable, + "maximumMigratable", + virTristateSwitchTypeToString); } - virBufferAsprintf(buf, "<mode name='%s' ", - virCPUModeTypeToString(VIR_CPU_MODE_HOST_MODEL)); + virXMLFormatElement(&childBuf, "mode", &maxModeAttrBuf, &maxModeChildBuf); + + virBufferAsprintf(&hostModeAttrBuf, " name='%s' supported='%s'", + virCPUModeTypeToString(VIR_CPU_MODE_HOST_MODEL), + cpu->hostModel ? "yes" : "no"); + if (cpu->hostModel) { - virBufferAddLit(buf, "supported='yes'>\n"); - virBufferAdjustIndent(buf, 2); - - virCPUDefFormatBuf(buf, cpu->hostModel); - - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</mode>\n"); - } else { - virBufferAddLit(buf, "supported='no'/>\n"); + virCPUDefFormatBuf(&hostModeChildBuf, cpu->hostModel); } - virBufferAsprintf(buf, "<mode name='%s' ", + virXMLFormatElement(&childBuf, "mode", &hostModeAttrBuf, &hostModeChildBuf); + + virBufferAsprintf(&customModeAttrBuf, " name='%s'", virCPUModeTypeToString(VIR_CPU_MODE_CUSTOM)); if (cpu->custom && cpu->custom->nmodels) { - virBufferAddLit(buf, "supported='yes'>\n"); - virBufferAdjustIndent(buf, 2); - virDomainCapsCPUCustomFormat(buf, cpu->custom); - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</mode>\n"); + virBufferAddLit(&customModeAttrBuf, " supported='yes'"); + virDomainCapsCPUCustomFormat(&customModeChildBuf, cpu->custom); } else { - virBufferAddLit(buf, "supported='no'/>\n"); + virBufferAddLit(&customModeAttrBuf, " supported='no'"); } - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</cpu>\n"); + virXMLFormatElement(&childBuf, "mode", &customModeAttrBuf, &customModeChildBuf); + + virXMLFormatElement(buf, "cpu", NULL, &childBuf); } static void -- 2.51.0
On Mon, Nov 10, 2025 at 09:39:48 +0100, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
Make the virDomainCapsCPUFormat() function use virXMLFormatElement() family of functions.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_capabilities.c | 77 ++++++++++++++++------------------ 1 file changed, 37 insertions(+), 40 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
From: Michal Privoznik <mprivozn@redhat.com> In the virDomainCaps struct there are some pointers that might be NULL (for instance 'sev', 'sgx', 'hyperv'). Teach FORMAT_PROLOGUE macro to check for NULL argument so that format functions (like virDomainCapsFeatureHypervFormat()) don't need to. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_capabilities.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 1c69a05685..78a5b1f56a 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -375,7 +375,7 @@ virDomainCapsStringValuesFormat(virBuffer *buf, #define FORMAT_PROLOGUE(item) \ do { \ - if (item->supported == VIR_TRISTATE_BOOL_ABSENT) \ + if (!item || item->supported == VIR_TRISTATE_BOOL_ABSENT) \ return; \ virBufferAsprintf(buf, "<" #item " supported='%s'%s\n", \ (item->supported == VIR_TRISTATE_BOOL_YES) ? "yes" : "no", \ @@ -818,9 +818,6 @@ virDomainCapsFeatureHypervFormat(virBuffer *buf, { virBuffer defaults = VIR_BUFFER_INIT_CHILD(buf); - if (!hyperv) - return; - FORMAT_PROLOGUE(hyperv); ENUM_PROCESS(hyperv, features, virDomainHypervTypeToString); -- 2.51.0
On Mon, Nov 10, 2025 at 09:39:49 +0100, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
In the virDomainCaps struct there are some pointers that might be NULL (for instance 'sev', 'sgx', 'hyperv'). Teach FORMAT_PROLOGUE macro to check for NULL argument so that format functions (like virDomainCapsFeatureHypervFormat()) don't need to.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_capabilities.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
From: Michal Privoznik <mprivozn@redhat.com> Domain capabilities XML is formatted (mostly) using FORMAT_PROLOGUE and FORMAT_EPILOGUE macros. These format opening and closing stanzas for given element. The FORMAT_PROLOGUE macro even tries to be clever and format element onto one line (if the element isn't supported), but that's not enough. Fortunately, we have virXMLFormatElement() which formats elements properly, so let's switch macros into using that. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_capabilities.c | 25 ++++++++++----------- tests/domaincapsdata/bhyve_basic.x86_64.xml | 3 +-- tests/domaincapsdata/bhyve_fbuf.x86_64.xml | 3 +-- tests/domaincapsdata/bhyve_uefi.x86_64.xml | 3 +-- 4 files changed, 15 insertions(+), 19 deletions(-) diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 78a5b1f56a..be3c4002ab 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -374,26 +374,25 @@ virDomainCapsStringValuesFormat(virBuffer *buf, #define FORMAT_PROLOGUE(item) \ + g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); \ + g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; \ do { \ if (!item || item->supported == VIR_TRISTATE_BOOL_ABSENT) \ return; \ - virBufferAsprintf(buf, "<" #item " supported='%s'%s\n", \ - (item->supported == VIR_TRISTATE_BOOL_YES) ? "yes" : "no", \ - (item->supported == VIR_TRISTATE_BOOL_YES) ? ">" : "/>"); \ - if (item->supported == VIR_TRISTATE_BOOL_NO) \ + virBufferAsprintf(&attrBuf, " supported='%s'", \ + (item->supported == VIR_TRISTATE_BOOL_YES) ? "yes" : "no"); \ + if (item->supported == VIR_TRISTATE_BOOL_NO) { \ + virXMLFormatElement(buf, #item, &attrBuf, NULL); \ return; \ - virBufferAdjustIndent(buf, 2); \ + } \ } while (0) #define FORMAT_EPILOGUE(item) \ - do { \ - virBufferAdjustIndent(buf, -2); \ - virBufferAddLit(buf, "</" #item ">\n"); \ - } while (0) + virXMLFormatElement(buf, #item, &attrBuf, &childBuf) #define ENUM_PROCESS(master, capsEnum, valToStr) \ do { \ - virDomainCapsEnumFormat(buf, &master->capsEnum, \ + virDomainCapsEnumFormat(&childBuf, &master->capsEnum, \ #capsEnum, valToStr); \ } while (0) @@ -417,7 +416,7 @@ virDomainCapsLoaderFormat(virBuffer *buf, { FORMAT_PROLOGUE(loader); - virDomainCapsStringValuesFormat(buf, &loader->values); + virDomainCapsStringValuesFormat(&childBuf, &loader->values); ENUM_PROCESS(loader, type, virDomainLoaderTypeToString); ENUM_PROCESS(loader, readonly, virTristateBoolTypeToString); ENUM_PROCESS(loader, secure, virTristateBoolTypeToString); @@ -435,7 +434,7 @@ virDomainCapsOSFormat(virBuffer *buf, ENUM_PROCESS(os, firmware, virDomainOsDefFirmwareTypeToString); - virDomainCapsLoaderFormat(buf, loader); + virDomainCapsLoaderFormat(&childBuf, loader); FORMAT_EPILOGUE(os); } @@ -851,7 +850,7 @@ virDomainCapsFeatureHypervFormat(virBuffer *buf, virBufferEscapeString(&defaults, "<vendor_id>%s</vendor_id>\n", hyperv->vendor_id); } - virXMLFormatElement(buf, "defaults", NULL, &defaults); + virXMLFormatElement(&childBuf, "defaults", NULL, &defaults); FORMAT_EPILOGUE(hyperv); } diff --git a/tests/domaincapsdata/bhyve_basic.x86_64.xml b/tests/domaincapsdata/bhyve_basic.x86_64.xml index 2dee7c6547..44527bbb7f 100644 --- a/tests/domaincapsdata/bhyve_basic.x86_64.xml +++ b/tests/domaincapsdata/bhyve_basic.x86_64.xml @@ -26,8 +26,7 @@ </disk> <graphics supported='no'/> <video supported='no'/> - <hostdev supported='yes'> - </hostdev> + <hostdev supported='yes'/> <console supported='yes'> <enum name='type'> <value>tcp</value> diff --git a/tests/domaincapsdata/bhyve_fbuf.x86_64.xml b/tests/domaincapsdata/bhyve_fbuf.x86_64.xml index d2f0dd6383..5b2044736c 100644 --- a/tests/domaincapsdata/bhyve_fbuf.x86_64.xml +++ b/tests/domaincapsdata/bhyve_fbuf.x86_64.xml @@ -43,8 +43,7 @@ <value>gop</value> </enum> </video> - <hostdev supported='yes'> - </hostdev> + <hostdev supported='yes'/> <console supported='yes'> <enum name='type'> <value>tcp</value> diff --git a/tests/domaincapsdata/bhyve_uefi.x86_64.xml b/tests/domaincapsdata/bhyve_uefi.x86_64.xml index b093358c49..d99dde98e1 100644 --- a/tests/domaincapsdata/bhyve_uefi.x86_64.xml +++ b/tests/domaincapsdata/bhyve_uefi.x86_64.xml @@ -35,8 +35,7 @@ </disk> <graphics supported='no'/> <video supported='no'/> - <hostdev supported='yes'> - </hostdev> + <hostdev supported='yes'/> <console supported='yes'> <enum name='type'> <value>tcp</value> -- 2.51.0
On Mon, Nov 10, 2025 at 09:39:50 +0100, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
Domain capabilities XML is formatted (mostly) using FORMAT_PROLOGUE and FORMAT_EPILOGUE macros. These format opening and closing stanzas for given element. The FORMAT_PROLOGUE macro even tries to be clever and format element onto one line (if the element isn't supported), but that's not enough. Fortunately, we have virXMLFormatElement() which formats elements properly, so let's switch macros into using that.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_capabilities.c | 25 ++++++++++----------- tests/domaincapsdata/bhyve_basic.x86_64.xml | 3 +-- tests/domaincapsdata/bhyve_fbuf.x86_64.xml | 3 +-- tests/domaincapsdata/bhyve_uefi.x86_64.xml | 3 +-- 4 files changed, 15 insertions(+), 19 deletions(-)
diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 78a5b1f56a..be3c4002ab 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -374,26 +374,25 @@ virDomainCapsStringValuesFormat(virBuffer *buf,
#define FORMAT_PROLOGUE(item) \ + g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); \ + g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; \
Hmm, this isn't great as it pollutes the parent scope.
do { \
And this bit is then definitely pointless as you definitely won't be using this macro as a body of e.g. an 'if' without an explicit block.
if (!item || item->supported == VIR_TRISTATE_BOOL_ABSENT) \ return; \ - virBufferAsprintf(buf, "<" #item " supported='%s'%s\n", \ - (item->supported == VIR_TRISTATE_BOOL_YES) ? "yes" : "no", \ - (item->supported == VIR_TRISTATE_BOOL_YES) ? ">" : "/>"); \ - if (item->supported == VIR_TRISTATE_BOOL_NO) \ + virBufferAsprintf(&attrBuf, " supported='%s'", \ + (item->supported == VIR_TRISTATE_BOOL_YES) ? "yes" : "no"); \ + if (item->supported == VIR_TRISTATE_BOOL_NO) { \ + virXMLFormatElement(buf, #item, &attrBuf, NULL); \ return; \ - virBufferAdjustIndent(buf, 2); \ + } \ } while (0)
#define FORMAT_EPILOGUE(item) \ - do { \ - virBufferAdjustIndent(buf, -2); \ - virBufferAddLit(buf, "</" #item ">\n"); \ - } while (0) + virXMLFormatElement(buf, #item, &attrBuf, &childBuf)
But I do like this change. I wonder if adding some documentation about the expected usage would be worth it. Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On 11/10/25 10:12, Peter Krempa wrote:
On Mon, Nov 10, 2025 at 09:39:50 +0100, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
Domain capabilities XML is formatted (mostly) using FORMAT_PROLOGUE and FORMAT_EPILOGUE macros. These format opening and closing stanzas for given element. The FORMAT_PROLOGUE macro even tries to be clever and format element onto one line (if the element isn't supported), but that's not enough. Fortunately, we have virXMLFormatElement() which formats elements properly, so let's switch macros into using that.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_capabilities.c | 25 ++++++++++----------- tests/domaincapsdata/bhyve_basic.x86_64.xml | 3 +-- tests/domaincapsdata/bhyve_fbuf.x86_64.xml | 3 +-- tests/domaincapsdata/bhyve_uefi.x86_64.xml | 3 +-- 4 files changed, 15 insertions(+), 19 deletions(-)
diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 78a5b1f56a..be3c4002ab 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -374,26 +374,25 @@ virDomainCapsStringValuesFormat(virBuffer *buf,
#define FORMAT_PROLOGUE(item) \ + g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); \ + g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; \
Hmm, this isn't great as it pollutes the parent scope.
do { \
And this bit is then definitely pointless as you definitely won't be using this macro as a body of e.g. an 'if' without an explicit block.
if (!item || item->supported == VIR_TRISTATE_BOOL_ABSENT) \ return; \ - virBufferAsprintf(buf, "<" #item " supported='%s'%s\n", \ - (item->supported == VIR_TRISTATE_BOOL_YES) ? "yes" : "no", \ - (item->supported == VIR_TRISTATE_BOOL_YES) ? ">" : "/>"); \ - if (item->supported == VIR_TRISTATE_BOOL_NO) \ + virBufferAsprintf(&attrBuf, " supported='%s'", \ + (item->supported == VIR_TRISTATE_BOOL_YES) ? "yes" : "no"); \ + if (item->supported == VIR_TRISTATE_BOOL_NO) { \ + virXMLFormatElement(buf, #item, &attrBuf, NULL); \ return; \ - virBufferAdjustIndent(buf, 2); \ + } \ } while (0)
#define FORMAT_EPILOGUE(item) \ - do { \ - virBufferAdjustIndent(buf, -2); \ - virBufferAddLit(buf, "</" #item ">\n"); \ - } while (0) + virXMLFormatElement(buf, #item, &attrBuf, &childBuf)
But I do like this change.
I wonder if adding some documentation about the expected usage would be worth it.
Sure! What about: /** * FORMAT_PROLOGUE: * @item: item to format * * Formats part of domain capabilities for @item. The element name is #item so * variable name is important. If the particular capability is not supported, * then the macro also returns early. * * Additionally, the macro declares two variables: @childBuf and @attrBuf where * the former holds contents of the child elements and the latter holds * contents of <#item/> attributes (so far limited to "supported='yes/no'"). */ Michal
On Mon, Nov 10, 2025 at 10:43:38 +0100, Michal Prívozník wrote:
On 11/10/25 10:12, Peter Krempa wrote:
On Mon, Nov 10, 2025 at 09:39:50 +0100, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
Domain capabilities XML is formatted (mostly) using FORMAT_PROLOGUE and FORMAT_EPILOGUE macros. These format opening and closing stanzas for given element. The FORMAT_PROLOGUE macro even tries to be clever and format element onto one line (if the element isn't supported), but that's not enough. Fortunately, we have virXMLFormatElement() which formats elements properly, so let's switch macros into using that.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_capabilities.c | 25 ++++++++++----------- tests/domaincapsdata/bhyve_basic.x86_64.xml | 3 +-- tests/domaincapsdata/bhyve_fbuf.x86_64.xml | 3 +-- tests/domaincapsdata/bhyve_uefi.x86_64.xml | 3 +-- 4 files changed, 15 insertions(+), 19 deletions(-)
diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 78a5b1f56a..be3c4002ab 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -374,26 +374,25 @@ virDomainCapsStringValuesFormat(virBuffer *buf,
#define FORMAT_PROLOGUE(item) \ + g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); \ + g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; \
Hmm, this isn't great as it pollutes the parent scope.
do { \
And this bit is then definitely pointless as you definitely won't be using this macro as a body of e.g. an 'if' without an explicit block.
if (!item || item->supported == VIR_TRISTATE_BOOL_ABSENT) \ return; \ - virBufferAsprintf(buf, "<" #item " supported='%s'%s\n", \ - (item->supported == VIR_TRISTATE_BOOL_YES) ? "yes" : "no", \ - (item->supported == VIR_TRISTATE_BOOL_YES) ? ">" : "/>"); \ - if (item->supported == VIR_TRISTATE_BOOL_NO) \ + virBufferAsprintf(&attrBuf, " supported='%s'", \ + (item->supported == VIR_TRISTATE_BOOL_YES) ? "yes" : "no"); \ + if (item->supported == VIR_TRISTATE_BOOL_NO) { \ + virXMLFormatElement(buf, #item, &attrBuf, NULL); \ return; \ - virBufferAdjustIndent(buf, 2); \ + } \ } while (0)
#define FORMAT_EPILOGUE(item) \ - do { \ - virBufferAdjustIndent(buf, -2); \ - virBufferAddLit(buf, "</" #item ">\n"); \ - } while (0) + virXMLFormatElement(buf, #item, &attrBuf, &childBuf)
But I do like this change.
I wonder if adding some documentation about the expected usage would be worth it.
Sure! What about:
/** * FORMAT_PROLOGUE: * @item: item to format * * Formats part of domain capabilities for @item. The element name is #item so * variable name is important. If the particular capability is not supported, * then the macro also returns early. * * Additionally, the macro declares two variables: @childBuf and @attrBuf where * the former holds contents of the child elements and the latter holds * contents of <#item/> attributes (so far limited to "supported='yes/no'"). */
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (3)
-
Michal Privoznik -
Michal Prívozník
-
Peter Krempa