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 <[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'"). > */
Reviewed-by: Peter Krempa <[email protected]>
