Re: [PATCH RFC v3 03/16] config: Introduce ThrottleGroup and corresponding XML parsing

On Fri, Aug 23, 2024 at 14:00:25 +0800, Chun Feng Wu wrote: > On 2024/7/26 21:03, Peter Krempa wrote: >>> +static virDomainThrottleGroupDef * >>> +virDomainThrottleGroupDefParseXML(xmlNodePtr node, >>> + xmlXPathContextPtr ctxt) >>> +{ >>> + g_autoptr(virDomainThrottleGroupDef) group = g_new0(virDomainThrottleGroupDef, 1); >>> + >>> + VIR_XPATH_NODE_AUTORESTORE(ctxt) >>> + ctxt->node = node; >>> + >>> + PARSE_THROTTLEGROUP(total_bytes_sec); >>> + PARSE_THROTTLEGROUP(read_bytes_sec); >>> + PARSE_THROTTLEGROUP(write_bytes_sec); >>> + PARSE_THROTTLEGROUP(total_iops_sec); >>> + PARSE_THROTTLEGROUP(read_iops_sec); >>> + PARSE_THROTTLEGROUP(write_iops_sec); >>> + >>> + PARSE_THROTTLEGROUP(total_bytes_sec_max); >>> + PARSE_THROTTLEGROUP(read_bytes_sec_max); >>> + PARSE_THROTTLEGROUP(write_bytes_sec_max); >>> + PARSE_THROTTLEGROUP(total_iops_sec_max); >>> + PARSE_THROTTLEGROUP(read_iops_sec_max); >>> + PARSE_THROTTLEGROUP(write_iops_sec_max); >>> + >>> + PARSE_THROTTLEGROUP(size_iops_sec); >>> + >>> + PARSE_THROTTLEGROUP(total_bytes_sec_max_length); >>> + PARSE_THROTTLEGROUP(read_bytes_sec_max_length); >>> + PARSE_THROTTLEGROUP(write_bytes_sec_max_length); >>> + PARSE_THROTTLEGROUP(total_iops_sec_max_length); >>> + PARSE_THROTTLEGROUP(read_iops_sec_max_length); >>> + PARSE_THROTTLEGROUP(write_iops_sec_max_length); >> I'd prefer if we had only one copy of the code parsing this and the >> equivalent disk throttling data so that the code doesn't need to be >> duplicated. The cleanup can be done as a followup though. > in v4 drafting, I am defining the following common macro to avoid > duplication of iterating all options in all parsing and formatting codes for > both iotune and throttlegroup: I was referring to the fact that the code that parses the old throttling XML is not refactored to use this new helper, thus keeping duplications. > #define FOR_EACH_IOTUNE_ULL_OPTION(process) \ >     process(total_bytes_sec) \ >     process(read_bytes_sec) \ >     process(write_bytes_sec) \ >     process(total_iops_sec) \ >     process(read_iops_sec) \ >     process(write_iops_sec) \ >     process(total_bytes_sec_max) \ >     process(read_bytes_sec_max) \ >     process(write_bytes_sec_max) \ >     process(total_iops_sec_max) \ >     process(read_iops_sec_max) \ >     process(write_iops_sec_max) \ >     process(size_iops_sec) \ >     process(total_bytes_sec_max_length) \ >     process(read_bytes_sec_max_length) \ >     process(write_bytes_sec_max_length) \ >     process(total_iops_sec_max_length) \ >     process(read_iops_sec_max_length) \ >     process(write_iops_sec_max_length) > > and define different "process" macro for parse and format, and this can be > reused by iotune and format by just calling e.g. > > FOR_EACH_IOTUNE_ULL_OPTION(PARSE_IOTUNE); > > FOR_EACH_IOTUNE_ULL_OPTION(FORMAT_IOTUNE); Please do not do this. Do not hide this any deeper into macros. It's hard to understand and harder to debug.