9:26 p.m.
---
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
7:27 a.m.
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
11:07 p.m.
2010/4/23 Denis Kenzior <denkenz(a)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
11:46 p.m.
On Fri, 23 Apr 2010 16:07:11 +0300, Pekka Pessi <ppessi(a)gmail.com> wrote:
2010/4/23 Denis Kenzior <denkenz(a)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
12:34 a.m.
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
12:57 a.m.
2010/4/23 Rémi Denis-Courmont <remi(a)remlab.net>:
On Fri, 23 Apr 2010 16:07:11 +0300, Pekka Pessi
<ppessi(a)gmail.com> wrote:
> 2010/4/23 Denis Kenzior <denkenz(a)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
4531
days inactive
4532
days old
6 comments
5 participants
participants (5)
-
Denis Kenzior -
Marcel Holtmann -
Pekka Pessi -
Pekka Pessi -
Rémi Denis-Courmont