[libvirt] [PATCH v4]: set and restore MAC address of a NIC when using PASSTHROUGH mode

Next try .. The following patch addresses the problem that when a PASSTHROUGH mode DIRECT NIC connection is made the MAC address of the NIC is not automatically set and reset to the configured VM MAC and back again. The attached patch fixes this problem by setting and resetting the MAC while remembering the previous setting while the VM is running. This also works if libvirtd is restarted while the VM is running. the patch passes make syntax-check Signed-off-by: Dirk Herrendoerfer <d.herrendoerfer at herrendoerfer.name> Signed-off-by: Gerhard Stenzel <gerhard.stenzel@de.ibm.com> --- Index: libvirt/src/qemu/qemu_command.c =================================================================== --- libvirt.orig/src/qemu/qemu_command.c +++ libvirt/src/qemu/qemu_command.c @@ -128,7 +128,7 @@ qemuPhysIfaceConnect(virDomainDefPtr def rc = openMacvtapTap(net->ifname, net->mac, net->data.direct.linkdev, net->data.direct.mode, vnet_hdr, def->uuid, &net->data.direct.virtPortProfile, &res_ifname, - vmop); + vmop, driver->stateDir); if (rc >= 0) { qemuAuditNetDevice(def, net, res_ifname, true); VIR_FREE(net->ifname); @@ -149,7 +149,9 @@ qemuPhysIfaceConnect(virDomainDefPtr def if (err) { VIR_FORCE_CLOSE(rc); delMacvtap(net->ifname, net->mac, net->data.direct.linkdev, - &net->data.direct.virtPortProfile); + net->data.direct.mode, + &net->data.direct.virtPortProfile, + driver->stateDir); VIR_FREE(net->ifname); } } Index: libvirt/src/qemu/qemu_process.c =================================================================== --- libvirt.orig/src/qemu/qemu_process.c +++ libvirt/src/qemu/qemu_process.c @@ -2876,7 +2876,8 @@ void qemuProcessStop(struct qemud_driver virDomainNetDefPtr net = def->nets[i]; if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { delMacvtap(net->ifname, net->mac, net->data.direct.linkdev, - &net->data.direct.virtPortProfile); + net->data.direct.mode, + &net->data.direct.virtPortProfile, driver->stateDir); VIR_FREE(net->ifname); } } Index: libvirt/src/util/macvtap.c =================================================================== --- libvirt.orig/src/util/macvtap.c +++ libvirt/src/util/macvtap.c @@ -545,6 +545,104 @@ configMacvtapTap(int tapfd, int vnet_hdr return 0; } +/** + * replaceMacAdress: + * @macaddress: new MAC address for interface + * @linkdev: name of interface + * @stateDir: directory to store old MAC address + * + * Returns 0 on success, -1 in case of fatal error, error code otherwise. + * + */ +static int +replaceMacAdress(const unsigned char *macaddress, + const char *linkdev, + char *stateDir) +{ + unsigned char oldmac[6]; + int rc; + + rc = ifaceGetMacaddr(linkdev, oldmac); + + if (rc) { + virReportSystemError(rc, + _("Getting MAC address from '%s' " + "to '%02x:%02x:%02x:%02x:%02x:%02x' failed."), + linkdev, + oldmac[0], oldmac[1], oldmac[2], + oldmac[3], oldmac[4], oldmac[5]); + } else { + char *path = NULL; + char macstr[VIR_MAC_STRING_BUFLEN]; + + if (virAsprintf(&path, "%s/%s", + stateDir, + linkdev) < 0) { + virReportOOMError(); + return errno; + } + virFormatMacAddr(oldmac, macstr); + if (virFileWriteStr(path, macstr, O_CREAT|O_TRUNC|O_WRONLY) < 0) { + virReportSystemError(errno, _("Unable to preserve mac for %s"), + linkdev); + return errno; + } + } + + rc = ifaceSetMacaddr(linkdev, macaddress); + if (rc) { + virReportSystemError(errno, + _("Setting MAC address on '%s' to " + "'%02x:%02x:%02x:%02x:%02x:%02x' failed."), + linkdev, + macaddress[0], macaddress[1], macaddress[2], + macaddress[3], macaddress[4], macaddress[5]); + } + return rc; +} + +/** + * restoreMacAddress: + * @linkdev: name of interface + * @stateDir: directory containing old MAC address + * + * Returns 0 on success, -1 in case of fatal error, error code otherwise. + * + */ +static int +restoreMacAddress(const char *linkdev, + char *stateDir) +{ + int ret; + char *oldmacname = NULL; + char *macstr = NULL; + char *path = NULL; + unsigned char oldmac[6]; + + if (virAsprintf(&path, "%s/%s", + stateDir, + linkdev) < 0) { + virReportOOMError(); + return -1; + } + + if (virFileReadAll(path, VIR_MAC_STRING_BUFLEN, &macstr) < 0) { + return errno; + } + + if (virParseMacAddr(macstr, &oldmac[0]) != 0) { + macvtapError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse MAC address from '%s'"), + oldmacname); + return -1; + } + + /*reset mac and remove file-ignore results*/ + ignore_value(ifaceSetMacaddr(linkdev, oldmac)); + ignore_value(unlink(path)); + VIR_FREE(macstr); + return 0; +} /** * openMacvtapTap: @@ -575,7 +673,8 @@ openMacvtapTap(const char *tgifname, const unsigned char *vmuuid, virVirtualPortProfileParamsPtr virtPortProfile, char **res_ifname, - enum virVMOperationType vmOp) + enum virVMOperationType vmOp, + char *stateDir) { const char *type = "macvtap"; int c, rc; @@ -589,6 +688,19 @@ openMacvtapTap(const char *tgifname, VIR_DEBUG("%s: VM OPERATION: %s", __FUNCTION__, virVMOperationTypeToString(vmOp)); + /** Note: When using PASSTHROUGH mode with MACVTAP devices the link + * device's MAC address must be set to the VMs MAC address. In + * order to not confuse the first switch or bridge in line this MAC + * address must be reset when the VM is shut down. + * This is especially important when using SRIOV capable cards that + * emulate their switch in firmware. + */ + if (mode == VIR_DOMAIN_NETDEV_MACVTAP_MODE_PASSTHRU) { + if (replaceMacAdress(macaddress, linkdev, stateDir) != 0) { + return -1; + } + } + if (tgifname) { if(ifaceGetIndex(false, tgifname, &ifindex) == 0) { if (STRPREFIX(tgifname, @@ -684,8 +796,14 @@ void delMacvtap(const char *ifname, const unsigned char *macaddr, const char *linkdev, - virVirtualPortProfileParamsPtr virtPortProfile) + int mode, + virVirtualPortProfileParamsPtr virtPortProfile, + char *stateDir) { + if (mode == VIR_DOMAIN_NETDEV_MACVTAP_MODE_PASSTHRU) { + restoreMacAddress(linkdev, stateDir); + } + if (ifname) { vpDisassociatePortProfileId(ifname, macaddr, linkdev, Index: libvirt/src/util/macvtap.h =================================================================== --- libvirt.orig/src/util/macvtap.h +++ libvirt/src/util/macvtap.h @@ -83,12 +83,15 @@ int openMacvtapTap(const char *ifname, const unsigned char *vmuuid, virVirtualPortProfileParamsPtr virtPortProfile, char **res_ifname, - enum virVMOperationType vmop); + enum virVMOperationType vmop, + char *stateDir); void delMacvtap(const char *ifname, const unsigned char *macaddress, const char *linkdev, - virVirtualPortProfileParamsPtr virtPortProfile); + int mode, + virVirtualPortProfileParamsPtr virtPortProfile, + char *stateDir); int vpAssociatePortProfileId(const char *macvtap_ifname, const unsigned char *macvtap_macaddr, Index: libvirt/src/qemu/qemu_hotplug.c =================================================================== --- libvirt.orig/src/qemu/qemu_hotplug.c +++ libvirt/src/qemu/qemu_hotplug.c @@ -1610,7 +1610,9 @@ int qemuDomainDetachNetDevice(struct qem #if WITH_MACVTAP if (detach->type == VIR_DOMAIN_NET_TYPE_DIRECT) { delMacvtap(detach->ifname, detach->mac, detach->data.direct.linkdev, - &detach->data.direct.virtPortProfile); + detach->data.direct.mode, + &detach->data.direct.virtPortProfile, + driver->stateDir); VIR_FREE(detach->ifname); } #endif Index: libvirt/src/util/interface.c =================================================================== --- libvirt.orig/src/util/interface.c +++ libvirt/src/util/interface.c @@ -390,3 +390,99 @@ ifaceGetVlanID(const char *vlanifname AT return ENOSYS; } #endif /* __linux__ */ + +/** + * ifaceGetMacaddr: + * @ifname: interface name to set MTU for + * @macaddr: MAC address (VIR_MAC_BUFLEN in size) + * + * This function gets the @macaddr for a given interface @ifname. + * + * Returns 0 in case of success or an errno code in case of failure. + */ +#ifdef __linux__ +int +ifaceGetMacaddr(const char *ifname ATTRIBUTE_UNUSED, + unsigned char *macaddr ATTRIBUTE_UNUSED) +{ + struct ifreq ifr; + int fd; + + if (!ifname) + return EINVAL; + + fd = socket(AF_INET, SOCK_STREAM, 0); + if (fd < 0) + return errno; + + memset(&ifr, 0, sizeof(struct ifreq)); + if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) + return EINVAL; + + if (ioctl(fd, SIOCGIFHWADDR, (char *)&ifr) != 0) + return errno; + + memcpy(macaddr, ifr.ifr_ifru.ifru_hwaddr.sa_data, VIR_MAC_BUFLEN); + + return 0; +} + +#else + +int +ifaceGetMacaddr(const char *ifname ATTRIBUTE_UNUSED, + unsigned char *macaddr ATTRIBUTE_UNUSED) +{ + return ENOSYS; +} + +#endif /* __linux__ */ + +/** + * ifaceSetMacaddr: + * @ifname: interface name to set MTU for + * @macaddr: MAC address (VIR_MAC_BUFLEN in size) + * + * This function sets the @macaddr for a given interface @ifname. This + * gets rid of the kernel's automatically assigned random MAC. + * + * Returns 0 in case of success or an errno code in case of failure. + */ +#ifdef __linux__ +int +ifaceSetMacaddr(const char *ifname ATTRIBUTE_UNUSED, + const unsigned char *macaddr ATTRIBUTE_UNUSED) +{ + struct ifreq ifr; + int fd; + + if (!ifname) + return EINVAL; + + fd = socket(AF_INET, SOCK_STREAM, 0); + if (fd < 0) + return errno; + + memset(&ifr, 0, sizeof(struct ifreq)); + if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) + return EINVAL; + + /* To fill ifr.ifr_hdaddr.sa_family field */ + if (ioctl(fd, SIOCGIFHWADDR, &ifr) != 0) + return errno; + + memcpy(ifr.ifr_hwaddr.sa_data, macaddr, VIR_MAC_BUFLEN); + + return ioctl(fd, SIOCSIFHWADDR, &ifr) == 0 ? 0 : errno; +} + +#else + +int +ifaceSetMacaddr(const char *ifname ATTRIBUTE_UNUSED, + const unsigned char *macaddr ATTRIBUTE_UNUSED) +{ + return ENOSYS; +} + +#endif /* __linux__ */ Index: libvirt/src/util/interface.h =================================================================== --- libvirt.orig/src/util/interface.h +++ libvirt/src/util/interface.h @@ -32,4 +32,8 @@ int ifaceGetIndex(bool reportError, cons int ifaceGetVlanID(const char *vlanifname, int *vlanid); +int ifaceSetMacaddr(const char *ifname, const unsigned char *macaddr); + +int ifaceGetMacaddr(const char *ifname, unsigned char *macaddr); + #endif /* __VIR_INTERFACE_H__ */ Index: libvirt/src/libvirt_private.syms =================================================================== --- libvirt.orig/src/libvirt_private.syms +++ libvirt/src/libvirt_private.syms @@ -507,7 +507,9 @@ ifaceCheck; ifaceCtrl; ifaceGetFlags; ifaceGetIndex; +ifaceGetMacaddr; ifaceGetVlanID; +ifaceSetMacaddr; ifaceIsUp; =================================================================== Best regards, Gerhard Stenzel ------------------------------------------------------------------------------------- IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschaeftsfuehrung: Dirk Wittkopp Sitz der Gesellschaft: Boeblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

On 06/21/2011 08:01 AM, Gerhard Stenzel wrote:
Next try ..
The following patch addresses the problem that when a PASSTHROUGH mode DIRECT NIC connection is made the MAC address of the NIC is not automatically set and reset to the configured VM MAC and back again.
The attached patch fixes this problem by setting and resetting the MAC while remembering the previous setting while the VM is running. This also works if libvirtd is restarted while the VM is running.
the patch passes make syntax-check
Signed-off-by: Dirk Herrendoerfer<d.herrendoerfer at herrendoerfer.name> Signed-off-by: Gerhard Stenzel<gerhard.stenzel@de.ibm.com>
---
Index: libvirt/src/qemu/qemu_command.c =================================================================== --- libvirt.orig/src/qemu/qemu_command.c +++ libvirt/src/qemu/qemu_command.c @@ -128,7 +128,7 @@ qemuPhysIfaceConnect(virDomainDefPtr def rc = openMacvtapTap(net->ifname, net->mac, net->data.direct.linkdev, net->data.direct.mode, vnet_hdr, def->uuid, &net->data.direct.virtPortProfile,&res_ifname, - vmop); + vmop, driver->stateDir); if (rc>= 0) { qemuAuditNetDevice(def, net, res_ifname, true); VIR_FREE(net->ifname); @@ -149,7 +149,9 @@ qemuPhysIfaceConnect(virDomainDefPtr def if (err) { VIR_FORCE_CLOSE(rc); delMacvtap(net->ifname, net->mac, net->data.direct.linkdev, -&net->data.direct.virtPortProfile); + net->data.direct.mode, +&net->data.direct.virtPortProfile, + driver->stateDir); VIR_FREE(net->ifname); } } Index: libvirt/src/qemu/qemu_process.c =================================================================== --- libvirt.orig/src/qemu/qemu_process.c +++ libvirt/src/qemu/qemu_process.c @@ -2876,7 +2876,8 @@ void qemuProcessStop(struct qemud_driver virDomainNetDefPtr net = def->nets[i]; if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { delMacvtap(net->ifname, net->mac, net->data.direct.linkdev, -&net->data.direct.virtPortProfile); + net->data.direct.mode, +&net->data.direct.virtPortProfile, driver->stateDir); VIR_FREE(net->ifname); } } Index: libvirt/src/util/macvtap.c =================================================================== --- libvirt.orig/src/util/macvtap.c +++ libvirt/src/util/macvtap.c @@ -545,6 +545,104 @@ configMacvtapTap(int tapfd, int vnet_hdr return 0; }
+/** + * replaceMacAdress: + * @macaddress: new MAC address for interface + * @linkdev: name of interface + * @stateDir: directory to store old MAC address + * + * Returns 0 on success, -1 in case of fatal error, error code otherwise. + * + */ +static int +replaceMacAdress(const unsigned char *macaddress, + const char *linkdev, + char *stateDir) +{ + unsigned char oldmac[6]; + int rc; + + rc = ifaceGetMacaddr(linkdev, oldmac); + + if (rc) { + virReportSystemError(rc, + _("Getting MAC address from '%s' " + "to '%02x:%02x:%02x:%02x:%02x:%02x' failed."), + linkdev, + oldmac[0], oldmac[1], oldmac[2], + oldmac[3], oldmac[4], oldmac[5]); + } else { + char *path = NULL; + char macstr[VIR_MAC_STRING_BUFLEN]; + + if (virAsprintf(&path, "%s/%s", + stateDir, + linkdev)< 0) { + virReportOOMError(); + return errno; + } + virFormatMacAddr(oldmac, macstr); + if (virFileWriteStr(path, macstr, O_CREAT|O_TRUNC|O_WRONLY)< 0) { + virReportSystemError(errno, _("Unable to preserve mac for %s"), + linkdev); + return errno; + } + } + + rc = ifaceSetMacaddr(linkdev, macaddress); + if (rc) { + virReportSystemError(errno, + _("Setting MAC address on '%s' to " + "'%02x:%02x:%02x:%02x:%02x:%02x' failed."), + linkdev, + macaddress[0], macaddress[1], macaddress[2], + macaddress[3], macaddress[4], macaddress[5]); + } + return rc; +} + +/** + * restoreMacAddress: + * @linkdev: name of interface + * @stateDir: directory containing old MAC address + * + * Returns 0 on success, -1 in case of fatal error, error code otherwise. + * + */ +static int +restoreMacAddress(const char *linkdev, + char *stateDir) +{ + int ret; + char *oldmacname = NULL; + char *macstr = NULL; + char *path = NULL; + unsigned char oldmac[6]; + + if (virAsprintf(&path, "%s/%s", + stateDir, + linkdev)< 0) { + virReportOOMError(); + return -1; + } + + if (virFileReadAll(path, VIR_MAC_STRING_BUFLEN,&macstr)< 0) { + return errno; + } + + if (virParseMacAddr(macstr,&oldmac[0]) != 0) { + macvtapError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse MAC address from '%s'"), + oldmacname); + return -1; + } + + /*reset mac and remove file-ignore results*/ + ignore_value(ifaceSetMacaddr(linkdev, oldmac)); + ignore_value(unlink(path)); + VIR_FREE(macstr); + return 0; +}
/** * openMacvtapTap: @@ -575,7 +673,8 @@ openMacvtapTap(const char *tgifname, const unsigned char *vmuuid, virVirtualPortProfileParamsPtr virtPortProfile, char **res_ifname, - enum virVMOperationType vmOp) + enum virVMOperationType vmOp, + char *stateDir) { const char *type = "macvtap"; int c, rc; @@ -589,6 +688,19 @@ openMacvtapTap(const char *tgifname,
VIR_DEBUG("%s: VM OPERATION: %s", __FUNCTION__, virVMOperationTypeToString(vmOp));
+ /** Note: When using PASSTHROUGH mode with MACVTAP devices the link + * device's MAC address must be set to the VMs MAC address. In + * order to not confuse the first switch or bridge in line this MAC + * address must be reset when the VM is shut down. + * This is especially important when using SRIOV capable cards that + * emulate their switch in firmware. + */ + if (mode == VIR_DOMAIN_NETDEV_MACVTAP_MODE_PASSTHRU) { + if (replaceMacAdress(macaddress, linkdev, stateDir) != 0) { + return -1; + } + } + if (tgifname) { if(ifaceGetIndex(false, tgifname,&ifindex) == 0) { if (STRPREFIX(tgifname, @@ -684,8 +796,14 @@ void delMacvtap(const char *ifname, const unsigned char *macaddr, const char *linkdev, - virVirtualPortProfileParamsPtr virtPortProfile) + int mode, + virVirtualPortProfileParamsPtr virtPortProfile, + char *stateDir) { + if (mode == VIR_DOMAIN_NETDEV_MACVTAP_MODE_PASSTHRU) { + restoreMacAddress(linkdev, stateDir); + } + if (ifname) { vpDisassociatePortProfileId(ifname, macaddr, linkdev, Index: libvirt/src/util/macvtap.h =================================================================== --- libvirt.orig/src/util/macvtap.h +++ libvirt/src/util/macvtap.h @@ -83,12 +83,15 @@ int openMacvtapTap(const char *ifname, const unsigned char *vmuuid, virVirtualPortProfileParamsPtr virtPortProfile, char **res_ifname, - enum virVMOperationType vmop); + enum virVMOperationType vmop, + char *stateDir);
void delMacvtap(const char *ifname, const unsigned char *macaddress, const char *linkdev, - virVirtualPortProfileParamsPtr virtPortProfile); + int mode, + virVirtualPortProfileParamsPtr virtPortProfile, + char *stateDir);
int vpAssociatePortProfileId(const char *macvtap_ifname, const unsigned char *macvtap_macaddr, Index: libvirt/src/qemu/qemu_hotplug.c =================================================================== --- libvirt.orig/src/qemu/qemu_hotplug.c +++ libvirt/src/qemu/qemu_hotplug.c @@ -1610,7 +1610,9 @@ int qemuDomainDetachNetDevice(struct qem #if WITH_MACVTAP if (detach->type == VIR_DOMAIN_NET_TYPE_DIRECT) { delMacvtap(detach->ifname, detach->mac, detach->data.direct.linkdev, -&detach->data.direct.virtPortProfile); + detach->data.direct.mode, +&detach->data.direct.virtPortProfile, + driver->stateDir); VIR_FREE(detach->ifname); } #endif Index: libvirt/src/util/interface.c =================================================================== --- libvirt.orig/src/util/interface.c +++ libvirt/src/util/interface.c @@ -390,3 +390,99 @@ ifaceGetVlanID(const char *vlanifname AT return ENOSYS; } #endif /* __linux__ */ + +/** + * ifaceGetMacaddr: + * @ifname: interface name to set MTU for + * @macaddr: MAC address (VIR_MAC_BUFLEN in size) + * + * This function gets the @macaddr for a given interface @ifname. + * + * Returns 0 in case of success or an errno code in case of failure. + */ +#ifdef __linux__ +int +ifaceGetMacaddr(const char *ifname ATTRIBUTE_UNUSED, + unsigned char *macaddr ATTRIBUTE_UNUSED) +{ Here the ATTRIBUTE_UNUSED are not necessary. + struct ifreq ifr; + int fd; + + if (!ifname) + return EINVAL; + + fd = socket(AF_INET, SOCK_STREAM, 0); + if (fd< 0) + return errno; + + memset(&ifr, 0, sizeof(struct ifreq)); + if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) + return EINVAL; + + if (ioctl(fd, SIOCGIFHWADDR, (char *)&ifr) != 0) + return errno; + + memcpy(macaddr, ifr.ifr_ifru.ifru_hwaddr.sa_data, VIR_MAC_BUFLEN); + + return 0; +} + +#else + +int +ifaceGetMacaddr(const char *ifname ATTRIBUTE_UNUSED, + unsigned char *macaddr ATTRIBUTE_UNUSED) +{ + return ENOSYS; +} + +#endif /* __linux__ */ + +/** + * ifaceSetMacaddr: + * @ifname: interface name to set MTU for + * @macaddr: MAC address (VIR_MAC_BUFLEN in size) + * + * This function sets the @macaddr for a given interface @ifname. This + * gets rid of the kernel's automatically assigned random MAC. + * + * Returns 0 in case of success or an errno code in case of failure. + */ +#ifdef __linux__ +int +ifaceSetMacaddr(const char *ifname ATTRIBUTE_UNUSED, + const unsigned char *macaddr ATTRIBUTE_UNUSED) +{ + struct ifreq ifr; + int fd; + + if (!ifname) + return EINVAL; + + fd = socket(AF_INET, SOCK_STREAM, 0); + if (fd< 0) + return errno; + + memset(&ifr, 0, sizeof(struct ifreq)); + if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) + return EINVAL; + + /* To fill ifr.ifr_hdaddr.sa_family field */ + if (ioctl(fd, SIOCGIFHWADDR,&ifr) != 0) + return errno; + + memcpy(ifr.ifr_hwaddr.sa_data, macaddr, VIR_MAC_BUFLEN); + + return ioctl(fd, SIOCSIFHWADDR,&ifr) == 0 ? 0 : errno; +} + +#else + +int +ifaceSetMacaddr(const char *ifname ATTRIBUTE_UNUSED, + const unsigned char *macaddr ATTRIBUTE_UNUSED) +{ + return ENOSYS; +} + +#endif /* __linux__ */ Index: libvirt/src/util/interface.h =================================================================== --- libvirt.orig/src/util/interface.h +++ libvirt/src/util/interface.h @@ -32,4 +32,8 @@ int ifaceGetIndex(bool reportError, cons
int ifaceGetVlanID(const char *vlanifname, int *vlanid);
+int ifaceSetMacaddr(const char *ifname, const unsigned char *macaddr); + +int ifaceGetMacaddr(const char *ifname, unsigned char *macaddr); + #endif /* __VIR_INTERFACE_H__ */ Index: libvirt/src/libvirt_private.syms =================================================================== --- libvirt.orig/src/libvirt_private.syms +++ libvirt/src/libvirt_private.syms @@ -507,7 +507,9 @@ ifaceCheck; ifaceCtrl; ifaceGetFlags; ifaceGetIndex; +ifaceGetMacaddr; ifaceGetVlanID; +ifaceSetMacaddr; ifaceIsUp;
===================================================================
ACK. Unless someone else opposes, I'd push later today cleaning up what I commented about above. Stefan

On 06/21/2011 09:44 AM, Stefan Berger wrote:
On 06/21/2011 08:01 AM, Gerhard Stenzel wrote:
Next try .. [...]
+ #endif /* __VIR_INTERFACE_H__ */ Index: libvirt/src/libvirt_private.syms =================================================================== --- libvirt.orig/src/libvirt_private.syms +++ libvirt/src/libvirt_private.syms @@ -507,7 +507,9 @@ ifaceCheck; ifaceCtrl; ifaceGetFlags; ifaceGetIndex; +ifaceGetMacaddr; ifaceGetVlanID; +ifaceSetMacaddr; ifaceIsUp;
===================================================================
ACK. Unless someone else opposes, I'd push later today cleaning up what I commented about above.
Pushed now. Stefan

On 06/21/2011 06:01 AM, Gerhard Stenzel wrote:
+ rc = ifaceGetMacaddr(linkdev, oldmac); + + if (rc) { + virReportSystemError(rc,
Sorry for not catching this sooner. This should probably be virReportSystemError(errno,...), assuming that ifaceGetMacaddr guarantees a sane errno setting (at any rate, you used errno for ifaceSetMacaddr later on). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 06/21/2011 01:16 PM, Eric Blake wrote:
On 06/21/2011 06:01 AM, Gerhard Stenzel wrote:
+ rc = ifaceGetMacaddr(linkdev, oldmac); + + if (rc) { + virReportSystemError(rc, Sorry for not catching this sooner. This should probably be virReportSystemError(errno,...), assuming that ifaceGetMacaddr guarantees a sane errno setting (at any rate, you used errno for ifaceSetMacaddr later on).
It should be ok the way it is since ifaceGetMacAddr() returns either EINVAL or the content of errno. Stefan
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 06/21/2011 12:21 PM, Stefan Berger wrote:
On 06/21/2011 01:16 PM, Eric Blake wrote:
On 06/21/2011 06:01 AM, Gerhard Stenzel wrote:
+ rc = ifaceGetMacaddr(linkdev, oldmac); + + if (rc) { + virReportSystemError(rc, Sorry for not catching this sooner. This should probably be virReportSystemError(errno,...), assuming that ifaceGetMacaddr guarantees a sane errno setting (at any rate, you used errno for ifaceSetMacaddr later on).
It should be ok the way it is since ifaceGetMacAddr() returns either EINVAL or the content of errno.
Oh, I see. But then we have the converse problem: + rc = ifaceSetMacaddr(linkdev, macaddress); + if (rc) { + virReportSystemError(errno, + _("Setting MAC address on '%s' to " + "'%02x:%02x:%02x:%02x:%02x:%02x' failed."), But ifaceSetMacaddr returns EINVAL without setting errno on at least one error path, which means this can print a bogus errno value. So we should either fix the iface* functions to follow convention of -1 return and valid errno used in other libvirt interfaces (rather than positive return), as well as fix its clients; or we should fix this particular ifaceSetMacaddr call to fit in with the different convention. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 06/21/2011 03:25 PM, Eric Blake wrote:
On 06/21/2011 12:21 PM, Stefan Berger wrote:
On 06/21/2011 01:16 PM, Eric Blake wrote:
On 06/21/2011 06:01 AM, Gerhard Stenzel wrote:
+ rc = ifaceGetMacaddr(linkdev, oldmac); + + if (rc) { + virReportSystemError(rc, Sorry for not catching this sooner. This should probably be virReportSystemError(errno,...), assuming that ifaceGetMacaddr guarantees a sane errno setting (at any rate, you used errno for ifaceSetMacaddr later on).
It should be ok the way it is since ifaceGetMacAddr() returns either EINVAL or the content of errno. Oh, I see. But then we have the converse problem:
+ rc = ifaceSetMacaddr(linkdev, macaddress); + if (rc) { + virReportSystemError(errno, + _("Setting MAC address on '%s' to " + "'%02x:%02x:%02x:%02x:%02x:%02x' failed."),
But ifaceSetMacaddr returns EINVAL without setting errno on at least one error path, which means this can print a bogus errno value.
So we should either fix the iface* functions to follow convention of -1 return and valid errno used in other libvirt interfaces (rather than positive return), as well as fix its clients; or we should fix this particular ifaceSetMacaddr call to fit in with the different convention.
I vote for consistency with the rest of the project, although I think that should be a separate patch (done as a prerequisite to this one)

On 06/21/2011 12:21 PM, Stefan Berger wrote:
On 06/21/2011 01:16 PM, Eric Blake wrote:
On 06/21/2011 06:01 AM, Gerhard Stenzel wrote:
+ rc = ifaceGetMacaddr(linkdev, oldmac); + + if (rc) { + virReportSystemError(rc, Sorry for not catching this sooner. This should probably be virReportSystemError(errno,...), assuming that ifaceGetMacaddr guarantees a sane errno setting (at any rate, you used errno for ifaceSetMacaddr later on).
It should be ok the way it is since ifaceGetMacAddr() returns either EINVAL or the content of errno. Oh, I see. But then we have the converse problem:
+ rc = ifaceSetMacaddr(linkdev, macaddress); + if (rc) { + virReportSystemError(errno, + _("Setting MAC address on '%s' to " + "'%02x:%02x:%02x:%02x:%02x:%02x' failed."),
But ifaceSetMacaddr returns EINVAL without setting errno on at least one error path, which means this can print a bogus errno value. I fixed this now to use 'rc' rather than this errno in the patch moving
On 06/21/2011 03:25 PM, Eric Blake wrote: this code into interface.c. Stefan
participants (4)
-
Eric Blake
-
Gerhard Stenzel
-
Laine Stump
-
Stefan Berger