On Fri, Apr 14, 2017 at 06:01:46PM +0800, Eli Qiao wrote:
This is a RFC patch for the reimplement of `support cache tune(CAT) in libvirt`[1].
I searched for 'lock' in this mail and found nothing...
This patch defines some structs to represent data struct in linux resctrl fs which will be used later to do cache allocation. The patch expose a private interface `virResctrlFreeSchemata`, which will be used to query the cache allocation on the host. Also added unit test cases to test this interface can works well. There are already patch sets[2] to address it, and functional works, but people doesn't like it cause it has global variable, and missing unit test case for new added capabilites, etc. Martin has proposed a test infra to do vircaps2xmltest, and I extened it on top of it to extend resctrl control[3], this is kinds of new desiged apart from [2], so I propose this RFC patch to do some rework on it. [1] https://www.redhat.com/archives/libvir-list/2017-January/msg00683.html [2] https://www.redhat.com/archives/libvir-list/2017-March/msg00181.html [3] https://www.redhat.com/archives/libvir-list/2017-April/msg00516.html --- include/libvirt/virterror.h | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 6 + src/util/virerror.c | 1 + src/util/virresctrl.c | 423 ++++++++++++++++++++++++++++++ src/util/virresctrl.h | 86 ++++++ tests/Makefile.am | 7 +- tests/virresctrldata/L3-free.schemata | 1 + tests/virresctrldata/L3CODE-free.schemata | 1 + tests/virresctrldata/L3DATA-free.schemata | 1 + tests/virresctrltest.c | 117 +++++++++ 11 files changed, 644 insertions(+), 1 deletion(-) create mode 100644 src/util/virresctrl.c create mode 100644 src/util/virresctrl.h create mode 100644 tests/virresctrldata/L3-free.schemata create mode 100644 tests/virresctrldata/L3CODE-free.schemata create mode 100644 tests/virresctrldata/L3DATA-free.schemata create mode 100644 tests/virresctrltest.c diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index 2efee8f..4bc0c74 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -132,6 +132,7 @@ typedef enum { VIR_FROM_PERF = 65, /* Error from perf */ VIR_FROM_LIBSSH = 66, /* Error from libssh connection transport */ + VIR_FROM_RESCTRL = 67, /* Error from resctrl */ # ifdef VIR_ENUM_SENTINELS VIR_ERR_DOMAIN_LAST diff --git a/src/Makefile.am b/src/Makefile.am index 60eba37..0ae2af5 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -165,6 +165,7 @@ UTIL_SOURCES = \ util/virprocess.c util/virprocess.h \ util/virqemu.c util/virqemu.h \ util/virrandom.h util/virrandom.c \ + util/virresctrl.h util/virresctrl.c \ util/virrotatingfile.h util/virrotatingfile.c \ util/virscsi.c util/virscsi.h \ util/virscsihost.c util/virscsihost.h \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9d7760d..b7225fe 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2396,6 +2396,12 @@ virRandomGenerateWWN; virRandomInt; +# util/virresctrl.h +virResctrlFreeSchemata; +virResctrlGetFreeCache; +virResctrlTypeToString; + + # util/virrotatingfile.h virRotatingFileReaderConsume; virRotatingFileReaderFree; diff --git a/src/util/virerror.c b/src/util/virerror.c index ef17fb5..02fabcc 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -139,6 +139,7 @@ VIR_ENUM_IMPL(virErrorDomain, VIR_ERR_DOMAIN_LAST, "Perf", /* 65 */ "Libssh transport layer", + "Resource Control", ) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c new file mode 100644 index 0000000..778c2ec --- /dev/null +++ b/src/util/virresctrl.c @@ -0,0 +1,423 @@ +/* + * virresctrl.c: methods for managing resource control + * + * Copyright (C) 2017 Intel, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Eli Qiao <liyong.qiao@xxxxxxxxx> + */ + +#include <config.h> +#include <fcntl.h> +#include <sys/file.h> +#include <sys/stat.h> +#include <sys/types.h> + +#include "virresctrl.h" +#include "virerror.h" +#include "virlog.h" +#include "viralloc.h" +#include "virstring.h" +#include "virfile.h" + +VIR_LOG_INIT("util.resctrl"); + +#define VIR_FROM_THIS VIR_FROM_RESCTRL +#define SYSFS_RESCTRL_PATH "/sys/fs/resctrl/" + +VIR_ENUM_IMPL(virResctrl, VIR_RESCTRL_TYPE_LAST, + "L3", + "L3CODE", + "L3DATA", + "L2")
Is there CAT for L2 currently?
+
+/**
+ * a virResctrlDomain represents a resource control group, it's a directory
+ * under /sys/fs/resctrl.
+ * eg: /sys/fs/resctrl/CG1
+ * |-- cpus
+ * |-- schemata
+ * `-- tasks
+ * # cat schemata
+ * L3DATA:0=fffff;1=fffff
+ * L3CODE:0=fffff;1=fffff
+ *
+ * Besides, it can also represent the default resource control group of the
+ * host.
+ */
+
+typedef struct _virResctrlGroup virResctrlGroup;
+typedef virResctrlGroup *virResctrlGroupPtr;
+struct _virResctrlGroup {
+ char *name; /* resource group name, eg: CG1. If it represent host's
s/eg/e.g./, but I feel the example is unnecessary
+ default resource group name, should be a NULL pointer */
Just "NULL for the default host group" is more than enough.
+ size_t n_tasks; /* number of task assigned to the resource group */
s/task /tasks/
+ char **tasks; /* task list which contains task id eg: 77454 */ +
Why is this list of strings? Hopefully I'll see that after I burrow down a few more lines.
+ size_t n_schematas; /* number of schemata the resource group contains, + eg: 2 */
again, no need for
+ virResctrlSchemataPtr *schematas; /* scheamta list */
'schemata' is plural, there is no such thing as 'schematas'.
+}; + +/* All resource control groups on this host, including default resource group */ +typedef struct _virResctrlDomain virResctrlDomain; +typedef virResctrlDomain *virResctrlDomainPtr;
If they are on the host, why is "Domain" in the name?
+struct _virResctrlDomain {
+ size_t n_groups; /* number of resource control group */
+ virResctrlGroupPtr *groups; /* list of resource control group */
+};
+
+void
+virResctrlFreeSchemata(virResctrlSchemataPtr ptr)
+{
+ size_t i;
+
+ if (!ptr)
+ return;
+
+ for (i = 0; i < ptr->n_schemata_items; i++)
+ VIR_FREE(ptr->schemata_items[i]);
Who is freeing ptr->schemata_items ? Also, it is common to free the @ptr here as well.
+}
+
+static void
+virResctrlFreeGroup(virResctrlGroupPtr ptr)
+{
+ size_t i;
+
+ if (!ptr)
+ return;
+
+ for (i = 0; i < ptr->n_tasks; i++)
+ VIR_FREE(ptr->tasks[i]);
if it needs to be strings (which I still doubt), then just use virStringList* functions.
+
+ for (i = 0; i < ptr->n_schematas; i++) {
+ virResctrlFreeSchemata(ptr->schematas[i]);
+ VIR_FREE(ptr->schematas[i]);
+ }
Who is freeing ptr->schematas ? Also same here with the @ptr
+}
+
+static void
+virResctrlFreeDomain(virResctrlDomainPtr ptr)
+{
+ size_t i;
+
+ if (!ptr)
+ return;
+
+ for (i = 0; i < ptr->n_groups; i++) {
+ virResctrlFreeGroup(ptr->groups[i]);
+ VIR_FREE(ptr->groups[i]);
+ }
Who is freeing ptr->groups ? Also same here with the @ptr
+}
+
+static int
+virResctrlCopySchemata(virResctrlSchemataPtr src,
+ virResctrlSchemataPtr *dst)
+{
+ size_t i;
+ virResctrlSchemataItemPtr schemataitem;
+ virResctrlSchemataPtr schemata;
schemata_item? It's really hard to read all this. So let's get some things straight. Each group can have some schemata (plural of schema/scheme). Each schema can then have multiple allocations (one per cache id or host socket or whatever is that). So if one schema is one line of the 'schemata' file, then 'group' should have 'schemata', 'schema' (singular) can then have 'items' or 'masks' or whatever. I know it sounds like bikeshedding, but it's really hard to review this way. Maybe just removing the plurals would make it more readable.
+
+ if (VIR_ALLOC(schemata) < 0)
+ return -1;
+
+ schemata->type = src->type;
+
+ for (i = 0; i < src->n_schemata_items; i++) {
+ if (VIR_ALLOC(schemataitem) < 0)
+ goto error;
+
+ schemataitem->cache_id = src->schemata_items[i]->cache_id;
+ schemataitem->continuous_schemata = src->schemata_items[i]->continuous_schemata;
+ schemataitem->schemata = src->schemata_items[i]->schemata;
What I don't get is how come 'schemataitem' can have 'schemata' in it. /That also reminds me I haven't seen Inception in a while, where's my spinning top, BTW/
+ schemataitem->size = src->schemata_items[i]->size; + + if (VIR_APPEND_ELEMENT(schemata->schemata_items, + schemata->n_schemata_items, + schemataitem) < 0)
Too many reallocations. You already know how many of them you have. Just VIR_ALLOC_N all of them, and then just do items[i] = src->items[i]. There will also be no need for checking errors every cycle. ... I just came back from a bunch of lines below. If there will be something allocated inside this item, then you can't do the simple assignment, so you can create a function for deep-copying this struct as well.
+ goto error; + } + + *dst = schemata; + + return 0; + + error: + virResctrlFreeSchemata(schemata);
Due to the way you did the *Free() functions, you leak 'schemata' here.
+ return -1;
+}
+
+static int
+virResctrlGetSchemataString(virResctrlType type,
+ const char *name,
+ char **schemata)
+{
+ int rc = -1;
+ char *tmp = NULL;
+ char *end = NULL;
+ char *buf = NULL;
+ char *type_suffix = NULL;
+
+ if (virFileReadValueString(&buf,
+ SYSFS_RESCTRL_PATH "%s/schemata",
+ name ? name : "") < 0)
+ return -1;
+
+ if (virAsprintf(&type_suffix,
+ "%s:",
+ virResctrlTypeToString(type)) < 0)
+ goto cleanup;
+
+ tmp = strstr(buf, type_suffix);
+
You are doing all this ^^ for every schema libvirt knows. How about reading the file once, doing virStringSplit() to split lines and then just parsing each line? Or while you are not keeping the lines anywhere, you can just simply walk the whole file in one or two loops over the read string, that might be a bit more readable.
+ if (!tmp)
+ goto cleanup;
+
+ end = strchr(tmp, '\n');
+ if (end != NULL)
+ *end = '\0';
+
+ if (VIR_STRDUP(*schemata, tmp) < 0)
+ goto cleanup;
+
+ rc = 0;
+
+ cleanup:
+ VIR_FREE(buf);
+ VIR_FREE(type_suffix);
+ return rc;
+}
+
+static int
+virResctrlLoadSchemata(const char* schemata_str,
+ virResctrlSchemataPtr schemata)
+{
+ VIR_DEBUG("%s, %p\n", schemata_str, schemata);
+
+ int ret = -1;
+ char **lists = NULL;
+ char **sms = NULL;
+ char **sis = NULL;
Again, I know it sounds like bikeshedding, but these variable names...
+ size_t i; + virResctrlSchemataItemPtr si; + + /* parse L3:0=fffff;1=f */ + lists = virStringSplit(schemata_str, ":", 2); +
Hint: If you are doing 'virStringSplit(..., 2)', then you probably want
to just use strchr().
+ if ((!lists) || (!lists[1]))
+ goto cleanup;
+
+ /* parse 0=fffff;1=f */
+ sms = virStringSplit(lists[1], ";", 0);
+ if (!sms)
+ goto cleanup;
+
+ for (i = 0; sms[i] != NULL; i++) {
+ /* parse 0=fffff */
+ sis = virStringSplit(sms[i], "=", 2);
I already mentioned what's wrong here.
+ if (!sis) + goto cleanup; + + if (VIR_ALLOC(si) < 0) + goto cleanup; + + if (virStrToLong_ui(sis[0], NULL, 10, &si->cache_id) < 0) + goto cleanup; + + if (virStrToLong_ui(sis[1], NULL, 16, &si->continuous_schemata) < 0) + goto cleanup;
Indentation of gotos ^^ Also, why is the mask saved as unsigned long? Wouldn't keeping it as virBitmap make more sense?
+
+ si->schemata = si->continuous_schemata;
+
+ if (VIR_APPEND_ELEMENT(schemata->schemata_items,
+ schemata->n_schemata_items,
+ si) < 0)
+ goto cleanup;
+
+ }
+
+ ret = 0;
+
+ cleanup:
+ VIR_FREE(si);
+ virStringListFree(lists);
+ virStringListFree(sms);
+ virStringListFree(sis);
+ return ret;
+}
+
+static int
+virResctrlLoadGroup(const char *name,
+ virResctrlDomainPtr dom)
+{
+ VIR_DEBUG("%s, %p\n", name, dom);
+
At least name=%s, dom=%p
+ int ret = -1; + char *path = NULL; + char *schemata_str; + virResctrlType i; + int rv; + virResctrlGroupPtr grp; + virResctrlSchemataPtr schemata; + + if ((virAsprintf(&path, "%s/%s", SYSFS_RESCTRL_PATH, name)) < 0)
Why so many parentheses?
+ return -1; + + if (!virFileExists(path))
You can check '-2' as a return value of virFileReadValue*() if you use the approach for parsing I mentioned above.
+ goto cleanup;
+
+ if (VIR_ALLOC(grp) < 0)
+ goto cleanup;
+
+ if (VIR_STRDUP(grp->name, name) < 0)
+ goto cleanup;
+
+ for (i = 0; i < VIR_RESCTRL_TYPE_LAST; i++) {
+ rv = virResctrlGetSchemataString(i, name, &schemata_str);
+ if (rv < 0)
+ continue;
+
+ if (VIR_ALLOC(schemata) < 0)
+ goto cleanup;
+
+ schemata->type = i;
+
+ if (virResctrlLoadSchemata(schemata_str, schemata) < 0)
+ goto cleanup;
You leak schemata if this function fails. And schemata_str.
+ + VIR_FREE(schemata_str); + + if (VIR_APPEND_ELEMENT(grp->schematas, + grp->n_schematas, + schemata) < 0) + goto cleanup; + + virResctrlFreeSchemata(schemata);
I think you meant:
if (VIR_APPEND_ELEMENT(grp->schematas, grp->n_schematas, schemata) < 0) {
virResctrlFreeSchemata(schemata);
goto cleanup;
}
Right? This way you are freeing everything in the schemata that you
just appended to the group. This does not crash for you?
+ } + + if (VIR_APPEND_ELEMENT(dom->groups, + dom->n_groups, + grp) < 0) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(path); + virResctrlFreeGroup(grp);
Same here. This really works for you?
+ return ret;
+}
+
+static int
+virResctrlLoadDomain(virResctrlDomainPtr dom)
+{
+ int ret = -1;
+ int rv = -1;
+ DIR *dirp = NULL;
+ char *path = NULL;
+ struct dirent *ent;
+
+ VIR_DEBUG("%s, %p\n", "", dom);
+
+ rv = virDirOpenIfExists(&dirp, SYSFS_RESCTRL_PATH);
+
+ if (rv < 0)
+ goto cleanup;
+
+ /* load default resctrl group */
+ if (virResctrlLoadGroup("", dom) < 0)
+ goto cleanup;
+
+ while ((rv = virDirRead(dirp, &ent, path)) > 0) {
+ /* only read directory in resctrl */
+ if ((ent->d_type != DT_DIR) || STREQ(ent->d_name, "info"))
+ continue;
+
Isn't the resctrl hierarchical? Maybe it was supposed to be and it changed. I can't seem to recall that right now.
+ if (virResctrlLoadGroup(ent->d_name, dom) < 0)
+ goto cleanup;
+ }
+
+ ret = 0;
+
+ cleanup:
+ virDirClose(&dirp);
+ return ret;
+}
+
+static void
+virResctrlRefreshDom(virResctrlDomainPtr dom, virResctrlType type)
+{
+ size_t i;
+ size_t j;
+ size_t k;
+
+ virResctrlGroupPtr default_grp = NULL;
+ virResctrlGroupPtr grp = NULL;
+ virResctrlSchemataPtr schemata = NULL;
+ virResctrlSchemataItemPtr schemataitem = NULL;
+
+ default_grp = dom->groups[0];
+
+ /* We are sure that the first group is the default one */
In case that could also be mentioned when parsing them, just so any future rework knows some other code in the tree depends on it.
+ for (i = 1; i < dom->n_groups; i++) {
+ grp = dom->groups[i];
+ for (j = 0; j < grp->n_schematas; j++) {
+ schemata = grp->schematas[j];
+ /* we can only calculate one type of schemata */
+ if (schemata->type != type)
+ continue;
+ for (k = 0; k < schemata->n_schemata_items; k++) {
+ schemataitem = schemata->schemata_items[k];
+ /* if the schemata = 1, ignore it */
Any explanation for that?
+ if (schemataitem->continuous_schemata > 1) + /* calculate default schemata, it can be non-continuous */ + default_grp->schematas[j]->schemata_items[k]->schemata &= ~(schemataitem->continuous_schemata);
Please elaborate on what are you trying to do here.
+ }
+ }
+ }
+}
+
+int virResctrlGetFreeCache(virResctrlType type,
+ virResctrlSchemataPtr *schemata)
+{
+ VIR_DEBUG("%d, %p\n", type, schemata);
+ int ret = -1;
+ size_t i;
+ virResctrlDomainPtr dom = NULL;
+ virResctrlGroupPtr grp = NULL;
+
+ if (VIR_ALLOC(dom) < 0)
+ return -1;
+
+ if (virResctrlLoadDomain(dom) < 0)
+ goto cleanup;
+
+ virResctrlRefreshDom(dom, type);
+ grp = dom->groups[0];
+
+ for (i = 0; i < grp->n_schematas; i ++)
+ if (grp->schematas[i]->type == type)
+ if (virResctrlCopySchemata(grp->schematas[i], schemata) < 0)
+ goto cleanup;
Brackets around multiline 'if' and 'for' bodies. Also if you find the right one you still loop until you went through all of them. Luckily there should not be two types. Otherwise this would also be a leak.
+ + if (schemata != NULL) + ret = 0;
You don't need the whole Copy stuff. Since you are just copying something you are going to free anyway, you can just steal the pointer and return it. no need to return 0/-1, no need to do a deep-copy, it will get much simpler.
+ + cleanup: + virResctrlFreeDomain(dom); + return ret; +} diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h new file mode 100644 index 0000000..1b040d4 --- /dev/null +++ b/src/util/virresctrl.h @@ -0,0 +1,86 @@ +/* + * virresctrl.h: header for managing resctrl control + * + * Copyright (C) 2017 Intel, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Eli Qiao <liyong.qiao@xxxxxxxxx> + */ + +#ifndef __VIR_RESCTRL_H__ +# define __VIR_RESCTRL_H__ + +#include "virutil.h" + +typedef enum { + VIR_RESCTRL_TYPE_L3, + VIR_RESCTRL_TYPE_L3_CODE, + VIR_RESCTRL_TYPE_L3_DATA, + VIR_RESCTRL_TYPE_L2, + + VIR_RESCTRL_TYPE_LAST +} virResctrlType; + +VIR_ENUM_DECL(virResctrl); + +/* + * a virResctrlSchemataItem represents one of schemata object in a + * resource control group. + * eg: 0=f
schemata object? that's pretty vague
+ */
+typedef struct _virResctrlSchemataItem virResctrlSchemataItem;
+typedef virResctrlSchemataItem *virResctrlSchemataItemPtr;
+struct _virResctrlSchemataItem {
+ unsigned int cache_id; /* cache resource id, eg: 0 */
+ unsigned int continuous_schemata; /* schemata, should be a continuous bits,
+ eg: f, this schemata can be persisted
+ to sysfs */
+ unsigned int schemata; /* schemata eg: f0f, a schemata which is calculated
+ at running time */
I still don't understand the difference between continuous_schemata and schemata. Even after reading this.
+ unsigned long long size; /* the cache size schemata represented in B, + eg: (min * bits of continuous_schemata) */
I don't see it used anywhere. Just copied in one place.
+};
+
+/*
+ * a virResctrlSchemata represents schemata objects of specific type of
+ * resource in a resource control group.
+ * eg: L3:0=f,1=ff
+ */
+typedef struct _virResctrlSchemata virResctrlSchemata;
+typedef virResctrlSchemata *virResctrlSchemataPtr;
+struct _virResctrlSchemata {
+ virResctrlType type; /* resource control type, eg: L3 */
+ size_t n_schemata_items; /* number of schemata item, eg: 2 */
+ virResctrlSchemataItemPtr *schemata_items; /* pointer list of schemata item */
+};
+
+/* Get free cache of the host, result saved in schemata */
+int virResctrlGetFreeCache(virResctrlType type,
+ virResctrlSchemataPtr *schemata);
+
+
+/* TODO Need to first define virDomainCachetunePtr */
+/* Set cache allocation for a VM domain */
+// int virResctrlSetCacheBanks(virDomainCachetunePtr cachetune,
+// unsigned char *group_name,
+// size_t n_pids,
+// pid_t *pids);
+//
+/* remove cache allocation for a VM domain */
+// int virResctrlRemoveCacheBanks(unsigned char *group_name);
Don't add stuff you don't use in this patch unless it is 'uncomment and compile' type of thing that just shows how stuff should be used in the future in case of implementing something we don't yet have. Also we don't use // comments (especially not in new code, there are only few lines left from some old days, I believe)
+void virResctrlFreeSchemata(virResctrlSchemataPtr ptr); +#endif diff --git a/tests/Makefile.am b/tests/Makefile.am index 3cc828d..0e09e43 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -229,6 +229,7 @@ if WITH_LINUX test_programs += fchosttest test_programs += scsihosttest test_programs += vircaps2xmltest +test_programs += virresctrltest test_libraries += virusbmock.la \ virnetdevbandwidthmock.la \ virnumamock.la \ @@ -1150,6 +1151,10 @@ vircaps2xmltest_SOURCES = \ vircaps2xmltest.c testutils.h testutils.c virfilemock.c vircaps2xmltest_LDADD = $(LDADDS) +virresctrltest_SOURCES = \ + virresctrltest.c testutils.h testutils.c virfilemock.c +virresctrltest_LDADD = $(LDADDS) + virnumamock_la_SOURCES = \ virnumamock.c virnumamock_la_CFLAGS = $(AM_CFLAGS) @@ -1157,7 +1162,7 @@ virnumamock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS) virnumamock_la_LIBADD = $(MOCKLIBS_LIBS) else ! WITH_LINUX -EXTRA_DIST += vircaps2xmltest.c virnumamock.c +EXTRA_DIST += vircaps2xmltest.c virresctrltest.c virnumamock.c
Yet another thing that's weird to review. Normally, I would expect people to add stuff at the end or in alphabetical order. When I look there, I see no difference and I'm rescanning the line. Why not just add it at the end?
endif ! WITH_LINUX if WITH_NSS diff --git a/tests/virresctrldata/L3-free.schemata b/tests/virresctrldata/L3-free.schemata new file mode 100644 index 0000000..9b47d25 --- /dev/null +++ b/tests/virresctrldata/L3-free.schemata @@ -0,0 +1 @@ +L3:0=1ffff;1=1ffff diff --git a/tests/virresctrldata/L3CODE-free.schemata b/tests/virresctrldata/L3CODE-free.schemata new file mode 100644 index 0000000..7039c45 --- /dev/null +++ b/tests/virresctrldata/L3CODE-free.schemata @@ -0,0 +1 @@ +L3CODE:0=cffff;1=cffff diff --git a/tests/virresctrldata/L3DATA-free.schemata b/tests/virresctrldata/L3DATA-free.schemata new file mode 100644 index 0000000..30f1cbd --- /dev/null +++ b/tests/virresctrldata/L3DATA-free.schemata @@ -0,0 +1 @@ +L3DATA:0=3ffff;1=3ffff diff --git a/tests/virresctrltest.c b/tests/virresctrltest.c new file mode 100644 index 0000000..4926468 --- /dev/null +++ b/tests/virresctrltest.c @@ -0,0 +1,117 @@ +/* + * Copyright (C) Intel, Inc. 2017 + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Eli Qiao <liyong.qiao@xxxxxxxxx> + */ + +#include <config.h> +#include <stdlib.h> + +#include "testutils.h" +#include "virbitmap.h" +#include "virfilemock.h" +#include "virresctrl.h" + + +#define VIR_FROM_THIS VIR_FROM_NONE + +struct virResctrlData { + const char *filename; + virResctrlType type; +}; + +static void +GetSchemataStr(virResctrlSchemataPtr schemata, char **str)
Is this the only function in libvirt starting with uppercase letter?
+{
+ size_t i;
Empty line after variable declarations makes it nicer to read. But that's just my weird thing.
+ virBuffer buf = VIR_BUFFER_INITIALIZER;
+ virBufferAsprintf(&buf, "%s:%u=%x",
+ virResctrlTypeToString(schemata->type),
+ schemata->schemata_items[0]->cache_id,
+ schemata->schemata_items[0]->schemata);
+
+ for (i = 1; i < schemata->n_schemata_items; i ++)
+ virBufferAsprintf(&buf, ";%u=%x",
+ schemata->schemata_items[i]->cache_id,
+ schemata->schemata_items[i]->schemata);
+
+ *str = virBufferContentAndReset(&buf);
+}
+
+static int
+test_virResctrl(const void *opaque)
+{
+ struct virResctrlData *data = (struct virResctrlData *) opaque;
+ char *dir = NULL;
+ char *resctrl = NULL;
+ int ret = -1;
+ virResctrlSchemataPtr schemata = NULL;
+ char *schemata_str;
+ char *schemata_file;
+
+ if (virAsprintf(&resctrl, "%s/virresctrldata/linux-%s/resctrl",
No such file is added anywhere in this patch.
+ abs_srcdir, data->filename) < 0)
+ goto cleanup;
+
+ if (virAsprintf(&schemata_file, "%s/virresctrldata/%s-free.schemata",
+ abs_srcdir, virResctrlTypeToString(data->type)) < 0)
+ goto cleanup;
+
+ virFileMockAddPrefix("/sys/fs/resctrl", resctrl);
+
+ if (virResctrlGetFreeCache(data->type, &schemata) < 0)
+ goto cleanup;
+
+ GetSchemataStr(schemata, &schemata_str);
+
+ if (virTestCompareToFile(schemata_str, schemata_file) < 0)
+ goto cleanup;
+
+ virFileMockClearPrefixes();
+
+ ret = 0;
+
+ cleanup:
+ VIR_FREE(dir);
+ VIR_FREE(resctrl);
+ VIR_FREE(schemata_str);
+ virResctrlFreeSchemata(schemata);
+ return ret;
+}
+
+static int
+mymain(void)
+{
+ int ret = 0;
+
+#define DO_TEST_FULL(filename, type) \
+ do { \
+ struct virResctrlData data = {filename, \
+ type}; \
+ if (virTestRun(filename, test_virResctrl, &data) < 0) \
+ ret = -1; \
+ } while (0)
+
+ DO_TEST_FULL("resctrl", VIR_RESCTRL_TYPE_L3);
+ DO_TEST_FULL("resctrl-cdp", VIR_RESCTRL_TYPE_L3_CODE);
+ DO_TEST_FULL("resctrl-cdp", VIR_RESCTRL_TYPE_L3_DATA);
+
So this really does not fail for you? Maybe check that you haven't missed adding some files into git's index?
+ return ret; +} + +VIR_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/virnumamock.so")
What is the need for this?
-- 1.9.1
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
