diff mbox series

[v2] tools/hotpug: only attempt to call 'ip route' if there is valid command

Message ID 1573145894-13305-1-git-send-email-paul@xen.org (mailing list archive)
State Superseded
Headers show
Series [v2] tools/hotpug: only attempt to call 'ip route' if there is valid command | expand

Commit Message

Paul Durrant Nov. 7, 2019, 4:58 p.m. UTC 
From: Paul Durrant <pdurrant@amazon.com>

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@amazon.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/hotplug/Linux/vif-route | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)
 mode change 100644 => 100755 tools/hotplug/Linux/vif-route

Comments

Wei Liu Nov. 7, 2019, 6:52 p.m. UTC | #1 
On Thu, Nov 07, 2019 at 04:58:14PM +0000, paul@xen.org wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> 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@amazon.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>

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@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
Wei Liu Nov. 8, 2019, 10:29 a.m. UTC | #2 
Cc xen-devel

Look forward to v3.

On Fri, Nov 08, 2019 at 09:17:37AM +0000, Paul Durrant wrote:
> On Thu, 7 Nov 2019 at 18:52, Wei Liu <wl@xen.org> wrote:
> >
> > On Thu, Nov 07, 2019 at 04:58:14PM +0000, paul@xen.org wrote:
> > > From: Paul Durrant <pdurrant@amazon.com>
> > >
> > > 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@amazon.com>
> > > ---
> > > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > > Cc: Wei Liu <wei.liu2@citrix.com>
> >
> > Looks like you need to update your address book. :-)
> >
> 
> Yeah, that is indeed weird... I can only think I ran get-maintainer
> with this rebased on an old branch.
> 
> > > ---
> > >  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.
> 
> I think that would be best.
> 
> >
> > >          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.
> 
> True.
> 
> >
> > > -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.
> 
> No, there's no need because of the "for addr in ${ip}" loop. If ${ip}
> is empty then it will do nothing (which is what made me believe the
> the ${ip} in the if statement was simply a typo).
> I'll just remove the if altogether.
> 
>   Paul
> 
> > 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@lists.xenproject.org
> > > https://lists.xenproject.org/mailman/listinfo/xen-devel
Anthony PERARD Nov. 8, 2019, 10:35 a.m. UTC | #3 
On Thu, Nov 07, 2019 at 06:52:00PM +0000, Wei Liu wrote:
> On Thu, Nov 07, 2019 at 04:58:14PM +0000, paul@xen.org wrote:
> > --- 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.

Arch Linux don't install Xen by default... so this script doesn't
exist... so it doesn't matter if ifconfig is available or not ...
It is fine to depends on 'ifconfig' when installing Xen.

Anyway, it might be a nice improvement to have one less dependency, but
it's not something that needs to be done right away.
diff mbox series

Patch

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