[libvirt] [PATCH v2 00/10] Introduce new libvirt-guest NSS module

v2 of: https://www.redhat.com/archives/libvir-list/2016-November/msg01456.html diff to v1: - New module is created instead of putting everything into already existing one - Few hints from review of v1 were worked in Michal Privoznik (10): network: Don't unlock non-locked network driver nssmock: Prefer free() over VIR_FREE() virstring: Introduce virStringListAdd virstring: Introduce virStringListRemove util: Introduce virFileRewriteStr util: Introduce virMACMap module network: Track MAC address map nss: Use macro to generate public API names nss: Move address appending code into a separate function nss: Introduce libvirt-guest module cfg.mk | 2 +- docs/news.html.in | 4 + docs/nss.html.in | 58 ++++- po/POTFILES.in | 1 + src/Makefile.am | 9 + src/conf/network_conf.h | 4 + src/conf/virsecretobj.c | 20 +- src/libvirt_private.syms | 12 ++ src/network/bridge_driver.c | 95 ++++++++- src/network/leaseshelper.c | 14 +- src/util/virfile.c | 24 ++- src/util/virfile.h | 7 +- src/util/virmacmap.c | 399 +++++++++++++++++++++++++++++++++++ src/util/virmacmap.h | 48 +++++ src/util/virstring.c | 84 ++++++++ src/util/virstring.h | 6 + src/util/virxml.c | 4 +- tests/Makefile.am | 36 +++- tests/nssdata/virbr0.macs | 23 ++ tests/nssdata/virbr0.status | 5 + tests/nssdata/virbr1.macs | 21 ++ tests/nssdata/virbr1.status | 5 + tests/nsslinktest.c | 2 +- tests/nssmock.c | 25 ++- tests/nsstest.c | 17 +- 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 ++ tests/virstringtest.c | 98 +++++++++ tools/Makefile.am | 46 +++- tools/nss/libvirt_guest_nss.syms | 12 ++ tools/nss/libvirt_nss.c | 216 +++++++++++++------ tools/nss/libvirt_nss.h | 30 +-- 36 files changed, 1531 insertions(+), 129 deletions(-) create mode 100644 src/util/virmacmap.c create mode 100644 src/util/virmacmap.h create mode 100644 tests/nssdata/virbr0.macs create mode 100644 tests/nssdata/virbr1.macs 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 create mode 100644 tools/nss/libvirt_guest_nss.syms -- 2.11.0

In dd7bfb2cdc5d I've removed locking of the network driver upon it's allocation. However, I forgot to remove one location of the driver unlock. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 9d94d65b4..073f5019a 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -718,8 +718,6 @@ networkStateInitialize(bool privileged, return ret; error: - if (network_driver) - networkDriverUnlock(network_driver); networkStateCleanup(); goto cleanup; } -- 2.11.0

