[RFC] irda: irttp: allow zero byte packets

[RFC] irda: irttp: allow zero byte packets

[Wolfram Sang]

Sending zero byte packets is not neccessarily an error (AF_INET accepts it,
too), so just apply a shortcut. This was discovered because of a non-working
software with WINE. See

  http://bugs.winehq.org/show_bug.cgi?id=19397#c86
  http://thread.gmane.org/gmane.linux.irda.general/1643

for very detailed debugging information and a testcase. Kudos to Wolfgang for
those!

Reported-by: Wolfgang Schwotzer <wolfgang.schwotzer@gmx.net>
Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Tested-by: Mike Evans <mike.evans@cardolan.com>
Cc: Samuel Ortiz <samuel@sortiz.org>
---

I found Wolfgang's very detailed report while looking for WINE-bugreports
affecting the kernel somehow. His mail sadly went to an almost dead
mailing-list and he told me he lost interest meanwhile. This is why I picked
the issue up and created this straightforward patch which helps the case at
least (thanks Mike for testing!).

 net/irda/irttp.c |   25 +++++++++++++++++++------
 1 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/net/irda/irttp.c b/net/irda/irttp.c
index 285761e..6cfaeaf 100644
--- a/net/irda/irttp.c
+++ b/net/irda/irttp.c
@@ -550,16 +550,23 @@ EXPORT_SYMBOL(irttp_close_tsap);
  */
 int irttp_udata_request(struct tsap_cb *self, struct sk_buff *skb)
 {
+	int ret = -1;
+
 	IRDA_ASSERT(self != NULL, return -1;);
 	IRDA_ASSERT(self->magic == TTP_TSAP_MAGIC, return -1;);
 	IRDA_ASSERT(skb != NULL, return -1;);
 
 	IRDA_DEBUG(4, "%s()\n", __func__);
 
+	/* Take shortcut on zero byte packets */
+	if (skb->len == 0) {
+		ret = 0;
+		goto err;
+	}
+
 	/* Check that nothing bad happens */
-	if ((skb->len == 0) || (!self->connected)) {
-		IRDA_DEBUG(1, "%s(), No data, or not connected\n",
-			   __func__);
+	if (!self->connected) {
+		IRDA_DEBUG(1, "%s(), Not connected\n", __func__);
 		goto err;
 	}
 
@@ -576,7 +583,7 @@ int irttp_udata_request(struct tsap_cb *self, struct sk_buff *skb)
 
 err:
 	dev_kfree_skb(skb);
-	return -1;
+	return ret;
 }
 EXPORT_SYMBOL(irttp_udata_request);
 
@@ -599,9 +606,15 @@ int irttp_data_request(struct tsap_cb *self, struct sk_buff *skb)
 	IRDA_DEBUG(2, "%s() : queue len = %d\n", __func__,
 		   skb_queue_len(&self->tx_queue));
 
+	/* Take shortcut on zero byte packets */
+	if (skb->len == 0) {
+		ret = 0;
+		goto err;
+	}
+
 	/* Check that nothing bad happens */
-	if ((skb->len == 0) || (!self->connected)) {
-		IRDA_WARNING("%s: No data, or not connected\n", __func__);
+	if (!self->connected) {
+		IRDA_WARNING("%s: Not connected\n", __func__);
 		ret = -ENOTCONN;
 		goto err;
 	}
-- 
1.7.2.3

Re: [RFC] irda: irttp: allow zero byte packets

[David Miller]

From: Wolfram Sang <w.sang@pengutronix.de>
Date: Wed, 10 Nov 2010 00:19:47 +0100

> Sending zero byte packets is not neccessarily an error (AF_INET accepts it,
> too), so just apply a shortcut. This was discovered because of a non-working
> software with WINE. See
> 
>   http://bugs.winehq.org/show_bug.cgi?id=19397#c86
>   http://thread.gmane.org/gmane.linux.irda.general/1643
> 
> for very detailed debugging information and a testcase. Kudos to Wolfgang for
> those!
> 
> Reported-by: Wolfgang Schwotzer <wolfgang.schwotzer@gmx.net>
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Tested-by: Mike Evans <mike.evans@cardolan.com>

This looks good to me, applied, thanks!

[PATCH] net: irda: irttp: sync error paths of data- and udata-requests

[Wolfram Sang]

irttp_data_request() returns meaningful errorcodes, while irttp_udata_request()
just returns -1 in similar situations. Sync the two and the loglevels of the
accompanying output.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: David Miller <davem@davemloft.net>
---

Thank you David for picking up the zero-byte-packet-patch. Now as it was
applied, this one might be interesting, too (on top of it)? Nothing seriously
needed, but looks more proper IMHO. LXR says that are callers of these
functions check with < 0 anyhow.

 net/irda/irttp.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/irda/irttp.c b/net/irda/irttp.c
index 6cfaeaf..f6054f9 100644
--- a/net/irda/irttp.c
+++ b/net/irda/irttp.c
@@ -550,7 +550,7 @@ EXPORT_SYMBOL(irttp_close_tsap);
  */
 int irttp_udata_request(struct tsap_cb *self, struct sk_buff *skb)
 {
-	int ret = -1;
+	int ret;
 
 	IRDA_ASSERT(self != NULL, return -1;);
 	IRDA_ASSERT(self->magic == TTP_TSAP_MAGIC, return -1;);
@@ -566,13 +566,14 @@ int irttp_udata_request(struct tsap_cb *self, struct sk_buff *skb)
 
 	/* Check that nothing bad happens */
 	if (!self->connected) {
-		IRDA_DEBUG(1, "%s(), Not connected\n", __func__);
+		IRDA_WARNING("%s(), Not connected\n", __func__);
+		ret = -ENOTCONN;
 		goto err;
 	}
 
 	if (skb->len > self->max_seg_size) {
-		IRDA_DEBUG(1, "%s(), UData is too large for IrLAP!\n",
-			   __func__);
+		IRDA_ERROR("%s(), UData is too large for IrLAP!\n", __func__);
+		ret = -EMSGSIZE;
 		goto err;
 	}
 
-- 
1.7.2.3

Re: [PATCH] net: irda: irttp: sync error paths of data- and udata-requests

[David Miller]

From: Wolfram Sang <w.sang@pengutronix.de>
Date: Tue, 16 Nov 2010 20:40:02 +0100

> irttp_data_request() returns meaningful errorcodes, while irttp_udata_request()
> just returns -1 in similar situations. Sync the two and the loglevels of the
> accompanying output.
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Cc: David Miller <davem@davemloft.net>
> ---
> 
> Thank you David for picking up the zero-byte-packet-patch. Now as it was
> applied, this one might be interesting, too (on top of it)? Nothing seriously
> needed, but looks more proper IMHO. LXR says that are callers of these
> functions check with < 0 anyhow.

Better error code reporting is always appreciated :-)

Applied, thanks.