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 <[email protected]> >> >> 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 <[email protected]> >> --- >> 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
