6:10 a.m.
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(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.)
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