Re: [libvirt] [PATCH 00/33] Refactor all network device handling code

2011-11-09 Thread Daniel P. Berrange
On Thu, Nov 03, 2011 at 05:29:56PM +, Daniel P. Berrange wrote:
 This series is a major re-arrangement and de-duplication of
 internal code for dealing with physical network interfaces.

Thanks for the reviews so far. I have pushed patches 01-10, with the
relevant review fixes applied.

Regards,
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


[libvirt] [PATCH 00/33] Refactor all network device handling code

2011-11-03 Thread Daniel P. Berrange
This series is a major re-arrangement and de-duplication of
internal code for dealing with physical network interfaces.
Currently code for physical network interfaces is split
across the following files

 - bridge.c: bridge management, TAP device management and
 interface IPv4 address management
 - network.c: socket address management, network bandwidth
  and virtual port profile data management
 - interface.c: some generic interface management and
macvtap/macvlan management
 - macvtap.c: more macvtap/macvlan management and virtual
  port profile interface setup code
 - lxc/veth.c: veth device management and generic interface
   management

Furthermore across these files

 - Argument ordering is inconsistent - sometimes ifname is
   the first parameter, sometimes it isn't

 - Error handing is terribly inconsistent

  1. Return errno values
  2. Raise errors
  3. Raise errors and return errno values
  4. Raise errors, except when a parameter tells
 it not to

 - API naming is inconsistent. Some APIs have a vir prefix,
   others have different prefixes, other don't even follow
   a standard prefix, eg virSocketFormatAddr vs virSocketAddrMaks
   vs virSocketGetPort

 - The bridge.c APIs have an annoying 'brControl *' struct
   that must be passed around just to avoid opening  closing
   an unbound socket.

 - Some APIs are implemented natively, while others call
   out to external commands
 - Duplication of functionality across APIs

 - XML parsing  formatting code is in src/util instead
   of src/conf

 - Some of the API declarations are hidden behind #ifdefs
   forcing yet more #ifdefs in the callers of the APIs

After applying this series, we get to a stage where the source
file split is:

  - src/util/virnetdev.c: APIs relating to any type of interface
  - src/util/virnetdevbridge.c: APIs relating to bridge interfaces
  - src/util/virnetdevmacvlan.c: APIs relating to macvlan/macvtap interfaces
  - src/util/virnetdevveth.c: APIs relating to veth interfaces

  - src/util/virnetdevbandwidth.c: APIs for network bandwidth controls
  - src/util/virnetdevvportprofile.c: APIs for 802.11Qb{g,h} port profiles
  - src/util/virsocketaddr.c: the socket address management APIs

  - src/conf/netdev_bandwidth_conf.c: Parsing/formatting bandwidth XML
  - src/conf/netdev_vport_profile_conf.c: parsing/formatting 801.11Qb{g,h} 
profile XML

The following style is followed

 - All APIs return -1 on error and raise a libvirt error. No exceptions
   or flags to turn off errors

 - Callers which don't want the raised error call virResetLastError

 - All APIs are annotated with ATTRIBUTE_NONNULL and ATTRIBUTE_RETURN_CHECK
   where appropriate

 - The first parameter of all APIs operating on a network interface is
   the interface name

 - All APIs have a fixed name prefix which matches the source file. No
   exceptions.

 - All XML handling code is under src/conf/
 - All APIs are unconditionally declared in header files, and stubbed
   out with virReportSystemError(ENOSYS...) where unsupported.

 - No functionality is duplicated across files

 - External commands are avoided except in case of setting IPv4
   addresses and network bandwidth controls

The diffstat is a little bit misleading, showing a slight growth in the
number of lines. This is primarily due to the extra GPL copyright header
lines in the new files, being much larger than those removed from old
files. Overall we have a  decrease in actual real code, through removal
of duplicated APIs

 b/configure.ac  |5 
 b/daemon/libvirtd.h |1 
 b/daemon/remote.c   |1 
 b/libvirt.spec.in   |2 
 b/po/POTFILES.in|   12 
 b/src/Makefile.am   |   26 
 b/src/conf/domain_conf.c|   59 -
 b/src/conf/domain_conf.h|   20 
 b/src/conf/netdev_bandwidth_conf.c  |  230 
 b/src/conf/netdev_bandwidth_conf.h  |   37 
 b/src/conf/netdev_vport_profile_conf.c  |  236 
 b/src/conf/netdev_vport_profile_conf.h  |   39 
 b/src/conf/network_conf.c   |  101 +-
 b/src/conf/network_conf.h   |   12 
 b/src/conf/nwfilter_conf.c  |   16 
 b/src/conf/nwfilter_conf.h  |2 
 b/src/esx/esx_util.h|2 
 b/src/libvirt_private.syms  |   71 -
 b/src/lxc/lxc_container.c   |9 
 b/src/lxc/lxc_controller.c  |7 
 b/src/lxc/lxc_driver.c  |   42 
 b/src/network/bridge_driver.c   |  192 +--
 b/src/nwfilter/nwfilter_ebiptables_driver.c |4 
 b/src/nwfilter/nwfilter_gentech_driver.c|   25 
 b/src/nwfilter/nwfilter_learnipaddr.c   |   32 
 b/src/openvz/openvz_driver.c|1 
 b/src/qemu/qemu_command.c