Xen devel - Sep 2013 - [PATCH net-next 2/2] xen-netback: handle frontends that fail to transition through Closing

Some old Windows frontends fail to transition through the xenbus Closing
state and move directly from Connected to Closed. Handle this case properly.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: David Vrabel <david.vrabel@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/net/xen-netback/xenbus.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index a53782e..bcaa25b 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -265,6 +265,8 @@ static void frontend_changed(struct xenbus_device *dev,
 		break;
 
 	case XenbusStateClosed:
+		if (dev->state == XenbusStateConnected)
+			disconnect_backend(dev);
 		xenbus_switch_state(dev, XenbusStateClosed);
 		if (xenbus_dev_is_online(dev))
 			break;
-- 
1.7.10.4

On Fri, Sep 20, 2013 at 01:56:31PM +0100, Paul Durrant
wrote:
> Some old Windows frontends fail to transition through the xenbus Closing
> state and move directly from Connected to Closed. Handle this case
properly.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: David Vrabel <david.vrabel@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> ---
>  drivers/net/xen-netback/xenbus.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/xen-netback/xenbus.c
b/drivers/net/xen-netback/xenbus.c
> index a53782e..bcaa25b 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -265,6 +265,8 @@ static void frontend_changed(struct xenbus_device *dev,
>  		break;
>  
>  	case XenbusStateClosed:
> +		if (dev->state == XenbusStateConnected)
> +			disconnect_backend(dev);
Could you please add a comment above this change stating that this is a
workaround for some old frontend that we cannot fix / upgrade.

We would still like to later frontend goes through the normal connected
-> closing -> closed path.

Wei.
>  		xenbus_switch_state(dev, XenbusStateClosed);
>  		if (xenbus_dev_is_online(dev))
>  			break;
> -- 
> 1.7.10.4

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@citrix.com]
> Sent: 20 September 2013 14:35
> To: Paul Durrant
> Cc: netdev@vger.kernel.org; xen-devel@lists.xen.org; David Vrabel; Wei Liu;
> Ian Campbell
> Subject: Re: [PATCH net-next 2/2] xen-netback: handle frontends that fail
to
> transition through Closing
> 
> On Fri, Sep 20, 2013 at 01:56:31PM +0100, Paul Durrant wrote:
> > Some old Windows frontends fail to transition through the xenbus
Closing
> > state and move directly from Connected to Closed. Handle this case
> properly.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Cc: David Vrabel <david.vrabel@citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  drivers/net/xen-netback/xenbus.c |    2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-
> netback/xenbus.c
> > index a53782e..bcaa25b 100644
> > --- a/drivers/net/xen-netback/xenbus.c
> > +++ b/drivers/net/xen-netback/xenbus.c
> > @@ -265,6 +265,8 @@ static void frontend_changed(struct xenbus_device
> *dev,
> >  		break;
> >
> >  	case XenbusStateClosed:
> > +		if (dev->state == XenbusStateConnected)
> > +			disconnect_backend(dev);
> 
> Could you please add a comment above this change stating that this is a
> workaround for some old frontend that we cannot fix / upgrade.
> 
> We would still like to later frontend goes through the normal connected
> -> closing -> closed path.
> 
Sure. I''ll re-post the series with that change.

  Paul

On 20/09/13 14:34, Wei Liu wrote:
> On Fri, Sep 20, 2013 at 01:56:31PM +0100, Paul Durrant wrote:
>> Some old Windows frontends fail to transition through the xenbus
Closing
>> state and move directly from Connected to Closed. Handle this case
properly.
>>
>> --- a/drivers/net/xen-netback/xenbus.c
>> +++ b/drivers/net/xen-netback/xenbus.c
>> @@ -265,6 +265,8 @@ static void frontend_changed(struct xenbus_device
*dev,
>>  		break;
>>  
>>  	case XenbusStateClosed:
>> +		if (dev->state == XenbusStateConnected)
>> +			disconnect_backend(dev);
> 
> Could you please add a comment above this change stating that this is a
> workaround for some old frontend that we cannot fix / upgrade.
Handling frontend CONNECTED -> CLOSED is a sensible thing for a backend
to do regardless of whether there are old frontends that do this or not.
> We would still like to later frontend goes through the normal connected
> -> closing -> closed path.
This should be documented as a full description of the two state
machines in public/io/netif.h in Xen.  Not scattered about in comments
in a particular backend implementation.

David

> -----Original Message-----
> From: David Vrabel
> Sent: 20 September 2013 14:39
> To: Wei Liu
> Cc: Paul Durrant; netdev@vger.kernel.org; xen-devel@lists.xen.org; Ian
> Campbell
> Subject: Re: [PATCH net-next 2/2] xen-netback: handle frontends that fail
to
> transition through Closing
> 
> On 20/09/13 14:34, Wei Liu wrote:
> > On Fri, Sep 20, 2013 at 01:56:31PM +0100, Paul Durrant wrote:
> >> Some old Windows frontends fail to transition through the xenbus
Closing
> >> state and move directly from Connected to Closed. Handle this case
> properly.
> >>
> >> --- a/drivers/net/xen-netback/xenbus.c
> >> +++ b/drivers/net/xen-netback/xenbus.c
> >> @@ -265,6 +265,8 @@ static void frontend_changed(struct
> xenbus_device *dev,
> >>  		break;
> >>
> >>  	case XenbusStateClosed:
> >> +		if (dev->state == XenbusStateConnected)
> >> +			disconnect_backend(dev);
> >
> > Could you please add a comment above this change stating that this is
a
> > workaround for some old frontend that we cannot fix / upgrade.
> 
> Handling frontend CONNECTED -> CLOSED is a sensible thing for a backend
> to do regardless of whether there are old frontends that do this or not.
> 
Agreed, but probably still worth the comment in case someone gets the wrong
idea.

  Paul
> > We would still like to later frontend goes through the normal
connected
> > -> closing -> closed path.
> 
> This should be documented as a full description of the two state
> machines in public/io/netif.h in Xen.  Not scattered about in comments
> in a particular backend implementation.
> 
> David

On Fri, Sep 20, 2013 at 02:38:46PM +0100, David Vrabel
wrote:
> On 20/09/13 14:34, Wei Liu wrote:
> > On Fri, Sep 20, 2013 at 01:56:31PM +0100, Paul Durrant wrote:
> >> Some old Windows frontends fail to transition through the xenbus
Closing
> >> state and move directly from Connected to Closed. Handle this case
properly.
> >>
> >> --- a/drivers/net/xen-netback/xenbus.c
> >> +++ b/drivers/net/xen-netback/xenbus.c
> >> @@ -265,6 +265,8 @@ static void frontend_changed(struct
xenbus_device *dev,
> >>  		break;
> >>  
> >>  	case XenbusStateClosed:
> >> +		if (dev->state == XenbusStateConnected)
> >> +			disconnect_backend(dev);
> > 
> > Could you please add a comment above this change stating that this is
a
> > workaround for some old frontend that we cannot fix / upgrade.
> 
> Handling frontend CONNECTED -> CLOSED is a sensible thing for a backend
> to do regardless of whether there are old frontends that do this or not.
> 
I agree handling connected -> closed is sensible here based on the fact
that old frontends could do such state change. However If the state
machine was documented well enough then I think connected -> closed
would not be considered sensible.

This code snippet without comment will cause confusion / encourage wrong
usage of state machine if someone comes here for reference.
> > We would still like to later frontend goes through the normal
connected
> > -> closing -> closed path.
> 
> This should be documented as a full description of the two state
> machines in public/io/netif.h in Xen.  Not scattered about in comments
Sure.
> in a particular backend implementation.
> 
The comment in implementation is still worthwhile in case someone comes
here for reference and gets confused.

Wei.
> David