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