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

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

 



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



[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