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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list