[libvirt] [PATCH v0] qemu driver FreeBSD support

[Eric Blake]

On 12/09/2012 10:17 AM, Roman Bogorodskiy wrote:
> Hello,
> 
> Attached an initial version of the patch providing FreeBSD support for
> qemu driver. Initial discussion on the topic started here:
> 
> https://www.redhat.com/archives/libvir-list/2012-November/msg00841.html

As I mentioned on IRC, it is extremely hard to review a monolithic patch
that is 200k large [also, the list moderation flags any message larger
than 150k, which adds moderation delay into the mix].  It would be much
better to break this into multiple commits, each which touches a smaller
task and can be reviewed in isolation (the intermediate results should
pass 'make check' on all platforms, but you don't have to have working
BSD support until the end of the series).  I haven't looked at
everything, but here's a few initial comments:

> diff --git a/README-freebsd b/README-freebsd
> new file mode 100644
> index 0000000..2c4079b

Why do you need a new file?  What is in here that cannot be applied to
the generic README?

> @@ -0,0 +1,65 @@
> +Configuring:
> +
> + ./configure --with-qemu --disable-werror --with-qemu-group=wheel --without-polkit --without-firewalld

How many of these configure options are the result of bad defaults in
configure.ac?  It would be better to patch configure.ac so that a
default './configure' does the right thing, rather than requiring the
user to pass lots of extra flags.

> +To allow a VM have internet connection:
> +
> +
> +ipfw nat 1000 config if re0
> +
> +ipfw add 1000 nat 1000 log ip from 192.168.122.0/24 to any out
> +ipfw add 1001 nat 1000 log ip from any to any in
> +

Why can't libvirt do this directly?  How much of this work can be added
to netcf, so that libvirt's wrappers around netcf just work?

> 
> +++ b/src/Makefile.am
> @@ -51,6 +51,14 @@ augeastest_DATA =
>  
>  # These files are not related to driver APIs. Simply generic
>  # helper APIs for various purposes
> +if WITH_FREEBSD
> +NETDEV = util/bsd_virnetdevtap.h util/bsd_virnetdevtap.c \
> +	 util/bsd_virnetdevbridge.h util/bsd_virnetdevbridge.c
> +else
> +NETDEV = util/virnetdevtap.h util/virnetdevtap.c \
> +	util/virnetdevbridge.h util/virnetdevbridge.c
> +endif

Yuck.  New files should be in the vir* namespace, not the bsd_vir*
namespace.  But why do we even need new files?  We should REALLY be
making the single file virndetdevtap.h generic enough in its interface
so that all other files just include the one header and it just works or
has no-op stubs.  It is okay to have more than one .c file but even
then, you should mirror how we did src/util/threads-{pthread,win32}.c,
by making the platform dependence a suffix rather than a prefix of the
file name of the implementation, and where src/util/threads.h is the
common header that the rest of the source files use.

Probably lots more that I could review, but at this point, I'd much
rather see smaller patches to individual problems, so that we can
quickly commit the no-brainers (such as things that are necessary to fix
broken compilation) while still improving on the things that will have
review comments (such as adding major features).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20121210/d000bc9b/attachment-0001.sig>