Wed 07 Mar 2007 01:18:49 PM UTC, comment #9:
I have prepared another patch, which adds LWIP_ASSERT in those places. Also, I have removed excessive set_errno() calls after get_socket(), because errno is set inside of get_socket().
And, yes, netconn API is mainly useful because it allows to avoid an extra copy (and extra memory too).
(file #12143)
|
Wed 07 Mar 2007 11:09:17 AM UTC, comment #8:
We're all agreed it would be nice, but I'm sure there are some people out there using the netconn API, and the risk of breaking the sockets API would be reasonably high, so this is not something I'd encourage people to spend time on.
As to the original issue, if these checks are really wanted I guess that ASSERTs would be OK. However, there are so many other places where we should really be doing similar things that the end result is barely improved, so don't worry too much about relatively minor problems like this.
|
Wed 07 Mar 2007 10:49:48 AM UTC, comment #7:
I definitively would like to, though not today ;-)
No, to be honest, as much as I would like a clean socket API directly interfacing the stack, I think it would certainly need some time until it is as stable as the current solution.
|
Wed 07 Mar 2007 10:38:00 AM UTC, comment #6:
Agree with you Simon. You do the job ? :)
|
Wed 07 Mar 2007 10:34:45 AM UTC, comment #5:
> In fact, I think that netconn_ is not so useful, except for the zero-copy.
In fact, that's what I think, also. I don't really know what the benefit is using netconn api (OK, except for the zero-copy). If you really want to design a fast application, you'd use the callback API. And if not, you can use the socket API.
The only benefit seems to be the zero-copy on receive. TCP sending with copy=0 (zero copy send) is rare since you must have real static (or ROM) data.
So, when receiving, socket API is of course slower, but does that really outweight the fact that we have to support 3 APIs instead of 2?
Also, I think a tightly integrated socket API could be more performant than the current "3-layer-solution".
|
Wed 07 Mar 2007 10:13:11 AM UTC, comment #4:
>So, if you worry about footprint and performance, it is better to use netconn_ interface.
In fact, I think that netconn_ is not so useful, except for the zero-copy. To my point of view, it's will be better to only one "sequential" interface, the BSD sockets.
>BTW, there is no need to use set_errno(EBADF) if get_socket(s) returns NULL, because get_socket has already set errno.
Agree with you.
|
Wed 07 Mar 2007 09:52:31 AM UTC, comment #3:
I believe that the whole socket.c is overhead that you are ready to pay for better compatibility with standard sockets. So, if you worry about footprint and performance, it is better to use netconn_ interface. And, we check the first parameter - socket descriptor, why don't have a reasonable check for others? If you think it adds big overhead, then let's make it ASSERT.
BTW, there is no need to use set_errno(EBADF) if get_socket(s) returns NULL, because get_socket has already set errno.
sock = get_socket(s);
if (!sock) {
- set_errno(EBADF);
return -1;
}
As to api_lib.c, I prefer to see ASSERT instead of that not NULL checking.
|
Wed 07 Mar 2007 09:21:22 AM UTC, comment #2:
Don't you think this checking would have to be disable to reduce footprint and increase performance in "production time", but enable to allow a higher checking during "debug time", or if you don't manage applications using lwip (in my case, I can trust the application developer, it's me :) )?
Same thing about api_lib.c:
Lot of code to enable parameters checking inside netconn functions, like :
if (conn == NULL) {
return ERR_VAL;
}
Perhaps an ASSERT about that ?
|
Wed 07 Mar 2007 08:57:03 AM UTC, comment #1:
Is this designed to catch misbehaving applications, or to prevent applications using a feature that we don't support?
If the former, I'm inclined to leave as is: to make the sockets layer completely robust to misbehaving applications would be a lot of work and a lot of code.
If it's to protect against stuff we don't support, then perhaps it would be OK.
|
Wed 07 Mar 2007 07:36:04 AM UTC, original submission:
I have attached the patch, which check namelen and sin_family in the aformentioned functions
if ((namelen != sizeof(struct sockaddr_in)) ||
((((struct sockaddr_in *)name)->sin_family) != AF_INET)) {
sock_set_errno(sock, EINVAL);
return -1;
}
|