[Libvir] PATCH 15/20: remove use of libsysfs in bridge code

[Daniel Veillard]

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 at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/