fix: domains do not get created when using vifname variable for bridged interfaces

PATCH In review Patrick Scharrenberg 3 participants 8 messages Feb 5, 2011 – Feb 11, 2011

Share & tools

Patrick Scharrenberg

author

2011-02-05 16:26 UTC #0 lore raw

PATCH 1/1 fix: domains do not get created when using vifname variable for bridged interfaces

In the two scripts vif-bridge and vif-route the variable containing the right interface-name, after an interface was renamed using "ifname", is $vif. Otherwise hotplug can't handle renamed interfaces and prevents xm from creating domains.

Signed-off-by Patrick Scharrenberg <[email protected]>

---

Please apply to unstable.

In 4.0.x the scripts are correct.
In unstable only vif-route contains the right variables

tools/hotplug/Linux/vif-bridge Sat Feb 05 14:11:52 2011 +0100 renamed from tools/hotplug/Linux/vif-bridge Mon Jan 31 17:46:55 2011 +0000 +7 -7

diff -r e7b31cc0093c tools/hotplug/Linux/vif-bridge
@@ -81,18 +81,18 @@
 
 case "$command" in
     online)
-        setup_virtual_bridge_port "$dev"
-        add_to_bridge "$bridge" "$dev"
+        setup_virtual_bridge_port "$vif"
+        add_to_bridge "$bridge" "$vif"
         ;;
 
     offline)
-        do_without_error brctl delif "$bridge" "$dev"
-        do_without_error ifconfig "$dev" down
+        do_without_error brctl delif "$bridge" "$vif"
+        do_without_error ifconfig "$vif" down
         ;;
 
     add)
-        setup_virtual_bridge_port "$dev"
-        add_to_bridge "$bridge" "$dev"
+        setup_virtual_bridge_port "$vif"
+        add_to_bridge "$bridge" "$vif"
         ;;
 esac

@@ -100,7 +100,7 @@
     handle_iptable
 fi
 
-log debug "Successful vif-bridge $command for $dev, bridge $bridge."
+log debug "Successful vif-bridge $command for $vif, bridge $bridge."
 if [ "$type_if" = vif -a "$command" = "online" ]
 then
   success

Ian Jackson

2011-02-07 16:11 UTC #1 lore raw

Patrick Scharrenberg writes ("[Xen-devel] [PATCH] fix: domains do not get created when using vifname variable for bridged interfaces"):

Quoted 4 lines

In the two scripts vif-bridge and vif-route the variable containing the right interface-name, after an interface was renamed using "ifname", is $vif. Otherwise hotplug can't handle renamed interfaces and prevents xm from creating domains.

This seems to be partly reverting 21922:0232bc7c9544 "tools/hotplug, Use udev rules instead of qemu script to setup the bridge."

I can't exactly follow the logic of 21922 but it seems to me that the bug is that in the case of an interface of type vif which is later renamed (by the code around line 75 of vif-common.sh in xen-unstable), the shell variable dev is not changed.

The change you are proposing would also an effect in the case of interfaces of type tap and I'm not convinced that in this case it would do the right thing.

Anthony, Stefano: as the authors of 21922 would you care to comment ?

Ian.

Stefano Stabellini

2011-02-07 17:23 UTC #2 lore raw

Quoted 14 lines · Ian Jackson

Patrick Scharrenberg writes ("[Xen-devel] [PATCH] fix: domains do not get created when using vifname variable for bridged interfaces"):
Quoted 4 lines
In the two scripts vif-bridge and vif-route the variable containing the right interface-name, after an interface was renamed using "ifname", is $vif. Otherwise hotplug can't handle renamed interfaces and prevents xm from creating domains.
This seems to be partly reverting 21922:0232bc7c9544 "tools/hotplug, Use udev rules instead of qemu script to setup the bridge."
I can't exactly follow the logic of 21922 but it seems to me that the bug is that in the case of an interface of type vif which is later renamed (by the code around line 75 of vif-common.sh in xen-unstable), the shell variable dev is not changed.

Quoted 5 lines

The change you are proposing would also an effect in the case of interfaces of type tap and I'm not convinced that in this case it would do the right thing.
Anthony, Stefano: as the authors of 21922 would you care to comment ?

