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 },
};
> +
> +
> +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 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
> + 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)'?
> + virReportSystemError(-err->error,
> + _("error dumping %d interface"),
> + ifindex);
> + rc = -1;
> + }
> + break;
> +
> + case GENL_ID_CTRL:
> + case NLMSG_DONE:
> + if (nlmsg_parse(resp, sizeof(struct ifinfomsg),
> + tb, IFLA_MAX, ifla_policy)) {
> + goto malformed_resp;
> + }
> + break;
> +
> + default:
> + goto malformed_resp;
> + }
> +
> + if (rc != 0)
> + VIR_FREE(*recvbuf);
> +
> + return rc;
> +
> +malformed_resp:
> + macvtapError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("malformed netlink response message"));
> + VIR_FREE(*recvbuf);
> + return -1;
> +
> +buffer_too_small:
> + macvtapError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("internal buffer is too small"));
> + return -1;
> +}
> +
> +
> +/* TODO: move this into interface.c after moving netlink functions into
> + * utils dir
> + */
> +/**
> + * ifaceGetNthParent
> + *
> + * @ifindex : the index of the interface or -1 if ifname is given
> + * @ifname : the name of the interface; ignored if ifindex is valid
> + * @nthParent : the nth parent interface to get
> + * @rootifname : pointer to buffer of size IFNAMSIZ
> + * @nth : the nth parent that is actually returned; if for example eth0.100
> + * was given and the 100th parent is to be returned, then eth0 will
> + * most likely be returned with nth set to 1 since the chain does
> + * not have more interfaces
> + *
> + * Get the nth parent interface of the given interface. 0 is the interface
> + * itself.
> + *
> + * Return 0 on success, != 0 otherwise
> + */
> +static int
> +ifaceGetNthParent(int ifindex, const char *ifname, unsigned int nthParent,
> + char *rootifname, unsigned int *nth)
> +{
> + int rc;
> + struct nlattr *tb[IFLA_MAX + 1];
> + char *recvbuf = NULL;
> + bool end = false;
> + unsigned int i = 0;
> +
> + while (!end && i <= nthParent) {
> + rc = link_dump(ifindex, ifname, tb, &recvbuf);
> + if (rc)
> + break;
> +
> + if (tb[IFLA_IFNAME]) {
> + if (!virStrcpy(rootifname, (char*)RTA_DATA(tb[IFLA_IFNAME]),
> + IFNAMSIZ)) {
> + macvtapError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("buffer for root interface name is too small"));
> + VIR_FREE(recvbuf);
> + return 1;
> + }
> + }
> +
> + if (tb[IFLA_LINK]) {
> + ifindex = *(int *)RTA_DATA(tb[IFLA_LINK]);
> + ifname = NULL;
> + } else
> + end = true;
> +
> + VIR_FREE(recvbuf);
> +
> + i++;
> + }
> +
> + if (nth)
> + *nth = i-1;
Spacing: i - 1
> +
> + return rc;
> +}
> +
> +
> +/**
> + * ifaceGetRootIface
> + *
> + * @ifindex : the index of the interface or -1 if ifname is given
> + * @ifname : the name of the interface; ignored if ifindex is valid
> + * @rootifname : pointer to buffer of size IFNAMSIZ
> + *
> + * Get the root interface of a given interface, i.e., if macvtap
> + * is linked to eth0.100, it will return eth0.
> + *
> + * Return 0 on success, != 0 otherwise
> + */
> +static int
> +ifaceGetRootIface(int ifindex, const char *ifname,
> + char *rootifname)
> +{
> + return ifaceGetNthParent(ifindex, ifname, ~0, rootifname, NULL);
> +}
> +
> +
> /* Open the macvtap's tap device.
> * @ifname: Name of the macvtap interface
> * @retries : Number of retries in case udev for example may need to be
> Index: libvirt-acl/src/Makefile.am
> ===================================================================
> --- 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.
> libvirt_lxc_CFLAGS = \
> $(LIBPARTED_CFLAGS) \
> $(NUMACTL_CFLAGS) \
> Index: libvirt-acl/configure.ac
> ===================================================================
> --- libvirt-acl.orig/configure.ac
> +++ libvirt-acl/configure.ac
> @@ -42,6 +42,7 @@ HAL_REQUIRED=0.5.0
> DEVMAPPER_REQUIRED=1.0.0
> LIBCURL_REQUIRED="7.18.0"
> LIBPCAP_REQUIRED="1.0.0"
> +LIBNL_REQUIRED="1.1"
>
> dnl Checks for C compiler.
> AC_PROG_CC
> @@ -2005,6 +2006,24 @@ fi
> AM_CONDITIONAL([WITH_MACVTAP], [test "$with_macvtap" = "yes"])
>
>
> +dnl netlink library
> +
> +LIBNL_CFLAGS=""
> +LIBNL_LIBS=""
> +
> +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], [
> + ], [
> + AC_MSG_ERROR([libnl >= $LIBNL_REQUIRED is required for macvtap support])
> + ])
> +fi
> +
> +AC_SUBST([LIBNL_CFLAGS])
> +AC_SUBST([LIBNL_LIBS])
> +
> +
> +
> +
> # Only COPYING.LIB is under version control, yet COPYING
> # is included as part of the distribution tarball.
> # Copy one to the other, but only if this is a srcdir-build.
> @@ -2183,6 +2202,11 @@ AC_MSG_NOTICE([ pcap: $LIBPCAP_CFLAGS
> else
> AC_MSG_NOTICE([ pcap: no])
> fi
> +if test "$with_macvtap" = "yes" ; then
> +AC_MSG_NOTICE([ nl: $LIBNL_CFLAGS $LIBNL_LIBS])
> +else
> +AC_MSG_NOTICE([ nl: no])
> +fi
> AC_MSG_NOTICE([])
> AC_MSG_NOTICE([Test suite])
> AC_MSG_NOTICE([])
> Index: libvirt-acl/libvirt.spec.in
> ===================================================================
> --- libvirt-acl.orig/libvirt.spec.in
> +++ libvirt-acl/libvirt.spec.in
> @@ -63,6 +63,7 @@
> %define with_yajl 0%{!?_without_yajl:0}
> %define with_nwfilter 0%{!?_without_nwfilter:0}
> %define with_libpcap 0%{!?_without_libpcap:0}
> +%define with_macvtap 0%{!?_without_macvtap:0}
>
> # Non-server/HV driver defaults which are always enabled
> %define with_python 0%{!?_without_python:1}
> @@ -153,6 +154,11 @@
> %if %{with_qemu}
> %define with_nwfilter 0%{!?_without_nwfilter:%{server_drivers}}
> %define with_libpcap 0%{!?_without_libpcap:%{server_drivers}}
> +%define with_macvtap 0%{!?_without_macvtap:%{server_drivers}}
> +%endif
> +
> +%if %{with_macvtap}
> +%define with_libnl 1
> %endif
>
> # Force QEMU to run as non-root
> @@ -282,6 +288,9 @@ BuildRequires: yajl-devel
> %if %{with_libpcap}
> BuildRequires: libpcap-devel
> %endif
> +%if %{with_libnl}
> +BuildRequires: libnl-devel
> +%endif
> %if %{with_avahi}
> BuildRequires: avahi-devel
> %endif
> @@ -531,6 +540,10 @@ of recent versions of Linux (and other O
> %define _without_libpcap --without-libpcap
> %endif
>
> +%if ! %{with_macvtap}
> +%define _without_macvtap --without-macvtap
> +%endif
> +
> %configure %{?_without_xen} \
> %{?_without_qemu} \
> %{?_without_openvz} \
> @@ -560,6 +573,7 @@ of recent versions of Linux (and other O
> %{?_without_udev} \
> %{?_without_yajl} \
> %{?_without_libpcap} \
> + %{?_without_macvtap} \
> --with-qemu-user=%{qemu_user} \
> --with-qemu-group=%{qemu_group} \
> --with-init-script=redhat \
I think I mentioned enough things that it's worth seeing a v2 before
giving an ack.
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list