4:11 a.m.
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(a)redhat.com>
> ---
> po/POTFILES.in | 1 +
> src/Makefile.am | 1 +
> src/libvirt_private.syms | 9 +
> src/util/virmacmap.c | 399
> +++++++++++++++++++++++++++++++++++
> src/util/virmacmap.h | 48 +++++
> tests/Makefile.am | 14 ++
> tests/virmacmapmock.c | 29 +++
> tests/virmacmaptest.c | 232 ++++++++++++++++++++
> tests/virmacmaptestdata/complex.json | 45 ++++
> tests/virmacmaptestdata/empty.json | 3 +
> tests/virmacmaptestdata/simple.json | 8 +
> tests/virmacmaptestdata/simple2.json | 16 ++
> 12 files changed, 805 insertions(+)
> create mode 100644 src/util/virmacmap.c
> create mode 100644 src/util/virmacmap.h
> create mode 100644 tests/virmacmapmock.c
> create mode 100644 tests/virmacmaptest.c
> create mode 100644 tests/virmacmaptestdata/complex.json
> create mode 100644 tests/virmacmaptestdata/empty.json
> create mode 100644 tests/virmacmaptestdata/simple.json
> create mode 100644 tests/virmacmaptestdata/simple2.json
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 9189f56fe..009a7b27c 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1927,6 +1927,15 @@ virMacAddrSet;
> virMacAddrSetRaw;
>
>
> +# util/virmacmap.h
> +virMACMapMgrAdd;
> +virMACMapMgrFlush;
> +virMACMapMgrFlushStr;
> +virMACMapMgrLookup;
> +virMACMapMgrNew;
> +virMACMapMgrRemove;
> +
What's the point of the "Mgr" part? It just adds mess to the name IMHO,
I'd like to see that removed.
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