On Tue, Dec 06, 2016 at 11:11:09AM +0100, Michal Privoznik wrote:
On 05.12.2016 16:39, Martin Kletzander wrote: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.Well, now everything in libvirt is a manager of something :-) But okay, I will remove it. I doesn't make much sense after all. Also, virMacMap or virMACMap? I think in other similar cases we stick with the former one (even though MAC is an abbrev.)
Our current naming would probably go well with MAC, but I don't really care if there's no Mgr.
+ # 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).Since this is used from a network context I think it's okay if error messages look like going from that direction.
Yeah, it's now used mostly in network_conf, so that's fine.
+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 true. I am checking for that. But since I'm reworking virStringListRemove() anyway I will remove this check, but not the one from virMacMapAddLocked().
OK
Not that it would cause any problem, but it would save more resources than this call IMHO.Agreed. That's exactly why I am checking for duplicates when adding a MAC address.+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.Yeah, my name generator brain submodule has gone for holidays while writing these patches.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.And that's exactly what I want to say. I mean, I have two sting lists: @macs and @data->macs. In this loop I am iterating over @macs trying to find the same string in @data->macs. If I haven't found anything, virMACMapMgrLookup returned unexpected MAC address and thus I error out. NB strings in @macs and @data->macs don't have to be in the same order.
Yes, looking at it now it makes total sense, that was just a brainfart, I guess.
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; + } + } +Then in here I check whether all strings from @data->macs occur in @macs too. If they do, that is all strings from @macs occur in @data->macs (checked in the first loop), and also all strings from @data->macs occur in @macs (checked here), the only possible reason is that both lists I've started with are the same. Michal
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
