On 07/04/2013 10:25 AM, Roman Bogorodskiy wrote:
> * Functions were renamed: s/Iptables/Firewall/
> to make names more general.
> * Platform specific things (e.g. firewalling and route
> collision checks) were moved into bridge_driver_platform
> * Created two platform specific implementations:
> - bridge_driver_linux: Linux implementation using iptables,
> it's actually the code moved from bridge_driver.c
> - bridge_driver_noop: dump implementation that does nothing
I've let review sit for so long that it no longer applies, and needs a
rebasing; but I'll at least skim the patch looking for potential issues
to be aware of during the rebase...
> ---
> po/POTFILES.in | 1 +
> src/Makefile.am | 5 +-
> src/network/bridge_driver.c | 729 +---------------------------------
> src/network/bridge_driver_linux.c | 709 +++++++++++++++++++++++++++++++++
> src/network/bridge_driver_noop.c | 80 ++++
> src/network/bridge_driver_platform.c | 32 ++
> src/network/bridge_driver_platform.h | 77 ++++
> 7 files changed, 915 insertions(+), 718 deletions(-)
> create mode 100644 src/network/bridge_driver_linux.c
> create mode 100644 src/network/bridge_driver_noop.c
> create mode 100644 src/network/bridge_driver_platform.c
> create mode 100644 src/network/bridge_driver_platform.h
>
If you generate the email with 'git send-email/format-patch
-O/path/to/file', then you can use a file that prioritizes .h files to
come first in the diff listing; this can sometimes help in interface
reviews.
>
> -/* Main driver state */
> -struct network_driver {
You moved this struct out of a .c file into driver_platform.h; as a
result, this poor choice of naming will potentially cause problems to
more files that include the .h. Can you please do a separate cleanup
pre-patch that s/network_driver/virNetworkDriver/, and uses a typedef so
that you don't have to repeat 'struct' everywhere?
> - virMutex lock;
> -
> - virNetworkObjList networks;
Also, it might be worth a patch to make this struct inherit from
virObjectLockable, instead of open-coding the locking ourselves. But
that's separate from your goal of splitting the files, so it's not a
prerequisite.
> @@ -114,7 +99,7 @@ static int networkStartNetworkExternal(struct network_driver *driver,
> static int networkShutdownNetworkExternal(struct network_driver *driver,
> virNetworkObjPtr network);
>
> -static void networkReloadIptablesRules(struct network_driver *driver);
> +static void networkReloadFirewallRules(struct network_driver *driver);
For ease of review, it might be worth splitting the function renames
into a separate patch from the code motion.
> +++ 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.
--
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