* [RFC PATCH] xen-netback: fix error handling on netback_probe()
@ 2016-09-13 12:11 Filipe Manco
2016-09-14 10:10 ` Wei Liu
0 siblings, 1 reply; 4+ messages in thread
From: Filipe Manco @ 2016-09-13 12:11 UTC (permalink / raw)
To: netdev; +Cc: wei.liu2, filipe.manco
In case of error during netback_probe() (e.g. an entry missing on the
xenstore) netback_remove() is called on the new device, which will set
the device backend state to XenbusStateClosed by calling
set_backend_state(). However, the backend state wasn't initialized by
netback_probe() at this point, which will cause and invalid transaction
and set_backend_state() to BUG().
Initialize the backend state at the beginning of netback_probe() to
XenbusStateInitialising, and create a new valid state transaction on
set_backend_state(), from XenbusStateInitialising to XenbusStateClosed.
Signed-off-by: Filipe Manco <filipe.manco@neclab.eu>
---
drivers/net/xen-netback/xenbus.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index 6a31f2610c23..c0e5f6994d01 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -270,6 +270,7 @@ static int netback_probe(struct xenbus_device *dev,
be->dev = dev;
dev_set_drvdata(&dev->dev, be);
+ be->state = XenbusStateInitialising;
sg = 1;
@@ -515,6 +516,15 @@ static void set_backend_state(struct backend_info *be,
{
while (be->state != state) {
switch (be->state) {
+ case XenbusStateInitialising:
+ switch (state) {
+ case XenbusStateClosed:
+ backend_switch_state(be, XenbusStateClosed);
+ break;
+ default:
+ BUG();
+ }
+ break;
case XenbusStateClosed:
switch (state) {
case XenbusStateInitWait:
--
2.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [RFC PATCH] xen-netback: fix error handling on netback_probe()
2016-09-13 12:11 [RFC PATCH] xen-netback: fix error handling on netback_probe() Filipe Manco
@ 2016-09-14 10:10 ` Wei Liu
2016-09-15 14:05 ` [Xen-devel] " Filipe Manco
0 siblings, 1 reply; 4+ messages in thread
From: Wei Liu @ 2016-09-14 10:10 UTC (permalink / raw)
To: Filipe Manco; +Cc: netdev, wei.liu2, Xen-devel
CC xen-devel as well.
On Tue, Sep 13, 2016 at 02:11:27PM +0200, Filipe Manco wrote:
> In case of error during netback_probe() (e.g. an entry missing on the
> xenstore) netback_remove() is called on the new device, which will set
> the device backend state to XenbusStateClosed by calling
> set_backend_state(). However, the backend state wasn't initialized by
> netback_probe() at this point, which will cause and invalid transaction
> and set_backend_state() to BUG().
>
> Initialize the backend state at the beginning of netback_probe() to
> XenbusStateInitialising, and create a new valid state transaction on
> set_backend_state(), from XenbusStateInitialising to XenbusStateClosed.
>
> Signed-off-by: Filipe Manco <filipe.manco@neclab.eu>
There is a state machine right before set_backend_state. You would also
need to update that.
According to the definition of XenbusStateInitialising, this patch looks
plausible to me.
Wei.
> ---
> drivers/net/xen-netback/xenbus.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> index 6a31f2610c23..c0e5f6994d01 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -270,6 +270,7 @@ static int netback_probe(struct xenbus_device *dev,
>
> be->dev = dev;
> dev_set_drvdata(&dev->dev, be);
> + be->state = XenbusStateInitialising;
>
> sg = 1;
>
> @@ -515,6 +516,15 @@ static void set_backend_state(struct backend_info *be,
> {
> while (be->state != state) {
> switch (be->state) {
> + case XenbusStateInitialising:
> + switch (state) {
> + case XenbusStateClosed:
> + backend_switch_state(be, XenbusStateClosed);
> + break;
> + default:
> + BUG();
> + }
> + break;
> case XenbusStateClosed:
> switch (state) {
> case XenbusStateInitWait:
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Xen-devel] [RFC PATCH] xen-netback: fix error handling on netback_probe()
2016-09-14 10:10 ` Wei Liu
@ 2016-09-15 14:05 ` Filipe Manco
2016-09-15 14:10 ` Wei Liu
0 siblings, 1 reply; 4+ messages in thread
From: Filipe Manco @ 2016-09-15 14:05 UTC (permalink / raw)
To: Wei Liu; +Cc: netdev, Xen-devel
On 14-09-2016 12:10, Wei Liu wrote:
> CC xen-devel as well.
>
> On Tue, Sep 13, 2016 at 02:11:27PM +0200, Filipe Manco wrote:
>> In case of error during netback_probe() (e.g. an entry missing on the
>> xenstore) netback_remove() is called on the new device, which will set
>> the device backend state to XenbusStateClosed by calling
>> set_backend_state(). However, the backend state wasn't initialized by
>> netback_probe() at this point, which will cause and invalid transaction
>> and set_backend_state() to BUG().
>>
>> Initialize the backend state at the beginning of netback_probe() to
>> XenbusStateInitialising, and create a new valid state transaction on
>> set_backend_state(), from XenbusStateInitialising to XenbusStateClosed.
>>
>> Signed-off-by: Filipe Manco <filipe.manco@neclab.eu>
> There is a state machine right before set_backend_state. You would also
> need to update that.
Good point I'll update the diagram.
After looking at the diagram and for consistency, shouldn't the transition
Initialising -> InitWait be handled using set_backend_state()? Currently it
is done directly in netback_probe() code. If you agree I'll submit a v2 with
these two changes.
> According to the definition of XenbusStateInitialising, this patch looks
> plausible to me.
>
> Wei.
>
Filipe
>> ---
>> drivers/net/xen-netback/xenbus.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
>> index 6a31f2610c23..c0e5f6994d01 100644
>> --- a/drivers/net/xen-netback/xenbus.c
>> +++ b/drivers/net/xen-netback/xenbus.c
>> @@ -270,6 +270,7 @@ static int netback_probe(struct xenbus_device *dev,
>>
>> be->dev = dev;
>> dev_set_drvdata(&dev->dev, be);
>> + be->state = XenbusStateInitialising;
>>
>> sg = 1;
>>
>> @@ -515,6 +516,15 @@ static void set_backend_state(struct backend_info *be,
>> {
>> while (be->state != state) {
>> switch (be->state) {
>> + case XenbusStateInitialising:
>> + switch (state) {
>> + case XenbusStateClosed:
>> + backend_switch_state(be, XenbusStateClosed);
>> + break;
>> + default:
>> + BUG();
>> + }
>> + break;
>> case XenbusStateClosed:
>> switch (state) {
>> case XenbusStateInitWait:
>> --
>> 2.7.4
>>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Xen-devel] [RFC PATCH] xen-netback: fix error handling on netback_probe()
2016-09-15 14:05 ` [Xen-devel] " Filipe Manco
@ 2016-09-15 14:10 ` Wei Liu
0 siblings, 0 replies; 4+ messages in thread
From: Wei Liu @ 2016-09-15 14:10 UTC (permalink / raw)
To: Filipe Manco; +Cc: Wei Liu, netdev, Xen-devel
On Thu, Sep 15, 2016 at 04:05:17PM +0200, Filipe Manco wrote:
> On 14-09-2016 12:10, Wei Liu wrote:
> >CC xen-devel as well.
> >
> >On Tue, Sep 13, 2016 at 02:11:27PM +0200, Filipe Manco wrote:
> >>In case of error during netback_probe() (e.g. an entry missing on the
> >>xenstore) netback_remove() is called on the new device, which will set
> >>the device backend state to XenbusStateClosed by calling
> >>set_backend_state(). However, the backend state wasn't initialized by
> >>netback_probe() at this point, which will cause and invalid transaction
> >>and set_backend_state() to BUG().
> >>
> >>Initialize the backend state at the beginning of netback_probe() to
> >>XenbusStateInitialising, and create a new valid state transaction on
> >>set_backend_state(), from XenbusStateInitialising to XenbusStateClosed.
> >>
> >>Signed-off-by: Filipe Manco <filipe.manco@neclab.eu>
> >There is a state machine right before set_backend_state. You would also
> >need to update that.
> Good point I'll update the diagram.
>
> After looking at the diagram and for consistency, shouldn't the transition
> Initialising -> InitWait be handled using set_backend_state()? Currently it
> is done directly in netback_probe() code. If you agree I'll submit a v2 with
> these two changes.
That's fine with me.
Wei.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-09-15 14:10 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-09-13 12:11 [RFC PATCH] xen-netback: fix error handling on netback_probe() Filipe Manco
2016-09-14 10:10 ` Wei Liu
2016-09-15 14:05 ` [Xen-devel] " Filipe Manco
2016-09-15 14:10 ` Wei Liu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).