Re: [Xen-devel] [PATCH] hotplug/Linux: update to new ip command syntax.

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Thu, Nov 21, 2013 at 10:01 AM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Wed, 2013-11-20 at 18:50 +0000, George Dunlap wrote:
>> On Wed, Nov 20, 2013 at 2:53 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> 
>> wrote:
>> > On Wed, 2013-11-20 at 14:37 +0000, Ian Jackson wrote:
>> >> Ian Campbell writes ("Re: [Xen-devel] [PATCH] hotplug/Linux: update to 
>> >> new ip command syntax."):
>> >> > On Mon, 2013-08-19 at 18:01 +0100, Ian Campbell wrote:
>> >> > > On Mon, 2013-08-19 at 17:44 +0100, Ian Jackson wrote:
>> >> > > > Do we know whether this new syntax is backwards-compatible ?
>> >> > >
>> >> > > Good question. Squeeze's ip(8) doesn't mention dev for "ip link show" 
>> >> > > in
>> >> > > the synopsis by virtue of not defining DEVICE anywhere but it does 
>> >> > > show
>> >> > > the use of dev later on and the tool accepts it. Likewise for "ip link
>> >> > > set". Given this, as Mike says, makes it impossible to name a device
>> >> > > "dev" it seems likely to be a documentation bug.
>> >> > >
>> >> > > "ip addr flush" is correctly documented as needing the dev (by saying
>> >> > > "dev STRING" and not "DEVICE" , I didn't try without but I assume it
>> >> > > works since our scripts do that right now.
>> >> > >
>> >> > > I wasn't easily able to come up with an older machine.
>> >> >
>> >> > Ian, was that a satisfactory explanation?
>> >>
>> >> Yes, it was.  Sorry.
>> >>
>> >> Acked-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
>> >
>> > Thanks.
>> >
>> > Now, WRT 4.4...
>> >
>> > With reference to bugs.debian.org/cgi-bin/bugreport.cgi?bug=705659 the
>> > patch allows the creation of a vif named "dev". Which although I'm sure
>> > useful it is easily worked around. But on the other hand we are early in
>> > the freeze and this could easily be considered a bug fix.
>> >
>> > At this stage I'm leaning towards say lets take it.
>>
>> Should I say 'no' once just to let people know who's in charge? :-)
>>
>> I might be inclined to do so actually, just on principle; except that
>> the best way to get this actually tested will be the upcoming test
>> days, when (hopefully) it will be tested on all the major distros.  If
>> we put it off until 4.5, I doubt it will get much more testing than it
>> would right now.
>>
>> One thing I'm not happy with the patch though -- it doesn't have a
>> proper description of the problem and what the patch is actually doing
>> in the changeset itself.  Links to supplemental information are fine,
>> I think, but we have to assume that 1) people will be browsing the
>> source code offline, and 2) links will disappear; so the critical
>> information needs to be included.
>
> Ack.
>
> 8>----------------------
>
> From aed6743365d17c58561481ce1d74613f9a731746 Mon Sep 17 00:00:00 2001
> From: Mike <debian@xxxxxxxxxxxxxxxxxxxxx>
> Date: Fri, 16 Aug 2013 15:31:43 +0100
> Subject: [PATCH] hotplug/Linux: update to new ip command syntax.
>
> The current usage prevents naming a vif "dev". Although the current syntax is
> documented in Squeeze's ip(8) it appears that this was a documentation bug.
> Newer versions of the man page describe the new syntax used here and Squeeze's
> implementation accepts it as well.
>
> This is Debian bug #705659.
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=705659
>
> Mike provided the initial patch to tools/hotplug/Linux/xen-network-common.sh
> and Ian extended it to catch some cases in vif-* too.
>
> Signed-off-by: Ian Campbell <ijc@xxxxxxxxxxxxxx>
> Signed-off-by: Mike <debian@xxxxxxxxxxxxxxxxxxxxx>
> Acked-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>

Much better, thanks.

Release-acked-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>

> ---
> v2: Updated commit message.
> ---
>  tools/hotplug/Linux/vif-bridge            |    2 +-
>  tools/hotplug/Linux/vif-nat               |    2 +-
>  tools/hotplug/Linux/xen-network-common.sh |   14 +++++++-------
>  3 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/tools/hotplug/Linux/vif-bridge b/tools/hotplug/Linux/vif-bridge
> index 678262d..b7dcbd6 100644
> --- a/tools/hotplug/Linux/vif-bridge
> +++ b/tools/hotplug/Linux/vif-bridge
> @@ -72,7 +72,7 @@ else
>  fi
>
>  RET=0
> -ip link show $bridge 1>/dev/null 2>&1 || RET=1
> +ip link show dev $bridge 1>/dev/null 2>&1 || RET=1
>  if [ "$RET" -eq 1 ]
>  then
>      fatal "Could not find bridge device $bridge"
> diff --git a/tools/hotplug/Linux/vif-nat b/tools/hotplug/Linux/vif-nat
> index 8d29fb6..0b900d5 100644
> --- a/tools/hotplug/Linux/vif-nat
> +++ b/tools/hotplug/Linux/vif-nat
> @@ -170,7 +170,7 @@ case "$command" in
>            exit 0
>          fi
>
> -        do_or_die ip link set "${dev}" up arp on
> +        do_or_die ip link set dev "${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
> diff --git a/tools/hotplug/Linux/xen-network-common.sh 
> b/tools/hotplug/Linux/xen-network-common.sh
> index 50b8711..3c63c55 100644
> --- a/tools/hotplug/Linux/xen-network-common.sh
> +++ b/tools/hotplug/Linux/xen-network-common.sh
> @@ -85,18 +85,18 @@ _setup_bridge_port() {
>      local virtual="$2"
>
>      # take interface down ...
> -    ip link set ${dev} down
> +    ip link set dev ${dev} down
>
>      if [ $virtual -ne 0 ] ; then
>          # Initialise a dummy MAC address. We choose the numerically
>          # largest non-broadcast address to prevent the address getting
>          # stolen by an Ethernet bridge for STP purposes.
>          # (FE:FF:FF:FF:FF:FF)
> -        ip link set ${dev} address fe:ff:ff:ff:ff:ff || true
> +        ip link set dev ${dev} address fe:ff:ff:ff:ff:ff || true
>      fi
>
>      # ... and configure it
> -    ip addr flush ${dev}
> +    ip address flush dev ${dev}
>  }
>
>  setup_physical_bridge_port() {
> @@ -125,20 +125,20 @@ add_to_bridge () {
>
>      # Don't add $dev to $bridge if it's already on a bridge.
>      if [ -e "/sys/class/net/${bridge}/brif/${dev}" ]; then
> -       ip link set ${dev} up || true
> +       ip link set dev ${dev} up || true
>         return
>      fi
>      brctl addif ${bridge} ${dev}
> -    ip link set ${dev} up
> +    ip link set dev ${dev} up
>  }
>
>  # Usage: set_mtu bridge dev
>  set_mtu () {
>      local bridge=$1
>      local dev=$2
> -    mtu="`ip link show ${bridge}| awk '/mtu/ { print $5 }'`"
> +    mtu="`ip link show dev ${bridge}| awk '/mtu/ { print $5 }'`"
>      if [ -n "$mtu" ] && [ "$mtu" -gt 0 ]
>      then
> -            ip link set ${dev} mtu $mtu || :
> +            ip link set dev ${dev} mtu $mtu || :
>      fi
>  }
> --
> 1.7.10.4
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel