Re: [libvirt] [PATCH v0] qemu driver FreeBSD support

On 12/09/2012 10:17 AM, Roman Bogorodskiy wrote: 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: Why do you need a new file? What is in here that cannot be applied to the generic README? 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. 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? 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