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

11 May 2010

...
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
Eric Blake <eblake@redhat.com> wrote on 05/10/2010 12:48:18 PM:


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

    