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>
---
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>
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);
}
[...]