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...
>
>> + *
>> + * 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.)
> solved it for me. Moreover, this variable will always
> be set before use. So your call wether to leave the initialization in or
> out.
>
>> + char MACStr[VIR_MAC_STRING_BUFLEN];
>> + int oldVlanTag = -1;
>> + char *filePath = NULL;
>> + char *fileStr = NULL;
>> + virBuffer buf = VIR_BUFFER_INITIALIZER;
>
> Michal
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
