Re: [libvirt] [PATCH v3 1/4] FreeBSD: implement virNetDevExists() and virNetDevSetName().

On 01/26/2013 08:13 AM, Roman Bogorodskiy wrote: Burying your v3 in reply to v1 and v2 made it hard to find this thread. When you post a v4, it might be nicer to post it as a fresh thread, and then in your cover letter, provide a URL back to the list archives of the earlier versions. Your patch failed 'make syntax-check' if you have the 'cppi' program installed: preprocessor_indentation cppi: src/util/virnetdev.c: line 84: not properly indented cppi: src/util/virnetdev.c: line 86: not properly indented cppi: src/util/virnetdev.c: line 88: not properly indented cppi: src/util/virnetdev.c: line 112: not properly indented cppi: src/util/virnetdev.c: line 114: not properly indented cppi: src/util/virnetdev.c: line 116: not properly indented cppi: src/util/virnetdev.c: line 489: not properly indented cppi: src/util/virnetdev.c: line 491: not properly indented cppi: src/util/virnetdev.c: line 498: not properly indented Basically, we mandate a coding style of: #if foo # if bar # define blah # else /* whatever */ # endif #endif where every nested preprocessor statement has one more space between # and the rest of the line to make it easier to detect the nesting. This feels wrong. Rather than hard-coding a check to __FreeBSD__, we should instead be doing a feature check. The earlier branch is a feature check: HAVE_STRUCT_IFREQ is determined at configure time. What particular _feature_ of FreeBSD are we using, different than 'struct ifreq', but similar enough that it will compile with this patch? Once we know what that feature is, then we should fix configure.ac to probe for that feature, and then use HAVE_STRUCT_XXX here instead of hard-coding __FreeBSD__. That way, other BSDs that share the same struct will work out of the box, without us having to revisit this #if. Is the only difference here AF_LOCAL vs. AF_PACKET? Could we hoist the #if out of the function body, based on feature checks, something like: #if HAVE_STRUCT_IFREQ /* such as Linux */ # define VIR_NETDEV_FAMILY AF_PACKET #elif HAVE_STRUCT_WHATEVER /* such as FreeBSD */ # define VIR_NETDEV_FAMILY AF_LOCAL #endif #ifdef VIR_NETDEV_FAMILY static int virNetDevSetupControl(const char *ifname, struct ifreq *ifr) { return virNetDevSetupControlFull(ifname, ifr, VIR_NETDEV_FAMILY, SOCK_DGRAM); } This #if would use the same feature check as earlier code, rather than hard-coding to FreeBSD. No need for #if or even a complicated comment here. I'd just write this as: if (ioctl(fd, SIOCGIFFLAGS, &ifr)) { if (errno == ENXIO || errno == ENODEV) Another place where we need a feature test, rather than FreeBSD. Ah, here we finally get to the meat of the change: Linux has ifr.ifr_newname, while FreeBSD has ifr.ifr_data. That's relatively easy to add a configure.ac feature probe for: AC_CHECK_MEMBERS([struct ifreq.ifr_data, struct ifreq.ifr_newname], [], [], [[#include <whatever.h>]]) with appropriate #include lines for struct ifreq, and where autoconf will then provide HAVE_STRUCT_IFREQ_IFR_DATA and HAVE_STRUCT_IFREQ_IFR_NEWNAME for use in the rest of our code, and we are no longer hard-coded to which OS provides which spelling. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org