<tt>Eric Blake <eblake@redhat.com> wrote on 05/10/2010 12:48:18 PM: > libvir-list</tt> <tt>> > 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 messages > > and for that I am making additions to configure.ac and the rpm spec file > > to check for the netlink and netlink-devel packages respectively. In the > > configure.ac the requirement to have the netlink library is dependent on > > 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 }, > };</tt> <tt>Ok. Done.</tt> <tt> > > > > + > > + > > +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?</tt> <tt>I replaced this with a #define now. Also 2 other places were touched.</tt> <tt> > > > + struct nlmsghdr *nlm = (struct nlmsghdr *)nlmsgbuf, *resp; > > + struct nlmsgerr *err; > > + char rtattbuf[64]; > > Likewise.</tt> <tt>Introduced #define.</tt> <tt> > > > + 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 };</tt> <tt>Doing that now.</tt> <tt> > > I'm not sure whether gcc generates code any differently for the two styles. > > > + > > + 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</tt> <tt>Done.</tt> <tt> > > > + 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)'?</tt> <tt>For consistency reasons with the other code I'd like to keep it that way.</tt> <tt> > > + > > + if (nth) > > + *nth = i-1; > > Spacing: i - 1</tt> <tt>Done.</tt> <tt> > > =================================================================== > > --- 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.</tt> <tt>I saw that this was changed for the above. I am not introducing </tt> <tt> > > +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], [</tt> <tt>Yes. Majority is without so far...</tt> <tt>> > I think I mentioned enough things that it's worth seeing a v2 before > giving an ack.</tt> <tt>I'll post v2 shortly.</tt> <tt> Stefan</tt>