On Tue, Apr 11, 2017 at 09:48:54AM +0200, Peter Krempa wrote:
On Tue, Apr 11, 2017 at 13:44:26 +0800, Eli Qiao wrote:This patch is based on Martin's cache branch. * This patch amends the cache bank capability as follow: <cache> <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'> <control min='768' unit='KiB' scope='BOTH' max_allocation='4'/> </bank> <bank id='1' level='3' type='unified' size='15360' unit='KiB' cpus='6-11'> <control min='768' unit='KiB' scope='BOTH' max_allocation='4'/> </bank> </cache> For CDP enabled on x86 arch, we will have: <cache> <bank id='0' level='3' type='unified' size='15360' unit='KiB' cpus='0-5'> <control min='768' unit='KiB' scope='CODE' max_allocation='4'/> <control min='768' unit='KiB' scope='DATA' max_allocation='4'/> </bank> ... * Added a new testdata directory `linux-resctrl-cdp` to test CDP enabled case. * Along with vircaps2xmltest case updated. P.S. here the scope is from /sys/fs/resctrl/info/L3{"" "CODE" "DATA"}, I keep it capital upper as I need to use a macro to convert from enum to string easily.We dont need to do that. The attributes should be lower case. The code can convert it to anything it needs.Signed-off-by: Eli Qiao <liyong.qiao@xxxxxxxxx> ---
[...]
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 416dd1a..c6641d1 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c
[...]
@@ -894,13 +898,38 @@ virCapabilitiesFormatCaches(virBufferPtr buf, */ virBufferAsprintf(buf, "<bank id='%u' level='%u' type='%s' " - "size='%llu' unit='%s' cpus='%s'/>\n", + "size='%llu' unit='%s' cpus='%s'", bank->id, bank->level, virCacheTypeToString(bank->type), bank->size >> (kilos * 10), kilos ? "KiB" : "B", cpus_str); + /* Format control */ + virBufferAdjustIndent(&controlBuf, indent + 4);This looks wrong. You should increase/decrease the indent by some number. The old value should not be used.
This is used everywhere in the code with childrenBuf, it's pretty widely used and I think readable as well. I agree with the rest of the review, though.
+ for (j = 0; j < bank->ncontrols; j++) { + bool min_kilos = !(bank->controls[j]->min % 1024); + virBufferAsprintf(&controlBuf, + "<control min='%llu' unit='%s' " + "scope='%s' max_allocation='%u'/>\n", + bank->controls[j]->min >> (min_kilos * 10), + min_kilos ? "KiB" : "B", + virResctrlCacheTypeToString(bank->controls[j]->scope), + bank->controls[j]->max_allocation); + } + + /* Put it all together */You don't need to point out obvious things.+ if (virBufferUse(&controlBuf)) {This logic looks weird and opaque. Can you decide by any other means than the filling of the buffer?
Same here.
+ virBufferAddLit(buf, ">\n"); + virBufferAddBuffer(buf, &controlBuf); + virBufferAddLit(buf, "</bank>\n"); + + } else { + virBufferAddLit(buf, "/>\n"); + } + + + virBufferFreeAndReset(&controlBuf); VIR_FREE(cpus_str); }
[...]
diff --git a/tests/vircaps2xmltest.c b/tests/vircaps2xmltest.c index f590249..db7de4c 100644 --- a/tests/vircaps2xmltest.c +++ b/tests/vircaps2xmltest.c @@ -58,6 +59,13 @@ test_virCapabilities(const void *opaque) data->resctrl ? "/system" : "") < 0) goto cleanup; + if (virAsprintf(&resctrl, "%s/vircaps2xmldata/linux-%s/resctrl", + abs_srcdir, + data->resctrl ? data->filename : "foo") < 0)"foo"? Some testing code leftover?
This should just be: + if (virAsprintf(&resctrl, "%s/vircaps2xmldata/linux-%s/resctrl", + abs_srcdir, data->filename) < 0) I think I said that in v3
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
