Re: [libvirt] [PATCH v5 2/2] Add de-association handling to macvlan code

On 02/22/2012 08:17 AM, D. Herrendoerfer wrote: > From: "D. Herrendoerfer" <d.herrendoerfer(a)herrendoerfer.name&gt; > > Add de-association handling for 802.1qbg (vepa) via lldpad > netlink messages. Also adds the possibility to perform an > association request without waiting for a confirmation. The main issue I see with this patch is with whitespace, but that can easily be fixed prior to pushing it, so ACK. Is the message format used by this patch, the absolute final version? (i.e. can we safely push it an know that it will be correct?)

> > Signed-off-by: D. Herrendoerfer <d.herrendoerfer(a)herrendoerfer.name&gt; > --- > src/qemu/qemu_migration.c | 2 +- > src/util/virnetdevmacvlan.c | 315 ++++++++++++++++++++++++++++ > +++++++++- > src/util/virnetdevvportprofile.c | 33 +++-- > src/util/virnetdevvportprofile.h | 3 +- > 4 files changed, 339 insertions(+), 14 deletions(-) > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c > index 12cfbde..31428f8 100644 > --- a/src/qemu/qemu_migration.c > +++ b/src/qemu/qemu_migration.c > @@ -2567,7 +2567,7 @@ > qemuMigrationVPAssociatePortProfiles(virDomainDefPtr def) { > net->mac, > > virDomainNetGetActualDirectDev(net), > def->uuid, > - > VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH) < 0) > + > VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH, false) < 0) > goto err_exit; > } > last_good_net = i; > diff --git a/src/util/virnetdevmacvlan.c b/src/util/ > virnetdevmacvlan.c > index 25d0846..b3e3325 100644 > --- a/src/util/virnetdevmacvlan.c > +++ b/src/util/virnetdevmacvlan.c > @@ -46,7 +46,6 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode, > VIR_NETDEV_MACVLAN_MODE_LAST, > "passthrough") > > #if WITH_MACVTAP > - spurious whitespace change. > # include <stdint.h> > # include <stdio.h> > # include <errno.h> > @@ -57,6 +56,8 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode, > VIR_NETDEV_MACVLAN_MODE_LAST, > # include <linux/if.h> > # include <linux/if_tun.h> > > +# include <c-ctype.h> > + > /* Older kernels lacked this enum value. */ > # if !HAVE_DECL_MACVLAN_MODE_PASSTHRU > # define MACVLAN_MODE_PASSTHRU 8 > @@ -68,6 +69,8 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode, > VIR_NETDEV_MACVLAN_MODE_LAST, > # include "virfile.h" > # include "virnetlink.h" > # include "virnetdev.h" > +# include "virpidfile.h" > + > > # define MACVTAP_NAME_PREFIX "macvtap" > # define MACVTAP_NAME_PATTERN "macvtap%d" > @@ -75,6 +78,7 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode, > VIR_NETDEV_MACVLAN_MODE_LAST, > # define MACVLAN_NAME_PREFIX "macvlan" > # define MACVLAN_NAME_PATTERN "macvlan%d" > > + Another spurious whitespace change. > /** > * virNetDevMacVLanCreate: > * > @@ -445,6 +449,293 @@ static const uint32_t > modeMap[VIR_NETDEV_MACVLAN_MODE_LAST] = { > [VIR_NETDEV_MACVLAN_MODE_PASSTHRU] = MACVLAN_MODE_PASSTHRU, > }; > > +/* Struct to hold the state and configuration of a 802.1qbg port */ > +struct virNetlinkCallbackData { > + char cr_ifname[64]; > + virNetDevVPortProfilePtr virtPortProfile; > + const unsigned char *macaddress; > + const char *linkdev; > + const unsigned char *vmuuid; > + enum virNetDevVPortProfileOp vmOp; > + unsigned int linkState; > +}; > + > +typedef struct virNetlinkCallbackData *virNetlinkCallbackDataPtr; > + > +#define INSTANCE_STRLEN 36 > + > +static int instance2str(const unsigned char *p, char *dst, size_t > size) > +{ > + if (dst && size > INSTANCE_STRLEN) { > + snprintf(dst, size, "%02x%02x%02x%02x-%02x%02x-%02x%02x-" > + "%02x%02x-%02x%02x%02x%02x%02x%02x", > + p[0], p[1], p[2], p[3], > + p[4], p[5], p[6], p[7], > + p[8], p[9], p[10], p[11], p[12], p[13], p[14], p[15]); > + return 0; > + } > + return -1; > +} > + > +# define LLDPAD_PID_FILE "/var/run/lldpad.pid" > +# define VIRIP_PID_FILE "/var/run/virip.pid" > + > +/** > + * virNetDevMacVLanVPortProfileCallback: > + * > + * @msg: The buffer containing the received netlink message > + * @length: The length of the received netlink message. > + * @peer: The netling sockaddr containing the peer information > + * @handeled: Contains information if the message has been replied > to yet > + * @opaque: Contains vital information regarding the associated vm > an interface > + * > + * This function is called when a netlink message is received. The > function > + * reads the message and responds if it is pertinent to the > running VMs > + * network interface. > + */ > + > +static void > +virNetDevMacVLanVPortProfileCallback( unsigned char *msg, > + int length, > + struct sockaddr_nl *peer, > + bool *handled, > + void *opaque) > +{ > + struct nla_policy ifla_vf_policy[IFLA_VF_MAX + 1] = { > + [IFLA_VF_MAC] = {.minlen = sizeof(struct ifla_vf_mac), > + .maxlen = sizeof(struct ifla_vf_mac)}, > + [IFLA_VF_VLAN] = {.minlen = sizeof(struct ifla_vf_vlan), > + .maxlen = sizeof(struct ifla_vf_vlan)}, The indentation looks off here. Don't know if it's the actual patch or my mail client... > + }; > + > + struct nla_policy ifla_port_policy[IFLA_PORT_MAX + 1] = { > + [IFLA_PORT_RESPONSE] = {.type = NLA_U16}, > + }; > + > + struct nlattr *tb[IFLA_MAX + 1], > + *tb3[IFLA_PORT_MAX + 1], > + *tb_vfinfo[IFLA_VF_MAX + 1], *tb_vfinfo_list; > + > + struct ifinfomsg ifinfo; > + struct nlmsghdr *hdr; > + void *data; > + int rem; > + char *ifname; > + bool indicate = false; The previous 4 lines all have tab characters instead of spaces. libvirt likes spaces only. > + virNetlinkCallbackDataPtr calld = opaque; > + pid_t lldpad_pid = 0; > + pid_t virip_pid = 0; > + > + hdr = (struct nlmsghdr *) msg; Another tab. I'll stop pointing these out, and just mention that "make syntax-check" will show these to you - you should try to run make syntax check before sending patches. > + data = nlmsg_data(hdr); > + > + /* Quickly decide if we want this or not */ > + > + if (virPidFileReadPath(LLDPAD_PID_FILE, &lldpad_pid) < 0) > + return; > + > + ignore_value(virPidFileReadPath(VIRIP_PID_FILE, &virip_pid)); > + > + if (hdr->nlmsg_pid != lldpad_pid && hdr->nlmsg_pid != virip_pid) > + return; // we only care for lldpad and virip messages > + if (hdr->nlmsg_type != RTM_SETLINK) > + return; // we only care for RTM_SETLINK > + if (*handled) > + return; // if it has been handeled - dont handle again misspelled "handled" (Could this if be further up in the function?) > + > + /* DEBUG start */ > + VIR_INFO("netlink message nl_sockaddr: %p len: %d", peer, > length); > + VIR_DEBUG("nlmsg_type = 0x%02x",hdr->nlmsg_type); > + VIR_DEBUG("nlmsg_len = 0x%04x",hdr->nlmsg_len ); > + VIR_DEBUG("nlmsg_pid = %d",hdr->nlmsg_pid ); > + VIR_DEBUG("nlmsg_seq = 0x%08x",hdr->nlmsg_seq ); > + VIR_DEBUG("nlmsg_flags = 0x%04x",hdr->nlmsg_flags ); > + > + VIR_DEBUG("lldpad pid = %d",lldpad_pid); > + > + switch (hdr->nlmsg_type) { > + case RTM_NEWLINK: > + case RTM_DELLINK: > + case RTM_SETLINK: > + case RTM_GETLINK: > + VIR_DEBUG(" IFINFOMSG\n"); > + VIR_DEBUG(" ifi_family = 0x%02x\n", > + ((struct ifinfomsg *)data)->ifi_family); > + VIR_DEBUG(" ifi_type = 0x%x\n", > + ((struct ifinfomsg *)data)->ifi_type); > + VIR_DEBUG(" ifi_index = %i\n", > + ((struct ifinfomsg *)data)->ifi_index); > + VIR_DEBUG(" ifi_flags = 0x%04x\n", > + ((struct ifinfomsg *)data)->ifi_flags); > + VIR_DEBUG(" ifi_change = 0x%04x\n", > + ((struct ifinfomsg *)data)->ifi_change); > + } > + /* DEBUG end */ > + > + /* Parse netlink message assume a setlink with vfports */ > + memcpy(&ifinfo, NLMSG_DATA(hdr), sizeof ifinfo); > + VIR_DEBUG("family:%#x type:%#x index:%d flags:%#x change:%#x", > + ifinfo.ifi_family, ifinfo.ifi_type, ifinfo.ifi_index, > + ifinfo.ifi_flags, ifinfo.ifi_change); > + if (nlmsg_parse(hdr, sizeof ifinfo, > + (struct nlattr **)&tb, IFLA_MAX, NULL)) { > + VIR_DEBUG("error parsing request..."); > + return; > + } > + > + if (tb[IFLA_VFINFO_LIST]) { > + VIR_DEBUG("FOUND IFLA_VFINFO_LIST!"); > + > + nla_for_each_nested(tb_vfinfo_list, tb[IFLA_VFINFO_LIST], > rem) { > + if (nla_type(tb_vfinfo_list) != IFLA_VF_INFO) { > + VIR_DEBUG("nested parsing of" > + "IFLA_VFINFO_LIST failed."); > + return; > + } > + if (nla_parse_nested(tb_vfinfo, IFLA_VF_MAX, > + tb_vfinfo_list, ifla_vf_policy)) { > + VIR_DEBUG("nested parsing of " > + "IFLA_VF_INFO failed."); > + return; > + } > + } > + > + if (tb_vfinfo[IFLA_VF_MAC]) { > + struct ifla_vf_mac *mac = > + RTA_DATA(tb_vfinfo[IFLA_VF_MAC]); > + unsigned char *m = mac->mac; > + > + VIR_DEBUG("IFLA_VF_MAC=%2x:%2x:%2x:%2x:%2x:%2x", > + m[0], m[1], m[2], m[3], m[4], m[5]); > + > + if (memcmp(calld->macaddress, m, VIR_MAC_BUFLEN)) > + { > + /* Repeat the same check for a broadcast mac */ > + unsigned char broadcastmac[VIR_MAC_BUFLEN]; > + int i; > + > + for (i=0;i<VIR_MAC_BUFLEN;i++) > + broadcastmac[i]=0xFF; > + > + if (memcmp(calld->macaddress, broadcastmac, > VIR_MAC_BUFLEN)) { > + VIR_DEBUG("MAC address match failed."); > + return; Why not just compare calld->macaddress[i] to 0xFF in the for loop, returning immediately when a non-FF byte is found? > + } > + } > + } > + > + if (tb_vfinfo[IFLA_VF_VLAN]) { > + struct ifla_vf_vlan *vlan = > + RTA_DATA(tb_vfinfo[IFLA_VF_VLAN]); > + > + VIR_DEBUG("IFLA_VF_VLAN=%d", vlan->vlan); > + } > + } > + > + if (tb[IFLA_IFNAME]) { > + ifname = (char *)RTA_DATA(tb[IFLA_IFNAME]); > + VIR_DEBUG("IFLA_IFNAME=%s\n", ifname); > + } > + > + if (tb[IFLA_OPERSTATE]) { > + rem = *(unsigned short *)RTA_DATA(tb[IFLA_OPERSTATE]); > + VIR_DEBUG("IFLA_OPERSTATE=%d\n", rem); > + } > + > + if (tb[IFLA_VF_PORTS]) { > + struct nlattr *tb_vf_ports; > + > + VIR_DEBUG("found IFLA_VF_PORTS\n"); > + nla_for_each_nested(tb_vf_ports, tb[IFLA_VF_PORTS], rem) { > + > + VIR_DEBUG("iterating\n"); > + if (nla_type(tb_vf_ports) != IFLA_VF_PORT) { > + VIR_DEBUG("not a IFLA_VF_PORT. skipping\n"); > + continue; > + } > + if (nla_parse_nested(tb3, IFLA_PORT_MAX, tb_vf_ports, > + ifla_port_policy)) { > + VIR_DEBUG("nested parsing on level 2" > + " failed."); > + } > + if (tb3[IFLA_PORT_VF]) { > + VIR_DEBUG("IFLA_PORT_VF=%d", > + *(uint32_t > + *) (RTA_DATA(tb3[IFLA_PORT_VF]))); > + } > + if (tb3[IFLA_PORT_PROFILE]) { > + VIR_DEBUG("IFLA_PORT_PROFILE=%s", (char *) > + RTA_DATA(tb3[IFLA_PORT_PROFILE])); > + } > + > + if (tb3[IFLA_PORT_VSI_TYPE]) { > + struct ifla_port_vsi *pvsi; > + int tid = 0; > + > + pvsi = (struct ifla_port_vsi *) > + RTA_DATA(tb3[IFLA_PORT_VSI_TYPE]); > + tid = pvsi->vsi_type_id[2] << 16 | > + pvsi->vsi_type_id[1] << 8 | > + pvsi->vsi_type_id[0]; > + > + VIR_DEBUG("mgr_id: %d", pvsi->vsi_mgr_id); > + VIR_DEBUG("type_id: %d", tid); > + VIR_DEBUG("type_version: %d", > + pvsi->vsi_type_version); > + } > + > + if (tb3[IFLA_PORT_INSTANCE_UUID]) { > + char instance[INSTANCE_STRLEN + 2]; > + unsigned char *uuid; > + > + uuid = (unsigned char *) > + RTA_DATA(tb3[IFLA_PORT_INSTANCE_UUID]); > + instance2str(uuid, instance, sizeof(instance)); > + VIR_DEBUG("IFLA_PORT_INSTANCE_UUID=%s\n", > + instance); > + } > + > + if (tb3[IFLA_PORT_REQUEST]) { > + uint8_t req = *(uint8_t *) > RTA_DATA(tb3[IFLA_PORT_REQUEST]); > + VIR_DEBUG("IFLA_PORT_REQUEST=%d", req); > + > + if ( req == PORT_REQUEST_DISASSOCIATE ) { > + VIR_DEBUG("Set dissaccociated."); > + indicate=true; > + } > + } > + > + if (tb3[IFLA_PORT_RESPONSE]) { > + VIR_DEBUG("IFLA_PORT_RESPONSE=%d\n", *(uint16_t *) > + RTA_DATA(tb3[IFLA_PORT_RESPONSE])); > + } > + } > + } > + > + if (!indicate) { > + return; > + } > + > + VIR_INFO("Re-send 802.1qbg associate request:"); > + VIR_INFO(" if: %s",calld->cr_ifname ); > + VIR_INFO(" lf: %s",calld->linkdev ); > + VIR_INFO(" mac: %02x:%02x:%02x:%02x:%02x:%02x",calld- > >macaddress[0], > + calld- > >macaddress[1], > + calld- > >macaddress[2], > + calld- > >macaddress[3], > + calld- > >macaddress[4], > + calld- > >macaddress[5] ); Bad indentation. > + > + ignore_value(virNetDevVPortProfileAssociate(calld->cr_ifname, > + calld->virtPortProfile, > + calld->macaddress, > + calld->linkdev, > + calld->vmuuid, calld- > >vmOp, true)); > + > + *handled = true; > + return; > +} > + > /** > * virNetDevMacVLanCreateWithVPortProfile: > * Create an instance of a macvtap device and open its tap character > @@ -547,7 +838,7 @@ create_name: > virtPortProfile, > macaddress, > linkdev, > - vmuuid, vmOp) < 0) { > + vmuuid, vmOp, false) < 0) { > rc = -1; > goto link_del_exit; > } > @@ -589,6 +880,23 @@ create_name: > goto disassociate_exit; > } > > + if (virNetlinkEventServiceIsRunning()) { > + virNetlinkCallbackDataPtr calld; > + > + if (VIR_ALLOC(calld) < 0) { > + virReportOOMError(); > + goto disassociate_exit; > + } > + > + strncpy(calld->cr_ifname,cr_ifname,64); > + calld->virtPortProfile=virtPortProfile; > + calld->macaddress=macaddress; > + calld->linkdev=linkdev; > + calld->vmuuid=vmuuid; > + calld->vmOp=vmOp; Put spaces around "=" signs. > + > + > virNetlinkEventAddClient(virNetDevMacVLanVPortProfileCallback, > calld, macaddress); Maybe we should check for a return value here. > + } > > return rc; > > @@ -638,6 +946,9 @@ int > virNetDevMacVLanDeleteWithVPortProfile(const char *ifname, > if (virNetDevMacVLanDelete(ifname) < 0) > ret = -1; > } > + > + virNetlinkEventRemoveClient(0,macaddr); > + > return ret; > } > > diff --git a/src/util/virnetdevvportprofile.c b/src/util/ > virnetdevvportprofile.c > index faadc3a..554f128 100644 > --- a/src/util/virnetdevvportprofile.c > +++ b/src/util/virnetdevvportprofile.c > @@ -650,7 +650,8 @@ virNetDevVPortProfileOpCommon(const char > *ifname, int ifindex, > const unsigned char *instanceId, > const unsigned char *hostUUID, > int32_t vf, > - uint8_t op) > + uint8_t op, > + bool setlink_only) > { > int rc; > unsigned char *recvbuf = NULL; > @@ -675,6 +676,9 @@ virNetDevVPortProfileOpCommon(const char > *ifname, int ifindex, > return rc; > } > > + if (setlink_only) /*for re-associations on existing links*/ > + return 0; > + > while (--repeats >= 0) { > rc = virNetDevVPortProfileLinkDump(NULL, ifindex, > nltarget_kernel, tb, > &recvbuf, > virNetDevVPortProfileGetLldpadPid); > @@ -751,7 +755,8 @@ static int > virNetDevVPortProfileOp8021Qbg(const char *ifname, > const unsigned char *macaddr, > const virNetDevVPortProfilePtr > virtPort, > - enum virNetDevVPortProfileLinkOp > virtPortOp) > + enum virNetDevVPortProfileLinkOp > virtPortOp, > + bool setlink_only) > { > int rc = 0; > int op = PORT_REQUEST_ASSOCIATE; > @@ -804,7 +809,8 @@ virNetDevVPortProfileOp8021Qbg(const char > *ifname, > virtPort- > >u.virtPort8021Qbg.instanceID, > NULL, > vf, > - op); > + op, > + setlink_only); > > err_exit: > > @@ -892,7 +898,8 @@ virNetDevVPortProfileOp8021Qbh(const char > *ifname, > (virtPortOp == > > VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE_RR) ? > > PORT_REQUEST_PREASSOCIATE_RR > - : > PORT_REQUEST_ASSOCIATE); > + : PORT_REQUEST_ASSOCIATE, > + false); > if (rc == -2) > /* Association timed out, disassociate */ > virNetDevVPortProfileOpCommon(NULL, ifindex, > @@ -904,7 +911,8 @@ virNetDevVPortProfileOp8021Qbh(const char > *ifname, > NULL, > NULL, > vf, > - > PORT_REQUEST_DISASSOCIATE); > + PORT_REQUEST_DISASSOCIATE, > + false); > break; > > case VIR_NETDEV_VPORT_PROFILE_LINK_OP_DISASSOCIATE: > @@ -917,7 +925,8 @@ virNetDevVPortProfileOp8021Qbh(const char > *ifname, > NULL, > NULL, > vf, > - > PORT_REQUEST_DISASSOCIATE); > + > PORT_REQUEST_DISASSOCIATE, > + false); > break; > > default: > @@ -938,6 +947,7 @@ err_exit: > * @virtPort: pointer to the object holding port profile parameters > * @vmuuid : the UUID of the virtual machine > * @vmOp : The VM operation (i.e., create, no-op) > + * @setlink_only : Only set the link - dont wait for the link to > come up > * > * Associate a port on a swtich with a profile. This function > * may notify a kernel driver or an external daemon to run > @@ -954,7 +964,8 @@ virNetDevVPortProfileAssociate(const char > *macvtap_ifname, > const unsigned char *macvtap_macaddr, > const char *linkdev, > const unsigned char *vmuuid, > - enum virNetDevVPortProfileOp vmOp) > + enum virNetDevVPortProfileOp vmOp, > + bool setlink_only) > { > int rc = 0; > > @@ -976,7 +987,8 @@ virNetDevVPortProfileAssociate(const char > *macvtap_ifname, > virtPort, > (vmOp == > VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START) > ? > VIR_NETDEV_VPORT_PROFILE_LINK_OP_PREASSOCIATE > - : > VIR_NETDEV_VPORT_PROFILE_LINK_OP_ASSOCIATE); > + : > VIR_NETDEV_VPORT_PROFILE_LINK_OP_ASSOCIATE, > + setlink_only); > break; > > case VIR_NETDEV_VPORT_PROFILE_8021QBH: > @@ -1033,7 +1045,7 @@ virNetDevVPortProfileDisassociate(const char > *macvtap_ifname, > case VIR_NETDEV_VPORT_PROFILE_8021QBG: > rc = virNetDevVPortProfileOp8021Qbg(macvtap_ifname, > macvtap_macaddr, > virtPort, > - > VIR_NETDEV_VPORT_PROFILE_LINK_OP_DISASSOCIATE); > + > VIR_NETDEV_VPORT_PROFILE_LINK_OP_DISASSOCIATE, false); > break; > > case VIR_NETDEV_VPORT_PROFILE_8021QBH: > @@ -1056,7 +1068,8 @@ int virNetDevVPortProfileAssociate(const char > *macvtap_ifname ATTRIBUTE_UNUSED, > const unsigned char *macvtap_macaddr > ATTRIBUTE_UNUSED, > const char *linkdev ATTRIBUTE_UNUSED, > const unsigned char *vmuuid > ATTRIBUTE_UNUSED, > - enum virNetDevVPortProfileOp vmOp > ATTRIBUTE_UNUSED) > + enum virNetDevVPortProfileOp vmOp > ATTRIBUTE_UNUSED, > + bool setlink_only) > { > virReportSystemError(ENOSYS, "%s", > _("Virtual port profile association not > supported on this platform")); > diff --git a/src/util/virnetdevvportprofile.h b/src/util/ > virnetdevvportprofile.h > index 7a8d81f..fe80086 100644 > --- a/src/util/virnetdevvportprofile.h > +++ b/src/util/virnetdevvportprofile.h > @@ -81,7 +81,8 @@ int virNetDevVPortProfileAssociate(const char > *ifname, > const unsigned char *macaddr, > const char *linkdev, > const unsigned char *vmuuid, > - enum virNetDevVPortProfileOp > vmOp) > + enum virNetDevVPortProfileOp > vmOp, > + bool setlink_only) > ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) > ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK; > -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list