On Fri, Mar 07, 2025 at 08:13:51 +0100, Andrea Righi via Devel wrote:
> Gentle ping. Is there any feedback, comment, suggestion about this?
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
