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)
+ *
+ *  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 };
+    char MACStr[VIR_MAC_STRING_BUFLEN];
+    int oldVlanTag = -1;
+    char *filePath = NULL;
+    char *fileStr = NULL;
+    virBuffer buf = VIR_BUFFER_INITIALIZER;
+
+    if (vf >= 0) {
+        /* linkdev is the PF */
+        pfDevName = linkdev;
+
+        /* linkdev should get the VF's netdev name (or NULL if none) */
+        if (virNetDevPFGetVF(pfDevName, vf, &vfDevOrig) < 0)
+            goto cleanup;
+
+        linkdev = vfDevOrig;
+
+    } else if (saveVlan && virNetDevIsVirtualFunction(linkdev) == 1) {
+        /* when vf is -1, linkdev might be a standard netdevice (not
+         * SRIOV), or it might be an SRIOV VF. If it's a VF, normalize
+         * it to PF + VFname
+         */
+
+        if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)
+            goto cleanup;
+
+        pfDevName = pfDevOrig;
+
+        if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)
+            goto cleanup;
+    }
+
+    /* if there is a PF, it's now in pfDevName, and linkdev is either
+     * the VF's name, or NULL (if the VF isn't bound to a net driver
+     * on the host)
+     */
+
+    if (pfDevName) {
+        /* get admin MAC and vlan tag */
+        if (virNetDevGetVfConfig(pfDevName, vf, &oldMAC,
+                                 saveVlan ? &oldVlanTag : NULL) < 0) {
+            goto cleanup;
+        }
+
+        virBufferAsprintf(&buf, "%s\n%d\n",
+                          virMacAddrFormat(&oldMAC, MACStr), oldVlanTag);
+
+        if (virAsprintf(&filePath, "%s/%s_vf%d", stateDir, pfDevName, vf) < 0)
+            goto cleanup;
+
+    } else {
+        if (virAsprintf(&filePath, "%s/%s", stateDir, linkdev) < 0)
+            goto cleanup;
+    }
+
+    if (linkdev) {
+        if (virNetDevGetMAC(linkdev, &oldMAC) < 0)
+            goto cleanup;
+
+        /* for interfaces with no pfDevName, this will be the first
+         * line of the file. For interfaces that do have pfDevName,
+         * this will be the 3rd line of the file.
+         */
+        virBufferAsprintf(&buf, "%s\n", virMacAddrFormat(&oldMAC, MACStr));
+    }
+
+    if (!(fileStr = virBufferContentAndReset(&buf)))
+        goto cleanup;
+
+    if (virFileWriteStr(filePath, fileStr, O_CREAT|O_TRUNC|O_WRONLY) < 0) {
+        virReportSystemError(errno, _("Unable to preserve mac/vlan tag "
+                                      "for device = %s, vf = %d"), linkdev, vf);
+        goto cleanup;
+    }
+
+    ret = 0;
+ cleanup:
+    VIR_FREE(pfDevOrig);
+    VIR_FREE(vfDevOrig);
+    VIR_FREE(filePath);
+    VIR_FREE(fileStr);
+    virBufferFreeAndReset(&buf);
+    return ret;
+}
+
+
+/**
+ * virNetDevReadNetConfig:
+ * @linkdev: name of the interface
+ * @vf: vf index if linkdev is a pf
+ * @stateDir: directory where net config is stored
+ * @adminMAC: returns admin MAC to store in the PF (if this is a VF)
+ * @MAC: returns MAC to set on device immediately
+ *
+ * Read saved MAC address and (if linkdev itself is a VF, or if @vf >=
+ * 0) "admin MAC address" and vlan tag of the device described by
+ * @linkdev:@vf from a file in @stateDir. (see virNetDevSaveNetConfig
+ * for details of file name and format).
+ *
+ * Returns 0 on success, -1 on failure.
+ *
+ * The caller MUST free adminMAC, vlan, and MAC when it is finished
+ * with them (they will be NULL if they weren't found in the file)
+ *
+ */
+int
+virNetDevReadNetConfig(const char *linkdev, int vf,
+                       const char *stateDir,
+                       virMacAddrPtr *adminMAC,
+                       virNetDevVlanPtr *vlan,
+                       virMacAddrPtr *MAC)
+{
+    int ret = -1;
+    const char *pfDevName = NULL;
+    char *pfDevOrig = NULL;
+    char *vfDevOrig = NULL;
+    char *filePath = NULL;
+    char *fileStr = NULL;
+    /* the following two do *not* point to strings that need to be freed! */
+    char *vlanStr = NULL;
+    char *MACStr = NULL;
+
+    *adminMAC = NULL;
+    *vlan = NULL;
+    *MAC = NULL;
+
+    if (vf >= 0) {
+        /* linkdev is the PF */
+        pfDevName = linkdev;
+
+        /* linkdev should get the VF's netdev name (or NULL if none) */
+        if (virNetDevPFGetVF(pfDevName, vf, &vfDevOrig) < 0)
+            goto cleanup;
+
+        linkdev = vfDevOrig;
+
+    } else if (virNetDevIsVirtualFunction(linkdev) == 1) {
+        /* when vf is -1, linkdev might be a standard netdevice (not
+         * SRIOV), or it might be an SRIOV VF. If it's a VF, normalize
+         * it to PF + VFname
+         */
+
+        if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)
+            goto cleanup;
+
+        pfDevName = pfDevOrig;
+
+        if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)
+            goto cleanup;
+    }
+
+    /* if there is a PF, it's now in pfDevName, and linkdev is either
+     * the VF's name, or NULL (if the VF isn't bound to a net driver
+     * on the host)
+     */
+
+    if (pfDevName) {
+        if (virAsprintf(&filePath, "%s/%s_vf%d", stateDir, pfDevName, vf) < 0)
+            goto cleanup;
+
+        if (linkdev && !virFileExists(filePath)) {
+            /* the device may have been stored in a file named for the
+             * VF due to saveVlan == false (or an older version of
+             * libvirt), so reset filePath so we'll try the other
+             * filename before failing.
+             */
+            VIR_FREE(filePath);
+            pfDevName = NULL;
+        }
+    }
+
+    if (!pfDevName) {
+        if (virAsprintf(&filePath, "%s/%s", stateDir, linkdev) < 0)
+            goto cleanup;
+    }
+
+    if (virFileReadAll(filePath, 128, &fileStr) < 0)
+        goto cleanup;
+
+    /* find the (up to) 3 lines in the input file */
+    if ((vlanStr = strchr(fileStr, '\n'))) {
+        vlanStr++;
+        if (*vlanStr) {
+            MACStr = strchr(vlanStr, '\n');
+            if (MACStr)
+                MACStr++;
+        }
+    }
+
+    if (VIR_ALLOC(*MAC) < 0)
+        goto cleanup;
+
+    if (virMacAddrParse(fileStr, *MAC) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Cannot parse MAC address from "
+                         "line 1 of '%s': '%s'"),
+                       filePath, fileStr);
+        goto cleanup;
+    }
+
+    if (vlanStr) {
+        int vlanTag = -1;
+        char *endptr;
+
+        if ((virStrToLong_i(vlanStr, &endptr, 10, &vlanTag) < 0) ||
+            (endptr && *endptr != '\n' && *endptr != 0)) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("Cannot parse vlan tag from "
+                             "line 2 of '%s': '%s'"),
+                           filePath, vlanStr);
+            goto cleanup;
+        }
+
+        if (vlanTag != -1) {
+            /* construct a simple virNetDevVlan object with a single
+             * tag
+             */
+            if (VIR_ALLOC(*vlan) < 0)
+                goto cleanup;
+            if (VIR_ALLOC((*vlan)->tag) < 0)
+                goto cleanup;
+            (*vlan)->nTags = 1;
+            (*vlan)->tag[0] = vlanTag;
+        }
+    }
+
+    if (MACStr) {
+        /* If there is a 3rd line, then the first MAC was adminMAC,
+         * and this line will be MAC
+         */
+        *adminMAC = *MAC;
+        if (VIR_ALLOC(*MAC) < 0)
+            goto cleanup;
+
+        if (virMacAddrParse(MACStr, *MAC) < 0) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("Cannot parse MAC address from "
+                             "line 3 of '%s': '%s'"),
+                           filePath, MACStr);
+            goto cleanup;
+        }
+    }
+
+    /* we won't need the file again */
+    ignore_value(unlink(filePath));
+
+    ret = 0;
+ cleanup:
+    if (ret < 0) {
+        VIR_FREE(*adminMAC);
+        VIR_FREE(*MAC);
+        VIR_FREE(*vlan);
+    }
+
+    VIR_FREE(pfDevOrig);
+    VIR_FREE(vfDevOrig);
+    VIR_FREE(filePath);
+    VIR_FREE(fileStr);
+    return ret;
+}
+
+
+/**
+ * virNetDevSetNetConfig:
+ * @linkdev: name of the interface
+ * @vf: vf index if linkdev is a PF
+ * @adminMAC: new admin MAC address (will be stored in PF and
+ *            used for next initialization of VF driver)
+ * @vlan: new vlan tag info (or NULL)
+ * @MAC: new MAC address to set on the device immediately
+ * @setVlan: true to enable setting vlan tag (even if @vlan is NULL,
+ *           the interface vlan tag will be set to 0).
+ *
+ *
+ * Set new MAC address and (optionally) admin MAC and vlan tag of
+ * @linkdev VF# @vf.
+ *
+ * Returns 0 on success, -1 on failure
+ *
+ */
+int
+virNetDevSetNetConfig(const char *linkdev, int vf,
+                      const virMacAddr *adminMAC,
+                      virNetDevVlanPtr vlan,
+                      const virMacAddr *MAC,
+                      bool setVlan)
+{
+    int ret = -1;
+    char MACStr[VIR_MAC_STRING_BUFLEN];
+    const char *pfDevName = NULL;
+    char *pfDevOrig = NULL;
+    char *vfDevOrig = NULL;
+    int vlanTag = -1;
+
+    if (vf >= 0) {
+        /* linkdev is the PF */
+        pfDevName = linkdev;
+
+        /* linkdev should get the VF's netdev name (or NULL if none) */
+        if (virNetDevPFGetVF(pfDevName, vf, &vfDevOrig) < 0)
+            goto cleanup;
+
+        linkdev = vfDevOrig;
+
+    } else if (virNetDevIsVirtualFunction(linkdev) == 1) {
+        /* when vf is -1, linkdev might be a standard netdevice (not
+         * SRIOV), or it might be an SRIOV VF. If it's a VF, normalize
+         * it to PF + VFname
+         */
+
+        if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)
+            goto cleanup;
+
+        pfDevName = pfDevOrig;
+
+        if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)
+            goto cleanup;
+    }
+
+
+    if (!pfDevName) {
+        /* if it's not SRIOV, then we can't set the admin MAC address
+         * or vlan tag
+         */
+        if (adminMAC) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                           _("admin MAC can only be set for SR-IOV VFs, but "
+                             "%s is not a VF"), linkdev);
+            goto cleanup;
+        }
+
+        if (vlan) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                           _("vlan can only be set for SR-IOV VFs, but "
+                             "%s is not a VF"), linkdev);
+            goto cleanup;
+        }
+
+    } else {
+        bool pfIsOnline;
+
+        /* Assure that PF is online before trying to use it to set
+         * anything up for this VF. It *should* be online already,
+         * but if it isn't online the changes made to the VF via the
+         * PF won't take effect, yet there will be no error
+         * reported. In the case that the PF isn't online, we need to
+         * fail and report the error, rather than automatically
+         * setting it online, since setting an unconfigured interface
+         * online automatically turns on IPv6 autoconfig, which may
+         * not be what the admin expects, so we require them to
+         * explicitly enable the PF in the host system network config.
+         */
+        if (virNetDevGetOnline(pfDevName, &pfIsOnline) < 0)
+            goto cleanup;
+
+        if (!pfIsOnline) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("Unable to configure VF %d of PF '%s' "
+                             "because the PF is not online. Please "
+                             "change host network config to put the "
+                             "PF online."),
+                           vf, pfDevName);
+            goto cleanup;
+        }
+
+        if (vlan) {
+            if (vlan->nTags != 1 || vlan->trunk) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                               _("vlan trunking is not supported "
+                                 "by SR-IOV network devices"));
+                goto cleanup;
+            }
+
+            if (!setVlan) {
+                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                               _("vlan tag set for interface %s but "
+                                 "caller requested it not be set"));
+                goto cleanup;
+            }
+
+            vlanTag = vlan->tag[0];
+
+        } else if (setVlan) {
+            vlanTag = 0; /* assure any existing vlan tag is reset */
+        }
+    }
+
+    if (MAC) {
+        if (!linkdev) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("VF %d of PF '%s' is not bound to a net driver, "
+                             "so its MAC address cannot be set to %s"),
+                           vf, pfDevName, virMacAddrFormat(MAC, MACStr));
+            goto cleanup;
+        }
+
+        if (virNetDevSetMAC(linkdev, MAC) < 0) {
+            /* This may have failed due to the "administratively
+             * set" flag being set in the PF for this VF. For now
+             * we will just fail, but in the future we should
+             * attempt to set the VF MAC via the PF.
+             */
+            goto cleanup;
+        }
+        if (pfDevOrig) {
+            /* if pfDevOrig is set, it means that the caller was
+             * *really* only interested in setting the MAC of the VF
+             * itself, *not* the admin MAC via the PF. In those cases,
+             * the adminMAC was only provided in case we need to set
+             * the VF's MAC by temporarily unbinding/rebinding the
+             * VF's net driver with the admin MAC set to the desired
+             * MAC, and then want to restore the admin MAC to its
+             * original setting when we're finished. We would only
+             * need to do that if the virNetDevSetMAC() above had
+             * failed; since it didn't, we don't need to set the
+             * adminMAC, so we are NULLing it out here to avoid that
+             * below.
+
+             * (NB: since setting the admin MAC sets the
+             * "administratively set" flag for the VF in the PF's
+             * driver, which prevents any future changes to the VF's
+             * MAC address, we want to avoid setting the admin MAC as
+             * much as possible.)
+             */
+            adminMAC = NULL;
+        }
+    }
+
+    if (adminMAC || vlanTag >= 0) {
+        /* Set vlanTag and admin MAC using an RTM_SETLINK request sent to
+         * PFdevname+VF#, if mac != NULL this will set the "admin MAC" via
+         * the PF, *not* the actual VF MAC - the admin MAC only takes
+         * effect the next time the VF's driver is initialized (either in
+         * guest or host). if there is a vlanTag to set, it will take
+         * effect immediately though.
+         */
+        if (virNetDevSetVfConfig(pfDevName, vf, adminMAC, vlanTag) < 0)
+            goto cleanup;
+    }
+
+    ret = 0;
+ cleanup:
+    VIR_FREE(pfDevOrig);
+    VIR_FREE(vfDevOrig);
+    return ret;
+}
+
+
 #else /* defined(__linux__) && defined(HAVE_LIBNL) */
 
 int
