Eric Blake <eblake@xxxxxxxxxx> wrote on 05/10/2010 12:48:18 PM:
> 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 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 },
> };
Ok. Done.
>
>
> > +
> > +
> > +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?
I replaced this with a #define now. Also 2 other places were touched.
>
> > + struct nlmsghdr *nlm = (struct nlmsghdr *)nlmsgbuf, *resp;
> > + struct nlmsgerr *err;
> > + char rtattbuf[64];
>
> Likewise.
Introduced #define.
>
> > + 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 };
Doing that now.
>
> 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
Done.
>
> > + 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)'?
For consistency reasons with the other code I'd like to keep it that way.
> > +
> > + if (nth)
> > + *nth = i-1;
>
> Spacing: i - 1
Done.
> > ===================================================================
> > --- 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.
I saw that this was changed for the above. I am not introducing
> > +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], [
Yes. Majority is without so far...
>
> I think I mentioned enough things that it's worth seeing a v2 before
> giving an ack.
I'll post v2 shortly.
Stefan
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list