On Wed, Jul 17, 2013 at 05:41:56PM -0600, Eric Blake wrote: > > +++ b/src/network/bridge_driver_platform.c > > @@ -0,0 +1,32 @@ > > +#include "bridge_driver_platform.h" > > > + > > +#if defined(__linux__) > > +# include "bridge_driver_linux.c" > > +#else > > +# include "bridge_driver_noop.c" > > One .c including another is not typical, but we've done it before for > virthread.c; seems like a reasonable way to split things, as long as all > branches of the file declare the same functions. > > The other alternative for splitting things is the way we've done it in > src/security - the public interface is in virSecurityManager, and > basically forwards all calls into a private interface, where each > backend registers a struct of callback pointers. That method is a bit > more robust - you can add a callback to one driver without having to > remember to add it to all drivers simultaneously. With your approach, > if we add a new function in bridge_driver_linux.c, we MUST remember to > add the same function in bridge_driver_noop.c, or it will break > compilation on non-Linux, but developers that don't use Linux will be > unaware of the breakage. With the callback approach, not adding a > callback to bridge_driver_noop.c is not fatal; the manager wrapper would > simply know to do nothing for a NULL callback. > > So, although I like the split, I can't help but wonder if your rebase > should take the road of adjusting things to use a callback struct, > rather than requiring matching implementations across multiple files. I think the callback struct approach is useful for the case where you have multiple possible implementations compiled in at once and we need to choose them dynamically. Here though, we're making the choice at compile time, so I think callback + dynamic dispatch is somewhat overkill. Following of the virthread approach of #include'ing .c file was my suggestion to Roman for how to structure this. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list