@@ -1924,6 +2414,47 @@ virNetDevRestoreNetConfig(const char *linkdev ATTRIBUTE_UNUSED,
     return -1;
 }
 
+
+int
+virNetDevSaveNetConfig(const char *linkdev ATTRIBUTE_UNUSED,
+                       int vf ATTRIBUTE_UNUSED,
+                       const char *stateDir ATTRIBUTE_UNUSED,
+                       bool saveVlan ATTRIBUTE_UNUSED)
+{
+    virReportSystemError(ENOSYS, "%s",
+                         _("Unable to save net device config on this platform"));
+    return -1;
+}
+
+
+int
+virNetDevReadNetConfig(const char *linkdev ATTRIBUTE_UNUSED,
+                       int vf ATTRIBUTE_UNUSED,
+                       const char *stateDir ATTRIBUTE_UNUSED,
+                       virMacAddrPtr *adminMAC ATTRIBUTE_UNUSED,
+                       virNetDevVLanPtr *vlan ATTRIBUTE_UNUSED,
+                       virMacAddrPtr *MAC ATTRIBUTE_UNUSED)
+{
+    virReportSystemError(ENOSYS, "%s",
+                         _("Unable to read net device config on this platform"));
+    return -1;
+}
+
+
+int
+virNetDevSetNetConfig(const char *linkdev ATTRIBUTE_UNUSED,
+                      int vf ATTRIBUTE_UNUSED,
+                      const virMacAddr *adminMAC ATTRIBUTE_UNUSED,
+                      virNetDevVlanPtr vlan ATTRIBUTE_UNUSED,
+                      const virMacAddr *MAC ATTRIBUTE_UNUSED,
+                      bool setVlan ATTRIBUTE_UNUSED)
+{
+    virReportSystemError(ENOSYS, "%s",
+                         _("Unable to set net device config on this platform"));
+    return -1;
+}
+
+
 #endif /* defined(__linux__) && defined(HAVE_LIBNL) */
 
 VIR_ENUM_IMPL(virNetDevIfState,
diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
index ecc28c8..e153d71 100644
--- a/src/util/virnetdev.h
+++ b/src/util/virnetdev.h
@@ -189,6 +189,28 @@ int virNetDevGetVirtualFunctions(const char *pfname,
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
     ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK;
 
+int virNetDevSaveNetConfig(const char *linkdev, int vf,
+                           const char *stateDir,
+                           bool saveVlan)
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK;
+
+int
+virNetDevReadNetConfig(const char *linkdev, int vf,
+                       const char *stateDir,
+                       virMacAddrPtr *adminMAC,
+                       virNetDevVlanPtr *vlan,
+                       virMacAddrPtr *MAC)
+   ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4)
+   ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6) ATTRIBUTE_RETURN_CHECK;
+
+int
+virNetDevSetNetConfig(const char *linkdev, int vf,
+                      const virMacAddr *adminMAC,
+                      virNetDevVlanPtr vlan,
+                      const virMacAddr *MAC,
+                      bool setVLan)
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
+
 int virNetDevReplaceNetConfig(const char *linkdev, int vf,
                               const virMacAddr *macaddress,
                               virNetDevVlanPtr vlan,
-- 
2.9.3

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

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?

> + *
> + *  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)); 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

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

