| Message ID | 20191024142103.962-1-paul@xen.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/hotpug: only attempt to call 'ip route' if there is valid command | expand |
On Thu, Oct 24, 2019 at 03:21:03PM +0100, 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. > > Signed-off-by: Paul Durrant <pdurrant@amazon.com> > --- > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Wei Liu <wei.liu2@citrix.com> > > This appears to have been broken forever. Or probably since c0efb8dc852805fd4d3c2691aca1f6c52f6b6ac7, 13 years ago. > --- > tools/hotplug/Linux/vif-route | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/hotplug/Linux/vif-route b/tools/hotplug/Linux/vif-route > index c149ffca73..98893d90b6 100644 > --- a/tools/hotplug/Linux/vif-route > +++ b/tools/hotplug/Linux/vif-route > @@ -35,7 +35,7 @@ case "${command}" in > ;; > esac > > -if [ "${ip}" ] ; then > +if [ "${ipcmd}" ] ; then The commit message and this new condition doesn't really explain what is happening. Does that means we only need to run `ip route` when command is 'online' or 'offline'? Is there something else to do 'add' and 'remove'? Could you add something in the commit message, and maybe a comment? Thanks,
On Mon, 28 Oct 2019 at 12:30, Anthony PERARD <anthony.perard@citrix.com> wrote: > > On Thu, Oct 24, 2019 at 03:21:03PM +0100, 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. > > > > Signed-off-by: Paul Durrant <pdurrant@amazon.com> > > --- > > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > > Cc: Wei Liu <wei.liu2@citrix.com> > > > > This appears to have been broken forever. > > Or probably since c0efb8dc852805fd4d3c2691aca1f6c52f6b6ac7, 13 years > ago. > Maybe. The heritage is not all that clear. > > --- > > tools/hotplug/Linux/vif-route | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/hotplug/Linux/vif-route b/tools/hotplug/Linux/vif-route > > index c149ffca73..98893d90b6 100644 > > --- a/tools/hotplug/Linux/vif-route > > +++ b/tools/hotplug/Linux/vif-route > > @@ -35,7 +35,7 @@ case "${command}" in > > ;; > > esac > > > > -if [ "${ip}" ] ; then > > +if [ "${ipcmd}" ] ; then > > The commit message and this new condition doesn't really explain what is > happening. It kind of does, but maybe not with the standard context. Looking at the whole file it does appear to be the intention that the clause is only invoked when ipcmd is set, and it was probably typoed. I can add some more explanation though. > Does that means we only need to run `ip route` when command is 'online' > or 'offline'? Is there something else to do 'add' and 'remove'? > Empirically online/offline appear to be used for PV interfaces and add/remove for emulated interfaces. I do actually have a newer version of the script now that adds a route metric and handles add/remove such that the emulated interface gets a higher priority... so it is used until the guest triggers an unplug. I need to do some more testing and I'll send that as v2 once I'm done. Cheers, Paul
diff --git a/tools/hotplug/Linux/vif-route b/tools/hotplug/Linux/vif-route index c149ffca73..98893d90b6 100644 --- a/tools/hotplug/Linux/vif-route +++ b/tools/hotplug/Linux/vif-route @@ -35,7 +35,7 @@ case "${command}" in ;; esac -if [ "${ip}" ] ; then +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