Problem with VIR_FREE() is that we are not linking libvirt-utils.so to our mock libs therefore there will be an unresolved symbol. Fortunately, nsstest that eventually links with the nssmock links also with libvirt-utils.so and thus the symbol is resolved after all. However, if one wants to run the test binary under valgrind it is impossible to do so. Because of the unresolved symbol. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 2 +- tests/nssmock.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cfg.mk b/cfg.mk index a4305a808..69e3f3a1a 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1159,7 +1159,7 @@ exclude_file_name_regexp--sc_prohibit_select = \ ^cfg\.mk$$ exclude_file_name_regexp--sc_prohibit_raw_allocation = \ - ^(docs/hacking\.html\.in|src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|vircgroupmock)\.c|tools/wireshark/src/packet-libvirt\.c)$$ + ^(docs/hacking\.html\.in|src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|(vircgroup|nss)mock)\.c|tools/wireshark/src/packet-libvirt\.c)$$ exclude_file_name_regexp--sc_prohibit_readlink = \ ^src/(util/virutil|lxc/lxc_container)\.c$$ diff --git a/tests/nssmock.c b/tests/nssmock.c index b0259a377..273af0654 100644 --- a/tests/nssmock.c +++ b/tests/nssmock.c @@ -91,7 +91,7 @@ open(const char *path, int flags, ...) ret = real_open(newpath ? newpath : path, flags); } - VIR_FREE(newpath); + free(newpath); return ret; } @@ -109,7 +109,7 @@ opendir(const char *path) ret = real_opendir(newpath ? newpath : path); - VIR_FREE(newpath); + free(newpath); return ret; } #else -- 2.11.0

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstring.c | 34 ++++++++++++++++++++++++++++++++++ src/util/virstring.h | 3 +++ tests/virstringtest.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 80 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 43220e065..bc6588969 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2460,6 +2460,7 @@ virStringEncodeBase64; virStringHasControlChars; virStringIsEmpty; virStringIsPrintable; +virStringListAdd; virStringListFree; virStringListFreeCount; virStringListGetFirstWithPrefix; diff --git a/src/util/virstring.c b/src/util/virstring.c index 740cf4d23..629f8ca32 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -168,6 +168,40 @@ char *virStringListJoin(const char **strings, } +/** + * virStringListAdd: + * @strings: a NULL-terminated array of strings + * @item: string to add + * + * Creates new strings list with all strings duplicated and @item + * at the end of the list. Callers is responsible for freeing + * both @strings and returned list. + */ +char ** +virStringListAdd(const char **strings, + const char *item) +{ + char **ret = NULL; + size_t i = virStringListLength(strings); + + if (VIR_ALLOC_N(ret, i + 2) < 0) + goto error; + + for (i = 0; strings && strings[i]; i++) { + if (VIR_STRDUP(ret[i], strings[i]) < 0) + goto error; + } + + if (VIR_STRDUP(ret[i], item) < 0) + goto error; + + return ret; + error: + virStringListFree(ret); + return NULL; +} + + /** * virStringListFree: * @str_array: a NULL-terminated array of strings to free diff --git a/src/util/virstring.h b/src/util/virstring.h index f6801fcd1..da9d35cca 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -41,6 +41,9 @@ char *virStringListJoin(const char **strings, const char *delim) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +char **virStringListAdd(const char **strings, + const char *item); + void virStringListFree(char **strings); void virStringListFreeCount(char **strings, size_t count); diff --git a/tests/virstringtest.c b/tests/virstringtest.c index a11d7c5d8..43657c84c 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -122,6 +122,46 @@ static int testJoin(const void *args) return ret; } + +static int testAdd(const void *args) +{ + const struct testJoinData *data = args; + char **list = NULL; + char *got = NULL; + int ret = -1; + size_t i; + + for (i = 0; data->tokens[i]; i++) { + char **tmp = virStringListAdd((const char **)list, data->tokens[i]); + if (!tmp) + goto cleanup; + virStringListFree(list); + list = tmp; + tmp = NULL; + } + + if (!list && + VIR_ALLOC(list) < 0) + goto cleanup; + + if (!(got = virStringListJoin((const char **)list, data->delim))) { + VIR_DEBUG("Got no result"); + goto cleanup; + } + + if (STRNEQ(got, data->string)) { + virFilePrintf(stderr, "Mismatch '%s' vs '%s'\n", got, data->string); + goto cleanup; + } + + ret = 0; + cleanup: + virStringListFree(list); + VIR_FREE(got); + return ret; +} + + static bool fail; static const char * @@ -594,6 +634,8 @@ mymain(void) ret = -1; \ if (virTestRun("Join " #str, testJoin, &joinData) < 0) \ ret = -1; \ + if (virTestRun("Add " #str, testAdd, &joinData) < 0) \ + ret = -1; \ } while (0) const char *tokens1[] = { NULL }; -- 2.11.0

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstring.c | 50 ++++++++++++++++++++++++++++++++++++++++++ src/util/virstring.h | 3 +++ tests/virstringtest.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 110 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bc6588969..3d4da7356 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2467,6 +2467,7 @@ virStringListGetFirstWithPrefix; virStringListHasString; virStringListJoin; virStringListLength; +virStringListRemove; virStringReplace; virStringSearch; virStringSortCompare; diff --git a/src/util/virstring.c b/src/util/virstring.c index 629f8ca32..96786db7f 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -202,6 +202,56 @@ virStringListAdd(const char **strings, } +/** + * virStringListRemove: + * @strings: a NULL-terminated array of strings + * @newStrings: new NULL-terminated array of strings + * @item: string to remove + * + * Creates new strings list with all strings duplicated except + * for every occurrence of @item. Callers is responsible for + * freeing both @strings and returned list. + * + * Returns the number of items in the new list (excluding NULL + * anchor), -1 on error. + */ +int +virStringListRemove(const char **strings, + char ***newStrings, + const char *item) +{ + char **ret = NULL; + size_t i, j = 0; + + for (i = 0; strings && strings[i]; i++) { + if (STRNEQ(strings[i], item)) + j++; + } + + if (!j) { + *newStrings = NULL; + return 0; + } + + if (VIR_ALLOC_N(ret, j + 1) < 0) + goto error; + + for (i = 0, j = 0; strings[i]; i++) { + if (STREQ(strings[i], item)) + continue; + if (VIR_STRDUP(ret[j], strings[i]) < 0) + goto error; + j++; + } + + *newStrings = ret; + return j; + error: + virStringListFree(ret); + return -1; +} + + /** * virStringListFree: * @str_array: a NULL-terminated array of strings to free diff --git a/src/util/virstring.h b/src/util/virstring.h index da9d35cca..88eacd404 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -43,6 +43,9 @@ char *virStringListJoin(const char **strings, char **virStringListAdd(const char **strings, const char *item); +int virStringListRemove(const char **strings, + char ***newStrings, + const char *item); void virStringListFree(char **strings); void virStringListFreeCount(char **strings, diff --git a/tests/virstringtest.c b/tests/virstringtest.c index 43657c84c..63a5e90db 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -162,6 +162,60 @@ static int testAdd(const void *args) } +static int testRemove(const void *args) +{ + const struct testSplitData *data = args; + char **list = NULL; + size_t ntokens; + size_t i; + int ret = -1; + size_t listLenght; + + if (!(list = virStringSplitCount(data->string, data->delim, + data->max_tokens, &ntokens))) { + VIR_DEBUG("Got no tokens at all"); + return -1; + } + + listLenght = virStringListLength((const char **) list); + + for (i = 0; data->tokens[i]; i++) { + char **tmp; + int rv; + size_t j, toRemove = 0; + + for (j = 0; list && list[j]; j++) + if (STREQ(list[j], data->tokens[i])) + toRemove++; + + if ((rv = virStringListRemove((const char **) list, + &tmp, data->tokens[i])) < 0) + goto cleanup; + virStringListFree(list); + list = tmp; + tmp = NULL; + listLenght -= toRemove; + + if (rv != listLenght) { + virFilePrintf(stderr, + "Unexpected length of new list: %d expected %zu", + rv, listLenght); + goto cleanup; + } + } + + if (list && list[0]) { + virFilePrintf(stderr, "Not removed all tokens: %s", list[0]); + goto cleanup; + } + + ret = 0; + cleanup: + virStringListFree(list); + return ret; +} + + static bool fail; static const char * @@ -636,6 +690,8 @@ mymain(void) ret = -1; \ if (virTestRun("Add " #str, testAdd, &joinData) < 0) \ ret = -1; \ + if (virTestRun("Remove " #str, testRemove, &splitData) < 0) \ + ret = -1; \ } while (0) const char *tokens1[] = { NULL }; -- 2.11.0

On Mon, Dec 05, 2016 at 11:31:50AM +0100, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstring.c | 50 ++++++++++++++++++++++++++++++++++++++++++ src/util/virstring.h | 3 +++ tests/virstringtest.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 110 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bc6588969..3d4da7356 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2467,6 +2467,7 @@ virStringListGetFirstWithPrefix; virStringListHasString; virStringListJoin; virStringListLength; +virStringListRemove; virStringReplace; virStringSearch; virStringSortCompare; diff --git a/src/util/virstring.c b/src/util/virstring.c index 629f8ca32..96786db7f 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -202,6 +202,56 @@ virStringListAdd(const char **strings, }
+/** + * virStringListRemove: + * @strings: a NULL-terminated array of strings + * @newStrings: new NULL-terminated array of strings + * @item: string to remove + * + * Creates new strings list with all strings duplicated except + * for every occurrence of @item. Callers is responsible for + * freeing both @strings and returned list. + * + * Returns the number of items in the new list (excluding NULL + * anchor), -1 on error. + */ +int +virStringListRemove(const char **strings, + char ***newStrings, + const char *item) +{ + char **ret = NULL; + size_t i, j = 0; + + for (i = 0; strings && strings[i]; i++) { + if (STRNEQ(strings[i], item)) + j++; + } + + if (!j) { + *newStrings = NULL; + return 0; + } + + if (VIR_ALLOC_N(ret, j + 1) < 0) + goto error; + + for (i = 0, j = 0; strings[i]; i++) { + if (STREQ(strings[i], item)) + continue; + if (VIR_STRDUP(ret[j], strings[i]) < 0) + goto error; + j++; + } +
Instead of going twice through the list, you can just do it once and then VIR_REALLOC_N the list.

On 05.12.2016 16:23, Martin Kletzander wrote:
On Mon, Dec 05, 2016 at 11:31:50AM +0100, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstring.c | 50 ++++++++++++++++++++++++++++++++++++++++++ src/util/virstring.h | 3 +++ tests/virstringtest.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 110 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bc6588969..3d4da7356 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2467,6 +2467,7 @@ virStringListGetFirstWithPrefix; virStringListHasString; virStringListJoin; virStringListLength; +virStringListRemove; virStringReplace; virStringSearch; virStringSortCompare; diff --git a/src/util/virstring.c b/src/util/virstring.c index 629f8ca32..96786db7f 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -202,6 +202,56 @@ virStringListAdd(const char **strings, }
+/** + * virStringListRemove: + * @strings: a NULL-terminated array of strings + * @newStrings: new NULL-terminated array of strings + * @item: string to remove + * + * Creates new strings list with all strings duplicated except + * for every occurrence of @item. Callers is responsible for + * freeing both @strings and returned list. + * + * Returns the number of items in the new list (excluding NULL + * anchor), -1 on error. + */ +int +virStringListRemove(const char **strings, + char ***newStrings, + const char *item) +{ + char **ret = NULL; + size_t i, j = 0; + + for (i = 0; strings && strings[i]; i++) { + if (STRNEQ(strings[i], item)) + j++; + } + + if (!j) { + *newStrings = NULL; + return 0; + } + + if (VIR_ALLOC_N(ret, j + 1) < 0) + goto error; + + for (i = 0, j = 0; strings[i]; i++) { + if (STREQ(strings[i], item)) + continue; + if (VIR_STRDUP(ret[j], strings[i]) < 0) + goto error; + j++; + } +
Instead of going twice through the list, you can just do it once and then VIR_REALLOC_N the list.
That would have the same time complexity - I'd need to use memmove() which iterates over the memory too. But it would have better memory complexity. Okay, will do in v3. Michal

There are couple of places where we have a string and want to save it to a file. Atomically. In all those places we use virFileRewrite() but also implement the very same callback which takes the string and write it into temp file. This makes no sense. Unify the callbacks and move them to one place. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virsecretobj.c | 20 ++------------------ src/libvirt_private.syms | 1 + src/network/leaseshelper.c | 14 +------------- src/util/virfile.c | 24 +++++++++++++++++++++++- src/util/virfile.h | 7 +++++-- src/util/virxml.c | 4 ++-- 6 files changed, 34 insertions(+), 36 deletions(-) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 2bdfe08ea..1351f18b2 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -692,20 +692,6 @@ virSecretObjDeleteData(virSecretObjPtr secret) has virSecretDef stored as XML in "$basename.xml". If a value of the secret is defined, it is stored as base64 (with no formatting) in "$basename.base64". "$basename" is in both cases the base64-encoded UUID. */ - -static int -virSecretRewriteFile(int fd, - void *opaque) -{ - char *data = opaque; - - if (safewrite(fd, data, strlen(data)) < 0) - return -1; - - return 0; -} - - int virSecretObjSaveConfig(virSecretObjPtr secret) { @@ -715,8 +701,7 @@ virSecretObjSaveConfig(virSecretObjPtr secret) if (!(xml = virSecretDefFormat(secret->def))) goto cleanup; - if (virFileRewrite(secret->configFile, S_IRUSR | S_IWUSR, - virSecretRewriteFile, xml) < 0) + if (virFileRewriteStr(secret->configFile, S_IRUSR | S_IWUSR, xml) < 0) goto cleanup; ret = 0; @@ -739,8 +724,7 @@ virSecretObjSaveData(virSecretObjPtr secret) if (!(base64 = virStringEncodeBase64(secret->value, secret->value_size))) goto cleanup; - if (virFileRewrite(secret->base64File, S_IRUSR | S_IWUSR, - virSecretRewriteFile, base64) < 0) + if (virFileRewriteStr(secret->base64File, S_IRUSR | S_IWUSR, base64) < 0) goto cleanup; ret = 0; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3d4da7356..9189f56fe 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1601,6 +1601,7 @@ virFileRemoveLastComponent; virFileResolveAllLinks; virFileResolveLink; virFileRewrite; +virFileRewriteStr; virFileSanitizePath; virFileSkipRoot; virFileStripSuffix; diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c index 16f6eb87b..a0262dd07 100644 --- a/src/network/leaseshelper.c +++ b/src/network/leaseshelper.c @@ -64,17 +64,6 @@ usage(int status) exit(status); } -static int -customLeaseRewriteFile(int fd, void *opaque) -{ - char **data = opaque; - - if (safewrite(fd, *data, strlen(*data)) < 0) - return -1; - - return 0; -} - /* Flags denoting actions for a lease */ enum virLeaseActionFlags { VIR_LEASE_ACTION_ADD, /* Create new lease */ @@ -252,8 +241,7 @@ main(int argc, char **argv) } /* Write to file */ - if (virFileRewrite(custom_lease_file, 0644, - customLeaseRewriteFile, &leases_str) < 0) + if (virFileRewriteStr(custom_lease_file, 0644, leases_str) < 0) goto cleanup; break; diff --git a/src/util/virfile.c b/src/util/virfile.c index fcd0d9288..1f0bfa906 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -443,7 +443,7 @@ int virFileRewrite(const char *path, mode_t mode, virFileRewriteFunc rewrite, - void *opaque) + const void *opaque) { char *newfile = NULL; int fd = -1; @@ -494,6 +494,28 @@ virFileRewrite(const char *path, } +static int +virFileRewriteStrHelper(int fd, const void *opaque) +{ + const char *data = opaque; + + if (safewrite(fd, data, strlen(data)) < 0) + return -1; + + return 0; +} + + +int +virFileRewriteStr(const char *path, + mode_t mode, + const char *str) +{ + return virFileRewrite(path, mode, + virFileRewriteStrHelper, str); +} + + int virFileTouch(const char *path, mode_t mode) { int fd = -1; diff --git a/src/util/virfile.h b/src/util/virfile.h index 836cc602c..5b810956e 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -101,11 +101,14 @@ void virFileWrapperFdFree(virFileWrapperFdPtr dfd); int virFileLock(int fd, bool shared, off_t start, off_t len, bool waitForLock); int virFileUnlock(int fd, off_t start, off_t len); -typedef int (*virFileRewriteFunc)(int fd, void *opaque); +typedef int (*virFileRewriteFunc)(int fd, const void *opaque); int virFileRewrite(const char *path, mode_t mode, virFileRewriteFunc rewrite, - void *opaque); + const void *opaque); +int virFileRewriteStr(const char *path, + mode_t mode, + const char *str); int virFileTouch(const char *path, mode_t mode); diff --git a/src/util/virxml.c b/src/util/virxml.c index 96c17fa9c..666024809 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -871,9 +871,9 @@ struct virXMLRewriteFileData { }; static int -virXMLRewriteFile(int fd, void *opaque) +virXMLRewriteFile(int fd, const void *opaque) { - struct virXMLRewriteFileData *data = opaque; + const struct virXMLRewriteFileData *data = opaque; if (data->warnCommand) { if (virXMLEmitWarning(fd, data->warnName, data->warnCommand) < 0) -- 2.11.0

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@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/po/POTFILES.in b/po/POTFILES.in index bdff67906..b0a1ed401 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -213,6 +213,7 @@ src/util/virkeyfile.c src/util/virlease.c src/util/virlockspace.c src/util/virlog.c +src/util/virmacmap.c src/util/virnetdev.c src/util/virnetdevbandwidth.c src/util/virnetdevbridge.c diff --git a/src/Makefile.am b/src/Makefile.am index d440548be..a9106fa97 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -137,6 +137,7 @@ UTIL_SOURCES = \ util/virlockspace.c util/virlockspace.h \ util/virlog.c util/virlog.h \ util/virmacaddr.h util/virmacaddr.c \ + util/virmacmap.h util/virmacmap.c \ util/virnetdev.h util/virnetdev.c \ util/virnetdevbandwidth.h util/virnetdevbandwidth.c \ util/virnetdevbridge.h util/virnetdevbridge.c \ 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; + + # 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 @@ -0,0 +1,399 @@ +/* + * virmacmap.c: MAC address <-> Domain name mapping + * + * Copyright (C) 2016 Red Hat, 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: + * Michal Privoznik <mprivozn@redhat.com> + */ + +#include <config.h> + +#include "virmacmap.h" +#include "virobject.h" +#include "virlog.h" +#include "virjson.h" +#include "virfile.h" +#include "virhash.h" +#include "virstring.h" +#include "viralloc.h" + +#define VIR_FROM_THIS VIR_FROM_NETWORK + +VIR_LOG_INIT("util.virmacmap"); + +/** + * VIR_MAC_MAP_FILE_SIZE_MAX: + * + * Macro providing the upper limit on the size of mac maps file + */ +#define VIR_MAC_MAP_FILE_SIZE_MAX (32 * 1024 * 1024) + +struct virMACMapMgr { + virObjectLockable parent; + + virHashTablePtr macs; +}; + + +static virClassPtr virMACMapMgrClass; + + +static void +virMACMapMgrDispose(void *obj) +{ + virMACMapMgrPtr mgr = obj; + virHashFree(mgr->macs); +} + + +static void +virMACMapMgrHashFree(void *payload, const void *name ATTRIBUTE_UNUSED) +{ + virStringListFree(payload); +} + + +static int virMACMapMgrOnceInit(void) +{ + if (!(virMACMapMgrClass = virClassNew(virClassForObjectLockable(), + "virMACMapMgrClass", + sizeof(virMACMapMgr), + virMACMapMgrDispose))) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virMACMapMgr); + + +static int +virMACMapMgrAddLocked(virMACMapMgrPtr mgr, + const char *domain, + const char *mac) +{ + int ret = -1; + const char **macsList = NULL; + char **newMacsList = NULL; + + if ((macsList = virHashLookup(mgr->macs, domain)) && + virStringListHasString(macsList, mac)) { + ret = 0; + goto cleanup; + } + + if (!(newMacsList = virStringListAdd(macsList, mac)) || + virHashUpdateEntry(mgr->macs, domain, newMacsList) < 0) + goto cleanup; + newMacsList = NULL; + + ret = 0; + cleanup: + virStringListFree(newMacsList); + return ret; +} + + +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; + } + + rv = virStringListRemove(macsList, &newMacsList, mac); + if (rv < 0) { + goto cleanup; + } else if (rv == 0) { + virHashRemoveEntry(mgr->macs, domain); + } else { + if (virHashUpdateEntry(mgr->macs, domain, newMacsList) < 0) + goto cleanup; + } + newMacsList = NULL; + + ret = 0; + cleanup: + virStringListFree(newMacsList); + return ret; +} + + +static int +virMACMapMgrLoadFile(virMACMapMgrPtr mgr, + const char *file) +{ + char *map_str = NULL; + virJSONValuePtr map = NULL; + int map_str_len = 0; + size_t i; + int ret = -1; + + if (virFileExists(file) && + (map_str_len = virFileReadAll(file, + VIR_MAC_MAP_FILE_SIZE_MAX, + &map_str)) < 0) + goto cleanup; + + if (map_str_len == 0) { + ret = 0; + goto cleanup; + } + + if (!(map = virJSONValueFromString(map_str))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid json in file: %s"), + file); + goto cleanup; + } + + if (!virJSONValueIsArray(map)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Malformed file structure: %s"), + file); + goto cleanup; + } + + for (i = 0; i < virJSONValueArraySize(map); i++) { + virJSONValuePtr tmp = virJSONValueArrayGet(map, i); + virJSONValuePtr macs; + const char *domain; + size_t j; + + if (!(domain = virJSONValueObjectGetString(tmp, "domain"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing domain")); + goto cleanup; + } + + if (!(macs = virJSONValueObjectGetArray(tmp, "macs"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing macs")); + goto cleanup; + } + + for (j = 0; j < virJSONValueArraySize(macs); j++) { + virJSONValuePtr macJSON = virJSONValueArrayGet(macs, j); + const char *mac = virJSONValueGetString(macJSON); + + if (virMACMapMgrAddLocked(mgr, domain, mac) < 0) + goto cleanup; + } + } + + ret = 0; + cleanup: + VIR_FREE(map_str); + virJSONValueFree(map); + return ret; +} + + +static int +virMACMapHashDumper(void *payload, + const void *name, + void *data) +{ + virJSONValuePtr obj = NULL; + virJSONValuePtr arr = NULL; + const char **macs = payload; + size_t i; + int ret = -1; + + if (!(obj = virJSONValueNewObject()) || + !(arr = virJSONValueNewArray())) + goto cleanup; + + for (i = 0; macs[i]; i++) { + virJSONValuePtr m = virJSONValueNewString(macs[i]); + + if (!m || + virJSONValueArrayAppend(arr, m) < 0) { + virJSONValueFree(m); + goto cleanup; + } + } + + if (virJSONValueObjectAppendString(obj, "domain", name) < 0 || + virJSONValueObjectAppend(obj, "macs", arr) < 0) + goto cleanup; + arr = NULL; + + if (virJSONValueArrayAppend(data, obj) < 0) + goto cleanup; + obj = NULL; + + ret = 0; + cleanup: + virJSONValueFree(obj); + virJSONValueFree(arr); + return ret; +} + + +static int +virMACMapMgrDumpStr(virMACMapMgrPtr mgr, + char **str) +{ + virJSONValuePtr arr; + int ret = -1; + + if (!(arr = virJSONValueNewArray())) + goto cleanup; + + if (virHashForEach(mgr->macs, virMACMapHashDumper, arr) < 0) + goto cleanup; + + if (!(*str = virJSONValueToString(arr, true))) + goto cleanup; + + ret = 0; + cleanup: + virJSONValueFree(arr); + return ret; +} + + +static int +virMACMapMgrWriteFile(virMACMapMgrPtr mgr, + const char *file) +{ + char *str; + int ret = -1; + + if (virMACMapMgrDumpStr(mgr, &str) < 0) + goto cleanup; + + if (virFileRewriteStr(file, 0644, str) < 0) + goto cleanup; + + ret = 0; + cleanup: + VIR_FREE(str); + return ret; +} + + +#define VIR_MAC_HASH_TABLE_SIZE 10 + +virMACMapMgrPtr +virMACMapMgrNew(const char *file) +{ + virMACMapMgrPtr mgr; + + if (virMACMapMgrInitialize() < 0) + return NULL; + + if (!(mgr = virObjectLockableNew(virMACMapMgrClass))) + return NULL; + + virObjectLock(mgr); + if (!(mgr->macs = virHashCreate(VIR_MAC_HASH_TABLE_SIZE, + virMACMapMgrHashFree))) + goto error; + + if (file && + virMACMapMgrLoadFile(mgr, file) < 0) + goto error; + + virObjectUnlock(mgr); + return mgr; + + error: + virObjectUnlock(mgr); + virObjectUnref(mgr); + return NULL; +} + + +int +virMACMapMgrAdd(virMACMapMgrPtr mgr, + const char *domain, + const char *mac) +{ + int ret; + + virObjectLock(mgr); + ret = virMACMapMgrAddLocked(mgr, domain, mac); + virObjectUnlock(mgr); + return ret; +} + + +int +virMACMapMgrRemove(virMACMapMgrPtr mgr, + const char *domain, + const char *mac) +{ + int ret; + + virObjectLock(mgr); + ret = virMACMapMgrRemoveLocked(mgr, domain, mac); + virObjectUnlock(mgr); + return ret; +} + + +const char *const * +virMACMapMgrLookup(virMACMapMgrPtr mgr, + const char *domain) +{ + const char *const *ret; + + virObjectLock(mgr); + ret = virHashLookup(mgr->macs, domain); + virObjectUnlock(mgr); + return ret; +} + + +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; +} diff --git a/src/util/virmacmap.h b/src/util/virmacmap.h new file mode 100644 index 000000000..6ebba7cb9 --- /dev/null +++ b/src/util/virmacmap.h @@ -0,0 +1,48 @@ +/* + * virmacmap.h: MAC address <-> Domain name mapping + * + * Copyright (C) 2016 Red Hat, 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: + * Michal Privoznik <mprivozn@redhat.com> + */ + +#ifndef __VIR_MACMAP_H__ +# define __VIR_MACMAP_H__ + +typedef struct virMACMapMgr virMACMapMgr; +typedef virMACMapMgr *virMACMapMgrPtr; + +virMACMapMgrPtr virMACMapMgrNew(const char *file); + +int virMACMapMgrAdd(virMACMapMgrPtr mgr, + const char *domain, + const char *mac); + +int virMACMapMgrRemove(virMACMapMgrPtr mgr, + const char *domain, + const char *mac); + +const char *const *virMACMapMgrLookup(virMACMapMgrPtr mgr, + const char *domain); + +int virMACMapMgrFlush(virMACMapMgrPtr mgr, + const char *filename); + +int virMACMapMgrFlushStr(virMACMapMgrPtr mgr, + char **str); +#endif /* __VIR_MACMAPPING_H__ */ diff --git a/tests/Makefile.am b/tests/Makefile.am index 6a24861ff..9d5583d43 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -144,6 +144,7 @@ EXTRA_DIST = \ vircgroupdata \ virconfdata \ virfiledata \ + virmacmaptestdata \ virmock.h \ virnetdaemondata \ virnetdevtestdata \ @@ -191,6 +192,7 @@ test_programs = virshtest sockettest \ domainconftest \ virhostdevtest \ vircaps2xmltest \ + virmacmaptest \ virnetdevtest \ virtypedparamtest \ $(NULL) @@ -406,6 +408,7 @@ test_libraries = libshunload.la \ virhostcpumock.la \ nssmock.la \ domaincapsmock.la \ + virmacmapmock.la \ $(NULL) if WITH_QEMU test_libraries += libqemumonitortestutils.la \ @@ -1137,6 +1140,17 @@ nsslinktest_CFLAGS = \ nsslinktest_LDADD = ../tools/nss/libnss_libvirt_impl.la nsslinktest_LDFLAGS = $(NULL) +virmacmapmock_la_SOURCES = \ + virmacmapmock.c +virmacmapmock_la_CFLAGS = $(AM_CFLAGS) +virmacmapmock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS) +virmacmapmock_la_LIBADD = $(MOCKLIBS_LIBS) + +virmacmaptest_SOURCES = \ + virmacmaptest.c testutils.h testutils.c +virmacmaptest_CLFAGS = $(AM_CFLAGS); +virmacmaptest_LDADD = $(LDADDS) + virnetdevtest_SOURCES = \ virnetdevtest.c testutils.h testutils.c virnetdevtest_CFLAGS = $(AM_CFLAGS) $(LIBNL_CFLAGS) diff --git a/tests/virmacmapmock.c b/tests/virmacmapmock.c new file mode 100644 index 000000000..d01f1c9e4 --- /dev/null +++ b/tests/virmacmapmock.c @@ -0,0 +1,29 @@ +/* + * Copyright (C) 2016 Red Hat, 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/>. + * + * Author: Michal Privoznik <mprivozn@redhat.com> + */ + +#include <config.h> + +#include "virrandom.h" + +uint64_t virRandomBits(int nbits ATTRIBUTE_UNUSED) +{ + return 4; /* chosen by fair dice roll. + guaranteed to be random. */ +} diff --git a/tests/virmacmaptest.c b/tests/virmacmaptest.c new file mode 100644 index 000000000..a983b5495 --- /dev/null +++ b/tests/virmacmaptest.c @@ -0,0 +1,232 @@ +/* + * Copyright (C) 2016 Red Hat, 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/>. + * + * Author: Michal Privoznik <mprivozn@redhat.com> + */ + +#include <config.h> + +#include "testutils.h" +#include "virmacmap.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +struct testData { + const char *file; + const char *domain; + const char * const * macs; + virMACMapMgrPtr mgr; +}; + + +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; + } + } + + 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; + } + } + + ret = 0; + cleanup: + VIR_FREE(file); + virObjectUnref(mgr); + return ret; +} + + +static int +testMACRemove(const void *opaque) +{ + const struct testData *data = opaque; + virMACMapMgrPtr mgr = NULL; + const char * const * macs; + size_t i; + 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; + + for (i = 0; data->macs && data->macs[i]; i++) { + if (virMACMapMgrRemove(mgr, data->domain, data->macs[i]) < 0) { + fprintf(stderr, + "Error when removing %s from the list of MACs\n", data->macs[i]); + goto cleanup; + } + } + + if ((macs = virMACMapMgrLookup(mgr, data->domain))) { + fprintf(stderr, + "Not removed all MACs for domain %s: %s\n", data->domain, macs[0]); + goto cleanup; + } + + ret = 0; + cleanup: + VIR_FREE(file); + virObjectUnref(mgr); + return ret; +} + + +static int +testMACFlush(const void *opaque) +{ + const struct testData *data = opaque; + char *file = NULL; + char *str = NULL; + int ret = -1; + + if (virAsprintf(&file, "%s/virmacmaptestdata/%s.json", + abs_srcdir, data->file) < 0) + goto cleanup; + + if (virMACMapMgrFlushStr(data->mgr, &str) < 0) + goto cleanup; + + if (virTestCompareToFile(str, file) < 0) + goto cleanup; + + ret = 0; + cleanup: + VIR_FREE(file); + VIR_FREE(str); + return ret; +} + + +static int +mymain(void) +{ + int ret = 0; + virMACMapMgrPtr mgr = NULL; + +#define DO_TEST_BASIC(f, d, ...) \ + do { \ + const char * const m[] = {__VA_ARGS__, NULL }; \ + struct testData data = {.file = f, .domain = d, .macs = m}; \ + if (virTestRun("Lookup " #d " in " #f, \ + testMACLookup, &data) < 0) \ + ret = -1; \ + if (virTestRun("Remove " #d " in " #f, \ + testMACRemove, &data) < 0) \ + ret = -1; \ + } while (0) + +#define DO_TEST_FLUSH_PROLOGUE \ + if (!(mgr = virMACMapMgrNew(NULL))) \ + ret = -1; + +#define DO_TEST_FLUSH(d, ...) \ + do { \ + const char * const m[] = {__VA_ARGS__, NULL }; \ + size_t i; \ + for (i = 0; m[i]; i++) { \ + if (virMACMapMgrAdd(mgr, d, m[i]) < 0) { \ + virObjectUnref(mgr); \ + mgr = NULL; \ + ret = -1; \ + } \ + } \ + } while (0) + + +#define DO_TEST_FLUSH_EPILOGUE(f) \ + do { \ + struct testData data = {.file = f, .mgr = mgr}; \ + if (virTestRun("Flush " #f, testMACFlush, &data) < 0) \ + ret = -1; \ + virObjectUnref(mgr); \ + mgr = NULL; \ + } while (0) + + DO_TEST_BASIC("empty", "none", NULL); + DO_TEST_BASIC("simple", "f24", "aa:bb:cc:dd:ee:ff"); + DO_TEST_BASIC("simple2", "f24", "aa:bb:cc:dd:ee:ff", "a1:b2:c3:d4:e5:f6"); + DO_TEST_BASIC("simple2", "f25", "00:11:22:33:44:55", "aa:bb:cc:00:11:22"); + + DO_TEST_FLUSH_PROLOGUE; + DO_TEST_FLUSH_EPILOGUE("empty"); + + DO_TEST_FLUSH_PROLOGUE; + DO_TEST_FLUSH("f24", "aa:bb:cc:dd:ee:ff"); + DO_TEST_FLUSH_EPILOGUE("simple"); + + DO_TEST_FLUSH_PROLOGUE; + DO_TEST_FLUSH("f24", "aa:bb:cc:dd:ee:ff", "a1:b2:c3:d4:e5:f6"); + DO_TEST_FLUSH("f25", "00:11:22:33:44:55", "aa:bb:cc:00:11:22"); + DO_TEST_FLUSH_EPILOGUE("simple2"); + + DO_TEST_FLUSH_PROLOGUE; + DO_TEST_FLUSH("dom0", "e1:81:5d:f3:41:57", "76:0a:2a:a0:51:86", "01:c7:fc:01:c7:fc"); + DO_TEST_FLUSH("dom0", "8e:82:53:60:32:4a", "14:7a:25:dc:7d:a0", "f8:d7:75:f8:d7:75"); + DO_TEST_FLUSH("dom0", "73:d2:50:fb:0f:b1", "82:ee:a7:9b:e3:69", "a8:b4:cb:a8:b4:cb"); + DO_TEST_FLUSH("dom0", "7e:81:86:0f:0b:fb", "94:e2:00:d9:4c:70", "dc:7b:83:dc:7b:83"); + DO_TEST_FLUSH("dom0", "d1:19:a5:a1:52:a8", "22:03:a0:bf:cb:4a", "e3:c7:f8:e3:c7:f8"); + DO_TEST_FLUSH("dom0", "aa:bf:3f:4f:21:8d", "28:67:45:72:8f:47", "eb:08:cd:eb:08:cd"); + DO_TEST_FLUSH("dom0", "bd:f8:a7:e5:e2:bd", "c7:80:e3:b9:18:4d", "ce:da:c0:ce:da:c0"); + DO_TEST_FLUSH("dom1", "8b:51:1d:9f:2f:29", "7c:ae:4c:3e:e1:11", "c6:68:4e:98:ff:6a"); + DO_TEST_FLUSH("dom1", "43:0e:33:a1:3f:0f", "7a:3e:ed:bb:15:27", "b1:17:fd:95:d2:1b"); + DO_TEST_FLUSH("dom1", "9e:89:49:99:51:0e", "89:b4:3f:08:88:2c", "54:0b:4c:e2:0a:39"); + DO_TEST_FLUSH("dom1", "bb:88:07:19:51:9d", "b7:f1:1a:40:a2:95", "88:94:39:a3:90:b4"); + DO_TEST_FLUSH_EPILOGUE("complex"); + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/virmacmapmock.so") diff --git a/tests/virmacmaptestdata/complex.json b/tests/virmacmaptestdata/complex.json new file mode 100644 index 000000000..192347c3f --- /dev/null +++ b/tests/virmacmaptestdata/complex.json @@ -0,0 +1,45 @@ +[ + { + "domain": "dom0", + "macs": [ + "e1:81:5d:f3:41:57", + "76:0a:2a:a0:51:86", + "01:c7:fc:01:c7:fc", + "8e:82:53:60:32:4a", + "14:7a:25:dc:7d:a0", + "f8:d7:75:f8:d7:75", + "73:d2:50:fb:0f:b1", + "82:ee:a7:9b:e3:69", + "a8:b4:cb:a8:b4:cb", + "7e:81:86:0f:0b:fb", + "94:e2:00:d9:4c:70", + "dc:7b:83:dc:7b:83", + "d1:19:a5:a1:52:a8", + "22:03:a0:bf:cb:4a", + "e3:c7:f8:e3:c7:f8", + "aa:bf:3f:4f:21:8d", + "28:67:45:72:8f:47", + "eb:08:cd:eb:08:cd", + "bd:f8:a7:e5:e2:bd", + "c7:80:e3:b9:18:4d", + "ce:da:c0:ce:da:c0" + ] + }, + { + "domain": "dom1", + "macs": [ + "8b:51:1d:9f:2f:29", + "7c:ae:4c:3e:e1:11", + "c6:68:4e:98:ff:6a", + "43:0e:33:a1:3f:0f", + "7a:3e:ed:bb:15:27", + "b1:17:fd:95:d2:1b", + "9e:89:49:99:51:0e", + "89:b4:3f:08:88:2c", + "54:0b:4c:e2:0a:39", + "bb:88:07:19:51:9d", + "b7:f1:1a:40:a2:95", + "88:94:39:a3:90:b4" + ] + } +] diff --git a/tests/virmacmaptestdata/empty.json b/tests/virmacmaptestdata/empty.json new file mode 100644 index 000000000..41b42e677 --- /dev/null +++ b/tests/virmacmaptestdata/empty.json @@ -0,0 +1,3 @@ +[ + +] diff --git a/tests/virmacmaptestdata/simple.json b/tests/virmacmaptestdata/simple.json new file mode 100644 index 000000000..ea0fb0836 --- /dev/null +++ b/tests/virmacmaptestdata/simple.json @@ -0,0 +1,8 @@ +[ + { + "domain": "f24", + "macs": [ + "aa:bb:cc:dd:ee:ff" + ] + } +] diff --git a/tests/virmacmaptestdata/simple2.json b/tests/virmacmaptestdata/simple2.json new file mode 100644 index 000000000..91b2cde0c --- /dev/null +++ b/tests/virmacmaptestdata/simple2.json @@ -0,0 +1,16 @@ +[ + { + "domain": "f25", + "macs": [ + "00:11:22:33:44:55", + "aa:bb:cc:00:11:22" + ] + }, + { + "domain": "f24", + "macs": [ + "aa:bb:cc:dd:ee:ff", + "a1:b2:c3:d4:e5:f6" + ] + } +] -- 2.11.0

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@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; + } + } +

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@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.
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.)
+ # 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.
+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().
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.
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

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@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.
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

On Mon, 2016-12-05 at 11:31 +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.
This appears to be broken on FreeBSD in a couple of interesting ways: $ VIR_TEST_DEBUG=1 ./tests/virmacmaptest TEST: virmacmaptest 1) Lookup "none" in "empty" ... OK ... 7) Lookup "f25" in "simple2" ... libvirt: Network Driver error : internal error: invalid json in file: .../tests/virmacmaptestdata/simple2.json ... 10) Flush "simple" ... Segmentation fault (core dumped) -- Andrea Bolognani / Red Hat / Virtualization

