On Fri, Jun 22, 2007 at 09:19:44AM -0400, Daniel Veillard wrote: > On Fri, Jun 22, 2007 at 03:17:07AM +0100, Daniel P. Berrange wrote: > > Sigh, missed another attachment... > > > > On Fri, Jun 22, 2007 at 03:12:11AM +0100, Daniel P. Berrange wrote: > > > The current code for setting up bridges in virtual networks links against > > > the libsysfs library. This is use to get/set the spanning-tree-protocol and > > > forward-delay parameters on the bridge device. None of the four methods > > > using libsysfs are ever called though. The fact that the setters are not > > > called during network start is a bug. There is no need for getters at all > > > since we have the config in memory all the time. The libsysfs is also not > > > exactly an ABI stable library - we're unable to compile libvirt on FC5 > > > for example because of this. This patch changes the bridge code to just > > > invoke the brctl command directly which is much more portable & avoids > > > the need for us to link libvirt.so against libsysfs.so It also fixes the > > > network creation process to actually set STP & forward-delay parameters > > > if specified. > > I don't have enough expertise to really juge the change from the library > to calling the system command, I like the idea of dropping the dependancy > to the library though. > > > > > +#define BRCTL_PATH "/usr/sbin/brctl" > > That should probably be tested in configure.in > I wonder what impact it would have for example on Solaris, expert feedback > would be welcome :-) Well the old code didn't work for Solaris, and neither will the new code :-) This code, and the iptables.c file will both need completely separate impls for Solaris vs Linux, though the top level API in the header files is probably portable enough. > > + if ((null = open(_PATH_DEVNULL, O_RDONLY)) < 0) > > + return errno; > > Hum probably worth raising an error. Though if /dev/null can't > be opened libvirt is probably the last of your worries. > > > + char **argv; > > + int retval = ENOMEM; > > + int n; > > + > > + n = 1 + /* brctl */ > > + 1 + /* setfd */ > > + 1 + /* brige name */ > > + 1; /* value */ > 1; /* NULL */ > > and then calloc with n instead, let's describe fully. > > + > > + if (!(argv = (char **)calloc(n + 1, sizeof(char *)))) > > + goto error; I just copied the approach from iptables.c here. I'll do a separate patch which fixes both files at once. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|