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

[Martin Kletzander]

On Tue, Nov 14, 2017 at 07:38:50AM -0500, John Ferlan wrote:
>
>
>On 11/13/2017 03:50 AM, Martin Kletzander wrote:
>> Signed-off-by: Martin Kletzander <mkletzan at 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 at 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];"
>

done

>> -            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 I add STRNEQ there it doesn't allow me to remove any code, it would just add
a line.  I need the math after that anyway.

>> +            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);
>>          }
>[...]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20171114/a4d8291d/attachment-0001.sig>