8:58 a.m.
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