<meta http-equiv="Content-Type" content="text/html; charset=utf-8"><div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:large"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">ср, 3 мар. 2021 г. в 17:06, Peter Krempa <<a href="mailto:pkrempa@redhat.com">pkrempa@redhat.com</a>>:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Mon, Jan 11, 2021 at 12:50:09 +0300, Nikolay Shirokovskiy wrote:<br>
> At first glance we don't get much win because of introduction of<br>
> virDomainBlockIoTuneFieldNames and virDomainBlockIoTuneFields. But we are going<br>
> to use these two in other places to remove usage of macros too.<br>
> <br>
> Signed-off-by: Nikolay Shirokovskiy <<a href="mailto:nshirokovskiy@virtuozzo.com" target="_blank">nshirokovskiy@virtuozzo.com</a>><br>
> ---<br>
> src/conf/domain_conf.c | 99 +++++++++++++++++++++++++++++++++++---------------<br>
> 1 file changed, 69 insertions(+), 30 deletions(-)<br>
> <br>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c<br>
> index 800bca5..024d0e3 100644<br>
> --- a/src/conf/domain_conf.c<br>
> +++ b/src/conf/domain_conf.c<br>
> @@ -8695,40 +8695,80 @@ virDomainBlockIoTuneValidate(virDomainBlockIoTuneInfoPtr iotune)<br>
> return 0;<br>
> }<br>
> <br>
> -#define PARSE_IOTUNE(val) \<br>
> - if (virXPathULongLong("string(./iotune/" #val ")", \<br>
> - ctxt, &def->blkdeviotune.val) == -2) { \<br>
> - virReportError(VIR_ERR_XML_ERROR, \<br>
> - _("disk iotune field '%s' must be an integer"), #val); \<br>
> - return -1; \<br>
> - }<br>
> +<br>
> +static const char* virDomainBlockIoTuneFieldNames[] = {<br>
> + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC,<br>
> + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC,<br>
> + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC,<br>
> + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC,<br>
> + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC,<br>
> + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC,<br>
> + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX,<br>
> + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX,<br>
> + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX,<br>
> + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX,<br>
> + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX,<br>
> + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX,<br>
> + VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC,<br>
> + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH,<br>
> + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH,<br>
> + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH,<br>
> + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH,<br>
> + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH,<br>
> + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH,<br>
> +};<br>
> +<br>
> +<br>
> +static unsigned long long**<br>
> +virDomainBlockIoTuneFields(virDomainBlockIoTuneInfoPtr iotune)<br>
> +{<br>
> + unsigned long long **ret = g_new0(unsigned long long*,<br>
> + G_N_ELEMENTS(virDomainBlockIoTuneFieldNames));<br>
> + size_t i = 0;<br>
> +<br>
> + ret[i++] = &iotune->total_bytes_sec;<br>
> + ret[i++] = &iotune->read_bytes_sec;<br>
> + ret[i++] = &iotune->write_bytes_sec;<br>
> + ret[i++] = &iotune->total_iops_sec;<br>
> + ret[i++] = &iotune->read_iops_sec;<br>
> + ret[i++] = &iotune->write_iops_sec;<br>
> + ret[i++] = &iotune->total_bytes_sec_max;<br>
> + ret[i++] = &iotune->read_bytes_sec_max;<br>
> + ret[i++] = &iotune->write_bytes_sec_max;<br>
> + ret[i++] = &iotune->total_iops_sec_max;<br>
> + ret[i++] = &iotune->read_iops_sec_max;<br>
> + ret[i++] = &iotune->write_iops_sec_max;<br>
> + ret[i++] = &iotune->size_iops_sec;<br>
> + ret[i++] = &iotune->total_bytes_sec_max_length;<br>
> + ret[i++] = &iotune->read_bytes_sec_max_length;<br>
> + ret[i++] = &iotune->write_bytes_sec_max_length;<br>
> + ret[i++] = &iotune->total_iops_sec_max_length;<br>
> + ret[i++] = &iotune->read_iops_sec_max_length;<br>
> + ret[i++] = &iotune->write_iops_sec_max_length;<br>
> +<br>
> + return ret;<br>
> +}<br>
> +<br>
> <br>
> static int<br>
> virDomainDiskDefIotuneParse(virDomainDiskDefPtr def,<br>
> xmlXPathContextPtr ctxt)<br>
> {<br>
> - PARSE_IOTUNE(total_bytes_sec);<br>
> - PARSE_IOTUNE(read_bytes_sec);<br>
> - PARSE_IOTUNE(write_bytes_sec);<br>
> - PARSE_IOTUNE(total_iops_sec);<br>
> - PARSE_IOTUNE(read_iops_sec);<br>
> - PARSE_IOTUNE(write_iops_sec);<br>
> -<br>
> - PARSE_IOTUNE(total_bytes_sec_max);<br>
> - PARSE_IOTUNE(read_bytes_sec_max);<br>
> - PARSE_IOTUNE(write_bytes_sec_max);<br>
> - PARSE_IOTUNE(total_iops_sec_max);<br>
> - PARSE_IOTUNE(read_iops_sec_max);<br>
> - PARSE_IOTUNE(write_iops_sec_max);<br>
> -<br>
> - PARSE_IOTUNE(size_iops_sec);<br>
> -<br>
> - PARSE_IOTUNE(total_bytes_sec_max_length);<br>
> - PARSE_IOTUNE(read_bytes_sec_max_length);<br>
> - PARSE_IOTUNE(write_bytes_sec_max_length);<br>
> - PARSE_IOTUNE(total_iops_sec_max_length);<br>
> - PARSE_IOTUNE(read_iops_sec_max_length);<br>
> - PARSE_IOTUNE(write_iops_sec_max_length);<br>
> + g_autofree unsigned long long **fields =<br>
> + virDomainBlockIoTuneFields(&def->blkdeviotune);<br>
> + size_t i;<br>
> +<br>
> + for (i = 0; i < G_N_ELEMENTS(virDomainBlockIoTuneFieldNames); i++) {<br>
> + const char *name = virDomainBlockIoTuneFieldNames[i];<br>
> + g_autofree char *sel = g_strdup_printf("string(./iotune/%s)", name);<br>
> +<br>
> + if (virXPathULongLong(sel, ctxt, fields[i]) == -2) {<br>
> + virReportError(VIR_ERR_XML_ERROR,<br>
> + _("disk iotune field '%s' must be an integer"),<br>
> + name);<br>
> + return -1;<br>
> + }<br>
> + }<br>
> <br>
> def->blkdeviotune.group_name =<br>
> virXPathString("string(./iotune/group_name)", ctxt);<br>
<br>
IMO this is worse than we had before. I'm especially not a fan of<br>
correlating arrays into named fields and the parser is actually harder<br>
to understand.<br>
<br>
Let's see the other places you are describing, but I don't think you can<br>
offset the damage done by correlating two arrays.<br>
<br></blockquote><div><br></div><div class="gmail_default" style="font-size:large">Hi, Peter.</div><div class="gmail_default" style="font-size:large"><br></div><div class="gmail_default" style="font-size:large">So it is finally a NACK as you don't say anything about this approach in later patches?</div><div class="gmail_default" style="font-size:large">Maybe there are other options to get rid of macros?</div><div class="gmail_default" style="font-size:large"><br></div><div class="gmail_default" style="font-size:large">At least I can factor out functions with macros to reuse them in qemu and test driver.</div><div class="gmail_default" style="font-size:large"><br></div><div class="gmail_default" style="font-size:large">I'll then make suggested changes for acked patches. Do I need to repost them or I can just push?</div><div class="gmail_default" style="font-size:large"><br></div><div class="gmail_default" style="font-size:large">Nikolay</div></div></div>