I don't think that patch is the right way to fix the issue.

Does this patch fix the problem for you?

tools/hotplug/Linux/vif-common.sh +5 -5

diff -r 9e463cb15658 tools/hotplug/Linux/vif-common.sh
@@ -69,16 +69,16 @@ if [ "$type_if" = vif ]; then
 if [ "$type_if" = vif ]; then
     # Check presence of compulsory args.
     XENBUS_PATH="${XENBUS_PATH:?}"
-    vif="${vif:?}"
+    dev="${dev:?}"
 
     vifname=$(xenstore_read_default "$XENBUS_PATH/vifname" "")
     if [ "$vifname" ]
     then
         if [ "$command" == "online" ] && ! ip link show "$vifname" >&/dev/null
         then
-            do_or_die ip link set "$vif" name "$vifname"
+            do_or_die ip link set "$dev" name "$vifname"
         fi
-        vif="$vifname"
+        dev="$vifname"
     fi
 elif [ "$type_if" = tap ]; then
     # Check presence of compulsory args.

@@ -105,9 +105,9 @@ frob_iptable()
     local c="-D"
   fi
 
-  iptables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-in "$vif" \
+  iptables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-in "$dev" \
     "$@" -j ACCEPT 2>/dev/null &&
-  iptables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-out "$vif" \
+  iptables "$c" FORWARD -m physdev --physdev-is-bridged --physdev-out "$dev" \
     -j ACCEPT 2>/dev/null
 
   if [ "$command" == "online" -a $? -ne 0 ]

Patrick Scharrenberg

author

2011-02-07 21:59 UTC #3 lore raw

Hi,

Quoted 4 lines

Quoted 2 lines
This seems to be partly reverting 21922:0232bc7c9544 "tools/hotplug, Use udev rules instead of qemu script to setup the bridge."
I don't think that patch is the right way to fix the issue.

Ok, I see. Progress was the other way round.

Quoted 1 line

Does this patch fix the problem for you?

Yes it does fix my problem.
So, if we consistently use "dev" instead of "vif" the we'll run into troubles in vif-route and vif-nat scripts sooner ot later if we don't fix these as well, don't we?

I changed "vif" to "dev" in both, though I haven't testet the nat one for lack of testing environment.

tools/hotplug/Linux/vif-route Mon Feb 07 22:44:23 2011 +0100 renamed from tools/hotplug/Linux/vif-route Mon Jan 31 17:46:55 2011 +0000 +6 -6

diff -r e7b31cc0093c tools/hotplug/Linux/vif-route
@@ -12,7 +12,7 @@
 # vif-route (add|remove|online|offline)
 #
 # Environment vars:
-# vif         vif interface name (required).
+# dev         vif interface name (required).
 # XENBUS_PATH path to this device's details in the XenStore (required).
 #
 # Read from the store:

@@ -21,19 +21,19 @@
 #============================================================================
 
 dir=$(dirname "$0")
-. "$dir/vif-common.sh"
+. "${dir}/vif-common.sh"
 
 main_ip=$(dom0_ip)
 
-case "$command" in
+case "${command}" in
     online)
-        ifconfig ${vif} ${main_ip} netmask 255.255.255.255 up
-        echo 1 >/proc/sys/net/ipv4/conf/${vif}/proxy_arp
+        ifconfig ${dev} ${main_ip} netmask 255.255.255.255 up
+        echo 1 >/proc/sys/net/ipv4/conf/${dev}/proxy_arp
         ipcmd='add'
         cmdprefix=''
         ;;
     offline)
-        do_without_error ifdown ${vif}
+        do_without_error ifdown ${dev}
         ipcmd='del'
         cmdprefix='do_without_error'
         ;;

@@ -43,14 +43,14 @@
     # If we've been given a list of IP addresses, then add routes from dom0

     # the guest using those addresses.
     for addr in ${ip} ; do
-      ${cmdprefix} ip route ${ipcmd} ${addr} dev ${vif} src ${main_ip}
+      ${cmdprefix} ip route ${ipcmd} ${addr} dev ${dev} src ${main_ip}
     done 
 fi

handle_iptable

