Re: [PATCH v2 0/6] qemu: acpi-generic-initiator support

On Fri, Mar 07, 2025 at 08:13:51 +0100, Andrea Righi via Devel wrote: I have a couple points about coding style and some general stuff. This feature is out of interest are and thus I will not try to understand whether the design or implementation makes sense. I'll summarize my points here as it's not a proper review: Patch 1/6: - Format the function header as: void virDomainAcpiInitiatorDefFree( - don't mix g_free and VIR_FREE; - Try to use proper type instead of '<text/>' in XML schema Patch 2/6: - virDomainAcpiInitiatorDefParseXML: - broken formatting of arguments - do not use the for-loop; I refactored most of other code to remove this style of parser, use XPaths and virXMLProp* instead - the function doesn't report libvirt errors on some code paths - virDomainAcpiInitiatorDefNew can't fail thus no point in checking return value - broken formatting in call of this function from virDomainDeviceDefParse - virDomainAcpiInitiatorDefCheckABIStability - broken formatting of function header - virDomainAcpiInitiatorDefFormat - broken formatting in header - attrBuf variable not actually used - use virBufferEscapeString for any strings that are parsed for the user to ensure XML entity escaping - extra space in 'alias' element - virDomainAcpiInitiatorDefValidate - first for loop must have a block (looks confusing) - it's also a rather peculiar way to check that 'acpiinitiator->numaNode' is < nodeCount; - don't use VIR_ERR_INTERNAL_ERROR for configuration errors - completely broken formatting of all virReportError calls patch 3/6: - patch breaks virReportError formatting without changing the error