Hi Peter, On Fri, Mar 07, 2025 at 01:45:01PM +0100, Peter Krempa wrote: > 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: Thanks for the review, I'll fix all the coding style issues in the next version. Do you think I should include more details in the cover letter to better explain the use cases of this feature? -Andrea > > > 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 >