-log debug "Successful vif-route $command for $vif." -if [ "$command" = "online" ]
+log debug "Successful vif-route ${command} for ${dev}." +if [ "${command}" = "online" ]
then
success
fi

tools/hotplug/Linux/vif-nat Mon Feb 07 22:44:42 2011 +0100 renamed from tools/hotplug/Linux/vif-nat Mon Jan 31 17:46:55 2011 +0000 +13 ~~-13~~

diff -r e7b31cc0093c tools/hotplug/Linux/vif-nat
@@ -12,7 +12,7 @@
 # vif-nat (add|remove|online|offline)
 #
 # Environment vars:
-# vif         vif interface name (required).
+# dev         vif interface name (required).
 # XENBUS_PATH path to this device's details in the XenStore (required).
 #
 # Parameters:

@@ -98,7 +98,7 @@
 dhcparg_remove_entry()
 { 
   local tmpfile=$(mktemp)
-  sed -e "s/$vif //" "$dhcpd_arg_file" >"$tmpfile"
+  sed -e "s/${dev} //" "$dhcpd_arg_file" >"$tmpfile"
   if diff "$tmpfile" "$dhcpd_arg_file" >/dev/null
   then
     rm "$tmpfile"

@@ -112,11 +112,11 @@
   dhcparg_remove_entry
   local tmpfile=$(mktemp)
   # handle Red Hat, SUSE, and Debian styles, with or without quotes
-  sed -e 's/^DHCPDARGS="*\([^"]*\)"*/DHCPDARGS="\1'"$vif "'"/' \
+  sed -e 's/^DHCPDARGS="*\([^"]*\)"*/DHCPDARGS="\1'"${dev} "'"/' \
      "$dhcpd_arg_file" >"$tmpfile" && mv "$tmpfile" "$dhcpd_arg_file"
-  sed -e 's/^DHCPD_INTERFACE="*\([^"]*\)"*/DHCPD_INTERFACE="\1'"$vif "'"/' \
+  sed -e 's/^DHCPD_INTERFACE="*\([^"]*\)"*/DHCPD_INTERFACE="\1'"${dev} "'"/' 
\
      "$dhcpd_arg_file" >"$tmpfile" && mv "$tmpfile" "$dhcpd_arg_file"
-  sed -e 's/^INTERFACES="*\([^"]*\)"*/INTERFACES="\1'"$vif "'"/' \
+  sed -e 's/^INTERFACES="*\([^"]*\)"*/INTERFACES="\1'"${dev} "'"/' \
      "$dhcpd_arg_file" >"$tmpfile" && mv "$tmpfile" "$dhcpd_arg_file"
   rm -f "$tmpfile"
 }

@@ -164,28 +164,28 @@
 
 case "$command" in
     online)
-        if ip route | grep -q "dev $vif"
+        if ip route | grep -q "dev ${dev}"
         then
-          log debug "$vif already up"
+          log debug "${dev} already up"
           exit 0
         fi
 
-        do_or_die ip link set "$vif" up arp on
-        do_or_die ip addr add "$router_ip" dev "$vif"
-        do_or_die ip route add "$vif_ip" dev "$vif" src "$router_ip"
-        echo 1 >/proc/sys/net/ipv4/conf/${vif}/proxy_arp
+        do_or_die ip link set "${dev}" up arp on
+        do_or_die ip addr add "$router_ip" dev "${dev}"
+        do_or_die ip route add "$vif_ip" dev "${dev}" src "$router_ip"
+        echo 1 >/proc/sys/net/ipv4/conf/${dev}/proxy_arp
         [ "$dhcp" != 'no' ] && dhcp_up
         ;;
     offline)
         [ "$dhcp" != 'no' ] && dhcp_down
-        do_without_error ifconfig "$vif" down
+        do_without_error ifconfig "${dev}" down
         ;;
 esac
 
 
 handle_iptable
 
-log debug "Successful vif-nat $command for $vif."
+log debug "Successful vif-nat $command for ${dev}."
 if [ "$command" = "online" ]
 then
   success

Stefano Stabellini

Acked-by

2011-02-08 11:44 UTC #4 lore raw

Quoted 11 lines · Patrick Scharrenberg

