[libvirt] [PATCH 11/19] util: new functions virNetDev(Save|Read|Set)NetConfig()

[Michal Privoznik]

On 03/17/2017 02:58 PM, Laine Stump wrote:
> On 03/17/2017 09:32 AM, Michal Privoznik wrote:
>> On 03/10/2017 09:35 PM, Laine Stump wrote:
>>> These three functions are destined to replace
>>> virNetDev(Replace|Restore)NetConfig() and
>>> virNetDev(Replace|Restore)MacAddress(), which both do the save and set
>>> together as a single step. We need to separate the save, read, and set
>>> steps because there will be situations where we need to do something
>>> else in between (in particular, we will need to rebind a VF's driver
>>> after save but before set).
>>>
>>> This patch creates the new functions, but doesn't call them - that
>>> will come in a subsequent patch.
>>> ---
>>>  src/libvirt_private.syms |   3 +
>>>  src/util/virnetdev.c     | 531
>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>  src/util/virnetdev.h     |  22 ++
>>>  3 files changed, 556 insertions(+)
>>>
>>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>>> index e9705ae..c983438 100644
>>> --- a/src/libvirt_private.syms
>>> +++ b/src/libvirt_private.syms
>>> @@ -1998,6 +1998,7 @@ virNetDevIfStateTypeFromString;
>>>  virNetDevIfStateTypeToString;
>>>  virNetDevIsVirtualFunction;
>>>  virNetDevPFGetVF;
>>> +virNetDevReadNetConfig;
>>>  virNetDevReplaceMacAddress;
>>>  virNetDevReplaceNetConfig;
>>>  virNetDevRestoreMacAddress;
>>> @@ -2007,11 +2008,13 @@ virNetDevRxFilterFree;
>>>  virNetDevRxFilterModeTypeFromString;
>>>  virNetDevRxFilterModeTypeToString;
>>>  virNetDevRxFilterNew;
>>> +virNetDevSaveNetConfig;
>>>  virNetDevSetMAC;
>>>  virNetDevSetMTU;
>>>  virNetDevSetMTUFromDevice;
>>>  virNetDevSetName;
>>>  virNetDevSetNamespace;
>>> +virNetDevSetNetConfig;
>>>  virNetDevSetOnline;
>>>  virNetDevSetPromiscuous;
>>>  virNetDevSetRcvAllMulti;
>>> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
>>> index 49a11f3..feb5ba7 100644
>>> --- a/src/util/virnetdev.c
>>> +++ b/src/util/virnetdev.c
>>> @@ -1899,6 +1899,496 @@ virNetDevRestoreNetConfig(const char *linkdev,
>>> int vf, const char *stateDir)
>>>      return ret;
>>>  }
>>>
>>> +
>>> +/**
>>> + * virNetDevSaveNetConfig:
>>> + * @linkdev: name of the interface
>>> + * @vf: vf index if linkdev is a pf
>>> + * @stateDir: directory to store old net config
>>> + * @saveVlan: false if we shouldn't attempt to save vlan tag info
>>> + *            (eg for interfaces using 802.1Qbg, since it handles
>>> + *            vlan tags internally)
>>> + *
>>> + * Save current MAC address and (if linkdev itself is a VF, or if @vf
>>> + * >= 0) the "admin MAC address" and vlan tag the device described by
>>> + * @linkdev:@vf to @stateDir. (the "admin MAC address" is stored in
>>> + * the PF, and is what the VF MAC will be initialized to the next time
>>> + * its driver is reloaded (either on host or guest).
>>> + *
>>> + * File Name and Format:
>>> + *
>>> + *  If the device is a VF and we're allowed to save vlan tag info, the
>>> + *  file will be named ${pfDevName_vf#{vf} (e.g. "enp2s0f0_vf5") and
>>> + *  will contain 2 or 3 lines of text:
>>> + *
>>> + *      line 1 - admin MAC address
>>> + *      line 2 - vlan tag
>>> + *      line 3 - VF MAC address (or missing if VF has no host net
>>> driver)
>>
>> I'd rather see a JSON format or something. This can bite us in the
>> future. What are your thoughts?
>
>
> Every time I touch the code that uses these files I have the same
> thought - "%$&*($% this is ugly, and it's bound to cause a headache
> someday. We should change it." But that's always a secondary issue about
> an existing condition, and I'm chasing something "more important" (and I
> figure that when the day comes that we really *need* to switch to a
> better format, it will be no more difficult to detect which is being
> used than it would be today; that makes it morally easier to procrastinate)
>
> I suppose I can spend some time and write a function that parses
> something more expandable, with a fallback to the "old" method based on
> what's seen at the beginning of the file.
>
> Why JSON rather than XML though? I don't have a real preference over one
> or the other, but libvirt lore is *steeped* in XML, and all the other
> libvirt config files are XML...

As discussed on IRC, I can write the code to save/parse the JSON here. 
You've done your part. However, I'm not sure I will manage to make it 
happen today, but maybe beginning of the next week if that is okay with you.

And for the future reference: I prefer JSON over XML because I find it 
producing smaller files in terms of size. And also easier to read by us 
humans at a first glance. These are the reasons I've gone with JSON in 
NSS modules. Unfortunately, we have to stick with XML for out public 
APIs, but for storing some pieces of internal information, we can use 
other formats too.

>
>
>>
>>> + *
>>> + *  If the device isn't a VF, or we're not allowed to save vlan tag
>>> + *  info, the file will be named ${linkdev} (e.g. "enp3s0f0") and will
>>> + *  contain a single line of text containing linkdev's MAC address.
>>> + *
>>> + * Returns 0 on success, -1 on failure
>>> + *
>>> + */
>>> +int
>>> +virNetDevSaveNetConfig(const char *linkdev, int vf,
>>> +                       const char *stateDir,
>>> +                       bool saveVlan)
>>> +{
>>> +    int ret = -1;
>>> +    const char *pfDevName = NULL;
>>> +    char *pfDevOrig = NULL;
>>> +    char *vfDevOrig = NULL;
>>> +    virMacAddr oldMAC = { 0 };
>>
>> This causes a compile error for me. calling memset(&oldMAC, 0,
>> sizeof(oldMAC));
>
> Strange. You must be running a newer compiler than me (I'm doing
> everything on F25 + updates-testing). I also wonder why I thought I
> needed to initialize it, and why I did it that way, since in other cases
> I use "= { .addr = { blah } };". (I think most likely at some point
> early on I had thought that I might end up saving it even if it hadn't
> been set, so I wanted it to be consistent. But it ended up that I only
> save it if it was set, so as you say, the initialization is pointless.)

Don't get me wrong. I like initialized variables. But in this specific 
case it broke the compilation for me. Oh, and also I probably did not 
point it out - the same is happening in 18/19 with @zeroMAC global 
variable. The correct form is:

static virMacAddr zeroMAC = { .addr = { 0 } };

Michal