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.0On 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>
© 2016 - 2025 Red Hat, Inc.