[PATCH] Fix: transaction id usage in gisi/server.c

[PATCH] Fix: transaction id usage in gisi/server.c

[Pekka Pessi]

[-- Attachment #1: Type: text/plain, Size: 4189 bytes --]

---
 gisi/server.c |   90 +++++++++++++++++++++++++-------------------------------
 1 files changed, 40 insertions(+), 50 deletions(-)

diff --git a/gisi/server.c b/gisi/server.c
index ef2d5dd..8eb28a7 100644
--- a/gisi/server.c
+++ b/gisi/server.c
@@ -44,7 +44,7 @@
 struct _GIsiIncoming
 {
 	struct sockaddr_pn spn;
-	uint8_t id;
+	uint8_t trans_id;
 };
 
 struct _GIsiServer {
@@ -61,8 +61,6 @@ struct _GIsiServer {
 	GIsiRequestFunc func[256];
 	void *data[256];
 
-	GIsiIncoming irq[1];
-
 	/* Debugging */
 	GIsiDebugFunc debug_func;
 	void *debug_data;
@@ -83,11 +81,11 @@ GIsiServer *g_isi_server_create(GIsiModem *modem, uint8_t resource,
 	GIsiServer *self;
 	GIOChannel *channel;
 
-	if (G_UNLIKELY(posix_memalign(&ptr, 256, sizeof(*self))))
+	if (G_UNLIKELY(posix_memalign(&ptr, 256, (sizeof *self))))
 		abort();
 
 	self = ptr;
-	memset (self, 0, sizeof *self);
+	memset (self, 0, (sizeof *self));
 	self->resource = resource;
 	self->version.major = major;
 	self->version.minor = minor;
@@ -174,9 +172,11 @@ g_isi_server_add_name(GIsiServer *self)
 			0, 0,
 		};
 
-		if (sendto(self->fd, req, sizeof(req), 0, (void *)&spn,
-				sizeof(spn)) != sizeof(spn))
-			return;
+		if (sendto(self->fd, req, (sizeof req), 0,
+			   (void *)&spn, (sizeof spn)) != (sizeof req)) {
+			g_warning("%s: %s", "sendto(PN_NAMESERVICE)",
+				  strerror(errno));
+		}
 	}
 }
 
@@ -236,7 +236,7 @@ int g_isi_vrespond(GIsiServer *self, const struct iovec *iov, size_t iovlen,
 		return -1;
 	}
 
-	_iov[0].iov_base = &irq->id;
+	_iov[0].iov_base = &irq->trans_id;
 	_iov[0].iov_len = 1;
 	for (i = 0, len = 1; i < iovlen; i++) {
 		_iov[1 + i] = iov[i];
@@ -245,8 +245,7 @@ int g_isi_vrespond(GIsiServer *self, const struct iovec *iov, size_t iovlen,
 
 	ret = sendmsg(self->fd, &msg, MSG_NOSIGNAL);
 
-	if (irq != self->irq)
-		g_free(irq);
+	g_free(irq);
 
 	return ret;
 }
@@ -290,59 +289,50 @@ static gboolean g_isi_server_callback(GIOChannel *channel, GIOCondition cond,
 					gpointer opaque)
 {
 	GIsiServer *self = opaque;
+	uint8_t msg[65536];
+	struct sockaddr_pn addr;
+	socklen_t addrlen = (sizeof addr);
 	int len;
-	GIsiIncoming *irq = self->irq;
-	struct sockaddr_pn *addr = &irq->spn;
-	socklen_t addrlen = sizeof(irq->spn);
+	uint8_t message_id;
+	uint8_t failure = 0x17;
+	GIsiRequestFunc func;
+	void *data;
 
 	if (cond & (G_IO_NVAL|G_IO_HUP)) {
 		g_warning("Unexpected event on Phonet channel %p", channel);
 		return FALSE;
 	}
 
-	len = phonet_peek_length(channel);
-	{
-		uint32_t buf[(len + 3) / 4];
-		uint8_t *msg;
-		uint8_t id;
-		uint8_t failure;
-		len = recvfrom(self->fd, buf, len, MSG_DONTWAIT,
-				(void *)addr, &addrlen);
-
-		if (len < 2 || irq->spn.spn_resource != self->resource)
-			return TRUE;
-
-		msg = (uint8_t *)buf;
+	len = recvfrom(self->fd, msg, (sizeof msg), MSG_DONTWAIT,
+			(void *)&addr, &addrlen);
 
-		if (self->debug_func)
-			self->debug_func(msg + 1, len - 1, self->debug_data);
+	if (len < 2 || addr.spn_resource != self->resource)
+		return TRUE;
 
-		irq->id = id = msg[1];
+	if (self->debug_func)
+		self->debug_func(msg + 1, len - 1, self->debug_data);
 
-		if (self->func[id]) {
-			irq = g_new0(GIsiIncoming, 1);
+	message_id = msg[1];
+	func = self->func[message_id];
+	data = self->data[message_id];
 
-			if (irq) {
-				*irq = *self->irq;
-				self->func[id](self, msg + 1, len - 1,
-						irq, self->data[id]);
-				return TRUE;
-			}
-			g_free(irq);
-			failure = 0x14;
+	if (func) {
+		GIsiIncoming *irq = g_new0(GIsiIncoming, 1);
 
-		} else {
+		if (irq) {
+			irq->spn = addr;
+			irq->trans_id = msg[0];
+			(*func)(self, msg + 1, len - 1, irq, data);
+			return TRUE;
+		}
 
-			failure = 0x17;
+		failure = 0x14;
+	}
 
-			{
-				uint8_t common[] = {
-					0xF0, failure, msg[1]
-				};
-				g_isi_respond(self, common, sizeof(common),
-						irq);
-			}
-		}
+	{
+		uint8_t common[] = { msg[0], 0xF0, failure, msg[1] };
+		sendto(self->fd, common, (sizeof common), MSG_NOSIGNAL,
+			(void *)&addr, addrlen);
 	}
 
 	return TRUE;
-- 
1.6.3.3

Re: [PATCH] Fix: transaction id usage in gisi/server.c

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 5020 bytes --]

Hi Pekka,

> ---
>  gisi/server.c |   90
>  +++++++++++++++++++++++++------------------------------- 1 files changed,
>  40 insertions(+), 50 deletions(-)
> 
> diff --git a/gisi/server.c b/gisi/server.c
> index ef2d5dd..8eb28a7 100644
> --- a/gisi/server.c
> +++ b/gisi/server.c
> @@ -44,7 +44,7 @@
>  struct _GIsiIncoming
>  {
>  	struct sockaddr_pn spn;
> -	uint8_t id;
> +	uint8_t trans_id;
>  };
> 
>  struct _GIsiServer {
> @@ -61,8 +61,6 @@ struct _GIsiServer {
>  	GIsiRequestFunc func[256];
>  	void *data[256];
> 
> -	GIsiIncoming irq[1];
> -
>  	/* Debugging */
>  	GIsiDebugFunc debug_func;
>  	void *debug_data;
> @@ -83,11 +81,11 @@ GIsiServer *g_isi_server_create(GIsiModem *modem,
>  uint8_t resource, GIsiServer *self;
>  	GIOChannel *channel;
> 
> -	if (G_UNLIKELY(posix_memalign(&ptr, 256, sizeof(*self))))
> +	if (G_UNLIKELY(posix_memalign(&ptr, 256, (sizeof *self))))

So first off, please refer to the Linux Coding Style document.  'sizeof foo' is 
not a proper construct according to our style guidelines.  It is sizeof(foo). 
 
>  		abort();
> 
>  	self = ptr;
> -	memset (self, 0, sizeof *self);
> +	memset (self, 0, (sizeof *self));

Again here.

>  	self->resource = resource;
>  	self->version.major = major;
>  	self->version.minor = minor;
> @@ -174,9 +172,11 @@ g_isi_server_add_name(GIsiServer *self)
>  			0, 0,
>  		};
> 
> -		if (sendto(self->fd, req, sizeof(req), 0, (void *)&spn,
> -				sizeof(spn)) != sizeof(spn))
> -			return;
> +		if (sendto(self->fd, req, (sizeof req), 0,

Again here

> +			   (void *)&spn, (sizeof spn)) != (sizeof req)) {

Casting to or from void is not necessary.

> +			g_warning("%s: %s", "sendto(PN_NAMESERVICE)",
> +				  strerror(errno));
> +		}
>  	}
>  }
> 
> @@ -236,7 +236,7 @@ int g_isi_vrespond(GIsiServer *self, const struct iovec
>  *iov, size_t iovlen, return -1;
>  	}
> 
> -	_iov[0].iov_base = &irq->id;
> +	_iov[0].iov_base = &irq->trans_id;
>  	_iov[0].iov_len = 1;
>  	for (i = 0, len = 1; i < iovlen; i++) {
>  		_iov[1 + i] = iov[i];
> @@ -245,8 +245,7 @@ int g_isi_vrespond(GIsiServer *self, const struct iovec
>  *iov, size_t iovlen,
> 
>  	ret = sendmsg(self->fd, &msg, MSG_NOSIGNAL);
> 
> -	if (irq != self->irq)
> -		g_free(irq);
> +	g_free(irq);
> 
>  	return ret;
>  }
> @@ -290,59 +289,50 @@ static gboolean g_isi_server_callback(GIOChannel
>  *channel, GIOCondition cond, gpointer opaque)
>  {
>  	GIsiServer *self = opaque;
> +	uint8_t msg[65536];

What is going on here? 65K buffer seems excessive and no comment in sight...

> +	struct sockaddr_pn addr;
> +	socklen_t addrlen = (sizeof addr);
>  	int len;
> -	GIsiIncoming *irq = self->irq;
> -	struct sockaddr_pn *addr = &irq->spn;
> -	socklen_t addrlen = sizeof(irq->spn);
> +	uint8_t message_id;
> +	uint8_t failure = 0x17;
> +	GIsiRequestFunc func;
> +	void *data;
> 
>  	if (cond & (G_IO_NVAL|G_IO_HUP)) {
>  		g_warning("Unexpected event on Phonet channel %p", channel);
>  		return FALSE;
>  	}
> 
> -	len = phonet_peek_length(channel);
> -	{
> -		uint32_t buf[(len + 3) / 4];
> -		uint8_t *msg;
> -		uint8_t id;
> -		uint8_t failure;
> -		len = recvfrom(self->fd, buf, len, MSG_DONTWAIT,
> -				(void *)addr, &addrlen);
> -
> -		if (len < 2 || irq->spn.spn_resource != self->resource)
> -			return TRUE;
> -
> -		msg = (uint8_t *)buf;
> +	len = recvfrom(self->fd, msg, (sizeof msg), MSG_DONTWAIT,
> +			(void *)&addr, &addrlen);

Again, no void casting please.

> 
> -		if (self->debug_func)
> -			self->debug_func(msg + 1, len - 1, self->debug_data);
> +	if (len < 2 || addr.spn_resource != self->resource)
> +		return TRUE;
> 
> -		irq->id = id = msg[1];
> +	if (self->debug_func)
> +		self->debug_func(msg + 1, len - 1, self->debug_data);
> 
> -		if (self->func[id]) {
> -			irq = g_new0(GIsiIncoming, 1);
> +	message_id = msg[1];
> +	func = self->func[message_id];
> +	data = self->data[message_id];
> 
> -			if (irq) {
> -				*irq = *self->irq;
> -				self->func[id](self, msg + 1, len - 1,
> -						irq, self->data[id]);
> -				return TRUE;
> -			}
> -			g_free(irq);
> -			failure = 0x14;
> +	if (func) {
> +		GIsiIncoming *irq = g_new0(GIsiIncoming, 1);
> 
> -		} else {
> +		if (irq) {
> +			irq->spn = addr;
> +			irq->trans_id = msg[0];
> +			(*func)(self, msg + 1, len - 1, irq, data);

Why the extra parens around func?

> +			return TRUE;
> +		}
> 
> -			failure = 0x17;
> +		failure = 0x14;
> +	}
> 
> -			{
> -				uint8_t common[] = {
> -					0xF0, failure, msg[1]
> -				};
> -				g_isi_respond(self, common, sizeof(common),
> -						irq);
> -			}
> -		}
> +	{
> +		uint8_t common[] = { msg[0], 0xF0, failure, msg[1] };
> +		sendto(self->fd, common, (sizeof common), MSG_NOSIGNAL,
> +			(void *)&addr, addrlen);
>  	}

Such style is really not encouraged...  Perhaps an inline function would be 
better here...

> 
>  	return TRUE;
> 

Regards,
-Denis

Re: [PATCH] Fix: transaction id usage in gisi/server.c

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 235 bytes --]

> So first off, please refer to the Linux Coding Style document.  'sizeof
>  foo' is not a proper construct according to our style guidelines.  It is
>  sizeof(foo).

Linux Kernel CodingStyle document that is.

Regards,
-Denis

Re: [PATCH] Fix: transaction id usage in gisi/server.c

[Pekka Pessi]

[-- Attachment #1: Type: text/plain, Size: 349 bytes --]

2010/4/23 Denis Kenzior <denkenz@gmail.com>:
>> +                        (void *)&spn, (sizeof spn)) != (sizeof req)) {
>
> Casting to or from void is not necessary.

Oh, it is. If a struct sockaddr_pn structure is casted directly to
struct sockaddr, gcc will issue alignment warning on ARM.

-- 
Pekka.Pessi mail at nokia.com

Re: [PATCH] Fix: transaction id usage in gisi/server.c

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

[-- Attachment #1: Type: text/plain, Size: 1427 bytes --]


On Fri, 23 Apr 2010 16:07:11 +0300, Pekka Pessi <ppessi@gmail.com> wrote:
> 2010/4/23 Denis Kenzior <denkenz@gmail.com>:
>>> +                        (void *)&spn, (sizeof spn)) !=
> (sizeof req)) {
>>
>> Casting to or from void is not necessary.
> 
> Oh, it is. If a struct sockaddr_pn structure is casted directly to
> struct sockaddr, gcc will issue alignment warning on ARM.

The cast-align warning is a can of false positives. First that warning
should probably not be enabled. It does nothing on x86, and it's a waste of
time with ARM and other pointer-picky platforms.

Most importantly, using -Werror _by_default_ is a _damn_idiotic_ idea. Been
there, done that. One cannot know what weird warning any given compiler or
version thereof will ever return. And then I don't need to mention "bugs"
in header files from underlying components. Even changing GCC optimization
levels can yield different conflicting warnings. That's known to happen for
uninitialized values detection, which relies on optimization-dependent code
analysis as an example.

Note that recent GCC versions allow turning only some warnings as errors
with -Werror... and -Wno-error.... That's much saner; for instance calling
a function without prototype is clearly a bug and it is quite reasonable to
make it an error.

-- 
Rémi Denis-Courmont
http://www.remlab.net
http://fi.linkedin.com/in/remidenis

Re: [PATCH] Fix: transaction id usage in gisi/server.c

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1668 bytes --]

Hi Remi,

> >> Casting to or from void is not necessary.
> > 
> > Oh, it is. If a struct sockaddr_pn structure is casted directly to
> > struct sockaddr, gcc will issue alignment warning on ARM.
> 
> The cast-align warning is a can of false positives. First that warning
> should probably not be enabled. It does nothing on x86, and it's a waste of
> time with ARM and other pointer-picky platforms.

I don't have an ARM platform easy available to test this, but this can
be fixed by just memcpy the data structures.

> Most importantly, using -Werror _by_default_ is a _damn_idiotic_ idea. Been
> there, done that. One cannot know what weird warning any given compiler or
> version thereof will ever return. And then I don't need to mention "bugs"
> in header files from underlying components. Even changing GCC optimization
> levels can yield different conflicting warnings. That's known to happen for
> uninitialized values detection, which relies on optimization-dependent code
> analysis as an example.

The rant about -Werror doesn't help. We are using it to catch nasty
constructs that might fail on any given platform. We have used -Werror
inside BlueZ and ConnMan for a long time now. Yes, sometimes new GCC
warnings come up and we fix that, but it was always good to have these.
And yes, it is true that you need to use -O2 to enable a bunch of these
warnings.

So in conclusion, the -Werror stays and we need to fix the code to
compile cleanly on any given platform. It is possible, so lets get this
done. Feel free to post the warnings on the mailing list and I am more
than happy to look into it.

Regards

Marcel

Re: [PATCH] Fix: transaction id usage in gisi/server.c

[Pekka Pessi]

[-- Attachment #1: Type: text/plain, Size: 828 bytes --]

2010/4/23 Rémi Denis-Courmont <remi@remlab.net>:
> On Fri, 23 Apr 2010 16:07:11 +0300, Pekka Pessi <ppessi@gmail.com> wrote:
>> 2010/4/23 Denis Kenzior <denkenz@gmail.com>:
>>>> +                        (void *)&spn, (sizeof spn)) !=
>> (sizeof req)) {
>>>
>>> Casting to or from void is not necessary.
>>
>> Oh, it is. If a struct sockaddr_pn structure is casted directly to
>> struct sockaddr, gcc will issue alignment warning on ARM.
>
> The cast-align warning is a can of false positives. First that warning
> should probably not be enabled. It does nothing on x86, and it's a waste of
> time with ARM and other pointer-picky platforms.

BTW, why we have __attribute__ ((packed)) in sockaddr_pn definition?
Looks like it is the cause of the problems here.

-- 
Pekka.Pessi mail at nokia.com

Re: [PATCH] Fix: transaction id usage in gisi/server.c

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

[-- Attachment #1: Type: text/plain, Size: 2692 bytes --]


On Fri, 23 Apr 2010 16:34:44 +0200, Marcel Holtmann <marcel@holtmann.org>
wrote:
>> The cast-align warning is a can of false positives. First that warning
>> should probably not be enabled. It does nothing on x86, and it's a waste
>> of time with ARM and other pointer-picky platforms.
>
> I don't have an ARM platform easy available to test this, but this can
> be fixed by just memcpy the data structures.

No, that wouldn't make sense.

Memory copying would make sense if we had an unaligned (or possibly
unaligned) byte range that we want to access. In this case, we have a
properly aligned structure but GCC does not know. GCC thinks we are passing
a struct sockaddr_pn (alignment: one byte) as a struct sockaddr (alignment:
two byte).

In this case however, the pointer will be passed to kernel. And the kernel
definitely does not care about alignment. It would be a trivial local DoS
if it did.

In this particular case, removing the packing of sockaddr_pn might just
work. But there are other cases that simply cannot be solved. For instance
casting a sockaddr_storage to a sockaddr_in or sockaddr_in6 exhibits the
same problem. Yet it has to be valid, by design of sockaddr_storage.

>> Most importantly, using -Werror _by_default_ is a _damn_idiotic_ idea.
>> Been there, done that. One cannot know what weird warning any given
>> compiler or version thereof will ever return. And then I don't need
>> to mention "bugs" in header files from underlying components. Even
changing GCC
> optimization
>> levels can yield different conflicting warnings. That's known to happen
> for
>> uninitialized values detection, which relies on optimization-dependent
> code
>> analysis as an example.
>
> The rant about -Werror doesn't help. We are using it to catch nasty
> constructs that might fail on any given platform. We have used -Werror
> inside BlueZ and ConnMan for a long time now. Yes, sometimes new GCC
> warnings come up and we fix that, but it was always good to have these.

No it is not. I gave the counter examples multiple times. I don't need to
quote them again.

> And yes, it is true that you need to use -O2 to enable a bunch of these
> warnings.

Yet another proof that it is a bad idea. You are self-contradictory.

> So in conclusion, the -Werror stays and we need to fix the code to
> compile cleanly on any given platform. It is possible, so lets get this
> done. Feel free to post the warnings on the mailing list and I am more
> than happy to look into it.

Your problem. If you don't want oFono to compile, I mean.

-- 
Rémi Denis-Courmont
http://www.remlab.net
http://fi.linkedin.com/in/remidenis

[PATCH] Fix: transaction id usage in gisi/server.c

[Pekka Pessi]

[-- Attachment #1: Type: text/plain, Size: 4732 bytes --]

---
 gisi/server.c |  111 +++++++++++++++++++++++++++-----------------------------
 1 files changed, 54 insertions(+), 57 deletions(-)

diff --git a/gisi/server.c b/gisi/server.c
index ef2d5dd..26386c1 100644
--- a/gisi/server.c
+++ b/gisi/server.c
@@ -44,7 +44,7 @@
 struct _GIsiIncoming
 {
 	struct sockaddr_pn spn;
-	uint8_t id;
+	uint8_t trans_id;
 };
 
 struct _GIsiServer {
@@ -61,8 +61,6 @@ struct _GIsiServer {
 	GIsiRequestFunc func[256];
 	void *data[256];
 
-	GIsiIncoming irq[1];
-
 	/* Debugging */
 	GIsiDebugFunc debug_func;
 	void *debug_data;
@@ -87,7 +85,7 @@ GIsiServer *g_isi_server_create(GIsiModem *modem, uint8_t resource,
 		abort();
 
 	self = ptr;
-	memset (self, 0, sizeof *self);
+	memset (self, 0, sizeof(*self));
 	self->resource = resource;
 	self->version.major = major;
 	self->version.minor = minor;
@@ -174,9 +172,11 @@ g_isi_server_add_name(GIsiServer *self)
 			0, 0,
 		};
 
-		if (sendto(self->fd, req, sizeof(req), 0, (void *)&spn,
-				sizeof(spn)) != sizeof(spn))
-			return;
+		if (sendto(self->fd, req, sizeof(req), 0,
+				(void *)&spn, sizeof(spn)) != sizeof(req)) {
+			g_warning("%s: %s", "sendto(PN_NAMESERVICE)",
+				  strerror(errno));
+		}
 	}
 }
 
@@ -236,7 +236,7 @@ int g_isi_vrespond(GIsiServer *self, const struct iovec *iov, size_t iovlen,
 		return -1;
 	}
 
-	_iov[0].iov_base = &irq->id;
+	_iov[0].iov_base = &irq->trans_id;
 	_iov[0].iov_len = 1;
 	for (i = 0, len = 1; i < iovlen; i++) {
 		_iov[1 + i] = iov[i];
@@ -245,8 +245,7 @@ int g_isi_vrespond(GIsiServer *self, const struct iovec *iov, size_t iovlen,
 
 	ret = sendmsg(self->fd, &msg, MSG_NOSIGNAL);
 
-	if (irq != self->irq)
-		g_free(irq);
+	g_free(irq);
 
 	return ret;
 }
@@ -285,65 +284,63 @@ void g_isi_server_unhandle(GIsiServer *self, uint8_t type)
 		self->func[type] = NULL;
 }
 
-/* Data callback */
-static gboolean g_isi_server_callback(GIOChannel *channel, GIOCondition cond,
-					gpointer opaque)
-{
-	GIsiServer *self = opaque;
-	int len;
-	GIsiIncoming *irq = self->irq;
-	struct sockaddr_pn *addr = &irq->spn;
-	socklen_t addrlen = sizeof(irq->spn);
 
-	if (cond & (G_IO_NVAL|G_IO_HUP)) {
-		g_warning("Unexpected event on Phonet channel %p", channel);
-		return FALSE;
-	}
+static void generic_error_response(GIsiServer *self,
+			uint8_t trans_id, uint8_t error, uint8_t message_id,
+			void *addr, socklen_t addrlen)
+{
+	uint8_t common[] = { trans_id, 0xF0, error, message_id };
 
-	len = phonet_peek_length(channel);
-	{
-		uint32_t buf[(len + 3) / 4];
-		uint8_t *msg;
-		uint8_t id;
-		uint8_t failure;
-		len = recvfrom(self->fd, buf, len, MSG_DONTWAIT,
-				(void *)addr, &addrlen);
+	sendto(self->fd, common, sizeof(common), MSG_NOSIGNAL, addr, addrlen);
+}
 
-		if (len < 2 || irq->spn.spn_resource != self->resource)
-			return TRUE;
+static void process_message(GIsiServer *self, int len)
+{
+	uint8_t msg[len + 1];
+	struct sockaddr_pn addr;
+	socklen_t addrlen = sizeof(addr);
+	uint8_t message_id;
+	GIsiRequestFunc func;
+	void *data;
 
-		msg = (uint8_t *)buf;
+	len = recvfrom(self->fd, msg, sizeof(msg), MSG_DONTWAIT,
+			(void *)&addr, &addrlen);
 
-		if (self->debug_func)
-			self->debug_func(msg + 1, len - 1, self->debug_data);
+	if (len < 2 || addr.spn_resource != self->resource)
+		return;
 
-		irq->id = id = msg[1];
+	if (self->debug_func)
+		self->debug_func(msg + 1, len - 1, self->debug_data);
 
-		if (self->func[id]) {
-			irq = g_new0(GIsiIncoming, 1);
+	message_id = msg[1];
+	func = self->func[message_id];
+	data = self->data[message_id];
 
-			if (irq) {
-				*irq = *self->irq;
-				self->func[id](self, msg + 1, len - 1,
-						irq, self->data[id]);
-				return TRUE;
-			}
-			g_free(irq);
-			failure = 0x14;
+	if (func) {
+		GIsiIncoming *irq = g_new0(GIsiIncoming, 1);
 
-		} else {
+		if (irq) {
+			irq->spn = addr;
+			irq->trans_id = msg[0];
+			func(self, msg + 1, len - 1, irq, data);
+			return;
+		}
+	}
 
-			failure = 0x17;
+	/* Respond with COMMON MESSAGE COMM_SERVICE_NOT_AUTHENTICATED_RESP */
+	generic_error_response(self, msg[0], 0x17, msg[1], &addr, addrlen);
+}
 
-			{
-				uint8_t common[] = {
-					0xF0, failure, msg[1]
-				};
-				g_isi_respond(self, common, sizeof(common),
-						irq);
-			}
-		}
+/* Data callback */
+static gboolean g_isi_server_callback(GIOChannel *channel, GIOCondition cond,
+					gpointer opaque)
+{
+	if (cond & (G_IO_NVAL|G_IO_HUP)) {
+		g_warning("Unexpected event on Phonet channel %p", channel);
+		return FALSE;
 	}
 
+	process_message(opaque, phonet_peek_length(channel));
+
 	return TRUE;
 }
-- 
1.6.3.3

Re: [PATCH] Fix: transaction id usage in gisi/server.c

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

[-- Attachment #1: Type: text/plain, Size: 693 bytes --]


On Fri, 23 Apr 2010 18:17:16 +0300, Pekka Pessi <Pekka.Pessi@nokia.com>
wrote:
> @@ -174,9 +172,11 @@ g_isi_server_add_name(GIsiServer *self)
>  			0, 0,
>  		};
> 
> -		if (sendto(self->fd, req, sizeof(req), 0, (void *)&spn,
> -				sizeof(spn)) != sizeof(spn))
> -			return;
> +		if (sendto(self->fd, req, sizeof(req), 0,
> +				(void *)&spn, sizeof(spn)) != sizeof(req)) {
> +			g_warning("%s: %s", "sendto(PN_NAMESERVICE)",
> +				  strerror(errno));
> +		}
>  	}

sendto() does not set errno if it returns less than sizeof(req) but more
than -1. I don't know if we care though.

-- 
Rémi Denis-Courmont
http://www.remlab.net
http://fi.linkedin.com/in/remidenis

Re: [PATCH] Fix: transaction id usage in gisi/server.c

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 473 bytes --]

Hi Pekka,

> ---
>  gisi/server.c |  111
>  +++++++++++++++++++++++++++----------------------------- 1 files changed,
>  54 insertions(+), 57 deletions(-)

WARNING: space prohibited between function name and open parenthesis '('
#95: FILE: gisi/server.c:88:
+	memset (self, 0, sizeof(*self));

I was nice and fixed it for you this time.  Please get into a habit of checking 
your patches with checkpatch.

Patch has been applied, thanks.

Regards,
-Denis