9:39 a.m.
On Mon, Dec 05, 2016 at 11:31:52AM +0100, Michal Privoznik wrote:
This module will be used to track:
<domain, mac address list>
pairs. It will be important to know these mappings without
libvirt connection (that is from a JSON file), because NSS
module will use those to provide better host name translation.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
po/POTFILES.in | 1 +
src/Makefile.am | 1 +
src/libvirt_private.syms | 9 +
src/util/virmacmap.c | 399 +++++++++++++++++++++++++++++++++++
src/util/virmacmap.h | 48 +++++
tests/Makefile.am | 14 ++
tests/virmacmapmock.c | 29 +++
tests/virmacmaptest.c | 232 ++++++++++++++++++++
tests/virmacmaptestdata/complex.json | 45 ++++
tests/virmacmaptestdata/empty.json | 3 +
tests/virmacmaptestdata/simple.json | 8 +
tests/virmacmaptestdata/simple2.json | 16 ++
12 files changed, 805 insertions(+)
create mode 100644 src/util/virmacmap.c
create mode 100644 src/util/virmacmap.h
create mode 100644 tests/virmacmapmock.c
create mode 100644 tests/virmacmaptest.c
create mode 100644 tests/virmacmaptestdata/complex.json
create mode 100644 tests/virmacmaptestdata/empty.json
create mode 100644 tests/virmacmaptestdata/simple.json
create mode 100644 tests/virmacmaptestdata/simple2.json
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9189f56fe..009a7b27c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1927,6 +1927,15 @@ virMacAddrSet;
virMacAddrSetRaw;
+# util/virmacmap.h
+virMACMapMgrAdd;
+virMACMapMgrFlush;
+virMACMapMgrFlushStr;
+virMACMapMgrLookup;
+virMACMapMgrNew;
+virMACMapMgrRemove;
+
What's the point of the "Mgr" part? It just adds mess to the name IMHO,
I'd like to see that removed.
+
# util/virnetdev.h
virNetDevAddMulti;
virNetDevDelMulti;
diff --git a/src/util/virmacmap.c b/src/util/virmacmap.c
new file mode 100644
index 000000000..38c2ffd1b
--- /dev/null
+++ b/src/util/virmacmap.c
[...]
+
+#define VIR_FROM_THIS VIR_FROM_NETWORK
+
VIR_FROM_NETWORK specifically says the error comes from the network
config. I'm not against using it, but it may be messy for users. Maybe
find a different one or just create a new one (although it's not the
many errors and they are very specific, so that might be a waste).
+static int
+virMACMapMgrRemoveLocked(virMACMapMgrPtr mgr,
+ const char *domain,
+ const char *mac)
+{
+ const char **macsList = NULL;
+ char **newMacsList = NULL;
+ int ret = -1;
+ int rv;
+
+ if (!(macsList = virHashLookup(mgr->macs, domain)))
+ return 0;
+
+ if (!virStringListHasString(macsList, mac)) {
+ ret = 0;
+ goto cleanup;
You have similar shortcut in virStringListRemove() already, I don't
think it's worth it. On the other hand, you are not checking for
multiple mac addresses for the same domain when adding it. Not that it
would cause any problem, but it would save more resources than this call
IMHO.
+int
+virMACMapMgrFlush(virMACMapMgrPtr mgr,
+ const char *filename)
+{
+ int ret;
+
+ virObjectLock(mgr);
+ ret = virMACMapMgrWriteFile(mgr, filename);
+ virObjectUnlock(mgr);
+ return ret;
+}
+
+
+int
+virMACMapMgrFlushStr(virMACMapMgrPtr mgr,
+ char **str)
+{
+ int ret;
+
+ virObjectLock(mgr);
+ ret = virMACMapMgrDumpStr(mgr, str);
+ virObjectUnlock(mgr);
+ return ret;
+}
So everything is named BlaBla(Locked) and these two are Flush and Dump
and WriteFile. I would prefer "WriteFile(Locked)" and
"DumpStr(Locked)" or something like that.
diff --git a/tests/virmacmaptest.c b/tests/virmacmaptest.c
new file mode 100644
index 000000000..a983b5495
--- /dev/null
+++ b/tests/virmacmaptest.c
[...]
+static int
+testMACLookup(const void *opaque)
+{
+ const struct testData *data = opaque;
+ virMACMapMgrPtr mgr = NULL;
+ const char * const * macs;
+ size_t i, j;
+ char *file = NULL;
+ int ret = -1;
+
+ if (virAsprintf(&file, "%s/virmacmaptestdata/%s.json",
+ abs_srcdir, data->file) < 0)
+ goto cleanup;
+
+ if (!(mgr = virMACMapMgrNew(file)))
+ goto cleanup;
+
+ macs = virMACMapMgrLookup(mgr, data->domain);
+
+ for (i = 0; macs && macs[i]; i++) {
+ for (j = 0; data->macs && data->macs[j]; j++) {
+ if (STREQ(macs[i], data->macs[j]))
+ break;
+ }
+
+ if (!data->macs || !data->macs[j]) {
+ fprintf(stderr,
+ "Unexpected %s in the returned list of MACs\n", macs[i]);
+ goto cleanup;
+ }
I think you meant to error out if (STRNEQ(macs[i], data->macs[j])),
otherwise you will say everything works fine if the first macs are the
same.
Maybe I misunderstood when reading 26K of code at once =)
+ }
+
+ for (i = 0; data->macs && data->macs[i]; i++) {
+ for (j = 0; macs && macs[j]; j++) {
+ if (STREQ(data->macs[i], macs[j]))
+ break;
+ }
+
+ if (!macs || !macs[j]) {
+ fprintf(stderr,
+ "Expected %s in the returned list of MACs\n",
data->macs[i]);
+ goto cleanup;
+ }
+ }
+