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@xxxxxxxxxx> --- 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;
+ }
+ }
+
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
