Re: [Qemu-devel] [RFC PATCH v2 21/23] COLO nic: implement colo nic device interface configure()

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Yang Hongyang (yanghy@cn.fujitsu.com) wrote:
> implement colo nic device interface configure()
> add a script to configure nic devices:
> ${QEMU_SCRIPT_DIR}/network-colo
> 
> Script for configuring the network of Master & Slaver.

You might also like to think about making this possible via libvirt;
it does a lot of network setup for VMs already.

> Usage:
>  network-colo (master|slaver) (install|uninstall) vif pif [ifb1 ifb2]

Can you just describe what the four interfaces are; I'm assuming
'vif' is the virtual interface for the VM, and 'pif' is a physical interface
the packets from that VM are going to go to/come from.
Then I was expecting one more interface where you copy the outgoing packets
from the slave so that they go to the primary VM for comparison; why are there
two?

> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
> ---
>  include/net/net.h |   1 +
>  net/colo-nic.c    | 106 +++++++++++++++++++++++++++++
>  network-colo      | 194 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 301 insertions(+)
>  create mode 100755 network-colo
> 
> diff --git a/include/net/net.h b/include/net/net.h
> index 62050c5..9cc9b5c 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -88,6 +88,7 @@ struct NetClientState {
>      char colo_script[1024];
>      char colo_nicname[128];
>      char ifname[128];
> +    char ifb[2][128];

It's a shame that these are all magic 128 character lengths.

>      unsigned receive_disabled : 1;
>      NetClientDestructor *destructor;
>      unsigned int queue_index;
> diff --git a/net/colo-nic.c b/net/colo-nic.c
> index 7255a48..d661d8b 100644
> --- a/net/colo-nic.c
> +++ b/net/colo-nic.c
> @@ -10,6 +10,7 @@
>   */
>  #include "net/net.h"
>  #include "net/colo-nic.h"
> +#include "qemu/error-report.h"
>  
>  typedef struct nic_device {
>      NetClientState *nc;
> @@ -26,11 +27,116 @@ static bool nic_support_colo(NetClientState *nc)
>      return nc && nc->colo_script[0] && nc->colo_nicname[0];
>  }
>  
> +#define STDOUT_BUF_LEN 1024
> +static char stdout_buf[STDOUT_BUF_LEN];
> +
> +static int launch_colo_script(char *argv[])
> +{
> +    int pid, status;
> +    char *script = argv[0];
> +    int fds[2];
> +
> +    bzero(stdout_buf, sizeof(stdout_buf));
> +
> +    if (pipe(fds) < 0) {
> +        return -1;
> +    }
> +    /* try to launch network script */
> +    pid = fork();
> +    if (pid == 0) {
> +        close(fds[0]);
> +        dup2(fds[1], STDOUT_FILENO);
> +        execv(script, argv);
> +        _exit(1);
> +    } else if (pid > 0) {
> +        FILE *stream;
> +        int n;
> +        close(fds[1]);
> +        stream = fdopen(fds[0], "r");
> +        n = fread(stdout_buf, 1, STDOUT_BUF_LEN - 1, stream);
> +        stdout_buf[n] = '\0';
> +        close(fds[0]);
> +
> +        while (waitpid(pid, &status, 0) != pid) {
> +            /* loop */
> +        }
> +
> +        if (WIFEXITED(status) && WEXITSTATUS(status) == 0) {
> +            return 0;
> +        }
> +    }
> +    fprintf(stderr, "%s\n", stdout_buf);
> +    fprintf(stderr, "%s: could not launch network script\n", script);
> +    return -1;
> +}
> +
> +static void store_ifbname(NetClientState *nc)
> +{
> +    char *str_b = NULL, *str_e = NULL;
> +
> +    str_b = strstr(stdout_buf, "ifb0=");
> +    if (str_b) {
> +        str_e = strstr(str_b, "\n");
> +    }
> +    if (str_e) {
> +        snprintf(nc->ifb[0], str_e - str_b - 5 + 1, "%s", str_b + 5);
> +    }
> +
> +    str_b = str_e = NULL;
> +    str_b = strstr(stdout_buf, "ifb1=");
> +    if (str_b) {
> +        str_e = strstr(str_b, "\n");
> +    }
> +    if (str_e) {
> +        snprintf(nc->ifb[1], str_e - str_b - 5 + 1, "%s", str_b + 5);
> +    }
> +}

I don't think these protect against a bad string that's longer
than the 128 character ifb[] length.
I think it would also be good to return sometype of status from this
function to know if it worked.

> +static int nic_configure(NetClientState *nc, bool up, bool is_slave)
> +{
> +    char *argv[8];
> +    char **parg;
> +    int ret = -1, i;
> +    int argc = (!is_slave && !up) ? 7 : 5;
> +
> +    if (!nc) {
> +        error_report("Can not parse colo_script or colo_nicname");
> +        return ret;
> +    }
> +
> +    parg = argv;
> +    *parg++ = nc->colo_script;
> +    *parg++ = (char *)(is_slave ? "slaver" : "master");
> +    *parg++ = (char *)(up ? "install" : "uninstall");
> +    *parg++ = nc->ifname;
> +    *parg++ = nc->colo_nicname;
> +    if (!is_slave && !up) {
> +        *parg++ = nc->ifb[0];
> +        *parg++ = nc->ifb[1];
> +    }
> +    *parg = NULL;
> +
> +    for (i = 0; i < argc; i++) {
> +        if (!argv[i][0]) {
> +            error_report("Can not get colo_script argument");
> +            return ret;
> +        }
> +    }
> +
> +    ret = launch_colo_script(argv);
> +    if (!is_slave && up && ret == 0) {
> +        store_ifbname(nc);
> +    }
> +
> +    return ret;
> +}
> +
>  void colo_add_nic_devices(NetClientState *nc)
>  {
>      struct nic_device *nic = g_malloc0(sizeof(*nic));
>  
>      nic->support_colo = nic_support_colo;
> +    nic->configure = nic_configure;
>  
>      /*
>       * TODO
> diff --git a/network-colo b/network-colo
> new file mode 100755
> index 0000000..9112888
> --- /dev/null
> +++ b/network-colo
> @@ -0,0 +1,194 @@
> +#! /bin/bash
> +#============================================================================
> +# ${QEMU_SCRIPT_DIR}/network-colo
> +#
> +# Script for configuring the network of Master & Slaver.
> +#
> +# Usage:
> +# network-colo (master|slaver) (install|uninstall) vif pif [ifb1 ifb2]
> +#============================================================================
> +
> +sides=$1
> +op=$2
> +vif=$3
> +pif=$4
> +ifb1=$5
> +ifb2=$6
> +BR=br1
> +
> +qlen=40960
> +module="HA_compare"
> +device="HA_compare"
> +
> +# start_master ifbx
> +function start_master() {
> +
> +    # In colo mode, we don't use gso, gro...
> +    ip link set dev $vif qlen $qlen
> +
> +    # copy and foward input packets to $pif
> +    tc qdisc add dev $vif root handle 1: prio
> +    tc filter add dev $vif parent 1: protocol ip prio 10 u32 match u32 0 0 flowid 1:2 action mirred egress mirror dev $pif
> +    tc filter add dev $vif parent 1: protocol arp prio 11 u32 match u32 0 0 flowid 1:2 action mirred egress mirror dev $pif
> +
> +    # foward output packets to ifbx
> +    tc qdisc add dev $vif ingress
> +    tc filter add dev $vif parent ffff: protocol ip prio 10 u32 match u32 0 0 flowid 1:2 action mirred egress redirect dev $1
> +    tc filter add dev $vif parent ffff: protocol arp prio 11 u32 match u32 0 0 flowid 1:2 action mirred egress redirect dev $1
> +}
> +
> +function stop_master() {
> +    # don't copy and foward input packets to $pif
> +    tc filter del dev $vif parent 1: protocol ip prio 10 u32
> +    tc filter del dev $vif parent 1: protocol arp prio 11 u32
> +    tc qdisc del dev $vif root handle 1: prio
> +
> +    # don't foward output packets to ifbx
> +    tc filter del dev $vif parent ffff: protocol ip prio 10 u32
> +    tc filter del dev $vif parent ffff: protocol arp prio 11 u32
> +    tc qdisc del dev $vif ingress
> +}
> +
> +# load_module module parameter
> +function load_module()
> +{
> +    local module=$1
> +    shift
> +
> +    lsmod | grep -q "$module"

That's perhaps not selective enough, especially since 'HA_compare' would
match in the 'HA_compare_icmp' line.

> +    if [[ $? -eq 0 ]]; then
> +        # The module has been loaded
> +        return
> +    fi
> +
> +    modprobe $module "$@"
> +}
> +
> +function select_ifb()
> +{
> +    local -i index
> +
> +    for (( index = 0; index < 100; index++)); do
> +        state=$(ip link show dev ifb$index | sed -n -e 's/.*state \([a-zA-Z]*\) .*/\1/p')
> +        if [[ $state == "DOWN" ]]; then
> +            return $index
> +        fi
> +    done

The fixed sized loop seems weird; maybe something like:
  ip link show |awk -F' ' ' { if  ( $9 == "DOWN" ) { print $2; }; }'

or even :
  grep down /sys/class/net/ifb*/operstate
> +
> +    return 100
> +}
> +
> +

> +function install_master() {
> +    load_module sch_colo
> +    load_module HA_compare
> +    load_module HA_compare_icmp
> +
> +    if [[ ! -e "/dev/$device" ]]; then
> +        major=$(awk "\$2==\"$module\" {print \$1}" /proc/devices)
> +        mknod /dev/$device c $major 0
> +    fi

So I think module at that point is HA_compare ?

> +    load_module ifb numifbs=100
> +
> +    select_ifb
> +    index1=$?
> +    if [[ $index1 -eq 100 ]]; then
> +        echo "index1 $index1 overflow" >>/root/network-colo.log
> +        exit 1
> +    fi
> +    ip link set ifb$index1 up
> +    ip link set ifb$index1 qlen $qlen
> +
> +    select_ifb
> +    index2=$?
> +    if [[ $index2 -eq 100 ]]; then
> +        echo "index1 $index1 overflow" >>/root/network-colo.log
> +        exit 1
> +    fi
> +    ip link set ifb$index2 up
> +    ip link set ifb$index2 qlen $qlen
> +    colo_tc qdisc add dev ifb$index1 root handle 1: colo dev ifb$index2 master
> +    colo_tc qdisc add dev ifb$index2 root handle 1: colo dev ifb$index1 slaver
> +
> +    ifconfig $pif promisc
> +    ip link set $pif qlen $qlen
> +
> +    # forward packets from $pif to ifb$index2
> +    tc qdisc add dev $pif ingress
> +    tc filter add dev $pif parent ffff: protocol ip prio 10 u32 match u32 0 0 flowid 1:2 action mirred egress redirect dev ifb$index2
> +    tc filter add dev $pif parent ffff: protocol arp prio 11 u32 match u32 0 0 flowid 1:2 action mirred egress redirect dev ifb$index2
> +
> +    start_master ifb$index1
> +}
> +
> +function uninstall_master() {
> +    stop_master
> +
> +    # shutdown $ifb1
> +    tc qdisc del dev $ifb1 root handle 1: colo
> +    ip link set $ifb1 down
> +
> +    # don't forward packets from $pif to $ifb2
> +    tc filter del dev $pif parent ffff: protocol ip prio 10 u32
> +    tc qdisc del dev $pif ingress
> +
> +    # shutdown $ifb2
> +    tc qdisc del dev $ifb2 root handle 1: colo
> +    ip link set $ifb2 down
> +
> +    ifconfig $pif -promisc
> +}
> +
> +function install_slaver()
> +{
> +    ifconfig $pif promisc
> +    ip link set $pif qlen $qlen
> +
> +    # forward packets from $pif to $vif
> +    tc qdisc add dev $pif ingress
> +    tc filter add dev $pif parent ffff: protocol ip prio 10 u32 match u32 0 0 flowid 1:2 action mirred egress redirect dev $vif
> +    tc filter add dev $pif parent ffff: protocol arp prio 11 u32 match u32 0 0 flowid 1:2 action mirred egress redirect dev $vif
> +
> +    ip link set $vif qlen $qlen
> +    # forward packets from $vif to $pif
> +    tc qdisc add dev $vif ingress
> +    tc filter add dev $vif parent ffff: protocol ip prio 10 u32 match u32 0 0 flowid 1:2 action mirred egress redirect dev $pif
> +    tc filter add dev $vif parent ffff: protocol arp prio 11 u32 match u32 0 0 flowid 1:2 action mirred egress redirect dev $pif
> +
> +    brctl delif $BR $vif
> +}
> +
> +function uninstall_slaver()
> +{
> +    # don't forward packets from $pif to $vif
> +    tc filter del dev $pif parent ffff: protocol ip prio 10 u32
> +    tc filter del dev $pif parent ffff: protocol arp prio 11 u32
> +    tc qdisc del dev $pif ingress
> +
> +    # don't forward packets from $vif to $pif
> +    tc filter del dev $vif parent ffff: protocol ip prio 10 u32
> +    tc filter del dev $vif parent ffff: protocol arp prio 11 u32
> +    tc qdisc del dev $vif ingress
> +
> +    ifconfig $pif -promisc
> +
> +    brctl addif $BR $vif
> +}
> +
> +echo "$@" >/root/network-colo.log
> +if [[ $1 != "master" && $1 != "slaver" ]]; then
> +    echo "$1 != master/slaver" >>/root/network-colo.log
> +    exit 1
> +fi
> +
> +if [[ $2 != "install" && $2 != "uninstall" ]]; then
> +    echo "$2 != install/uninstall" >>/root/network-colo.log
> +    exit 1
> +fi
> +
> +${op}_$sides  1>>/root/network-colo.log 2>&1
> +if [[ $1 == "master"  && $2 == "install" ]]; then
> +    echo ifb0=ifb$index1
> +    echo ifb1=ifb$index2
> +fi
> -- 
> 1.9.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK