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 :-)
+ 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'm fine with the patch to the extend I understand it implications +1 Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/