Hi,
Quoted 4 lines
Quoted 2 lines
This seems to be partly reverting 21922:0232bc7c9544 "tools/hotplug, Use udev rules instead of qemu script to setup the bridge."
I don't think that patch is the right way to fix the issue.
Ok, I see. Progress was the other way round.
Quoted 1 line
Does this patch fix the problem for you?

Quoted 8 lines

Yes it does fix my problem.
So, if we consistently use "dev" instead of "vif" the we'll run into troubles in vif-route and vif-nat scripts sooner ot later if we don't fix these as well, don't we?
I changed "vif" to "dev" in both, though I haven't testet the nat one for lack of testing environment.

Yes you are right.

Acked-by Stefano Stabellini <[email protected]>

Ian Jackson

2011-02-08 16:34 UTC #5 lore raw

In reply to Patrick Scharrenberg

Patrick Scharrenberg writes ("Re: [Xen-devel] [PATCH] fix: domains do not get created when using vifname variable for bridged interfaces"):

Quoted 2 lines

I changed "vif" to "dev" in both, though I haven't testet the nat one for lack of testing environment.

Something at your end has word-wrapped this patch. Can you send it again, uncorrupted, please ?

Also a Signed-off-by is needed to confirm the legal status of your contribution (see below).

Thanks,
Ian.

Quoted 1 line

From Documentation/SubmittingPatches in the Linux kernel tree:

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I have the right to submit it under the open source license indicated in the file; or

(b) The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file; or

(c) The contribution was provided directly to me by some other person who certified (a), (b) or (c) and I have not modified it.

(d) I understand and agree that this project and the contribution are public and that a record of the contribution (including all personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved.

Patrick Scharrenberg

author

2011-02-09 19:29 UTC #6 lore raw

Consistently use "dev" variable for virtual interfaces in vif-network scripts.

Signed-off-by Patrick Scharrenberg <[email protected]>

---

resend this patch due to line wrapping problems and missing signed-off line

tools/hotplug/Linux/vif-route Wed Feb 09 20:20:51 2011 +0100 renamed from tools/hotplug/Linux/vif-route Mon Jan 31 17:46:55 2011 +0000 +9 -9

diff -r e7b31cc0093c tools/hotplug/Linux/vif-route
@@ -12,7 +12,7 @@
 # vif-route (add|remove|online|offline)
 #
 # Environment vars:
-# vif         vif interface name (required).
+# dev         vif interface name (required).
 # XENBUS_PATH path to this device's details in the XenStore (required).
 #
 # Read from the store:

@@ -21,19 +21,19 @@
 #============================================================================
 
 dir=$(dirname "$0")
-. "$dir/vif-common.sh"
+. "${dir}/vif-common.sh"
 
 main_ip=$(dom0_ip)
 
-case "$command" in
+case "${command}" in
     online)
-        ifconfig ${vif} ${main_ip} netmask 255.255.255.255 up
-        echo 1 >/proc/sys/net/ipv4/conf/${vif}/proxy_arp
+        ifconfig ${dev} ${main_ip} netmask 255.255.255.255 up
+        echo 1 >/proc/sys/net/ipv4/conf/${dev}/proxy_arp
         ipcmd='add'
         cmdprefix=''
         ;;
     offline)
-        do_without_error ifdown ${vif}
+        do_without_error ifdown ${dev}
         ipcmd='del'
         cmdprefix='do_without_error'
         ;;

@@ -43,14 +43,14 @@
     # If we've been given a list of IP addresses, then add routes from dom0 to
     # the guest using those addresses.
     for addr in ${ip} ; do
-      ${cmdprefix} ip route ${ipcmd} ${addr} dev ${vif} src ${main_ip}
+      ${cmdprefix} ip route ${ipcmd} ${addr} dev ${dev} src ${main_ip}
     done
 fi
 
 handle_iptable
 
-log debug "Successful vif-route $command for $vif."
-if [ "$command" = "online" ]
+log debug "Successful vif-route ${command} for ${dev}."
+if [ "${command}" = "online" ]
 then
   success
 fi

tools/hotplug/Linux/vif-nat Wed Feb 09 20:23:38 2011 +0100 renamed from tools/hotplug/Linux/vif-nat Mon Jan 31 17:46:55 2011 +0000 +13 ~~-13~~

