[PATCH] fix invalid frontend path for set_mtu

[PATCH] fix invalid frontend path for set_mtu

[James Dingwall]

Hi,

The set_mtu() function of xen-network-common.sh currently has this code:

        if [ ${type_if} = vif ]
        then
            local dev_=${dev#vif}
            local domid=${dev_%.*}
            local devid=${dev_#*.}

            local FRONTEND_PATH="/local/domain/$domid/device/vif/$devid"

            xenstore_write "$FRONTEND_PATH/mtu" ${mtu}
        fi

This works fine if the device has its default name but if the xen config
defines the vifname parameter the FRONTEND_PATH is incorrectly constructed.
Learn the frontend path by reading the appropriate value from the backend.

diff --git a/tools/hotplug/Linux/xen-network-common.sh b/tools/hotplug/Linux/xen-network-common.sh
index 02e2388600..cd98f0d486 100644
--- a/tools/hotplug/Linux/xen-network-common.sh
+++ b/tools/hotplug/Linux/xen-network-common.sh
@@ -163,11 +163,7 @@ set_mtu () {
 
         if [ ${type_if} = vif ]
         then
-            local dev_=${dev#vif}
-            local domid=${dev_%.*}
-            local devid=${dev_#*.}
-
-            local FRONTEND_PATH="/local/domain/$domid/device/vif/$devid"
+            local FRONTEND_PATH=$(xenstore_read "$XENBUS_PATH/frontend")
 
             xenstore_write "$FRONTEND_PATH/mtu" ${mtu}
         fi



Thanks,
James

Re: [PATCH] fix invalid frontend path for set_mtu

[Anthony PERARD]

Hi James,

On Tue, Mar 01, 2022 at 09:35:13AM +0000, James Dingwall wrote:
> The set_mtu() function of xen-network-common.sh currently has this code:
> 
>         if [ ${type_if} = vif ]
>         then
>             local dev_=${dev#vif}
>             local domid=${dev_%.*}
>             local devid=${dev_#*.}
> 
>             local FRONTEND_PATH="/local/domain/$domid/device/vif/$devid"
> 
>             xenstore_write "$FRONTEND_PATH/mtu" ${mtu}
>         fi
> 
> This works fine if the device has its default name but if the xen config
> defines the vifname parameter the FRONTEND_PATH is incorrectly constructed.
> Learn the frontend path by reading the appropriate value from the backend.

The patch looks fine, thanks. It is only missing a line
"Signed-off-by: your_name <your_email>" at the end of the description.
The meaning of this line is described in the file CONTRIBUTING, section
"Developer's Certificate of Origin".

Thanks,

-- 
Anthony PERARD

Re: [PATCH] fix invalid frontend path for set_mtu

[James Dingwall]

[-- Attachment #1: Type: text/plain, Size: 1371 bytes --]

Hi Anthony,

On Tue, Apr 12, 2022 at 02:03:17PM +0100, Anthony PERARD wrote:
> Hi James,
> 
> On Tue, Mar 01, 2022 at 09:35:13AM +0000, James Dingwall wrote:
> > The set_mtu() function of xen-network-common.sh currently has this code:
> > 
> >         if [ ${type_if} = vif ]
> >         then
> >             local dev_=${dev#vif}
> >             local domid=${dev_%.*}
> >             local devid=${dev_#*.}
> > 
> >             local FRONTEND_PATH="/local/domain/$domid/device/vif/$devid"
> > 
> >             xenstore_write "$FRONTEND_PATH/mtu" ${mtu}
> >         fi
> > 
> > This works fine if the device has its default name but if the xen config
> > defines the vifname parameter the FRONTEND_PATH is incorrectly constructed.
> > Learn the frontend path by reading the appropriate value from the backend.
> 
> The patch looks fine, thanks. It is only missing a line
> "Signed-off-by: your_name <your_email>" at the end of the description.
> The meaning of this line is described in the file CONTRIBUTING, section
> "Developer's Certificate of Origin".
> 

Thank you for your feedback.  I've updated the patch as suggested.  I've also
incorporated two other changes, one is a simple style change for consistency,
the other is to change a the test for a valid mtu from > 0 to >= 68.  I can
resubmit the original patch if either of these are a problem.

Thanks,
James

[-- Attachment #2: xen-network-common-frontend.patch --]
[-- Type: text/x-diff, Size: 2178 bytes --]

commit 03ad5670f8a7402e30b288a55d088e87685cd1a1
Author: James Dingwall <james@dingwall.me.uk>
Date:   Tue Apr 19 12:45:31 2022 +0100

    The set_mtu() function of xen-network-common.sh currently has this code:
    
            if [ ${type_if} = vif ]
            then
                local dev_=${dev#vif}
                local domid=${dev_%.*}
                local devid=${dev_#*.}
    
                local FRONTEND_PATH="/local/domain/$domid/device/vif/$devid"
    
                xenstore_write "$FRONTEND_PATH/mtu" ${mtu}
            fi
    
    This works fine if the device has its default name but if the xen config
    defines the vifname parameter the FRONTEND_PATH is incorrectly constructed.
    Learn the frontend path by reading the appropriate value from the backend.
    
    Also change use of `...` to $(...) for a consistent style in the script
    and adjust the valid check from `mtu > 0` to `mtu >= 68` per RFC 791.
    
    Signed-off-by: James Dingwall <james@dingwall.me.uk>

diff --git a/tools/hotplug/Linux/xen-network-common.sh b/tools/hotplug/Linux/xen-network-common.sh
index 42fa704e8d..9a382c39f4 100644
--- a/tools/hotplug/Linux/xen-network-common.sh
+++ b/tools/hotplug/Linux/xen-network-common.sh
@@ -171,24 +171,20 @@ set_mtu () {
     local mtu=$(xenstore_read_default "$XENBUS_PATH/mtu" "")
     if [ -z "$mtu" ]
     then
-        mtu="`ip link show dev ${bridge}| awk '/mtu/ { print $5 }'`"
+        mtu="$(ip link show dev ${bridge}| awk '/mtu/ { print $5 }')"
         if [ -n "$mtu" ]
         then
             log debug "$bridge MTU is $mtu"
         fi
     fi
-    if [ -n "$mtu" ] && [ "$mtu" -gt 0 ]
+    if [ -n "$mtu" ] && [ "$mtu" -ge 68 ]
     then
         log debug "setting $dev MTU to $mtu"
         ip link set dev ${dev} mtu ${mtu} || :
 
         if [ ${type_if} = vif ]
         then
-            local dev_=${dev#vif}
-            local domid=${dev_%.*}
-            local devid=${dev_#*.}
-
-            local FRONTEND_PATH="/local/domain/$domid/device/vif/$devid"
+            local FRONTEND_PATH="$(xenstore_read "$XENBUS_PATH/frontend")"
 
             xenstore_write "$FRONTEND_PATH/mtu" ${mtu}
         fi

Re: [PATCH] fix invalid frontend path for set_mtu

[Anthony PERARD]

On Tue, Apr 19, 2022 at 01:04:18PM +0100, James Dingwall wrote:
> Thank you for your feedback.  I've updated the patch as suggested.  I've also
> incorporated two other changes, one is a simple style change for consistency,
> the other is to change a the test for a valid mtu from > 0 to >= 68.  I can
> resubmit the original patch if either of these are a problem.

The style change is fine, but I'd rather have the change to the
mtu check in a different patch.

Otherwise, the patch looks better, thanks.

-- 
Anthony PERARD

Re: [PATCH] fix invalid frontend path for set_mtu

[James Dingwall]

[-- Attachment #1: Type: text/plain, Size: 661 bytes --]

On 2022-04-27 10:17, Anthony PERARD wrote:
> On Tue, Apr 19, 2022 at 01:04:18PM +0100, James Dingwall wrote:
>> Thank you for your feedback.  I've updated the patch as suggested.  
>> I've also
>> incorporated two other changes, one is a simple style change for 
>> consistency,
>> the other is to change a the test for a valid mtu from > 0 to >= 68.  
>> I can
>> resubmit the original patch if either of these are a problem.
> 
> The style change is fine, but I'd rather have the change to the
> mtu check in a different patch.
> 
> Otherwise, the patch looks better, thanks.

Here is a revised version of the patch that removes the mtu change.

Thanks,
James

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: set-mtu-frontend-path.diff --]
[-- Type: text/x-diff; name=set-mtu-frontend-path.diff, Size: 1928 bytes --]

commit f6ec92717522e74b4cc3aa4160b8ad6884e0b50c
Author: James Dingwall <james@dingwall.me.uk>
Date:   Tue Apr 19 12:45:31 2022 +0100

    The set_mtu() function of xen-network-common.sh currently has this code:
    
            if [ ${type_if} = vif ]
            then
                local dev_=${dev#vif}
                local domid=${dev_%.*}
                local devid=${dev_#*.}
    
                local FRONTEND_PATH="/local/domain/$domid/device/vif/$devid"
    
                xenstore_write "$FRONTEND_PATH/mtu" ${mtu}
            fi
    
    This works fine if the device has its default name but if the xen config
    defines the vifname parameter the FRONTEND_PATH is incorrectly constructed.
    Learn the frontend path by reading the appropriate value from the backend.
    
    Also change use of `...` to $(...) for a consistent style in the script.
    
    Signed-off-by: James Dingwall <james@dingwall.me.uk>

diff --git a/tools/hotplug/Linux/xen-network-common.sh b/tools/hotplug/Linux/xen-network-common.sh
index 42fa704e8d..7a63308a9e 100644
--- a/tools/hotplug/Linux/xen-network-common.sh
+++ b/tools/hotplug/Linux/xen-network-common.sh
@@ -171,7 +171,7 @@ set_mtu () {
     local mtu=$(xenstore_read_default "$XENBUS_PATH/mtu" "")
     if [ -z "$mtu" ]
     then
-        mtu="`ip link show dev ${bridge}| awk '/mtu/ { print $5 }'`"
+        mtu="$(ip link show dev ${bridge}| awk '/mtu/ { print $5 }')"
         if [ -n "$mtu" ]
         then
             log debug "$bridge MTU is $mtu"
@@ -184,11 +184,7 @@ set_mtu () {
 
         if [ ${type_if} = vif ]
         then
-            local dev_=${dev#vif}
-            local domid=${dev_%.*}
-            local devid=${dev_#*.}
-
-            local FRONTEND_PATH="/local/domain/$domid/device/vif/$devid"
+            local FRONTEND_PATH="$(xenstore_read "$XENBUS_PATH/frontend")"
 
             xenstore_write "$FRONTEND_PATH/mtu" ${mtu}
         fi

Re: [PATCH] fix invalid frontend path for set_mtu

[Anthony PERARD]

On Wed, Apr 27, 2022 at 02:20:53PM +0100, James Dingwall wrote:
> commit f6ec92717522e74b4cc3aa4160b8ad6884e0b50c
> Author: James Dingwall <james@dingwall.me.uk>
> Date:   Tue Apr 19 12:45:31 2022 +0100
> 
>     The set_mtu() function of xen-network-common.sh currently has this code:
>     
>             if [ ${type_if} = vif ]
>             then
>                 local dev_=${dev#vif}
>                 local domid=${dev_%.*}
>                 local devid=${dev_#*.}
>     
>                 local FRONTEND_PATH="/local/domain/$domid/device/vif/$devid"
>     
>                 xenstore_write "$FRONTEND_PATH/mtu" ${mtu}
>             fi
>     
>     This works fine if the device has its default name but if the xen config
>     defines the vifname parameter the FRONTEND_PATH is incorrectly constructed.
>     Learn the frontend path by reading the appropriate value from the backend.
>     
>     Also change use of `...` to $(...) for a consistent style in the script.
>     
>     Signed-off-by: James Dingwall <james@dingwall.me.uk>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks!


> diff --git a/tools/hotplug/Linux/xen-network-common.sh b/tools/hotplug/Linux/xen-network-common.sh
> index 42fa704e8d..7a63308a9e 100644
> --- a/tools/hotplug/Linux/xen-network-common.sh
> +++ b/tools/hotplug/Linux/xen-network-common.sh
> @@ -171,7 +171,7 @@ set_mtu () {
>      local mtu=$(xenstore_read_default "$XENBUS_PATH/mtu" "")
>      if [ -z "$mtu" ]
>      then
> -        mtu="`ip link show dev ${bridge}| awk '/mtu/ { print $5 }'`"
> +        mtu="$(ip link show dev ${bridge}| awk '/mtu/ { print $5 }')"
>          if [ -n "$mtu" ]
>          then
>              log debug "$bridge MTU is $mtu"
> @@ -184,11 +184,7 @@ set_mtu () {
>  
>          if [ ${type_if} = vif ]
>          then
> -            local dev_=${dev#vif}
> -            local domid=${dev_%.*}
> -            local devid=${dev_#*.}
> -
> -            local FRONTEND_PATH="/local/domain/$domid/device/vif/$devid"
> +            local FRONTEND_PATH="$(xenstore_read "$XENBUS_PATH/frontend")"
>  
>              xenstore_write "$FRONTEND_PATH/mtu" ${mtu}
>          fi


-- 
Anthony PERARD