Re: [Xen-devel] [PATCH v2] tools/hotpug: only attempt to call 'ip route' if there is valid command

[Wei Liu <wl@xxxxxxx>]

On Thu, Nov 07, 2019 at 04:58:14PM +0000, paul@xxxxxxx wrote:
> From: Paul Durrant <pdurrant@xxxxxxxxxx>
> 
> The vif-route script should only call 'ip route' when 'ipcmd' has been
> set, otherwise it will fail due to an incorrect command string.
> 
> This patch also adds routes for 'tap' (i.e. emulated) devices as well as
> 'vif' (i.e. PV) devices by making use of the route metric. Emulated
> devices are used by HVM guests until they are unplugged, at which point the
> PV device becomes active. Thus 'tap' devices should get a higher priority
> (i.e. lower numbered) metric than 'vif' devices.
> 
> There is also one small whitespace fix.
> 
> NOTE: Empirically offline/online commands relate to 'vif' devices, and
>       add/remove commands relate to 'tap' devices. However, this patch
>       treats them equally and uses ${type_if} to distinguish.
> 
> Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx>
> ---
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>

Looks like you need to update your address book. :-)

> ---
>  tools/hotplug/Linux/vif-route | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
>  mode change 100644 => 100755 tools/hotplug/Linux/vif-route
> 
> diff --git a/tools/hotplug/Linux/vif-route b/tools/hotplug/Linux/vif-route
> old mode 100644
> new mode 100755
> index c149ffc..e71acae
> --- a/tools/hotplug/Linux/vif-route
> +++ b/tools/hotplug/Linux/vif-route
> @@ -22,12 +22,16 @@ dir=$(dirname "$0")
>  main_ip=$(dom0_ip)
>  
>  case "${command}" in
> +    add)
> +        ;&
>      online)
>          ifconfig ${dev} ${main_ip} netmask 255.255.255.255 up

Hmm... I think we may need to replace ifconfig with ip because now
distros (at least Debian and Arch) don't install ifconfig by default.

This can be done with a separate patch though.

>          echo 1 >/proc/sys/net/ipv4/conf/${dev}/proxy_arp
>          ipcmd='add'
>          cmdprefix=''
>          ;;
> +    remove)
> +        ;&
>      offline)
>          do_without_error ifdown ${dev}
>          ipcmd='del'
> @@ -35,11 +39,23 @@ case "${command}" in
>          ;;
>  esac
>  

The list of action here is exhaustive per the comment of this file,
which means ipcmd will always be set. The test for ipcmd below becomes
unnecessary.

> -if [ "${ip}" ] ; then
> +case "${type_if}" in
> +    tap)
> +     metric=1
> +     ;;
> +    vif)
> +     metric=2
> +     ;;
> +    *)
> +     fatal "Unrecognised interface type ${type_if}"
> +     ;;
> +esac
> +
> +if [ "${ipcmd}" ] ; then
>      # If we've been given a list of IP addresses, then add routes from dom0 
> to
>      # the guest using those addresses.

I _think_ testing ${ip} here is still the correct action. The comment
suggests there could be no ip given. If that assumption is not correct,
please fix the comment as well.

Wei.

>      for addr in ${ip} ; do
> -      ${cmdprefix} ip route ${ipcmd} ${addr} dev ${dev} src ${main_ip}
> +      ${cmdprefix} ip route ${ipcmd} ${addr} dev ${dev} src ${main_ip} 
> metric ${metric}
>      done
>  fi
>  
> @@ -50,5 +66,5 @@ call_hooks vif post
>  log debug "Successful vif-route ${command} for ${dev}."
>  if [ "${command}" = "online" ]
>  then
> -  success
> +    success
>  fi
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel