On 01/26/2013 08:13 AM, Roman Bogorodskiy wrote:
> ---
> src/util/virnetdev.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
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.
>
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 295884f..503db9d 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -44,7 +44,7 @@
>
> #define VIR_FROM_THIS VIR_FROM_NONE
>
> -#if defined(HAVE_STRUCT_IFREQ)
> +#if defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__)
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.
> static int virNetDevSetupControlFull(const char *ifname,
> struct ifreq *ifr,
> int domain,
> @@ -81,12 +81,15 @@ static int virNetDevSetupControlFull(const char *ifname,
> static int virNetDevSetupControl(const char *ifname,
> struct ifreq *ifr)
> {
> +#if defined(__FreeBSD__)
> + return virNetDevSetupControlFull(ifname, ifr, AF_LOCAL, SOCK_DGRAM);
> +#else
> return virNetDevSetupControlFull(ifname, ifr, AF_PACKET, SOCK_DGRAM);
> +#endif
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);
}
> }
> #endif
>
> -
> -#if defined(SIOCGIFFLAGS) && defined(HAVE_STRUCT_IFREQ)
> +#if defined(SIOCGIFFLAGS) && (defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__))
This #if would use the same feature check as earlier code, rather than
hard-coding to FreeBSD.
> /**
> * virNetDevExists:
> * @ifname
> @@ -105,7 +108,12 @@ int virNetDevExists(const char *ifname)
> return -1;
>
> if (ioctl(fd, SIOCGIFFLAGS, &ifr)) {
> + /* FreeBSD throws ENXIO when interface doesn't exist */
> +#if defined(__FreeBSD__)
> + if (errno == ENXIO)
> +#else
> if (errno == ENODEV)
> +#endif
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)
> ret = 0;
> else
> virReportSystemError(errno,
> @@ -459,7 +467,7 @@ int virNetDevSetNamespace(const char *ifname, pid_t pidInNs)
> return rc;
> }
>
> -#if defined(SIOCSIFNAME) && defined(HAVE_STRUCT_IFREQ)
> +#if defined(SIOCSIFNAME) && (defined(HAVE_STRUCT_IFREQ) || defined(__FreeBSD__))
Another place where we need a feature test, rather than FreeBSD.
> /**
> * virNetDevSetName:
> * @ifname: name of device
> @@ -478,12 +486,16 @@ int virNetDevSetName(const char* ifname, const char *newifname)
> if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0)
> return -1;
>
> +#if defined(__FreeBSD__)
> + ifr.ifr_data = (caddr_t)newifname;
> +#else
> if (virStrcpyStatic(ifr.ifr_newname, newifname) == NULL) {
> virReportSystemError(ERANGE,
> _("Network interface name '%s' is too long"),
> newifname);
> goto cleanup;
> }
> +#endif
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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list