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

[Daniel P. Berrange]

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  -=|