Now that we have a module that's able to track <domain, mac addres list> pairs, hook it up into our network driver. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.h | 4 ++ src/network/bridge_driver.c | 93 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+) diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 3b227db6f..e95d68d8f 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -41,6 +41,7 @@ # include "virbitmap.h" # include "networkcommon_conf.h" # include "virobject.h" +# include "virmacmap.h" typedef enum { VIR_NETWORK_FORWARD_NONE = 0, @@ -284,6 +285,9 @@ struct _virNetworkObj { unsigned long long floor_sum; /* sum of all 'floor'-s of attached NICs */ unsigned int taint; + + /* Immutable pointer, self locking APIs */ + virMACMapMgrPtr macmap; }; virNetworkObjPtr virNetworkObjNew(void); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 073f5019a..435601aae 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -289,6 +289,17 @@ networkRadvdConfigFileName(virNetworkDriverStatePtr driver, return configfile; } +static char * +networkMacMgrFileName(virNetworkDriverStatePtr driver, + const char *bridge) +{ + char *filename; + + ignore_value(virAsprintf(&filename, "%s/%s.macs", + driver->dnsmasqStateDir, bridge)); + return filename; +} + /* do needed cleanup steps and remove the network from the list */ static int networkRemoveInactive(virNetworkDriverStatePtr driver, @@ -300,6 +311,7 @@ networkRemoveInactive(virNetworkDriverStatePtr driver, char *configfile = NULL; char *radvdpidbase = NULL; char *statusfile = NULL; + char *macMapFile = NULL; dnsmasqContext *dctx = NULL; virNetworkDefPtr def = virNetworkObjGetPersistentDef(net); @@ -329,12 +341,18 @@ networkRemoveInactive(virNetworkDriverStatePtr driver, if (!(statusfile = virNetworkConfigFile(driver->stateDir, def->name))) goto cleanup; + if (!(macMapFile = networkMacMgrFileName(driver, def->bridge))) + goto cleanup; + /* dnsmasq */ dnsmasqDelete(dctx); unlink(leasefile); unlink(customleasefile); unlink(configfile); + /* MAC map manager */ + unlink(macMapFile); + /* radvd */ unlink(radvdconfigfile); virPidFileDelete(driver->pidDir, radvdpidbase); @@ -354,10 +372,71 @@ networkRemoveInactive(virNetworkDriverStatePtr driver, VIR_FREE(radvdconfigfile); VIR_FREE(radvdpidbase); VIR_FREE(statusfile); + VIR_FREE(macMapFile); dnsmasqContextFree(dctx); return ret; } +static int +networkMacMgrAdd(virNetworkDriverStatePtr driver, + virNetworkObjPtr network, + const char *domain, + const virMacAddr *mac) +{ + char macStr[VIR_MAC_STRING_BUFLEN]; + char *file = NULL; + int ret = -1; + + if (!network->macmap) + return 0; + + virMacAddrFormat(mac, macStr); + + if (!(file = networkMacMgrFileName(driver, network->def->bridge))) + goto cleanup; + + if (virMACMapMgrAdd(network->macmap, domain, macStr) < 0) + goto cleanup; + + if (virMACMapMgrFlush(network->macmap, file) < 0) + goto cleanup; + + ret = 0; + cleanup: + VIR_FREE(file); + return ret; +} + +static int +networkMacMgrDel(virNetworkDriverStatePtr driver, + virNetworkObjPtr network, + const char *domain, + const virMacAddr *mac) +{ + char macStr[VIR_MAC_STRING_BUFLEN]; + char *file = NULL; + int ret = -1; + + if (!network->macmap) + return 0; + + virMacAddrFormat(mac, macStr); + + if (!(file = networkMacMgrFileName(driver, network->def->bridge))) + goto cleanup; + + if (virMACMapMgrRemove(network->macmap, domain, macStr) < 0) + goto cleanup; + + if (virMACMapMgrFlush(network->macmap, file) < 0) + goto cleanup; + + ret = 0; + cleanup: + VIR_FREE(file); + return ret; +} + static char * networkBridgeDummyNicName(const char *brname) { @@ -2127,6 +2206,7 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver, virNetworkIPDefPtr ipdef; virNetDevIPRoutePtr routedef; char *macTapIfName = NULL; + char *macMapFile = NULL; int tapfd = -1; /* Check to see if any network IP collides with an existing route */ @@ -2173,6 +2253,10 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver, } } + if (!(macMapFile = networkMacMgrFileName(driver, network->def->bridge)) || + !(network->macmap = virMACMapMgrNew(macMapFile))) + goto err1; + /* Set bridge options */ /* delay is configured in seconds, but virNetDevBridgeSetSTPDelay @@ -2272,6 +2356,7 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver, goto err5; VIR_FREE(macTapIfName); + VIR_FREE(macMapFile); return 0; @@ -2308,6 +2393,7 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver, ignore_value(virNetDevTapDelete(macTapIfName, NULL)); VIR_FREE(macTapIfName); } + VIR_FREE(macMapFile); err0: if (!save_err) @@ -2329,6 +2415,8 @@ networkShutdownNetworkVirtual(virNetworkDriverStatePtr driver, if (network->def->bandwidth) virNetDevBandwidthClear(network->def->bridge); + virObjectUnref(network->macmap); + if (network->radvdPid > 0) { char *radvdpidbase; @@ -4353,6 +4441,9 @@ networkAllocateActualDevice(virDomainDefPtr dom, } } + if (networkMacMgrAdd(driver, network, dom->name, &iface->mac) < 0) + goto error; + if (virNetDevVPortProfileCheckComplete(virtport, true) < 0) goto error; @@ -4739,6 +4830,8 @@ networkReleaseActualDevice(virDomainDefPtr dom, } success: + networkMacMgrDel(driver, network, dom->name, &iface->mac); + if (iface->data.network.actual) { netdef->connections--; if (dev) -- 2.11.0