diff -r e7b31cc0093c tools/hotplug/Linux/vif-nat
@@ -12,7 +12,7 @@
 # vif-nat (add|remove|online|offline)
 #
 # Environment vars:
-# vif         vif interface name (required).
+# dev         vif interface name (required).
 # XENBUS_PATH path to this device's details in the XenStore (required).
 #
 # Parameters:

@@ -98,7 +98,7 @@
 dhcparg_remove_entry()
 { 
   local tmpfile=$(mktemp)
-  sed -e "s/$vif //" "$dhcpd_arg_file" >"$tmpfile"
+  sed -e "s/${dev} //" "$dhcpd_arg_file" >"$tmpfile"
   if diff "$tmpfile" "$dhcpd_arg_file" >/dev/null
   then
     rm "$tmpfile"

@@ -112,11 +112,11 @@
   dhcparg_remove_entry
   local tmpfile=$(mktemp)
   # handle Red Hat, SUSE, and Debian styles, with or without quotes
-  sed -e 's/^DHCPDARGS="*\([^"]*\)"*/DHCPDARGS="\1'"$vif "'"/' \
+  sed -e 's/^DHCPDARGS="*\([^"]*\)"*/DHCPDARGS="\1'"${dev} "'"/' \
      "$dhcpd_arg_file" >"$tmpfile" && mv "$tmpfile" "$dhcpd_arg_file"
-  sed -e 's/^DHCPD_INTERFACE="*\([^"]*\)"*/DHCPD_INTERFACE="\1'"$vif "'"/' \
+  sed -e 's/^DHCPD_INTERFACE="*\([^"]*\)"*/DHCPD_INTERFACE="\1'"${dev} "'"/' \
      "$dhcpd_arg_file" >"$tmpfile" && mv "$tmpfile" "$dhcpd_arg_file"
-  sed -e 's/^INTERFACES="*\([^"]*\)"*/INTERFACES="\1'"$vif "'"/' \
+  sed -e 's/^INTERFACES="*\([^"]*\)"*/INTERFACES="\1'"${dev} "'"/' \
      "$dhcpd_arg_file" >"$tmpfile" && mv "$tmpfile" "$dhcpd_arg_file"
   rm -f "$tmpfile"
 }

@@ -164,28 +164,28 @@
 
 case "$command" in
     online)
-        if ip route | grep -q "dev $vif"
+        if ip route | grep -q "dev ${dev}"
         then
-          log debug "$vif already up"
+          log debug "${dev} already up"
           exit 0
         fi
 
-        do_or_die ip link set "$vif" up arp on
-        do_or_die ip addr add "$router_ip" dev "$vif"
-        do_or_die ip route add "$vif_ip" dev "$vif" src "$router_ip"
-        echo 1 >/proc/sys/net/ipv4/conf/${vif}/proxy_arp
+        do_or_die ip link set "${dev}" up arp on
+        do_or_die ip addr add "$router_ip" dev "${dev}"
+        do_or_die ip route add "$vif_ip" dev "${dev}" src "$router_ip"
+        echo 1 >/proc/sys/net/ipv4/conf/${dev}/proxy_arp
         [ "$dhcp" != 'no' ] && dhcp_up
         ;;
     offline)
         [ "$dhcp" != 'no' ] && dhcp_down
-        do_without_error ifconfig "$vif" down
+        do_without_error ifconfig "${dev}" down
         ;;
 esac
 
 
 handle_iptable
 
-log debug "Successful vif-nat $command for $vif."
+log debug "Successful vif-nat $command for ${dev}."
 if [ "$command" = "online" ]
 then
   success

Ian Jackson

2011-02-11 18:22 UTC #7 lore raw

Re: [PATCH] fix: domains do not get created when using vifname variable for bridged interfaces [and 1 more messages]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH] fix: domains do not get created when using vifname variable for bridged interfaces"):

Quoted 1 line

Does this patch fix the problem for you?

Patrick Scharrenberg writes ("Re: [Xen-devel] [PATCH] fix: domains do not get created when using vifname variable for bridged interfaces"):

Quoted 1 line

Consistently use "dev" variable for virtual interfaces in vif-network scripts.

Thanks, I have applied these patches (as a single commit).

Ian.