Eric Blake <eblake@redhat.com> 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