Re: [libvirt] [PATCH 11/21] conf: Format cache banks in capabilities with virPrettySize

On 11/13/2017 03:50 AM, Martin Kletzander wrote: > Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; > --- > src/conf/capabilities.c | 45 ++++++++++++++-------- > tests/vircaps2xmldata/vircaps-x86_64-caches.xml | 2 +- > .../vircaps2xmldata/vircaps-x86_64-resctrl-cdp.xml | 4 +- > .../vircaps2xmldata/vircaps-x86_64-resctrl-skx.xml | 4 +- > tests/vircaps2xmldata/vircaps-x86_64-resctrl.xml | 4 +- > 5 files changed, 36 insertions(+), 23 deletions(-) > Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John couple of noisy review thoughts below... > diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c > index 095ef51c424a..5bf8ac2019f9 100644 > --- a/src/conf/capabilities.c > +++ b/src/conf/capabilities.c > @@ -883,7 +883,8 @@ virCapabilitiesFormatCaches(virBufferPtr buf, > for (i = 0; i < ncaches; i++) { > virCapsHostCacheBankPtr bank = caches[i]; > char *cpus_str = virBitmapFormat(bank->cpus); > - bool kilos = !(bank->size % 1024); > + const char *unit = NULL; > + unsigned long long short_size = virPrettySize(bank->size, &unit); > > if (!cpus_str) > return -1; > @@ -897,32 +898,44 @@ virCapabilitiesFormatCaches(virBufferPtr buf, > "size='%llu' unit='%s' cpus='%s'", > bank->id, bank->level, > virCacheTypeToString(bank->type), > - bank->size >> (kilos * 10), > - kilos ? "KiB" : "B", > - cpus_str); > + short_size, unit, cpus_str); > VIR_FREE(cpus_str); > > virBufferSetChildIndent(&controlBuf, buf); > for (j = 0; j < bank->ncontrols; j++) { You could have "virResctrlInfoPtr controls = bank->controls[j];"

> - bool min_kilos = !(bank->controls[j]->granularity % 1024); > - > - /* Only use KiB if both values are divisible */ > - if (bank->controls[j]->min) > - min_kilos = min_kilos && !(bank->controls[j]->min % 1024); > + const char *min_unit; > + unsigned long long gran_short_size = bank->controls[j]->granularity; > + unsigned long long min_short_size = bank->controls[j]->min; > + > + gran_short_size = virPrettySize(gran_short_size, &unit); > + min_short_size = virPrettySize(min_short_size, &min_unit); > + > + /* Only use the smaller unit if they are different */ Or "if (STRNEQ(unit, min_unit))" - to be more faithful to the comment! I read this as - if min_short_size is there, then we check the unit by knowing the math to get the value. To some degree if the pretty format function allowed one to "choose" a specific format size that'd perhaps work too, but that's perhaps more work than it's worth.

> + if (min_short_size) { > + unsigned long long gran_div; > + unsigned long long min_div; > + > + gran_div = bank->controls[j]->granularity / gran_short_size; > + min_div = bank->controls[j]->min / min_short_size; > + > + if (min_div > gran_div) { > + min_short_size *= min_div / gran_div; > + } else if (min_div < gran_div) { > + unit = min_unit; > + gran_short_size *= gran_div / min_div; > + } > + } > > virBufferAsprintf(&controlBuf, > "<control granularity='%llu'", > - bank->controls[j]->granularity >> (min_kilos * 10)); > + gran_short_size); > > - if (bank->controls[j]->min) { > - virBufferAsprintf(&controlBuf, > - " min='%llu'", > - bank->controls[j]->min >> (min_kilos * 10)); > - } > + if (min_short_size) > + virBufferAsprintf(&controlBuf, " min='%llu'", min_short_size); > > virBufferAsprintf(&controlBuf, > " unit='%s' type='%s' maxAllocs='%u'/>\n", > - min_kilos ? "KiB" : "B", > + unit, > virCacheTypeToString(bank->controls[j]->scope), > bank->controls[j]->max_allocation); > } [...]