Re: [PATCH v2 06/10] util: Introduce virMACMap module

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux