1. Patchew
  2. Xen
  3. View series

[PATCH] fix invalid frontend path for set_mtu

James Dingwall posted 1 patch 2 years, 2 months ago
Failed in applying to current master (apply log)
Test gitlab-ci failed
[PATCH] fix invalid frontend path for set_mtu
Posted by James Dingwall 2 years, 2 months ago 
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
Posted by Anthony PERARD 2 years ago 
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
Posted by James Dingwall 2 years ago 
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
Re: [PATCH] fix invalid frontend path for set_mtu
Posted by Anthony PERARD 2 years ago 
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
Posted by James Dingwall 2 years ago 
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
Re: [PATCH] fix invalid frontend path for set_mtu
Posted by Anthony PERARD 2 years ago 
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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.