On Fri, Mar 17, 2017 at 03:40:33PM +0100, Michal Privoznik wrote:
> On 03/17/2017 02:58 PM, Laine Stump wrote:
> > 
> > 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.

Agreed, if I were starting libvirt from scratch I don't think we'd use XML,
but rather JSON / YAML in APIs. Given where we are today though, our normal
practice should be to use XML in any public APIs. For stuff not related to
public APIs, we should choose the virConf format or JSON as appropriate.
virConf if a simple flat set of data, JSON for anything needing structure

Definitely don't invent any new text formats, unless it is needed for
interoperability with a pre-existing apps we need to integrate with.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

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

On 03/17/2017 03:40 PM, Michal Privoznik wrote:
>
> 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 as promised, here's my code:

https://github.com/zippy2/libvirt/commit/4e0538298f91ce62153a9e11f9eb6b198f318864

Caution: I haven't tested it and frankly found it quite difficult to get 
right. The parsing that was there too big for my small brain. So perhaps 
in the parser I've exchanged adminMAC and MAC? You'll see when you test it.

Michal

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

On 03/21/2017 07:21 AM, Michal Privoznik wrote:
> On 03/17/2017 03:40 PM, Michal Privoznik wrote:
>>
>> 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 as promised, here's my code:
> 
> https://github.com/zippy2/libvirt/commit/4e0538298f91ce62153a9e11f9eb6b198f318864

Thanks for doing that. Made it *much* easier to pick out which were the
useful functions and how to use them. I modified what you did a bit and
squashed it into this patch. (I made the file reader able to read both
the old and new version of the file, for those cases where a libvirt
upgrade is done while a domain is active).

Since you had ACKed everything in the series except this one, I pushed 1
- 10 (after making the changes you suggested) and am reposting the
remainder of the series starting with this one (only for completeness in
case you want to test it yourself - I've only made the minor changes you
suggested to the other patches (and I have tested the scenarios I can
think of and it's doing the right thing).

> Caution: I haven't tested it and frankly found it quite difficult to get
> right. The parsing that was there too big for my small brain. So perhaps
> in the parser I've exchanged adminMAC and MAC? You'll see when you test it.

Yep, you did swap them, but that's fine. It's easy to understand why you
would swap them. There are some cases where line 1 of the file would be
the adminMAC, and some cases where it would be MAC (for backward
compatibility with hostdev and macvtap vf and non-vf). Now that it's
JSON, that confusion is a thing of the past. I'm glad you insisted on
fixing this now rather than continuing to propagate it :-)

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