Hi Dirk,

Your patch mail is line wrapped. I'd recommend resending your patch
using git send-email if possible, will at least increase the chances
that someone reviews/comments.

Thanks,
Cole

On 06/08/2011 08:10 AM, D. Herrendoerfer wrote:
> Hi to all,
> 
> there is a problem present in libvirt 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 storing of the previous MAC address is done in a link ( currently  
> in /tmp,
> but should probably go into /var/run somewhere ).
> Why a link ? It only uses one inode, no need to waste a whole data block
> for 16 bytes or less. It's also more readable by simply looking at it  
> with 'ls -l'.
> 
> Best regards
> 
> Dirk
> 
> Signed-off-by: Dirk Herrendoerfer <d.herrendoerfer at  
> herrendoerfer.name>
> 
> diff --git a/src/libvirt_macvtap.syms b/src/libvirt_macvtap.syms
> index b48565b..0520cb5 100644
> --- a/src/libvirt_macvtap.syms
> +++ b/src/libvirt_macvtap.syms
> @@ -6,5 +6,7 @@
>   # macvtap.h
>   delMacvtap;
>   openMacvtapTap;
> +get_macaddr;
> +set_macaddr;
>   vpAssociatePortProfileId;
>   vpDisassociatePortProfileId;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ef2d002..4cddbba 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -118,6 +118,7 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
>       int rc;
>   #if WITH_MACVTAP
>       char *res_ifname = NULL;
> +    unsigned char old_macaddr[6];
>       int vnet_hdr = 0;
>       int err;
> 
> @@ -125,6 +126,53 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
>           net->model && STREQ(net->model, "virtio"))
>           vnet_hdr = 1;
> 
> +    /** Note: When using PASSTHROUGH mode with MACVTAP devices the link
> +     * devices'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 ( net->data.direct.mode ==  
> VIR_DOMAIN_NETDEV_MACVTAP_MODE_PASSTHRU ) {
> +	rc = get_macaddr(&old_macaddr, 6, net->data.direct.linkdev);
> +        if (rc) {
> +            virReportSystemError(rc,
> +                                 _("Getting MAC address from '%s' "
> +                                 "to '%02x:%02x:%02x:%02x:%02x:%02x'  
> failed."),
> +                                 net->data.direct.linkdev,
> +                                  
> old_macaddr[0],old_macaddr[1],old_macaddr[2],
> +                                  
> old_macaddr[3],old_macaddr[4],old_macaddr[5]);
> +        } else {
> +            char oldmacname[256], newmacname[256];
> +
> +            sprintf(oldmacname,"%02x:%02x:%02x:%02x:%02x:%02x",
> +                    old_macaddr[0],old_macaddr[1],old_macaddr[2],
> +                    old_macaddr[3],old_macaddr[4],old_macaddr[5]);
> +
> +            sprintf(newmacname,"/tmp/%s@%02x:%02x:%02x:%02x:%02x:%02x",
> +                    net->data.direct.linkdev,
> +                    net->mac[0],net->mac[1],net->mac[2],
> +                    net->mac[3],net->mac[4],net->mac[5]);
> +
> +	    rc = symlink (oldmacname, newmacname);
> +            if (rc) {
> +                virReportSystemError(rc,
> +                                     _("MAC link file creation failed  
> for %s."),
> +                                     net->data.direct.linkdev);
> +            }
> +        }
> +
> +	rc = set_macaddr(net->mac, 6, net->data.direct.linkdev);
> +        if (rc) {
> +            virReportSystemError(rc,
> +                                 _("Setting MAC address on  '%s' to "
> +                                 "'%02x:%02x:%02x:%02x:%02x:%02x'  
> failed."),
> +                                 net->data.direct.linkdev,
> +                                 net->mac[0],net->mac[1],net->mac[2],
> +                                 net->mac[3],net->mac[4],net->mac[5]);
> +        }
> +    }
> +
>       rc = openMacvtapTap(net->ifname, net->mac, net- 
>  >data.direct.linkdev,
>                           net->data.direct.mode, vnet_hdr, def->uuid,
>                           &net->data.direct.virtPortProfile,  
> &res_ifname,
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 1efe024..127acb8 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2663,6 +2663,51 @@ void qemuProcessStop(struct qemud_driver *driver,
>           if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) {
>               delMacvtap(net->ifname, net->mac, net- 
>  >data.direct.linkdev,
>                          &net->data.direct.virtPortProfile);
> +
> +            if ( net->data.direct.mode ==  
> VIR_DOMAIN_NETDEV_MACVTAP_MODE_PASSTHRU ) {
> +                char newmacname[256], linkdata[64];
> +                unsigned int old_macaddr[6];
> +
> +                sprintf(newmacname,"/tmp/%s@%02x:%02x:%02x:%02x:%02x: 
> %02x",
> +                        net->data.direct.linkdev,
> +                        net->mac[0],net->mac[1],net->mac[2],
> +                        net->mac[3],net->mac[4],net->mac[5]);
> +
> +                ret = readlink (newmacname, linkdata, 64);
> +                if ( ret == 17 ) {
> +                    linkdata[17] = 0;
> +
> +                    ret = sscanf(linkdata,"%x:%x:%x:%x:%x:%x",
> +                                  
> &old_macaddr[0],&old_macaddr[1],&old_macaddr[2],
> +                                  
> &old_macaddr[3],&old_macaddr[4],&old_macaddr[5]);
> +                    if ( ret == 6 ) {
> +                        unsigned char oldmac[6];
> +			oldmac[0] = (unsigned char)(old_macaddr[0] & 0xFF) ;
> +			oldmac[1] = (unsigned char)(old_macaddr[1] & 0xFF) ;
> +			oldmac[2] = (unsigned char)(old_macaddr[2] & 0xFF) ;
> +			oldmac[3] = (unsigned char)(old_macaddr[3] & 0xFF) ;
> +			oldmac[4] = (unsigned char)(old_macaddr[4] & 0xFF) ;
> +			oldmac[5] = (unsigned char)(old_macaddr[5] & 0xFF) ;
> +
> +                        VIR_WARN(("Resetting MAC address on '%s' "
> +                                  "to '%02x:%02x:%02x:%02x:%02x: 
> %02x'."),
> +                                  net->data.direct.linkdev,
> +                                  oldmac[0],oldmac[1],oldmac[2],
> +                                  oldmac[3],oldmac[4],oldmac[5]);
> +                        /*reset mac and remove link file-ignore  
> restults*/
> +                        ret = set_macaddr(oldmac, 6, net- 
>  >data.direct.linkdev);
> +			ret = unlink(newmacname);
> +                    } else {
> +                        VIR_WARN(("Scanning MAC address from '%s' "
> +                                  "failed ! Got %i values."),
> +                                  linkdata, ret);
> +                    }
> +                } else {
> +                    VIR_WARN(("Reading MAC address from '%s' failed ! "
> +                              "Got %i bytes."),
> +                              newmacname, ret);
> +                }
> +            }
>               VIR_FREE(net->ifname);
>           }
>       }
> diff --git a/src/util/macvtap.c b/src/util/macvtap.c
> index 068638e..1026a84 100644
> --- a/src/util/macvtap.c
> +++ b/src/util/macvtap.c
> @@ -191,6 +191,155 @@ err_exit:
> 
>   # if WITH_MACVTAP
> 
> +/**
> + * get_macaddr:
> + * Get the MAC address of a network device
> + *
> + * @macaddress: Pointer where the MAC address will be stored
> + * @macaddrsize: Size of the MAC address.
> + * @srcdev: The interface name of the NIC to get the MAC from
> + *
> + * Returns zero in case of success,
> + * negative value otherwise with error reported.
> + *
> + */
> +int
> +get_macaddr(const unsigned char *macaddress, int macaddrsize,
> +            const char *srcdev )
> +{
> +    int sockfd;
> +    int io;
> +    char buffer[1024];
> +    struct ifreq ifr;
> +
> +    strcpy(ifr.ifr_name, srcdev);
> +
> +    sockfd = socket(AF_INET, SOCK_STREAM, 0);
> +    if(sockfd < 0){
> +        return -1;
> +    }
> +
> +    io = ioctl(sockfd, SIOCGIFHWADDR, (char *)&ifr);
> +    if(io < 0){
> +        return -1;
> +    }
> +
> +    bcopy( ifr.ifr_ifru.ifru_hwaddr.sa_data, macaddress, macaddrsize);
> +
> +    return 0;
> +}
> +
> +/**
> + * Set_macaddr:
> + * Set the MAC address of a network device
> + *
> + * @macaddress: MAC address to assign to the NIC
> + * @macaddrsize: Size of the MAC address.
> + * @srcdev: The interface name of the NIC
> + *
> + * Returns zero in case of success,
> + * negative value otherwise with error reported.
> + *
> + */
> +int
> +set_macaddr(const unsigned char *macaddress, int macaddrsize,
> +            const char *srcdev )
> +{
> +    int rc = 0;
> +    struct nlmsghdr *resp;
> +    struct nlmsgerr *err;
> +    struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
> +    int ifindex;
> +    unsigned char *recvbuf = NULL;
> +    unsigned int recvbuflen;
> +    struct nl_msg *nl_msg;
> +    struct nlattr *linkinfo;
> +
> +    if (ifaceGetIndex(true, srcdev, &ifindex) != 0)
> +        return -1;
> +
> +    nl_msg = nlmsg_alloc_simple(RTM_SETLINK, NLM_F_REQUEST);
> +
> +    if (!nl_msg) {
> +        virReportOOMError();
> +        return -1;
> +    }
> +
> +    if (nlmsg_append(nl_msg,  &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO)  
> < 0)
> +        goto buffer_too_small;
> +
> +    if (nla_put_u32(nl_msg, IFLA_LINK, ifindex) < 0)
> +        goto buffer_too_small;
> +
> +    if (nla_put(nl_msg, IFLA_ADDRESS, macaddrsize, macaddress) < 0)
> +        goto buffer_too_small;
> +
> +    if (srcdev &&
> +        nla_put(nl_msg, IFLA_IFNAME, strlen(srcdev)+1, srcdev) < 0)
> +        goto buffer_too_small;
> +
> +    if (nlComm(nl_msg, &recvbuf, &recvbuflen, 0) < 0) {
> +        rc = -1;
> +        goto err_exit;
> +    }
> +
> +    if (recvbuflen < NLMSG_LENGTH(0) || recvbuf == NULL)
> +        goto malformed_resp;
> +
> +    resp = (struct nlmsghdr *)recvbuf;
> +
> +    switch (resp->nlmsg_type) {
> +    case NLMSG_ERROR:
> +        err = (struct nlmsgerr *)NLMSG_DATA(resp);
> +        if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
> +            goto malformed_resp;
> +
> +        switch (err->error) {
> +
> +        case 0:
> +            break;
> +
> +        case -EEXIST:
> +            rc = -1;
> +            break;
> +
> +        default:
> +            macvtapError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                         _("error setting device mac address"));
> +            rc = -1;
> +        }
> +        break;
> +
> +    case NLMSG_DONE:
> +        break;
> +
> +    default:
> +        goto malformed_resp;
> +    }
> +
> +err_exit:
> +    nlmsg_free(nl_msg);
> +
> +    VIR_FREE(recvbuf);
> +
> +    return rc;
> +
> +malformed_resp:
> +    nlmsg_free(nl_msg);
> +
> +    macvtapError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                 _("malformed netlink response message"));
> +    VIR_FREE(recvbuf);
> +    return -1;
> +
> +buffer_too_small:
> +    nlmsg_free(nl_msg);
> +
> +    macvtapError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                 _("allocated netlink buffer is too small"));
> +    return -1;
> +}
> +
>   static int
>   link_add(const char *type,
>            const unsigned char *macaddress, int macaddrsize,
> diff --git a/src/util/macvtap.h b/src/util/macvtap.h
> index a1383c4..d71546a 100644
> --- a/src/util/macvtap.h
> +++ b/src/util/macvtap.h
> @@ -75,6 +75,12 @@ enum virVMOperationType {
> 
>   #  include "internal.h"
> 
> +int get_macaddr(const unsigned char *macaddress, int macaddrsize,
> +            const char *srcdev );
> +
> +int set_macaddr(const unsigned char *macaddress, int macaddrsize,
> +            const char *srcdev );
> +
>   int openMacvtapTap(const char *ifname,
>                      const unsigned char *macaddress,
>                      const char *linkdev,
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list