[libvirt] [PATCH] lxc: do not require 'ifconfig' or 'ipconfig' in container

[Daniel P. Berrange]

On Wed, Aug 31, 2011 at 02:02:10PM -0500, Serge Hallyn wrote:
> Quoting Scott Moser (smoser at ubuntu.com):
> > Currently, the lxc implementation invokes 'ip' and 'ifconfig' commands
> > inside a container using 'virRun'.  That has the side effect of requiring
> > those commands to be present and to function in a manner consistent with
> > the usage.  Some small roots (such as ttylinux) may not have 'ip' or
> > 'ifconfig'.
> > 
> > This patch replaces the use of these commands with usage of
> > netdevice.  The result is that lxc containers do not have to implement
> > those commands, and lxc in libvirt is only dependent on the netdevice
> > interface.
> 
> Requiring fewer tools in the container seems like a very good thing,
> especially when trying to create a tiny busybox-based container.

Yep, and we already directly do this kind of thing elsewhere in
libvirt without external tools, so there is precedent.

> > I've tested this patch locally against the ubuntu libvirt version enough
> > to verify its generally sane.  I attempted to build upstream today, but
> > failed with:
> >   /usr/bin/ld:
> >     ../src/.libs/libvirt_driver_qemu.a(libvirt_driver_qemu_la-qemu_domain.o):
> >    undefined reference to symbol 'xmlXPathRegisterNs@@LIBXML2_2.4.30
> > 
> > Thats probably a local issue only, but I wanted to get this patch up and
> > see what others thought of it.  This is ubuntu bug
> > https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/828211 .
> > 
> > diff --git a/src/lxc/veth.c b/src/lxc/veth.c
> > index 34cb804..c24df91 100644
> > --- a/src/lxc/veth.c
> > +++ b/src/lxc/veth.c
> > @@ -12,8 +12,11 @@
> > 
> >  #include <config.h>
> > 
> > +#include <linux/sockios.h>
> > +#include <net/if.h>
> >  #include <string.h>
> >  #include <stdio.h>
> > +#include <sys/ioctl.h>
> >  #include <sys/types.h>
> >  #include <sys/wait.h>
> > 
> > @@ -186,41 +189,49 @@ int vethDelete(const char *veth)
> >   * @veth: name of veth device
> >   * @upOrDown: 0 => down, 1 => up
> >   *
> > - * Enables a veth device using the ifconfig command.  A NULL inetAddress
> > - * will cause it to be left off the command line.
> > + * Enables a veth device using SIOCSIFFLAGS
> >   *
> > - * Returns 0 on success or -1 in case of error
> > + * Returns 0 on success, -1 on failure, with errno set
> >   */
> >  int vethInterfaceUpOrDown(const char* veth, int upOrDown)
> >  {
> > -    int rc;
> > -    const char *argv[] = {"ifconfig", veth, NULL, NULL};
> > -    int cmdResult = 0;
> > +    struct ifreq ifr;
> > +    int fd, ret;
> > 
> > -    if (0 == upOrDown)
> > -        argv[2] = "down";
> > -    else
> > -        argv[2] = "up";
> > +    if ((fd = socket(PF_PACKET, SOCK_DGRAM, 0)) == -1)
> > +        return(-1);
> > 
> > -    rc = virRun(argv, &cmdResult);
> > +    memset(&ifr, 0, sizeof(struct ifreq));
> > 
> > -    if (rc != 0 ||
> > -        (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
> > -        if (0 == upOrDown)
> > +    if (virStrcpyStatic(ifr.ifr_name, veth) == NULL) {
> > +        errno = EINVAL;
> > +        return -1;
> > +    }
> > +
> > +    if ((ret = ioctl(fd, SIOCGIFFLAGS, &ifr)) == 0) {
> > +        if (upOrDown)
> > +            ifr.ifr_flags |= IFF_UP;
> > +        else
> > +            ifr.ifr_flags &= ~(IFF_UP | IFF_RUNNING);
> > +
> > +        ret = ioctl(fd, SIOCSIFFLAGS, &ifr);
> > +    }
> > +
> > +    close(fd);
> > +    if (ret == -1)
> > +        if (upOrDown == 0)
> >              /*
> >               * Prevent overwriting an error log which may be set
> >               * where an actual failure occurs.
> >               */
> > -            VIR_DEBUG("Failed to disable '%s' (%d)",
> > -                      veth, WEXITSTATUS(cmdResult));
> > +            VIR_DEBUG("Failed to disable '%s'", veth);
> >          else
> >              vethError(VIR_ERR_INTERNAL_ERROR,
> > -                      _("Failed to enable '%s' (%d)"),
> > -                      veth, WEXITSTATUS(cmdResult));
> > -        rc = -1;
> > -    }
> > +                      _("Failed to enable '%s'"), veth);
> > +    else
> > +        ret = 0;
> > 
> > -    return rc;
> > +    return(ret);
> >  }
> > 
> >  /**
> > @@ -279,17 +290,29 @@ int setMacAddr(const char* iface, const char* macaddr)
> >   * @iface: name of device
> >   * @new: new name of @iface
> >   *
> > - * Changes the name of the given device with the
> > - * given new name using this command:
> > - *     ip link set @iface name @new
> > + * Changes the name of the given device.
> >   *
> > - * Returns 0 on success or -1 in case of error
> > + * Returns 0 on success, -1 on failure with errno set.
> >   */
> >  int setInterfaceName(const char* iface, const char* new)
> >  {
> > -    const char *argv[] = {
> > -        "ip", "link", "set", iface, "name", new, NULL
> > -    };
> > +    struct ifreq ifr;
> > +    int fd = socket(PF_PACKET, SOCK_DGRAM, 0);
> > 
> > -    return virRun(argv, NULL);
> > +    memset(&ifr, 0, sizeof(struct ifreq));
> > +
> > +    if (virStrcpyStatic(ifr.ifr_name, iface) == NULL) {
> > +        errno = EINVAL;
> > +        return -1;
> > +    }
> > +
> > +    if (virStrcpyStatic(ifr.ifr_newname, new) == NULL) {
> > +        errno = EINVAL;
> > +        return -1;
> > +    }
> > +
> > +    if (ioctl(fd, SIOCSIFNAME, &ifr))
> > +        return -1;
> > +
> > +    return 0;
> >  }
> > 


ACK


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