[PATCH 0/3 v2] xen/xenbus: Several improvements for 'xenbus_client.c'

[PATCH 0/3 v2] xen/xenbus: Several improvements for 'xenbus_client.c'

[Chen Gang]

Hello Maintainers:

Please help check when you have time. Thanks.

Chen Gang (3):
  xen/xenbus: Correct the comments for xenbus_grant_ring()
  xen/xenbus: Remove BUG_ON() when error string trucated
  xen/xenbus: Improve failure processing code for __xenbus_switch_state()

 drivers/xen/xenbus/xenbus_client.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

-- 
1.9.3

[PATCH 1/3 v2] xen/xenbus: Correct the comments for xenbus_grant_ring()

[Chen Gang]

A grant reference (which is a positive number) can indicate success, so
the original comments need be improved.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 drivers/xen/xenbus/xenbus_client.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
index 439c9dc..aa9b2fc 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -361,8 +361,8 @@ static void xenbus_switch_fatal(struct xenbus_device *dev, int depth, int err,
  * @ring_mfn: mfn of ring to grant
 
  * Grant access to the given @ring_mfn to the peer of the given device.  Return
- * 0 on success, or -errno on error.  On error, the device will switch to
- * XenbusStateClosing, and the error will be saved in the store.
+ * a grant reference on success, or -errno on error. On error, the device will
+ * switch to XenbusStateClosing, and the error will be saved in the store.
  */
 int xenbus_grant_ring(struct xenbus_device *dev, unsigned long ring_mfn)
 {
-- 
1.9.3

[PATCH 2/3 v2] xen/xenbus: Remove BUG_ON() when error string trucated

[Chen Gang]

xenbus_va_dev_error() is for printing error, so when error string is
too long to be trancated, need not BUG_ON(), still return truncation
string is OK.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 drivers/xen/xenbus/xenbus_client.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
index aa9b2fc..ca74410 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -259,7 +259,6 @@ static char *error_path(struct xenbus_device *dev)
 static void xenbus_va_dev_error(struct xenbus_device *dev, int err,
 				const char *fmt, va_list ap)
 {
-	int ret;
 	unsigned int len;
 	char *printf_buffer = NULL;
 	char *path_buffer = NULL;
@@ -270,9 +269,7 @@ static void xenbus_va_dev_error(struct xenbus_device *dev, int err,
 		goto fail;
 
 	len = sprintf(printf_buffer, "%i ", -err);
-	ret = vsnprintf(printf_buffer+len, PRINTF_BUFFER_SIZE-len, fmt, ap);
-
-	BUG_ON(len + ret > PRINTF_BUFFER_SIZE-1);
+	vsnprintf(printf_buffer+len, PRINTF_BUFFER_SIZE-len, fmt, ap);
 
 	dev_err(&dev->dev, "%s\n", printf_buffer);
 
-- 
1.9.3

[PATCH 3/3 v2] xen/xenbus: Improve failure processing code for __xenbus_switch_state()

[Chen Gang]

When failure occurs, need return failure code instead of 0, or some of
indirect upper callers may misunderstand.

e.g. in "block/xen-blkback/xenbus.c":

    connect() -> xenbus_switch_state() -> __xenbus_switch_state().

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 drivers/xen/xenbus/xenbus_client.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
index ca74410..642a476 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -36,6 +36,7 @@
 #include <linux/spinlock.h>
 #include <linux/vmalloc.h>
 #include <linux/export.h>
+#include <linux/delay.h>
 #include <asm/xen/hypervisor.h>
 #include <asm/xen/page.h>
 #include <xen/interface/xen.h>
@@ -196,7 +197,7 @@ again:
 	err = xenbus_transaction_start(&xbt);
 	if (err) {
 		xenbus_switch_fatal(dev, depth, err, "starting transaction");
-		return 0;
+		return err;
 	}
 
 	err = xenbus_scanf(xbt, dev->nodename, "state", "%d", &current_state);
@@ -219,7 +220,7 @@ abort:
 	} else
 		dev->state = state;
 
-	return 0;
+	return err;
 }
 
 /**
-- 
1.9.3

Re: [Xen-devel] [PATCH 3/3 v2] xen/xenbus: Improve failure processing code for __xenbus_switch_state()

[David Vrabel]

On 18/09/14 15:01, Chen Gang wrote:
> When failure occurs, need return failure code instead of 0, or some of
> indirect upper callers may misunderstand.
> 
> e.g. in "block/xen-blkback/xenbus.c":
> 
>     connect() -> xenbus_switch_state() -> __xenbus_switch_state().

Can you make xenbus_switch_state() void?  The callers don't need to do
any error handling.

David

Re: [Xen-devel] [PATCH 3/3 v2] xen/xenbus: Improve failure processing code for __xenbus_switch_state()

[Chen Gang]

On 09/22/2014 11:04 PM, David Vrabel wrote:
> On 18/09/14 15:01, Chen Gang wrote:
>> When failure occurs, need return failure code instead of 0, or some of
>> indirect upper callers may misunderstand.
>>
>> e.g. in "block/xen-blkback/xenbus.c":
>>
>>     connect() -> xenbus_switch_state() -> __xenbus_switch_state().
> 
> Can you make xenbus_switch_state() void?  The callers don't need to do
> any error handling.
> 

After "grep rn xenbus_switch_state *" under "drivers/", it is not one
place to check the return value of xenbus_switch_state(), and also it
is export to outside for individual modules.

So we need change many subsystems for it, and also need face the rick
for incompatible with the old individual modules which source code are
not in upstream.

And are you sure the caller need not notice about it, when it really
fails? (for me, I guess they need notice about it)

Thanks.
-- 
Chen Gang

Open share and attitude like air water and life which God blessed

Re: [Xen-devel] [PATCH 3/3 v2] xen/xenbus: Improve failure processing code for __xenbus_switch_state()

[David Vrabel]

On 22/09/14 16:51, Chen Gang wrote:
> On 09/22/2014 11:04 PM, David Vrabel wrote:
>> On 18/09/14 15:01, Chen Gang wrote:
>>> When failure occurs, need return failure code instead of 0, or some of
>>> indirect upper callers may misunderstand.
>>>
>>> e.g. in "block/xen-blkback/xenbus.c":
>>>
>>>     connect() -> xenbus_switch_state() -> __xenbus_switch_state().
>>
>> Can you make xenbus_switch_state() void?  The callers don't need to do
>> any error handling.
>>
> 
> After "grep rn xenbus_switch_state *" under "drivers/", it is not one
> place to check the return value of xenbus_switch_state(), and also it
> is export to outside for individual modules.
> 
> So we need change many subsystems for it, and also need face the rick
> for incompatible with the old individual modules which source code are
> not in upstream.

Having to update 9 callers doesn't seem like much work.

> And are you sure the caller need not notice about it, when it really
> fails? (for me, I guess they need notice about it)

Yes.  xenbus_switch_state() already signals the fatal error to the
toolstack with xenbus_dev_fatal().

David

Re: [Xen-devel] [PATCH 3/3 v2] xen/xenbus: Improve failure processing code for __xenbus_switch_state()

[Chen Gang]

On 9/23/14 0:01, David Vrabel wrote:
> On 22/09/14 16:51, Chen Gang wrote:
>> On 09/22/2014 11:04 PM, David Vrabel wrote:
>>> On 18/09/14 15:01, Chen Gang wrote:
>>>> When failure occurs, need return failure code instead of 0, or some of
>>>> indirect upper callers may misunderstand.
>>>>
>>>> e.g. in "block/xen-blkback/xenbus.c":
>>>>
>>>>     connect() -> xenbus_switch_state() -> __xenbus_switch_state().
>>>
>>> Can you make xenbus_switch_state() void?  The callers don't need to do
>>> any error handling.
>>>
>>
>> After "grep rn xenbus_switch_state *" under "drivers/", it is not one
>> place to check the return value of xenbus_switch_state(), and also it
>> is export to outside for individual modules.
>>
>> So we need change many subsystems for it, and also need face the rick
>> for incompatible with the old individual modules which source code are
>> not in upstream.
> 
> Having to update 9 callers doesn't seem like much work.
> 

OK, thanks, and I guess, we need not care about the individual modules
which source code are not in upstream.

>> And are you sure the caller need not notice about it, when it really
>> fails? (for me, I guess they need notice about it)
> 
> Yes.  xenbus_switch_state() already signals the fatal error to the
> toolstack with xenbus_dev_fatal().
> 

OK, what you said sound reasonable to me, I shall send patch v3 for it
within this week (do not consider about the individual modules which
source code are not in upstream).


Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed