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.
