[libvirt] [PATCH v2 06/10] util: Introduce virMACMap module
Martin Kletzander
mkletzan at redhat.com
Tue Dec 6 12:10:22 UTC 2016
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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161206/c86e4c97/attachment-0001.sig>
More information about the libvir-list
mailing list