[libvirt] [PATCH 0/8] Make NSS module lookup by libvirt domain names too
So far, the NSS module relies on guests providing hostnames during DHCP transaction, because if they do it's recorded in a json file that NSS module consults when doing the hostname translation. But there are few cases where this is not enough: some system (e.g. old RHEL6) don't provide hostname at all. If that's the case we can maintain a separate file $bridge.macs where we would keep a list of pairs <domain, mac address list>. Thus next time when doing hostname translation we can look up MAC address list for given domain, and then lookup the MAC <-> IP mapping in $bridge.status. It's a bit clunky, but it's our best shot. Michal Privoznik (8): 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: Lookup by libvirt domain names too cfg.mk | 2 +- docs/nss.html.in | 13 +- 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 | 87 ++++++++ src/util/virstring.h | 6 + src/util/virxml.c | 4 +- tests/Makefile.am | 14 ++ tests/nssdata/virbr0.macs | 23 ++ tests/nssdata/virbr0.status | 5 + tests/nssdata/virbr1.macs | 21 ++ tests/nssdata/virbr1.status | 5 + tests/nssmock.c | 25 ++- tests/nsstest.c | 2 + 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 | 80 +++++++ tools/nss/libvirt_nss.c | 173 +++++++++++---- 31 files changed, 1340 insertions(+), 86 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 -- 2.8.4
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 9d94d65..073f501 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.8.4
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 a4305a8..69e3f3a 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 b0259a3..273af06 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.8.4
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstring.c | 37 +++++++++++++++++++++++++++++++++++++ src/util/virstring.h | 3 +++ tests/virstringtest.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 83 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7cc7bf8..bd46e5f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2458,6 +2458,7 @@ virStringEncodeBase64; virStringHasControlChars; virStringIsEmpty; virStringIsPrintable; +virStringListAdd; virStringListFree; virStringListFreeCount; virStringListGetFirstWithPrefix; diff --git a/src/util/virstring.c b/src/util/virstring.c index 2ab252c..d2fb543 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -169,6 +169,43 @@ 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; + + for (i = 0; strings && strings[i]; i++) + ; + + 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 f6801fc..da9d35c 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 a11d7c5..43657c8 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.8.4
On Wednesday, 30 November 2016 10:59:30 CET Michal Privoznik wrote:
+char ** +virStringListAdd(const char **strings, + const char *item) +{ + char **ret = NULL; + size_t i; + + for (i = 0; strings && strings[i]; i++) + ;
virStringListLength, I guess. -- Pino Toscano
On 30.11.2016 11:22, Pino Toscano wrote:
On Wednesday, 30 November 2016 10:59:30 CET Michal Privoznik wrote:
+char ** +virStringListAdd(const char **strings, + const char *item) +{ + char **ret = NULL; + size_t i; + + for (i = 0; strings && strings[i]; i++) + ;
virStringListLength, I guess.
Yeah, that could work too. For some reason I wanted to avoid another function call, but I guess compiler will optimize that anyway. Michal
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 | 38 ++++++++++++++++++++++++++++++++++++ 4 files changed, 92 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bd46e5f..b1f42f2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2465,6 +2465,7 @@ virStringListGetFirstWithPrefix; virStringListHasString; virStringListJoin; virStringListLength; +virStringListRemove; virStringReplace; virStringSearch; virStringSortCompare; diff --git a/src/util/virstring.c b/src/util/virstring.c index d2fb543..7d2c4c7 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -206,6 +206,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 da9d35c..88eacd4 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 43657c8..a421f7b 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -162,6 +162,42 @@ 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; + + if (!(list = virStringSplitCount(data->string, data->delim, + data->max_tokens, &ntokens))) { + VIR_DEBUG("Got no tokens at all"); + return -1; + } + + for (i = 0; data->tokens[i]; i++) { + char **tmp; + if (virStringListRemove((const char **) list, + &tmp, data->tokens[i]) < 0) + goto cleanup; + virStringListFree(list); + list = tmp; + tmp = NULL; + } + + 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 +672,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.8.4
On Wednesday, 30 November 2016 10:59:31 CET 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 | 38 ++++++++++++++++++++++++++++++++++++ 4 files changed, 92 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bd46e5f..b1f42f2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2465,6 +2465,7 @@ virStringListGetFirstWithPrefix; virStringListHasString; virStringListJoin; virStringListLength; +virStringListRemove; virStringReplace; virStringSearch; virStringSortCompare; diff --git a/src/util/virstring.c b/src/util/virstring.c index d2fb543..7d2c4c7 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -206,6 +206,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.
Wouldn't it be better to return the number of items removed? After all, if the size of the new list is needed, virStringListLength can be used.
+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; + }
Shouldn't this produce an empty list instead? I.e. I'd expect that: char **elems = { "foo", NULL }; char **newStrings; int count = virStringListRemove(elems, &newStrings, "foo"); assert(count == 0); assert(newStrings != NULL); assert(newStrings[0] == NULL); Ditto when trying to remove anything from an empty list. The exception would be when strings == NULL, so virStringListRemove should just directly set *newStrings to NULL and return 0.
+static int testRemove(const void *args) +{ + const struct testSplitData *data = args; + char **list = NULL; + size_t ntokens; + size_t i; + int ret = -1; + + if (!(list = virStringSplitCount(data->string, data->delim, + data->max_tokens, &ntokens))) { + VIR_DEBUG("Got no tokens at all"); + return -1; + } + + for (i = 0; data->tokens[i]; i++) { + char **tmp; + if (virStringListRemove((const char **) list, + &tmp, data->tokens[i]) < 0)
IMHO the test should also check the return value is what is expected. Thanks, -- Pino Toscano
On Wed, Nov 30, 2016 at 11:25:55AM +0100, Pino Toscano wrote:
On Wednesday, 30 November 2016 10:59:31 CET 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 | 38 ++++++++++++++++++++++++++++++++++++ 4 files changed, 92 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bd46e5f..b1f42f2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2465,6 +2465,7 @@ virStringListGetFirstWithPrefix; virStringListHasString; virStringListJoin; virStringListLength; +virStringListRemove; virStringReplace; virStringSearch; virStringSortCompare; diff --git a/src/util/virstring.c b/src/util/virstring.c index d2fb543..7d2c4c7 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -206,6 +206,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.
Wouldn't it be better to return the number of items removed? After all, if the size of the new list is needed, virStringListLength can be used.
I'm not sure to what use would such an information serve. The way the patch suggest removes the necessity for yet another call just to determine the length of the new list. In python, sure, that would be IMHO the correct reasoning as the intended use of the objects and its methods but since the caller is indeed responsible for freeing both of the lists, returning the size of the new list, stripped down from the non-desired elements, is a valuable information for free. Additionally, I'd expect the caller to also know how many elements they're trying to remove.
+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; + }
Shouldn't this produce an empty list instead? I.e. I'd expect that:
char **elems = { "foo", NULL }; char **newStrings; int count = virStringListRemove(elems, &newStrings, "foo"); assert(count == 0); assert(newStrings != NULL); assert(newStrings[0] == NULL);
I think in this case you might want to look at this as a black-box, you know what the function should return and that should be enough, therefore returning 0 should be an indicator that there either is a some rubbish assigned by the function or your variable wasn't touched at all, but you cannot tell, the function does not state explicitly what it does with it, you only know that you have to call free() unless there was an error returned by the function.
Ditto when trying to remove anything from an empty list.
The exception would be when strings == NULL, so virStringListRemove should just directly set *newStrings to NULL and return 0.
+static int testRemove(const void *args) +{ + const struct testSplitData *data = args; + char **list = NULL; + size_t ntokens; + size_t i; + int ret = -1; + + if (!(list = virStringSplitCount(data->string, data->delim, + data->max_tokens, &ntokens))) { + VIR_DEBUG("Got no tokens at all"); + return -1; + } + + for (i = 0; data->tokens[i]; i++) { + char **tmp; + if (virStringListRemove((const char **) list, + &tmp, data->tokens[i]) < 0)
IMHO the test should also check the return value is what is expected.
Agreed. Erik
Thanks, -- Pino Toscano
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 30.11.2016 11:25, Pino Toscano wrote:
On Wednesday, 30 November 2016 10:59:31 CET 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 | 38 ++++++++++++++++++++++++++++++++++++ 4 files changed, 92 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bd46e5f..b1f42f2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2465,6 +2465,7 @@ virStringListGetFirstWithPrefix; virStringListHasString; virStringListJoin; virStringListLength; +virStringListRemove; virStringReplace; virStringSearch; virStringSortCompare; diff --git a/src/util/virstring.c b/src/util/virstring.c index d2fb543..7d2c4c7 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -206,6 +206,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.
Wouldn't it be better to return the number of items removed? After all, if the size of the new list is needed, virStringListLength can be used.
Since this function is used at exactly one point in our source code I've tailored it to my needs. Moreover, it's internal function so we can change it later.
+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; + }
Shouldn't this produce an empty list instead? I.e. I'd expect that:
char **elems = { "foo", NULL }; char **newStrings; int count = virStringListRemove(elems, &newStrings, "foo"); assert(count == 0); assert(newStrings != NULL); assert(newStrings[0] == NULL);
Should it? All our StringList APIs threat { NULL } and NULL as the same empty string list.
Ditto when trying to remove anything from an empty list.
The exception would be when strings == NULL, so virStringListRemove should just directly set *newStrings to NULL and return 0.
+static int testRemove(const void *args) +{ + const struct testSplitData *data = args; + char **list = NULL; + size_t ntokens; + size_t i; + int ret = -1; + + if (!(list = virStringSplitCount(data->string, data->delim, + data->max_tokens, &ntokens))) { + VIR_DEBUG("Got no tokens at all"); + return -1; + } + + for (i = 0; data->tokens[i]; i++) { + char **tmp; + if (virStringListRemove((const char **) list, + &tmp, data->tokens[i]) < 0)
IMHO the test should also check the return value is what is expected.
Ah, good point. Will fix that. Michal
On Wed, Nov 30, 2016 at 10:59:31AM +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 | 38 ++++++++++++++++++++++++++++++++++++ 4 files changed, 92 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bd46e5f..b1f42f2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2465,6 +2465,7 @@ virStringListGetFirstWithPrefix; virStringListHasString; virStringListJoin; virStringListLength; +virStringListRemove; virStringReplace; virStringSearch; virStringSortCompare; diff --git a/src/util/virstring.c b/src/util/virstring.c index d2fb543..7d2c4c7 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -206,6 +206,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) {
In on of the reviews I received I was advised not to use this^^ with anything else than pointers, so I'd like to pass this advice further on.
+ *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 da9d35c..88eacd4 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 43657c8..a421f7b 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -162,6 +162,42 @@ 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; + + if (!(list = virStringSplitCount(data->string, data->delim, + data->max_tokens, &ntokens))) { + VIR_DEBUG("Got no tokens at all"); + return -1; + } + + for (i = 0; data->tokens[i]; i++) { + char **tmp; + if (virStringListRemove((const char **) list, + &tmp, data->tokens[i]) < 0)
As Pino noted, this is a functional primitive other APIs will rely on, so I think we'd better be sure it always returns the correct size, at least I usually don't run valgring on our test suite. Otherwise looks perfectly fine, ACK. Erik
+ goto cleanup; + virStringListFree(list); + list = tmp; + tmp = NULL; + } + + 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 +672,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.8.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 2bdfe08..1351f18 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 b1f42f2..0641030 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1600,6 +1600,7 @@ virFileRemoveLastComponent; virFileResolveAllLinks; virFileResolveLink; virFileRewrite; +virFileRewriteStr; virFileSanitizePath; virFileSkipRoot; virFileStripSuffix; diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c index 16f6eb8..a0262dd 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 1fb89ce..c9ecb8a 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 b4ae6ea..060067d 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 96c17fa..6660248 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.8.4
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 bdff679..b0a1ed4 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 d440548..a9106fa 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 0641030..963876b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1926,6 +1926,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 0000000..38c2ffd --- /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 0000000..6ebba7c --- /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 6a24861..9d5583d 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 0000000..d01f1c9 --- /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 0000000..a983b54 --- /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 0000000..192347c --- /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 0000000..41b42e6 --- /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 0000000..ea0fb08 --- /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 0000000..91b2cde --- /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.8.4
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 3b227db..e95d68d 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 073f501..435601a 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.8.4
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 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/nss.html.in | 13 ++-- src/Makefile.am | 8 ++ 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 | 2 + tools/nss/libvirt_nss.c | 173 ++++++++++++++++++++++++++++++++++---------- 9 files changed, 226 insertions(+), 45 deletions(-) create mode 100644 tests/nssdata/virbr0.macs create mode 100644 tests/nssdata/virbr1.macs diff --git a/docs/nss.html.in b/docs/nss.html.in index 84ea8df..dc0ebfa 100644 --- a/docs/nss.html.in +++ b/docs/nss.html.in @@ -100,9 +100,10 @@ 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>v2.6.0</code> the libvirt nss module + translates both hostnames provided by guest and libvirt domain 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> @@ -134,8 +135,10 @@ 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>v2.6.0</code> the NSS module should provide hostname + translation even if no output had been produced by aforementioned command. </p> </body> </html> diff --git a/src/Makefile.am b/src/Makefile.am index a9106fa..8c620d5 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/nssdata/virbr0.macs b/tests/nssdata/virbr0.macs new file mode 100644 index 0000000..45f9e2b --- /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 6ebe954..d040774 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 0000000..2140737 --- /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 f8a9157..4951d45 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 273af06..7929c30 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 8648c4a..3cf2795 100644 --- a/tests/nsstest.c +++ b/tests/nsstest.c @@ -189,6 +189,8 @@ mymain(void) 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); + DO_TEST("debian", AF_INET, "192.168.122.2"); + DO_TEST("suse", AF_INET, "192.168.122.3"); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index 0d59825..c7802fc 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(...) \ @@ -79,6 +81,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 @@ -112,11 +176,13 @@ findLease(const char *name, const char *leaseDir = LEASEDIR; struct dirent *entry; virJSONValuePtr leases_array = NULL; - ssize_t i, nleases; + ssize_t i, j, nleases; leaseAddress *tmpAddress = NULL; size_t ntmpAddress = 0; time_t currtime; long long expirytime; + virMACMapMgrPtr *macmaps = NULL; + size_t nMacmaps = 0; *address = NULL; *naddress = 0; @@ -127,7 +193,6 @@ findLease(const char *name, goto cleanup; } - if (virDirOpenQuiet(&dir, leaseDir) < 0) { ERROR("Failed to open dir '%s'", leaseDir); goto cleanup; @@ -142,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); @@ -172,9 +250,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 +279,51 @@ 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); + for (i = 0; i < nMacmaps; i++) { + const char **macs = (const char **) virMACMapMgrLookup(macmaps[i], name); - 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); + if (!macs) continue; - } - if (VIR_REALLOC_N_QUIET(tmpAddress, ntmpAddress + 1) < 0) { - ERROR("Out of memory"); - goto cleanup; - } + 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; - 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++; + 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; + } } *address = tmpAddress; @@ -248,6 +338,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; } -- 2.8.4
On Wed, Nov 30, 2016 at 10:59:35AM +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
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
So, IIUC, with this change the nss module is able to lookup based on hostname *or* the guest name. I think it is desirable if the admin can control which is used. In particular as an admin I'd like to prevent the ability to use hostname at all, since this data may come from an untrustworthy guest. IOW, should we actually create two separate NSS modules, one that does DHCP hostname based lookups and one that does guest name based lookups. Admins can then choose which to use, or even list both in nssswitch.conf Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
On 30.11.2016 11:16, Daniel P. Berrange wrote:
On Wed, Nov 30, 2016 at 10:59:35AM +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
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
So, IIUC, with this change the nss module is able to lookup based on hostname *or* the guest name.
Correct. If you have a libvirt domain 'fedora' but set its hostname to 'fedora2', both 'ping fedora' and 'ping fedora2' will work (and result in the same IP address). Without this patch just 'ping fedora2' would work.
I think it is desirable if the admin can control which is used. In particular as an admin I'd like to prevent the ability to use hostname at all, since this data may come from an untrustworthy guest.
Which can happen on a real network too. Guests can initialize DHCP transaction with spoofed hostname just to trick DNS. If admins don't want this to happen they just configure static DHCP/DNS. With libvirt, they don't enable the NSS module.
IOW, should we actually create two separate NSS modules, one that does DHCP hostname based lookups and one that does guest name based lookups. Admins can then choose which to use, or even list both in nssswitch.conf
I was thinking about this and honestly, I don't have preference. I could argue both ways. Ideally, there would be a way to pass arguments to an NSS module, but looks like there is none. I've seen the following in nsswitch.conf: netmasks: nisplus [NOTFOUND=return] files which would suggest so, but digging deep into glibc those are just args to glibc function that loads the modules and calls the functions from them. So yes, maybe we need two modules after all. Any suggestions on the naming? I'm out of ideas. Michal
On 30.11.2016 11:41, Michal Privoznik wrote:
On 30.11.2016 11:16, Daniel P. Berrange wrote:
On Wed, Nov 30, 2016 at 10:59:35AM +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
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
So, IIUC, with this change the nss module is able to lookup based on hostname *or* the guest name.
Correct. If you have a libvirt domain 'fedora' but set its hostname to 'fedora2', both 'ping fedora' and 'ping fedora2' will work (and result in the same IP address). Without this patch just 'ping fedora2' would work.
I think it is desirable if the admin can control which is used. In particular as an admin I'd like to prevent the ability to use hostname at all, since this data may come from an untrustworthy guest.
Which can happen on a real network too. Guests can initialize DHCP transaction with spoofed hostname just to trick DNS. If admins don't want this to happen they just configure static DHCP/DNS. With libvirt, they don't enable the NSS module.
IOW, should we actually create two separate NSS modules, one that does DHCP hostname based lookups and one that does guest name based lookups. Admins can then choose which to use, or even list both in nssswitch.conf
I was thinking about this and honestly, I don't have preference. I could argue both ways. Ideally, there would be a way to pass arguments to an NSS module, but looks like there is none. I've seen the following in nsswitch.conf:
netmasks: nisplus [NOTFOUND=return] files
which would suggest so, but digging deep into glibc those are just args to glibc function that loads the modules and calls the functions from them.
So yes, maybe we need two modules after all. Any suggestions on the naming? I'm out of ideas.
Just an idea: what if I rename the current module to libvirt_guest (and also install symlink named libvirt that would point to it - just to maintain backward compatibility). And this new module would be called libvirt_host. So that we would have: libvirt_guest: to resolve IP addresses based on what guests say libvirt_host: to resolve IP addresses based on what libvirt thinks the guest name is. Still crappy names though. Michal
On Thu, Dec 01, 2016 at 09:42:57AM +0100, Michal Privoznik wrote:
On 30.11.2016 11:41, Michal Privoznik wrote:
On 30.11.2016 11:16, Daniel P. Berrange wrote:
On Wed, Nov 30, 2016 at 10:59:35AM +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
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
So, IIUC, with this change the nss module is able to lookup based on hostname *or* the guest name.
Correct. If you have a libvirt domain 'fedora' but set its hostname to 'fedora2', both 'ping fedora' and 'ping fedora2' will work (and result in the same IP address). Without this patch just 'ping fedora2' would work.
I think it is desirable if the admin can control which is used. In particular as an admin I'd like to prevent the ability to use hostname at all, since this data may come from an untrustworthy guest.
Which can happen on a real network too. Guests can initialize DHCP transaction with spoofed hostname just to trick DNS. If admins don't want this to happen they just configure static DHCP/DNS. With libvirt, they don't enable the NSS module.
IOW, should we actually create two separate NSS modules, one that does DHCP hostname based lookups and one that does guest name based lookups. Admins can then choose which to use, or even list both in nssswitch.conf
I was thinking about this and honestly, I don't have preference. I could argue both ways. Ideally, there would be a way to pass arguments to an NSS module, but looks like there is none. I've seen the following in nsswitch.conf:
netmasks: nisplus [NOTFOUND=return] files
which would suggest so, but digging deep into glibc those are just args to glibc function that loads the modules and calls the functions from them.
So yes, maybe we need two modules after all. Any suggestions on the naming? I'm out of ideas.
Just an idea: what if I rename the current module to libvirt_guest (and also install symlink named libvirt that would point to it - just to maintain backward compatibility). And this new module would be called libvirt_host. So that we would have:
libvirt_guest: to resolve IP addresses based on what guests say libvirt_host: to resolve IP addresses based on what libvirt thinks the guest name is.
Still crappy names though.
I don't think naming hugely matters as long as we document which does what. Personally I'd go for "libvirt-dhcp" (DHCP recorded name) and "libvirt-guest" (Libvirt guest name), or just leave the current one called libvirt forever. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
I must say that using the xen-ism "domain" in contexts where you are dealing with name->ip resolution is quite confusing. I'd suggest to use VM instead at least for this series. On Wed, Nov 30, 2016 at 10:59:27 +0100, Michal Privoznik wrote:
So far, the NSS module relies on guests providing hostnames
Peter
On 30.11.2016 13:14, Peter Krempa wrote:
I must say that using the xen-ism "domain" in contexts where you are dealing with name->ip resolution is quite confusing. I'd suggest to use VM instead at least for this series.
Well I always thought that we have adopted that term and use VM/domain/guest interchangeably. But okay, I can switch to that. Michal
On Wed, Nov 30, 2016 at 13:39:22 +0100, Michal Privoznik wrote:
On 30.11.2016 13:14, Peter Krempa wrote:
I must say that using the xen-ism "domain" in contexts where you are dealing with name->ip resolution is quite confusing. I'd suggest to use VM instead at least for this series.
Well I always thought that we have adopted that term and use VM/domain/guest interchangeably. But okay, I can switch to that.
In most cases it's fine. But when dealing with DNS and similar stuff it's more confusing to use 'domain' as of the dual meaning.
participants (5)
-
Daniel P. Berrange -
Erik Skultety -
Michal Privoznik -
Peter Krempa -
Pino Toscano