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

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

Posted by paul@xen.org 4 years, 5 months ago

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


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

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

Posted by Anthony PERARD 4 years, 5 months ago

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,


-- 
Anthony PERARD

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

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

Posted by Paul Durrant 4 years, 5 months ago

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

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