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

[Wei Liu <wl@xxxxxxx>]

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@xxxxxxx> wrote:
> >
> > 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. :-)
> >
> 
> 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@xxxxxxxxxxxxxxxxxxxx
> > > https://lists.xenproject.org/mailman/listinfo/xen-devel

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