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>