On Mon, Dec 05, 2016 at 11:31:53AM +0100, Michal Privoznik wrote:
Now that we have a module that's able to track <domain, mac addres list> pairs, hook it up into our network driver.
Shouldn't networkUpdateState() (or similar) be updated so that we don't loose all the info on daemon restart?

On 05.12.2016 16:52, Martin Kletzander wrote:
On Mon, Dec 05, 2016 at 11:31:53AM +0100, Michal Privoznik wrote:
Now that we have a module that's able to track <domain, mac addres list> pairs, hook it up into our network driver.
Shouldn't networkUpdateState() (or similar) be updated so that we don't loose all the info on daemon restart?
Hm. Good point. I wonder how could I miss that. Michal

The name of the exported functions for an NSS module is quite fixed, it is derived from the module name: _nss_$module_$function Since we will create another NSS module with very similar implementation we might as well generate the function names at the compile time. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/nsslinktest.c | 2 +- tests/nsstest.c | 12 ++++++------ tools/nss/libvirt_nss.c | 41 ++++++++++++++++++++--------------------- tools/nss/libvirt_nss.h | 26 ++++++++++++++------------ 4 files changed, 41 insertions(+), 40 deletions(-) diff --git a/tests/nsslinktest.c b/tests/nsslinktest.c index 0232e362a..a81a6d331 100644 --- a/tests/nsslinktest.c +++ b/tests/nsslinktest.c @@ -33,7 +33,7 @@ int main(int argc ATTRIBUTE_UNUSED, * the fact this test has been built successfully means * there's no linkage problem and therefore success is * returned. */ - _nss_libvirt_gethostbyname_r(NULL, NULL, NULL, 0, &err, &herrno); + NSS_NAME(gethostbyname)(NULL, NULL, NULL, 0, &err, &herrno); return EXIT_SUCCESS; } diff --git a/tests/nsstest.c b/tests/nsstest.c index 8648c4afb..c7fb11fd5 100644 --- a/tests/nsstest.c +++ b/tests/nsstest.c @@ -53,12 +53,12 @@ testGetHostByName(const void *opaque) memset(&resolved, 0, sizeof(resolved)); - rv = _nss_libvirt_gethostbyname2_r(data->hostname, - data->af, - &resolved, - buf, sizeof(buf), - &tmp_errno, - &tmp_herrno); + rv = NSS_NAME(gethostbyname2)(data->hostname, + data->af, + &resolved, + buf, sizeof(buf), + &tmp_errno, + &tmp_herrno); if (rv == NSS_STATUS_TRYAGAIN || rv == NSS_STATUS_UNAVAIL || diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index 0d5982529..d77c9ece5 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -253,23 +253,23 @@ findLease(const char *name, enum nss_status -_nss_libvirt_gethostbyname_r(const char *name, struct hostent *result, - char *buffer, size_t buflen, int *errnop, - int *herrnop) +NSS_NAME(gethostbyname)(const char *name, struct hostent *result, + char *buffer, size_t buflen, int *errnop, + int *herrnop) { int af = ((_res.options & RES_USE_INET6) ? AF_INET6 : AF_INET); - return _nss_libvirt_gethostbyname3_r(name, af, result, buffer, buflen, - errnop, herrnop, NULL, NULL); + return NSS_NAME(gethostbyname3)(name, af, result, buffer, buflen, + errnop, herrnop, NULL, NULL); } enum nss_status -_nss_libvirt_gethostbyname2_r(const char *name, int af, struct hostent *result, - char *buffer, size_t buflen, int *errnop, - int *herrnop) +NSS_NAME(gethostbyname2)(const char *name, int af, struct hostent *result, + char *buffer, size_t buflen, int *errnop, + int *herrnop) { - return _nss_libvirt_gethostbyname3_r(name, af, result, buffer, buflen, - errnop, herrnop, NULL, NULL); + return NSS_NAME(gethostbyname3)(name, af, result, buffer, buflen, + errnop, herrnop, NULL, NULL); } static inline void * @@ -287,9 +287,9 @@ move_and_align(void *buf, size_t len, size_t *idx) } enum nss_status -_nss_libvirt_gethostbyname3_r(const char *name, int af, struct hostent *result, - char *buffer, size_t buflen, int *errnop, - int *herrnop, int32_t *ttlp, char **canonp) +NSS_NAME(gethostbyname3)(const char *name, int af, struct hostent *result, + char *buffer, size_t buflen, int *errnop, + int *herrnop, int32_t *ttlp, char **canonp) { enum nss_status ret = NSS_STATUS_UNAVAIL; char *r_name, **r_aliases, *r_addr, *r_addr_next, **r_addr_list; @@ -405,9 +405,9 @@ _nss_libvirt_gethostbyname3_r(const char *name, int af, struct hostent *result, #ifdef HAVE_STRUCT_GAIH_ADDRTUPLE enum nss_status -_nss_libvirt_gethostbyname4_r(const char *name, struct gaih_addrtuple **pat, - char *buffer, size_t buflen, int *errnop, - int *herrnop, int32_t *ttlp) +NSS_NAME(gethostbyname4)(const char *name, struct gaih_addrtuple **pat, + char *buffer, size_t buflen, int *errnop, + int *herrnop, int32_t *ttlp) { enum nss_status ret = NSS_STATUS_UNAVAIL; leaseAddress *addr = NULL; @@ -517,9 +517,9 @@ aiforaf(const char *name, int af, struct addrinfo *pai, struct addrinfo **aip) struct addrinfo hints, *res0, *res; char **addrList; - if ((ret = _nss_libvirt_gethostbyname2_r(name, af, &resolved, - buf, sizeof(buf), - &err, &herr)) != NS_SUCCESS) + if ((ret = NSS_NAME(gethostbyname2)(name, af, &resolved, + buf, sizeof(buf), + &err, &herr)) != NS_SUCCESS) return; addrList = resolved.h_addr_list; @@ -604,8 +604,7 @@ _nss_compat_gethostbyname2_r(void *retval, void *mdata ATTRIBUTE_UNUSED, va_list errnop = va_arg(ap, int *); herrnop = va_arg(ap, int *); - ret = _nss_libvirt_gethostbyname2_r( - name, af, result, buffer, buflen, errnop, herrnop); + ret = NSS_NAME(gethostbyname2)(name, af, result, buffer, buflen, errnop, herrnop); *(struct hostent **)retval = (ret == NS_SUCCESS) ? result : NULL; return ret; diff --git a/tools/nss/libvirt_nss.h b/tools/nss/libvirt_nss.h index e025e6362..481d8828b 100644 --- a/tools/nss/libvirt_nss.h +++ b/tools/nss/libvirt_nss.h @@ -32,24 +32,26 @@ # include <nss.h> # include <netdb.h> +# define NSS_NAME(s) _nss_libvirt_##s##_r + enum nss_status -_nss_libvirt_gethostbyname_r(const char *name, struct hostent *result, - char *buffer, size_t buflen, int *errnop, - int *herrnop); +NSS_NAME(gethostbyname)(const char *name, struct hostent *result, + char *buffer, size_t buflen, int *errnop, + int *herrnop); enum nss_status -_nss_libvirt_gethostbyname2_r(const char *name, int af, struct hostent *result, - char *buffer, size_t buflen, int *errnop, - int *herrnop); +NSS_NAME(gethostbyname2)(const char *name, int af, struct hostent *result, + char *buffer, size_t buflen, int *errnop, + int *herrnop); enum nss_status -_nss_libvirt_gethostbyname3_r(const char *name, int af, struct hostent *result, - char *buffer, size_t buflen, int *errnop, - int *herrnop, int32_t *ttlp, char **canonp); +NSS_NAME(gethostbyname3)(const char *name, int af, struct hostent *result, + char *buffer, size_t buflen, int *errnop, + int *herrnop, int32_t *ttlp, char **canonp); # ifdef HAVE_STRUCT_GAIH_ADDRTUPLE enum nss_status -_nss_libvirt_gethostbyname4_r(const char *name, struct gaih_addrtuple **pat, - char *buffer, size_t buflen, int *errnop, - int *herrnop, int32_t *ttlp); +NSS_NAME(gethostbyname4)(const char *name, struct gaih_addrtuple **pat, + char *buffer, size_t buflen, int *errnop, + int *herrnop, int32_t *ttlp); # endif /* HAVE_STRUCT_GAIH_ADDRTUPLE */ # if defined(HAVE_BSD_NSS) -- 2.11.0

On Mon, Dec 05, 2016 at 11:31:54AM +0100, Michal Privoznik wrote:
The name of the exported functions for an NSS module is quite fixed, it is derived from the module name:
_nss_$module_$function
Since we will create another NSS module with very similar implementation we might as well generate the function names at the compile time.
I'd be nice to say that the NSS_NAME macro comes from nss.h (I presume). It's starting to look like libvirt-php now =D

On Mon, Dec 05, 2016 at 04:55:18PM +0100, Martin Kletzander wrote:
On Mon, Dec 05, 2016 at 11:31:54AM +0100, Michal Privoznik wrote:
The name of the exported functions for an NSS module is quite fixed, it is derived from the module name:
_nss_$module_$function
Since we will create another NSS module with very similar implementation we might as well generate the function names at the compile time.
I'd be nice to say that the NSS_NAME macro comes from nss.h (I presume). It's starting to look like libvirt-php now =D
I totally missed the hunk that's adding the macro, sorry, act like nothing happened O:-)

The part of the code that appends found IP address into a list is going to be re-used. Instead of copying it over, move it to a separate function. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/nss/libvirt_nss.c | 95 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 63 insertions(+), 32 deletions(-) diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index d77c9ece5..979bf81e1 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -79,6 +79,68 @@ typedef struct { int af; } leaseAddress; + +static int +appendAddr(leaseAddress **tmpAddress, + size_t *ntmpAddress, + virJSONValuePtr lease, + int af) +{ + int ret = -1; + const char *ipAddr; + virSocketAddr sa; + int family; + size_t i; + + if (!(ipAddr = virJSONValueObjectGetString(lease, "ip-address"))) { + ERROR("ip-address field missing for %s", name); + goto cleanup; + } + + DEBUG("IP address: %s", ipAddr); + + if (virSocketAddrParse(&sa, ipAddr, AF_UNSPEC) < 0) { + ERROR("Unable to parse %s", ipAddr); + goto cleanup; + } + + family = VIR_SOCKET_ADDR_FAMILY(&sa); + if (af != AF_UNSPEC && af != family) { + DEBUG("Skipping address which family is %d, %d requested", family, af); + ret = 0; + goto cleanup; + } + + for (i = 0; i < *ntmpAddress; i++) { + if (memcmp((*tmpAddress)[i].addr, + (family == AF_INET ? + (void *) &sa.data.inet4.sin_addr.s_addr : + (void *) &sa.data.inet6.sin6_addr.s6_addr), + FAMILY_ADDRESS_SIZE(family)) == 0) { + DEBUG("IP address already in the list"); + ret = 0; + goto cleanup; + } + } + + if (VIR_REALLOC_N_QUIET(*tmpAddress, *ntmpAddress + 1) < 0) { + ERROR("Out of memory"); + goto cleanup; + } + + (*tmpAddress)[*ntmpAddress].af = family; + memcpy((*tmpAddress)[*ntmpAddress].addr, + (family == AF_INET ? + (void *) &sa.data.inet4.sin_addr.s_addr : + (void *) &sa.data.inet6.sin6_addr.s6_addr), + FAMILY_ADDRESS_SIZE(family)); + (*ntmpAddress)++; + ret = 0; + cleanup: + return ret; +} + + /** * findLease: * @name: domain name to lookup @@ -172,9 +234,6 @@ findLease(const char *name, for (i = 0; i < nleases; i++) { virJSONValuePtr lease; const char *lease_name; - virSocketAddr sa; - const char *ipAddr; - int family; lease = virJSONValueArrayGet(leases_array, i); @@ -204,36 +263,8 @@ findLease(const char *name, DEBUG("Found record for %s", lease_name); *found = true; - if (!(ipAddr = virJSONValueObjectGetString(lease, "ip-address"))) { - ERROR("ip-address field missing for %s", name); + if (appendAddr(&tmpAddress, &ntmpAddress, lease, af) < 0) goto cleanup; - } - - DEBUG("IP address: %s", ipAddr); - - if (virSocketAddrParse(&sa, ipAddr, AF_UNSPEC) < 0) { - ERROR("Unable to parse %s", ipAddr); - goto cleanup; - } - - family = VIR_SOCKET_ADDR_FAMILY(&sa); - if (af != AF_UNSPEC && af != family) { - DEBUG("Skipping address which family is %d, %d requested", family, af); - continue; - } - - if (VIR_REALLOC_N_QUIET(tmpAddress, ntmpAddress + 1) < 0) { - ERROR("Out of memory"); - goto cleanup; - } - - tmpAddress[ntmpAddress].af = family; - memcpy(tmpAddress[ntmpAddress].addr, - (family == AF_INET ? - (void *) &sa.data.inet4.sin_addr.s_addr : - (void *) &sa.data.inet6.sin6_addr.s6_addr), - FAMILY_ADDRESS_SIZE(family)); - ntmpAddress++; } *address = tmpAddress; -- 2.11.0

So far the NSS module looks up only hostnames as provided by guests themselves. However, there are some cases where this is not enough: e.g. when there's a fresh new guest being installed (with some generic hostname) say from a live ISO image; or some (older) systems don't advertise their hostname in DHCP transactions at all. In cases like that it would be helpful if we translate domain name as seen by libvirt too so that users can: # virsh start $dom && ssh $dom In order to achieve that new libvirt-guest module is introduced, while older libvirt module maintains its current behaviour (that is translating guest provided names into IP addresses). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/news.html.in | 4 ++ docs/nss.html.in | 58 ++++++++++++++++++++++--- src/Makefile.am | 8 ++++ tests/Makefile.am | 22 +++++++++- tests/nssdata/virbr0.macs | 23 ++++++++++ tests/nssdata/virbr0.status | 5 +++ tests/nssdata/virbr1.macs | 21 +++++++++ tests/nssdata/virbr1.status | 5 +++ tests/nssmock.c | 21 +++++++++ tests/nsstest.c | 5 +++ tools/Makefile.am | 46 ++++++++++++++++++-- tools/nss/libvirt_guest_nss.syms | 12 ++++++ tools/nss/libvirt_nss.c | 92 ++++++++++++++++++++++++++++++++++------ tools/nss/libvirt_nss.h | 6 ++- 14 files changed, 305 insertions(+), 23 deletions(-) create mode 100644 tests/nssdata/virbr0.macs create mode 100644 tests/nssdata/virbr1.macs create mode 100644 tools/nss/libvirt_guest_nss.syms diff --git a/docs/news.html.in b/docs/news.html.in index 12dcb0141..2ea409894 100644 --- a/docs/news.html.in +++ b/docs/news.html.in @@ -15,6 +15,10 @@ <h3>v3.0.0 (<i>unreleased</i>)</h3> <ul> <li><strong>New features</strong> + <ul> + <li>New <code>libvirt-guest</code> nss module that translates libvirt + guest names into IP addresses</li> + </ul> </li> <li><strong>Improvements</strong> </li> diff --git a/docs/nss.html.in b/docs/nss.html.in index 84ea8df6d..ee18c9b78 100644 --- a/docs/nss.html.in +++ b/docs/nss.html.in @@ -62,6 +62,48 @@ hosts: files libvirt dns lookup given host name. </p> + <h2><a name="Sources">Sources of information</a></h2> + + <p> + As of <code>v3.0.0</code> release, libvirt offers two NSS modules + implementing two different methods of hostname translation. The first and + older method is implemented by <code>libvirt</code> plugin and + basically looks up the hostname to IP address translation in DHCP server + records. Therefore this is dependent on hostname provided by guests. Thing + is, not all the guests out there provide one in DHCP transactions, nor + every sysadmin out there believes all the guests. Hence libvirt implements + second method in <code>libvirt-guest</code> module which does libvirt guest + name to IP address translation (regardless of hostname set in the guest). + </p> + + <p> + To enable either of the modules put their name into the + <code>nsswitch.conf</code> file. For instance, to enable + <code>libvirt-guest</code> module: + </p> + +<pre> +$ cat /etc/nsswitch.conf +# /etc/nsswitch.conf: +hosts: files libvirt-guest dns +# ... +</pre> + <p>Or users can enable both at the same time:</p> +<pre> +$ cat /etc/nsswitch.conf +# /etc/nsswitch.conf: +hosts: files libvirt libvirt-guest dns +# ... +</pre> + + <p> + This configuration will mean that if hostname is not found by the + <code>libvirt</code> module (e.g. because a guest did not sent hostname + during DHCP transaction), the <code>libvirt-guest</code> module is + consulted (and if the hostname matches libvirt guest name it will be + resolved). + </p> + <h2><a name="Internals">How does it work?</a></h2> <p> @@ -100,15 +142,18 @@ hosts: files libvirt dns <h2><a name="Limitations">Limitations</a></h2> <ol> - <li>The libvirt NSS module matches only hostnames provided by guest. If - the libvirt name and one advertised by guest differs, the latter is - matched.</li> + <li><del>The libvirt NSS module matches only hostnames provided by guest. + If the libvirt name and one advertised by guest differs, the latter is + matched.</del> As of <code>v3.0.0</code> there are two libvirt NSS modules + translating both hostnames provided by guest and libvirt guest names.</li> <li>The module works only in that cases where IP addresses are assigned by dnsmasq spawned by libvirt. Libvirt NATed networks are typical example.</li> </ol> <p> + <i>The following paragraph describes implementation limitation of the + <code>libvirt</code> NSS module.</i> These limitation are result of libvirt's internal implementation. While libvirt can report IP addresses regardless of their origin, a public API must be used to obtain those. However, for the API a connection object is @@ -134,8 +179,11 @@ virsh domifaddr --source lease $domain </pre> <p> - If there's no record for either of the aforementioned commands, it's very - likely that NSS module won't find anything and vice versa. + <del>If there's no record for either of the aforementioned commands, it's + very likely that NSS module won't find anything and vice versa.</del> + As of <code>v3.0.0</code> libvirt provides <code>libvirt-guest</code> NSS + module that doesn't have this limitation. However, the statement is still + true for the <code>libvirt</code> NSS module. </p> </body> </html> diff --git a/src/Makefile.am b/src/Makefile.am index a9106fa97..8c620d5e0 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -3033,6 +3033,10 @@ libvirt_nss_la_SOURCES = \ util/virerror.h \ util/virfile.c \ util/virfile.h \ + util/virhash.c \ + util/virhash.h \ + util/virhashcode.c \ + util/virhashcode.h \ util/virjson.c \ util/virjson.h \ util/virkmod.c \ @@ -3041,12 +3045,16 @@ libvirt_nss_la_SOURCES = \ util/virlease.h \ util/virlog.c \ util/virlog.h \ + util/virmacmap.c \ + util/virmacmap.h \ util/virobject.c \ util/virobject.h \ util/virpidfile.c \ util/virpidfile.h \ util/virprocess.c \ util/virprocess.h \ + util/virrandom.c \ + util/virrandom.h \ util/virsocketaddr.c \ util/virsocketaddr.h \ util/virstring.c \ diff --git a/tests/Makefile.am b/tests/Makefile.am index 9d5583d43..ecd04e895 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -335,8 +335,8 @@ test_programs += virscsitest endif WITH_LINUX if WITH_NSS -test_helpers += nsslinktest -test_programs += nsstest +test_helpers += nsslinktest nssguestlinktest +test_programs += nsstest nssguesttest endif WITH_NSS test_programs += storagevolxml2xmltest storagepoolxml2xmltest @@ -1125,6 +1125,16 @@ nsstest_LDADD = \ $(LDADDS) \ ../tools/nss/libnss_libvirt_impl.la +nssguesttest_SOURCES = \ + nsstest.c testutils.h testutils.c +nssguesttest_CFLAGS = \ + -DLIBVIRT_NSS_GUEST \ + $(AM_CFLAGS) \ + -I$(top_srcdir)/tools/nss +nssguesttest_LDADD = \ + $(LDADDS) \ + ../tools/nss/libnss_libvirt_guest_impl.la + nssmock_la_SOURCES = \ nssmock.c nssmock_la_CFLAGS = $(AM_CFLAGS) @@ -1140,6 +1150,14 @@ nsslinktest_CFLAGS = \ nsslinktest_LDADD = ../tools/nss/libnss_libvirt_impl.la nsslinktest_LDFLAGS = $(NULL) +nssguestlinktest_SOURCES = nsslinktest.c +nssguestlinktest_CFLAGS = \ + -DLIBVIRT_NSS_GUEST \ + $(AM_CFLAGS) \ + -I$(top_srcdir)/tools/nss +nssguestlinktest_LDADD = ../tools/nss/libnss_libvirt_guest_impl.la +nssguestlinktest_LDFLAGS = $(NULL) + virmacmapmock_la_SOURCES = \ virmacmapmock.c virmacmapmock_la_CFLAGS = $(AM_CFLAGS) diff --git a/tests/nssdata/virbr0.macs b/tests/nssdata/virbr0.macs new file mode 100644 index 000000000..45f9e2bed --- /dev/null +++ b/tests/nssdata/virbr0.macs @@ -0,0 +1,23 @@ +[ + { + "domain": "fedora", + "macs": [ + "52:54:00:a4:6f:91", + "52:54:00:a4:6f:92" + ] + }, + { + "domain": "gentoo", + "macs": [ + "52:54:00:3a:b5:0c" + ] + }, + { + "domain": "debian", + "macs": [ + "52:54:00:11:22:33", + "52:54:00:a2:37:4c", + "52:54:00:f8:5b:1d" + ] + } +] diff --git a/tests/nssdata/virbr0.status b/tests/nssdata/virbr0.status index 6ebe95454..d04077426 100644 --- a/tests/nssdata/virbr0.status +++ b/tests/nssdata/virbr0.status @@ -16,5 +16,10 @@ "mac-address": "52:54:00:3a:b5:0c", "hostname": "gentoo", "expiry-time": 2000000000 + }, + { + "ip-address": "192.168.122.2", + "mac-address": "52:54:00:11:22:33", + "expiry-time": 2000000000 } ] diff --git a/tests/nssdata/virbr1.macs b/tests/nssdata/virbr1.macs new file mode 100644 index 000000000..214073755 --- /dev/null +++ b/tests/nssdata/virbr1.macs @@ -0,0 +1,21 @@ +[ + { + "domain": "fedora", + "macs": [ + "52:54:00:a4:6f:93", + "52:54:00:a4:6f:94" + ] + }, + { + "domain": "gentoo", + "macs": [ + "52:54:00:04:be:64" + ] + }, + { + "domain": "suse", + "macs": [ + "52:54:00:aa:bb:cc" + ] + } +] diff --git a/tests/nssdata/virbr1.status b/tests/nssdata/virbr1.status index f8a9157f3..4951d4513 100644 --- a/tests/nssdata/virbr1.status +++ b/tests/nssdata/virbr1.status @@ -16,5 +16,10 @@ "mac-address": "52:54:00:a4:6f:94", "hostname": "fedora", "expiry-time": 1 + }, + { + "ip-address": "192.168.122.3", + "mac-address": "52:54:00:aa:bb:cc", + "expiry-time": 2000000000 } ] diff --git a/tests/nssmock.c b/tests/nssmock.c index 273af0654..7929c3061 100644 --- a/tests/nssmock.c +++ b/tests/nssmock.c @@ -26,6 +26,7 @@ # include <dirent.h> # include <sys/stat.h> # include <fcntl.h> +# include <unistd.h> # include "configmake.h" # include "virstring.h" @@ -33,6 +34,7 @@ static int (*real_open)(const char *path, int flags, ...); static DIR * (*real_opendir)(const char *name); +static int (*real_access)(const char *path, int mode); # define LEASEDIR LOCALSTATEDIR "/lib/libvirt/dnsmasq/" @@ -47,6 +49,7 @@ init_syms(void) VIR_MOCK_REAL_INIT(open); VIR_MOCK_REAL_INIT(opendir); + VIR_MOCK_REAL_INIT(access); } static int @@ -112,6 +115,24 @@ opendir(const char *path) free(newpath); return ret; } + +int +access(const char *path, int mode) +{ + int ret; + char *newpath = NULL; + + init_syms(); + + if (STRPREFIX(path, LEASEDIR) && + getrealpath(&newpath, path) < 0) + return -1; + + ret = real_access(newpath ? newpath : path, mode); + + free(newpath); + return ret; +} #else /* Nothing to override if NSS plugin is not enabled */ #endif diff --git a/tests/nsstest.c b/tests/nsstest.c index c7fb11fd5..915a8bceb 100644 --- a/tests/nsstest.c +++ b/tests/nsstest.c @@ -184,11 +184,16 @@ mymain(void) ret = -1; \ } while (0) +# if !defined(LIBVIRT_NSS_GUEST) DO_TEST("fedora", AF_INET, "192.168.122.197", "192.168.122.198", "192.168.122.199"); DO_TEST("gentoo", AF_INET, "192.168.122.254"); DO_TEST("gentoo", AF_INET6, "2001:1234:dead:beef::2"); DO_TEST("gentoo", AF_UNSPEC, "192.168.122.254"); DO_TEST("non-existent", AF_UNSPEC, NULL); +# else /* defined(LIBVIRT_NSS_GUEST) */ + DO_TEST("debian", AF_INET, "192.168.122.2"); + DO_TEST("suse", AF_INET, "192.168.122.3"); +# endif /* defined(LIBVIRT_NSS_GUEST) */ return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tools/Makefile.am b/tools/Makefile.am index 100e657f7..e6ae15025 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -436,18 +436,26 @@ endif WITH_WIRESHARK_DISSECTOR if WITH_BSD_NSS LIBVIRT_NSS_SYMBOL_FILE = \ $(srcdir)/nss/libvirt_nss_bsd.syms +LIBVIRT_GUEST_NSS_SYMBOL_FILE = \ + $(LIBVIRT_NSS_SYMBOL_FILE) NSS_SO_VER = 1 install-nss: ( cd $(DESTDIR)$(libdir) && \ rm -f nss_libvirt.so.$(NSS_SO_VER) && \ - $(LN_S) libnss_libvirt.so.$(NSS_SO_VER) nss_libvirt.so.$(NSS_SO_VER) ) + $(LN_S) libnss_libvirt.so.$(NSS_SO_VER) nss_libvirt.so.$(NSS_SO_VER) && \ + rm -f nss_libvirt_guest.so.$(NSS_SO_VER) && \ + $(LN_S) libnss_libvirt_guest.so.$(NSS_SO_VER) \ + nss_libvirt_guest.so.$(NSS_SO_VER)) uninstall-nss: -rm -f $(DESTDIR)$(libdir)/nss_libvirt.so.$(NSS_SO_VER) + -rm -f $(DESTDIR)$(libdir)/nss_libvirt_guest.so.$(NSS_SO_VER) else ! WITH_BSD_NSS LIBVIRT_NSS_SYMBOL_FILE = \ $(srcdir)/nss/libvirt_nss.syms +LIBVIRT_GUEST_NSS_SYMBOL_FILE = \ + $(srcdir)/nss/libvirt_guest_nss.syms NSS_SO_VER = 2 install-nss: @@ -488,14 +496,46 @@ nss_libnss_libvirt_la_LDFLAGS = \ nss_libnss_libvirt_la_LIBADD = \ nss/libnss_libvirt_impl.la +noinst_LTLIBRARIES += nss/libnss_libvirt_guest_impl.la +nss_libnss_libvirt_guest_impl_la_SOURCES = \ + $(LIBVIRT_NSS_SOURCES) + +nss_libnss_libvirt_guest_impl_la_CFLAGS = \ + -DLIBVIRT_NSS \ + -DLIBVIRT_NSS_GUEST \ + $(AM_CFLAGS) \ + $(WARN_CFLAGS) \ + $(PIE_CFLAGS) \ + $(COVERAGE_CFLAGS) \ + $(LIBXML_CFLAGS) + +nss_libnss_libvirt_guest_impl_la_LIBADD = \ + ../gnulib/lib/libgnu.la \ + ../src/libvirt-nss.la + +nss_libnss_libvirt_guest_la_SOURCES = +nss_libnss_libvirt_guest_la_LDFLAGS = \ + $(VERSION_SCRIPT_FLAGS)$(LIBVIRT_GUEST_NSS_SYMBOL_FILE) \ + $(AM_LDFLAGS) \ + -module \ + -export-dynamic \ + -avoid-version \ + -shared \ + -shrext .so.$(NSS_SO_VER) + +nss_libnss_libvirt_guest_la_LIBADD = \ + nss/libnss_libvirt_guest_impl.la + lib_LTLIBRARIES = \ - nss/libnss_libvirt.la + nss/libnss_libvirt.la \ + nss/libnss_libvirt_guest.la endif WITH_NSS EXTRA_DIST += $(LIBVIRT_NSS_SOURCES) \ $(srcdir)/nss/libvirt_nss.syms \ - $(srcdir)/nss/libvirt_nss_bsd.syms + $(srcdir)/nss/libvirt_nss_bsd.syms \ + $(srcdir)/nss/libvirt_guest_nss.syms clean-local: -rm -rf wireshark/src/libvirt diff --git a/tools/nss/libvirt_guest_nss.syms b/tools/nss/libvirt_guest_nss.syms new file mode 100644 index 000000000..061298725 --- /dev/null +++ b/tools/nss/libvirt_guest_nss.syms @@ -0,0 +1,12 @@ +# +# Officially exported symbols. +# + +{ +global: + _nss_libvirt_guest_gethostbyname_r; + _nss_libvirt_guest_gethostbyname2_r; + _nss_libvirt_guest_gethostbyname3_r; + _nss_libvirt_guest_gethostbyname4_r; +local: *; +}; diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index 979bf81e1..6cadf8c43 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -47,6 +47,8 @@ #include "virstring.h" #include "virsocketaddr.h" #include "configmake.h" +#include "virmacmap.h" +#include "virobject.h" #if 0 # define ERROR(...) \ @@ -179,6 +181,8 @@ findLease(const char *name, size_t ntmpAddress = 0; time_t currtime; long long expirytime; + virMACMapMgrPtr *macmaps = NULL; + size_t nMacmaps = 0; *address = NULL; *naddress = 0; @@ -189,7 +193,6 @@ findLease(const char *name, goto cleanup; } - if (virDirOpenQuiet(&dir, leaseDir) < 0) { ERROR("Failed to open dir '%s'", leaseDir); goto cleanup; @@ -204,23 +207,36 @@ findLease(const char *name, while ((ret = virDirRead(dir, &entry, leaseDir)) > 0) { char *path; - if (!virFileHasSuffix(entry->d_name, ".status")) - continue; + if (virFileHasSuffix(entry->d_name, ".status")) { + if (!(path = virFileBuildPath(leaseDir, entry->d_name, NULL))) + goto cleanup; - if (!(path = virFileBuildPath(leaseDir, entry->d_name, NULL))) - goto cleanup; + DEBUG("Processing %s", path); + if (virLeaseReadCustomLeaseFile(leases_array, path, NULL, NULL) < 0) { + ERROR("Unable to parse %s", path); + VIR_FREE(path); + goto cleanup; + } + VIR_FREE(path); + } else if (virFileHasSuffix(entry->d_name, ".macs")) { + if (!(path = virFileBuildPath(leaseDir, entry->d_name, NULL))) + goto cleanup; - DEBUG("Processing %s", path); + if (VIR_REALLOC_N_QUIET(macmaps, nMacmaps + 1) < 0) { + VIR_FREE(path); + goto cleanup; + } - if (virLeaseReadCustomLeaseFile(leases_array, path, NULL, NULL) < 0) { - ERROR("Unable to parse %s", path); + DEBUG("Processing %s", path); + if (!(macmaps[nMacmaps] = virMACMapMgrNew(path))) { + ERROR("Unable to parse %s", path); + VIR_FREE(path); + goto cleanup; + } + nMacmaps++; VIR_FREE(path); - goto cleanup; } - - VIR_FREE(path); } - VIR_DIR_CLOSE(dir); nleases = virJSONValueArraySize(leases_array); @@ -231,6 +247,7 @@ findLease(const char *name, goto cleanup; } +#if !defined(LIBVIRT_NSS_GUEST) for (i = 0; i < nleases; i++) { virJSONValuePtr lease; const char *lease_name; @@ -267,6 +284,54 @@ findLease(const char *name, goto cleanup; } +#else /* defined(LIBVIRT_NSS_GUEST) */ + + for (i = 0; i < nMacmaps; i++) { + const char **macs = (const char **) virMACMapMgrLookup(macmaps[i], name); + size_t j; + + if (!macs) + continue; + + for (j = 0; j < nleases; j++) { + virJSONValuePtr lease = virJSONValueArrayGet(leases_array, j); + const char *macAddr; + + if (!lease) { + /* This should never happen (TM) */ + ERROR("Unable to get element %zd of %zd", j, nleases); + goto cleanup; + } + + macAddr = virJSONValueObjectGetString(lease, "mac-address"); + if (!macAddr) + continue; + + if (!virStringListHasString(macs, macAddr)) + continue; + + if (virJSONValueObjectGetNumberLong(lease, "expiry-time", &expirytime) < 0) { + /* A lease cannot be present without expiry-time */ + ERROR("expiry-time field missing for %s", name); + goto cleanup; + } + + /* Do not report expired lease */ + if (expirytime < (long long) currtime) { + DEBUG("Skipping expired lease for %s", name); + continue; + } + + DEBUG("Found record for %s", name); + *found = true; + + if (appendAddr(&tmpAddress, &ntmpAddress, lease, af) < 0) + goto cleanup; + } + } + +#endif /* defined(LIBVIRT_NSS_GUEST) */ + *address = tmpAddress; *naddress = ntmpAddress; tmpAddress = NULL; @@ -279,6 +344,9 @@ findLease(const char *name, VIR_FREE(tmpAddress); virJSONValueFree(leases_array); VIR_DIR_CLOSE(dir); + while (nMacmaps) + virObjectUnref(macmaps[--nMacmaps]); + VIR_FREE(macmaps); return ret; } diff --git a/tools/nss/libvirt_nss.h b/tools/nss/libvirt_nss.h index 481d8828b..a25fe0110 100644 --- a/tools/nss/libvirt_nss.h +++ b/tools/nss/libvirt_nss.h @@ -32,7 +32,11 @@ # include <nss.h> # include <netdb.h> -# define NSS_NAME(s) _nss_libvirt_##s##_r +# if !defined(LIBVIRT_NSS_GUEST) +# define NSS_NAME(s) _nss_libvirt_##s##_r +# else +# define NSS_NAME(s) _nss_libvirt_guest_##s##_r +# endif enum nss_status NSS_NAME(gethostbyname)(const char *name, struct hostent *result, -- 2.11.0

On Mon, Dec 05, 2016 at 11:31:56AM +0100, Michal Privoznik wrote:
So far the NSS module looks up only hostnames as provided by guests themselves. However, there are some cases where this is not enough: e.g. when there's a fresh new guest being installed (with some generic hostname) say from a live ISO image; or some (older) systems don't advertise their hostname in DHCP transactions at all. In cases like that it would be helpful if we translate domain name as seen by libvirt too so that users can:
# virsh start $dom && ssh $dom
In order to achieve that new libvirt-guest module is introduced, while older libvirt module maintains its current behaviour (that is translating guest provided names into IP addresses).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/news.html.in | 4 ++ docs/nss.html.in | 58 ++++++++++++++++++++++--- src/Makefile.am | 8 ++++ tests/Makefile.am | 22 +++++++++- tests/nssdata/virbr0.macs | 23 ++++++++++ tests/nssdata/virbr0.status | 5 +++ tests/nssdata/virbr1.macs | 21 +++++++++ tests/nssdata/virbr1.status | 5 +++ tests/nssmock.c | 21 +++++++++ tests/nsstest.c | 5 +++ tools/Makefile.am | 46 ++++++++++++++++++-- tools/nss/libvirt_guest_nss.syms | 12 ++++++ tools/nss/libvirt_nss.c | 92 ++++++++++++++++++++++++++++++++++------ tools/nss/libvirt_nss.h | 6 ++- 14 files changed, 305 insertions(+), 23 deletions(-) create mode 100644 tests/nssdata/virbr0.macs create mode 100644 tests/nssdata/virbr1.macs create mode 100644 tools/nss/libvirt_guest_nss.syms
diff --git a/docs/news.html.in b/docs/news.html.in index 12dcb0141..2ea409894 100644 --- a/docs/news.html.in +++ b/docs/news.html.in @@ -15,6 +15,10 @@ <h3>v3.0.0 (<i>unreleased</i>)</h3> <ul> <li><strong>New features</strong> + <ul> + <li>New <code>libvirt-guest</code> nss module that translates libvirt + guest names into IP addresses</li>
It would be nice to have the "summary<br/>description" here. Also you're missing newline before </li>.
+ </ul> </li> <li><strong>Improvements</strong> </li> diff --git a/docs/nss.html.in b/docs/nss.html.in index 84ea8df6d..ee18c9b78 100644 --- a/docs/nss.html.in +++ b/docs/nss.html.in @@ -62,6 +62,48 @@ hosts: files libvirt dns lookup given host name. </p>
+ <h2><a name="Sources">Sources of information</a></h2> + + <p> + As of <code>v3.0.0</code> release, libvirt offers two NSS modules + implementing two different methods of hostname translation. The first and + older method is implemented by <code>libvirt</code> plugin and + basically looks up the hostname to IP address translation in DHCP server + records. Therefore this is dependent on hostname provided by guests. Thing + is, not all the guests out there provide one in DHCP transactions, nor
I am being said (often) that "nor" can only be combined with "neither" and not even always (because I do that mistake often as well) and that "or" should be used in this context instead. Native speaker should probably correct me or you.
+ every sysadmin out there believes all the guests. Hence libvirt implements + second method in <code>libvirt-guest</code> module which does libvirt guest + name to IP address translation (regardless of hostname set in the guest). + </p> + + <p> + To enable either of the modules put their name into the + <code>nsswitch.conf</code> file. For instance, to enable + <code>libvirt-guest</code> module: + </p> + +<pre> +$ cat /etc/nsswitch.conf +# /etc/nsswitch.conf: +hosts: files libvirt-guest dns +# ... +</pre> + <p>Or users can enable both at the same time:</p> +<pre> +$ cat /etc/nsswitch.conf +# /etc/nsswitch.conf: +hosts: files libvirt libvirt-guest dns +# ... +</pre> + + <p> + This configuration will mean that if hostname is not found by the + <code>libvirt</code> module (e.g. because a guest did not sent hostname + during DHCP transaction), the <code>libvirt-guest</code> module is + consulted (and if the hostname matches libvirt guest name it will be + resolved). + </p> + <h2><a name="Internals">How does it work?</a></h2>
<p> @@ -100,15 +142,18 @@ hosts: files libvirt dns <h2><a name="Limitations">Limitations</a></h2>
<ol> - <li>The libvirt NSS module matches only hostnames provided by guest. If - the libvirt name and one advertised by guest differs, the latter is - matched.</li> + <li><del>The libvirt NSS module matches only hostnames provided by guest. + If the libvirt name and one advertised by guest differs, the latter is + matched.</del> As of <code>v3.0.0</code> there are two libvirt NSS modules + translating both hostnames provided by guest and libvirt guest names.</li>
Is <del/> just strike-through text? What's the point in that? Just make the documentation reflect the newest, current, state.
<li>The module works only in that cases where IP addresses are assigned by dnsmasq spawned by libvirt. Libvirt NATed networks are typical example.</li> </ol>
<p> + <i>The following paragraph describes implementation limitation of the + <code>libvirt</code> NSS module.</i> These limitation are result of libvirt's internal implementation. While libvirt can report IP addresses regardless of their origin, a public API must be used to obtain those. However, for the API a connection object is @@ -134,8 +179,11 @@ virsh domifaddr --source lease $domain </pre>
<p> - If there's no record for either of the aforementioned commands, it's very - likely that NSS module won't find anything and vice versa. + <del>If there's no record for either of the aforementioned commands, it's + very likely that NSS module won't find anything and vice versa.</del> + As of <code>v3.0.0</code> libvirt provides <code>libvirt-guest</code> NSS + module that doesn't have this limitation. However, the statement is still + true for the <code>libvirt</code> NSS module.
Same here.
diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index 979bf81e1..6cadf8c43 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -267,6 +284,54 @@ findLease(const char *name, goto cleanup; }
+#else /* defined(LIBVIRT_NSS_GUEST) */ + + for (i = 0; i < nMacmaps; i++) { + const char **macs = (const char **) virMACMapMgrLookup(macmaps[i], name); + size_t j; + + if (!macs) + continue; + + for (j = 0; j < nleases; j++) { + virJSONValuePtr lease = virJSONValueArrayGet(leases_array, j); + const char *macAddr; + + if (!lease) { + /* This should never happen (TM) */ + ERROR("Unable to get element %zd of %zd", j, nleases); + goto cleanup; + } + + macAddr = virJSONValueObjectGetString(lease, "mac-address"); + if (!macAddr) + continue; + + if (!virStringListHasString(macs, macAddr)) + continue; + + if (virJSONValueObjectGetNumberLong(lease, "expiry-time", &expirytime) < 0) { + /* A lease cannot be present without expiry-time */ + ERROR("expiry-time field missing for %s", name); + goto cleanup; + } + + /* Do not report expired lease */ + if (expirytime < (long long) currtime) { + DEBUG("Skipping expired lease for %s", name); + continue; + } + + DEBUG("Found record for %s", name); + *found = true; + + if (appendAddr(&tmpAddress, &ntmpAddress, lease, af) < 0) + goto cleanup; + } + } + +#endif /* defined(LIBVIRT_NSS_GUEST) */ +
I feel like lot of this code is duplicated, but that can be fixed later. Also it might look more complicated and this is the best way to keep it. Did you try any other approach by any chance (just out of curiosity).

On 05.12.2016 17:25, Martin Kletzander wrote:
On Mon, Dec 05, 2016 at 11:31:56AM +0100, Michal Privoznik wrote:
So far the NSS module looks up only hostnames as provided by guests themselves. However, there are some cases where this is not enough: e.g. when there's a fresh new guest being installed (with some generic hostname) say from a live ISO image; or some (older) systems don't advertise their hostname in DHCP transactions at all. In cases like that it would be helpful if we translate domain name as seen by libvirt too so that users can:
# virsh start $dom && ssh $dom
In order to achieve that new libvirt-guest module is introduced, while older libvirt module maintains its current behaviour (that is translating guest provided names into IP addresses).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/news.html.in | 4 ++ docs/nss.html.in | 58 ++++++++++++++++++++++--- src/Makefile.am | 8 ++++ tests/Makefile.am | 22 +++++++++- tests/nssdata/virbr0.macs | 23 ++++++++++ tests/nssdata/virbr0.status | 5 +++ tests/nssdata/virbr1.macs | 21 +++++++++ tests/nssdata/virbr1.status | 5 +++ tests/nssmock.c | 21 +++++++++ tests/nsstest.c | 5 +++ tools/Makefile.am | 46 ++++++++++++++++++-- tools/nss/libvirt_guest_nss.syms | 12 ++++++ tools/nss/libvirt_nss.c | 92 ++++++++++++++++++++++++++++++++++------ tools/nss/libvirt_nss.h | 6 ++- 14 files changed, 305 insertions(+), 23 deletions(-) create mode 100644 tests/nssdata/virbr0.macs create mode 100644 tests/nssdata/virbr1.macs create mode 100644 tools/nss/libvirt_guest_nss.syms
diff --git a/docs/news.html.in b/docs/news.html.in index 12dcb0141..2ea409894 100644 --- a/docs/news.html.in +++ b/docs/news.html.in @@ -15,6 +15,10 @@ <h3>v3.0.0 (<i>unreleased</i>)</h3> <ul> <li><strong>New features</strong> + <ul> + <li>New <code>libvirt-guest</code> nss module that translates libvirt + guest names into IP addresses</li>
It would be nice to have the "summary<br/>description" here. Also you're missing newline before </li>.
+ </ul> </li> <li><strong>Improvements</strong> </li> diff --git a/docs/nss.html.in b/docs/nss.html.in index 84ea8df6d..ee18c9b78 100644 --- a/docs/nss.html.in +++ b/docs/nss.html.in @@ -62,6 +62,48 @@ hosts: files libvirt dns lookup given host name. </p>
+ <h2><a name="Sources">Sources of information</a></h2> + + <p> + As of <code>v3.0.0</code> release, libvirt offers two NSS modules + implementing two different methods of hostname translation. The first and + older method is implemented by <code>libvirt</code> plugin and + basically looks up the hostname to IP address translation in DHCP server + records. Therefore this is dependent on hostname provided by guests. Thing + is, not all the guests out there provide one in DHCP transactions, nor
I am being said (often) that "nor" can only be combined with "neither" and not even always (because I do that mistake often as well) and that "or" should be used in this context instead. Native speaker should probably correct me or you.
+ every sysadmin out there believes all the guests. Hence libvirt implements + second method in <code>libvirt-guest</code> module which does libvirt guest + name to IP address translation (regardless of hostname set in the guest). + </p> + + <p> + To enable either of the modules put their name into the + <code>nsswitch.conf</code> file. For instance, to enable + <code>libvirt-guest</code> module: + </p> + +<pre> +$ cat /etc/nsswitch.conf +# /etc/nsswitch.conf: +hosts: files libvirt-guest dns +# ... +</pre> + <p>Or users can enable both at the same time:</p> +<pre> +$ cat /etc/nsswitch.conf +# /etc/nsswitch.conf: +hosts: files libvirt libvirt-guest dns +# ... +</pre> + + <p> + This configuration will mean that if hostname is not found by the + <code>libvirt</code> module (e.g. because a guest did not sent hostname + during DHCP transaction), the <code>libvirt-guest</code> module is + consulted (and if the hostname matches libvirt guest name it will be + resolved). + </p> + <h2><a name="Internals">How does it work?</a></h2>
<p> @@ -100,15 +142,18 @@ hosts: files libvirt dns <h2><a name="Limitations">Limitations</a></h2>
<ol> - <li>The libvirt NSS module matches only hostnames provided by guest. If - the libvirt name and one advertised by guest differs, the latter is - matched.</li> + <li><del>The libvirt NSS module matches only hostnames provided by guest. + If the libvirt name and one advertised by guest differs, the latter is + matched.</del> As of <code>v3.0.0</code> there are two libvirt NSS modules + translating both hostnames provided by guest and libvirt guest names.</li>
Is <del/> just strike-through text? What's the point in that? Just make the documentation reflect the newest, current, state.
<li>The module works only in that cases where IP addresses are assigned by dnsmasq spawned by libvirt. Libvirt NATed networks are typical example.</li> </ol>
<p> + <i>The following paragraph describes implementation limitation of the + <code>libvirt</code> NSS module.</i> These limitation are result of libvirt's internal implementation. While libvirt can report IP addresses regardless of their origin, a public API must be used to obtain those. However, for the API a connection object is @@ -134,8 +179,11 @@ virsh domifaddr --source lease $domain </pre>
<p> - If there's no record for either of the aforementioned commands, it's very - likely that NSS module won't find anything and vice versa. + <del>If there's no record for either of the aforementioned commands, it's + very likely that NSS module won't find anything and vice versa.</del> + As of <code>v3.0.0</code> libvirt provides <code>libvirt-guest</code> NSS + module that doesn't have this limitation. However, the statement is still + true for the <code>libvirt</code> NSS module.
Same here.
diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index 979bf81e1..6cadf8c43 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -267,6 +284,54 @@ findLease(const char *name, goto cleanup; }
+#else /* defined(LIBVIRT_NSS_GUEST) */ + + for (i = 0; i < nMacmaps; i++) { + const char **macs = (const char **) virMACMapMgrLookup(macmaps[i], name); + size_t j; + + if (!macs) + continue; + + for (j = 0; j < nleases; j++) { + virJSONValuePtr lease = virJSONValueArrayGet(leases_array, j); + const char *macAddr; + + if (!lease) { + /* This should never happen (TM) */ + ERROR("Unable to get element %zd of %zd", j, nleases); + goto cleanup; + } + + macAddr = virJSONValueObjectGetString(lease, "mac-address"); + if (!macAddr) + continue; + + if (!virStringListHasString(macs, macAddr)) + continue; + + if (virJSONValueObjectGetNumberLong(lease, "expiry-time", &expirytime) < 0) { + /* A lease cannot be present without expiry-time */ + ERROR("expiry-time field missing for %s", name); + goto cleanup; + } + + /* Do not report expired lease */ + if (expirytime < (long long) currtime) { + DEBUG("Skipping expired lease for %s", name); + continue; + } + + DEBUG("Found record for %s", name); + *found = true; + + if (appendAddr(&tmpAddress, &ntmpAddress, lease, af) < 0) + goto cleanup; + } + } + +#endif /* defined(LIBVIRT_NSS_GUEST) */ +
I feel like lot of this code is duplicated, but that can be fixed later. Also it might look more complicated and this is the best way to keep it. Did you try any other approach by any chance (just out of curiosity).
Hm.. good point. Let me see if I can fix that. Michal

On Mon, 2016-12-05 at 11:31 +0100, Michal Privoznik wrote: [...]
+<pre> +$ cat /etc/nsswitch.conf +# /etc/nsswitch.conf: +hosts: files libvirt-guest dns +# ... +</pre> + <p>Or users can enable both at the same time:</p> +<pre> +$ cat /etc/nsswitch.conf +# /etc/nsswitch.conf: +hosts: files libvirt libvirt-guest dns +# ... +</pre>
I just realized that, while the documentation consistently refers to the new module as "libvirt-guest", the module is actually called "libvirt_guest". In particular, the examples above won't work at all. We should change one or the other before release. -- Andrea Bolognani / Red Hat / Virtualization

On 01/13/2017 11:27 AM, Andrea Bolognani wrote:
On Mon, 2016-12-05 at 11:31 +0100, Michal Privoznik wrote: [...]
+<pre> +$ cat /etc/nsswitch.conf +# /etc/nsswitch.conf: +hosts: files libvirt-guest dns +# ... +</pre> + <p>Or users can enable both at the same time:</p> +<pre> +$ cat /etc/nsswitch.conf +# /etc/nsswitch.conf: +hosts: files libvirt libvirt-guest dns +# ... +</pre>
I just realized that, while the documentation consistently refers to the new module as "libvirt-guest", the module is actually called "libvirt_guest". In particular, the examples above won't work at all.
We should change one or the other before release.
Ah, good catch. Unfortunately, due to way that glibc works, we cannot use libvirt-guest and have to stick with libvirt_guest. The problem is: when gethost*() is called, glibc loads all the plugins listed in nsswitch.conf and tries to see if they implement the name translation function. This function, however, has to be unique otherwise symbol clash would occur. Therefore glibc developers decided to go with: _nss_$(pluginName)_$(implemntedFunction)_r Now, if $pluginName was "libvirt-guest" you could hardly declare the function in C as its name would have to be: _nss_libvirt-guest_gethostbyname_r C compiler sees two tokens there. Therefore we have to stick with a bit more ugly "libvirt_guest" name. Patch on its way. Michal

On Mon, Dec 05, 2016 at 11:31:46AM +0100, Michal Privoznik wrote:
v2 of:
https://www.redhat.com/archives/libvir-list/2016-November/msg01456.html
I feel like this can go in if the nits are solved (fixed or explained). So conditional ACK for that.

On 05.12.2016 17:31, Martin Kletzander wrote:
On Mon, Dec 05, 2016 at 11:31:46AM +0100, Michal Privoznik wrote:
v2 of:
https://www.redhat.com/archives/libvir-list/2016-November/msg01456.html
I feel like this can go in if the nits are solved (fixed or explained). So conditional ACK for that.
Thank you. I think everything has been either fixed or discussed so I've pushed these. Michal
participants (3)
-
Andrea Bolognani
-
Martin Kletzander
-
Michal Privoznik