On Tue, Jun 21, 2011 at 11:08:38AM +0200, Gerhard Stenzel wrote:
>
> This is another rework of the patch from Dirk addressing all comments
> received so far.
>
> 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@xxxxxxxxxx>
>
> ---
>
>
> 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,106 @@ 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) {
> + virReportSystemError(errno, _("Unable to preserve mac for %s"),
> + linkdev);
> + return errno;
> + }
The virFileReadAll method already reports error messages.
> +
> + if (virParseMacAddr(macstr, &oldmac[0]) != 0) {
> + virReportSystemError(VIR_ERR_INTERNAL_ERROR,
> + _("Cannot parse MAC address from '%s'"),
> + oldmacname);
> + return -1;
> + }
> +
> + /*reset mac and remove file-ignore results*/
> + ret = ifaceSetMacaddr(linkdev, oldmac);
> + ret = unlink(path);
To ignore results you can instead do
ignore_value(ifaceSetMacaddr(linkdev, oldmac));
ignore_value(unlink(path));
> + VIR_FREE(macstr);
> + return 0;
> +}
>
> /**
> * openMacvtapTap:
> @@ -575,7 +675,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 +690,21 @@ 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) {
> + virReportSystemError(errno,
> + _("cannot replace MAC address on $s"),
> + linkdev);
Missing a 'return -1' here, and 'replaceMacAddress already reports
an error message, so no need todo so again.
> + }
> + }
> +
> if (tgifname) {
> if(ifaceGetIndex(false, tgifname, &ifindex) == 0) {
> if (STRPREFIX(tgifname,
> @@ -684,8 +800,18 @@ 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) {
> + if (restoreMacAddress(linkdev, stateDir) != 0) {
> + virReportSystemError(errno,
> + _("cannot restore MAC address on $s"),
> + linkdev);
> + }
> + }
Again, restoreMacAddress already reports an error message. No
neeed for a 'return -1' here, since we need to make a best
effort to run the next bit of code too.
> Index: libvirt/src/util/interface.c
> ===================================================================
> --- libvirt.orig/src/util/interface.c
> +++ libvirt/src/util/interface.c
> @@ -390,3 +390,102 @@ 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,
> + unsigned char *macaddr)
> +{
> + struct ifreq ifr;
> + int fd;
> +
> + if (!ifname)
> + return EINVAL;
> +
> + fd = socket(AF_INET, SOCK_STREAM, 0);
> + if(fd < 0){
> + return errno;
> + }
Normal coding style for libvirt is to have a space
before & after the () on an 'if' condition
> +
> + memset(&ifr, 0, sizeof(struct ifreq));
> + if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL)
> + return EINVAL;
> +
> + if(ioctl(fd, SIOCGIFHWADDR, (char *)&ifr) != 0){
Again here.
> + return errno;
> + }
> +
> + memcpy(macaddr, ifr.ifr_ifru.ifru_hwaddr.sa_data, VIR_MAC_BUFLEN);
> +
> + return 0;
> +}
> +
> +#else
> +
> +int
> +ifaceGetMacaddr(const char *ifname,
> + unsigned char *macaddr)
This needs ATTRIBUTE_UNUSED on both params to avoid a warning
> +{
> + 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,
> + const unsigned char *macaddr)
> +{
> + struct ifreq ifr;
> + int fd;
> +
> + if (!ifname)
> + return EINVAL;
> +
> + fd = socket(AF_INET, SOCK_STREAM, 0);
> + if(fd < 0){
> + return errno;
Same style item
> + }
> +
> + 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,
> + const unsigned char *macaddr)
And 'ATTRIBUTE_UNUSED' on these too
> +{
> + 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;
>
> ===================================================================
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list