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

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

[Patrick Scharrenberg]

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 <pittipatti@web.de>

---

Please apply to unstable.

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


diff -r e7b31cc0093c tools/hotplug/Linux/vif-bridge
--- a/tools/hotplug/Linux/vif-bridge    Mon Jan 31 17:46:55 2011 +0000
+++ b/tools/hotplug/Linux/vif-bridge    Sat Feb 05 14:11:52 2011 +0100
@@ -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

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

[Ian Jackson]

Patrick Scharrenberg writes ("[Xen-devel] [PATCH] 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.

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.

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

[Stefano Stabellini]

On Mon, 7 Feb 2011, Ian Jackson wrote:
> Patrick Scharrenberg writes ("[Xen-devel] [PATCH] 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.
> 
> 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 ?

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

Does this patch fix the problem for you?


diff -r 9e463cb15658 tools/hotplug/Linux/vif-common.sh
--- a/tools/hotplug/Linux/vif-common.sh	Mon Feb 07 17:02:46 2011 +0000
+++ b/tools/hotplug/Linux/vif-common.sh	Mon Feb 07 17:19:32 2011 +0000
@@ -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 ]

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

[Patrick Scharrenberg]

Hi,

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

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


diff -r e7b31cc0093c tools/hotplug/Linux/vif-route
--- a/tools/hotplug/Linux/vif-route     Mon Jan 31 17:46:55 2011 +0000
+++ b/tools/hotplug/Linux/vif-route     Mon Feb 07 22:44:23 2011 +0100
@@ -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

diff -r e7b31cc0093c tools/hotplug/Linux/vif-nat
--- a/tools/hotplug/Linux/vif-nat       Mon Jan 31 17:46:55 2011 +0000
+++ b/tools/hotplug/Linux/vif-nat       Mon Feb 07 22:44:42 2011 +0100
@@ -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

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

[Stefano Stabellini]

On Mon, 7 Feb 2011, Patrick Scharrenberg wrote:
> Hi,
> 
> > > 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.
> 
> > 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.
> 

Yes you are right.


Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

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

[Ian Jackson]

Patrick Scharrenberg writes ("Re: [Xen-devel] [PATCH] fix: domains do not get created when using vifname variable for bridged interfaces"):
> 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.

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

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

[Patrick Scharrenberg]

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

Signed-off-by: Patrick Scharrenberg <pittipatti@web.de>

---

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


diff -r e7b31cc0093c tools/hotplug/Linux/vif-route
--- a/tools/hotplug/Linux/vif-route     Mon Jan 31 17:46:55 2011 +0000
+++ b/tools/hotplug/Linux/vif-route     Wed Feb 09 20:20:51 2011 +0100
@@ -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
diff -r e7b31cc0093c tools/hotplug/Linux/vif-nat
--- a/tools/hotplug/Linux/vif-nat       Mon Jan 31 17:46:55 2011 +0000
+++ b/tools/hotplug/Linux/vif-nat       Wed Feb 09 20:23:38 2011 +0100
@@ -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

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

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH] fix: domains do not get created when using vifname variable for bridged interfaces"):
> 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"):
> Consistently use "dev" variable for virtual interfaces in vif-network scripts.
Thanks, I have applied these patches (as a single commit).

Ian.