Re: [libvirt] [PATCH] Determine the root physical interface of a given interface

libvir-list On 05/07/2010 12:09 PM, Stefan Berger wrote: > In this patch I am adding functions that help to iteratively determine > the root physical interface of a given interface. An example would be > that a macvtap device is linked to eth0.100 which in turn is linked to > eth0. Given the name or interface index of the macvtap device that is > linked to eth0.100, eth0 is found by following the links to the end. I > am using now the netlink library to parse the returned netlink

> and for that I am making additions to configure.ac and the rpm spec

> to check for the netlink and netlink-devel packages respectively. In

> configure.ac the requirement to have the netlink library is dependent

> having macvtap. > > > +static struct nla_policy ifla_policy[ IFLA_MAX + 1] = > +{ > + [IFLA_IFNAME ] = {.type = NLA_STRING }, > + [IFLA_LINK] = {.type = NLA_U32 }, > +}; Spacing is inconsistent here. How about: static struct nla_policy ifla_policy[IFLA_MAX + 1] = { [IFLA_IFNAME] = { .type = NLA_STRING }, [IFLA_LINK] = { .type = NLA_U32 }, };

> + > + > +static int > +link_dump(int ifindex, const char *ifname, struct nlattr **tb, > + char **recvbuf) > +{ > + int rc = 0; > + char nlmsgbuf[256]; Is there a #define for this, to avoid a magic number?

> + struct nlmsghdr *nlm = (struct nlmsghdr *)nlmsgbuf, *resp; > + struct nlmsgerr *err; > + char rtattbuf[64]; Likewise.

> + struct rtattr *rta; > + struct ifinfomsg i = { > + .ifi_family = AF_UNSPEC, > + .ifi_index = ifindex > + }; > + int recvbuflen; > + > + *recvbuf = NULL; > + > + memset(&nlmsgbuf, 0, sizeof(nlmsgbuf)); Instead of using memset here, I would zero-initialize the array at its declaration: char nlmsgbuf[256] = { 0 };

I'm not sure whether gcc generates code any differently for the two

> + > + nlInit(nlm, NLM_F_REQUEST, RTM_GETLINK); > + > + if (!nlAppend(nlm, sizeof(nlmsgbuf), &i, sizeof(i))) > + goto buffer_too_small; > + > + if (ifindex < 0 && ifname != NULL) { > + rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_IFNAME, > + ifname, strlen(ifname)+1); Spacing: strlen(ifname) + 1

> + if (!rta) > + goto buffer_too_small; > + > + if (!nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len)) > + goto buffer_too_small; > + } > + > + if (nlComm(nlm, recvbuf, &recvbuflen) < 0) > + return -1; > + > + 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; > + > + default: Given that you have only one non-default case, is this worth a switch, or is it simpler to write as 'if (err->error)'?

> + > + if (nth) > + *nth = i-1; Spacing: i - 1

> =================================================================== > --- libvirt-acl.orig/src/Makefile.am > +++ libvirt-acl/src/Makefile.am > @@ -932,7 +932,7 @@ libvirt_la_LDFLAGS = $(VERSION_SCRIPT_FL > -version-info $(LIBVIRT_VERSION_INFO) \ > $(COVERAGE_CFLAGS:-f%=-Wc,-f%) \ > $(LIBXML_LIBS) \ > - $(LIBPCAP_LIBS) \ > + $(LIBPCAP_LIBS) $(LIBNL_LIBS) \ > $(DRIVER_MODULE_LIBS) \ > $(CYGWIN_EXTRA_LDFLAGS) $(MINGW_EXTRA_LDFLAGS) > libvirt_la_CFLAGS = $(COVERAGE_CFLAGS) -DIN_LIBVIRT > @@ -985,7 +985,7 @@ libvirt_lxc_SOURCES = \ > $(NWFILTER_PARAM_CONF_SOURCES) > libvirt_lxc_LDFLAGS = $(WARN_CFLAGS) $(COVERAGE_LDCFLAGS) $ (CAPNG_LIBS) $(YAJL_LIBS) > libvirt_lxc_LDADD = $(LIBXML_LIBS) $(NUMACTL_LIBS) $(LIB_PTHREAD) \ > - ../gnulib/lib/libgnu.la > + $(LIBNL_LIBS) ../gnulib/lib/libgnu.la When rebasing this patch, remember that these additions go in LIBADD, not LDADD.

> +if test "$with_macvtap" = "yes"; then > + PKG_CHECK_MODULES(LIBNL, libnl-1 >= $LIBNL_REQUIRED, [ Missing M4 quoting: PKG_CHECK_MODULES([LIBNL], [libnl-1 >= $LIBNL_REQUIRED], [

I think I mentioned enough things that it's worth seeing a v2 before giving an ack.