You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/02/07 08:27:01 UTC
[GitHub] [incubator-nuttx] anchao opened a new pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
anchao opened a new pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434
## Summary
net/udp: remove psock hook to avoid invalid reference
net/tcp: remove the socket hook reference from netdev callback
net/inet: move socket timestamp into socket_conn_s
net/inet: move socket linger into socket_conn_s
net/inet: move recv/send timeout into socket_conn_s
net/inet: move socket options into socket_conn_s
net/inet: move socket error into socket_conn_s
net/inet: move socket flags into socket_conn_s
net/socket: remove unused _SF_CLOSED
net/socket: remove unused _SF_INITD
net/ieee802154: replace the common connect prologue
net/can: replace the common connect prologue
net/bluetooth: replace the common connect prologue
net/tcp: replace the common connect prologue
net/local: replace the common connect prologue
net/local: remove duplicate assert condition
net/pkt: replace the common connect prologue
net/icmpv6: replace the common connect prologue
net/udp: replace the common connect prologue
net/icmp: replace the common connect prologue
net/usrsock: replace the common connect prologue
net/netlink: replace the common connect prologue
net/conn: complete the list_tail member
net/tcp/udp: move the send callback into tcp/udp structure
## Impact
tcp/udp stack
## Testing
on going
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r801706570
##########
File path: net/ieee802154/ieee802154_sockif.c
##########
@@ -375,15 +375,17 @@ static int ieee802154_bind(FAR struct socket *psock,
/* Bind a PF_IEEE802154 socket to an network device. */
- if (psock->s_type != SOCK_DGRAM)
+ if (ppsock->s_conn == NULL || sock->s_type != SOCK_DGRAM)
Review comment:
~ppsock~
psock
Or (better) move "conn = (FAR struct ieee802154_conn_s *)psock->s_conn;" from line 384 before line 378.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
a-lunev commented on pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#issuecomment-1032857749
For some reason ci fails for esp32-devkitc/wapi config with the following error (I have seen that already the 3-rd time):
`gzip: stdin: not in gzip format`
Is it some network stability issue on ci server?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r804555083
##########
File path: net/rpmsg/rpmsg_sockif.c
##########
@@ -782,7 +786,7 @@ static int rpmsg_socket_accept(FAR struct socket *psock,
}
else
{
- if (_SS_ISNONBLOCK(psock->s_flags))
+ if (_SS_ISNONBLOCK(conn->sconn.s_flags))
Review comment:
Here in "else" block the conn variable is NULL.
_SS_ISNONBLOCK(conn->sconn.s_flags) results in crash.
(It's another issue, not related to telnetd of course).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
a-lunev commented on pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#issuecomment-1031535586
> 1. Remove the hook reference to psock in the tcp/udp protocol stack:
> ...
> 2. Unify the semantics of socket option for connection:
> ...
@anchao if it is possible, please split the patch into two separate ones.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
a-lunev commented on pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#issuecomment-1035687632
The same is on sim:tcpblaster config.
telnetd does not work.
Though, if on NuttX I run netcat server instead of telnetd, telnet client normally connects from Linux to NuttX:
`nsh> netcat -l 23`
@anchao telnetd uses task_create() (whereas netcat does not use any commands that may result in calling dup() / dup2()). It looks like the issue is related to task_create() / dup() / dup2().
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r800868289
##########
File path: net/udp/udp.h
##########
@@ -99,14 +100,11 @@ struct udp_conn_s
{
/* Common prologue of all connection structures. */
- dq_entry_t node; /* Supports a doubly linked list */
-
/* This is a list of UDP connection callbacks. Each callback represents
* a thread that is stalled, waiting for a device-specific event.
*/
Review comment:
ditto
##########
File path: net/icmp/icmp.h
##########
@@ -77,14 +78,11 @@ struct icmp_conn_s
{
/* Common prologue of all connection structures. */
- dq_entry_t node; /* Supports a doubly linked list */
-
/* This is a list of ICMP callbacks. Each callback represents a thread
* that is stalled, waiting for a device-specific event.
*/
Review comment:
remove the stale comment
##########
File path: net/udp/udp.h
##########
@@ -147,6 +147,10 @@ struct udp_conn_s
sq_queue_t write_q; /* Write buffering for UDP packets */
FAR struct net_driver_s *dev; /* Last device */
+
+ /* Callback instance for UDP sendto() */
+
+ FAR struct devif_callback_s *sndcb;
Review comment:
should we guard this field by CONFIG_NET_UDP_WRITE_BUFFERS?
##########
File path: net/usrsock/usrsock.h
##########
@@ -83,14 +83,11 @@ struct usrsock_conn_s
{
/* Common prologue of all connection structures. */
- dq_entry_t node; /* Supports a doubly linked list */
-
/* This is a list of usrsock callbacks. Each callback represents a thread
* that is stalled, waiting for a specific event.
*/
Review comment:
remove the stale comment
##########
File path: net/pkt/pkt.h
##########
@@ -55,14 +57,11 @@ struct pkt_conn_s
{
/* Common prologue of all connection structures. */
- dq_entry_t node; /* Supports a double linked list */
-
/* This is a list of Pkt connection callbacks. Each callback represents
* a thread that is stalled, waiting for a device-specific event.
Review comment:
ditto
##########
File path: net/local/local.h
##########
@@ -118,20 +118,11 @@ struct local_conn_s
{
/* Common prologue of all connection structures. */
- /* lc_node supports a doubly linked list: Listening SOCK_STREAM servers
- * will be linked into a list of listeners; SOCK_STREAM clients will be
- * linked to the lc_conn lists.
- */
-
- dq_entry_t lc_node; /* Supports a doubly linked list */
-
/* This is a list of Local connection callbacks. Each callback represents
Review comment:
remove
##########
File path: net/bluetooth/bluetooth.h
##########
@@ -78,14 +80,11 @@ struct bluetooth_conn_s
{
/* Common prologue of all connection structures. */
- dq_entry_t bc_node; /* Supports a doubly linked list */
-
/* This is a list of Bluetooth callbacks. Each callback represents
* a thread that is stalled, waiting for a device-specific event.
*/
Review comment:
ditto
##########
File path: include/nuttx/net/net.h
##########
@@ -208,6 +208,10 @@ struct socket_conn_s
FAR struct devif_callback_s *list_tail;
/* Connection-specific content may follow */
+
+ /* Definitions of 8-bit socket flags */
+
+ uint8_t s_flags; /* See _SF_* definitions */
Review comment:
move before line 210
##########
File path: net/inet/inet_sockif.c
##########
@@ -718,7 +718,8 @@ static int inet_connect(FAR struct socket *psock,
{
/* Verify that the socket is not already connected */
- if (_SS_ISCONNECTED(psock->s_flags))
+ if (_SS_ISCONNECTED(((FAR struct socket_conn_s *)
+ psock->s_conn)->s_flags))
Review comment:
let's add a temp conn variable to avoid the cast repeatly
##########
File path: net/icmpv6/icmpv6.h
##########
@@ -79,14 +80,11 @@ struct icmpv6_conn_s
{
/* Common prologue of all connection structures. */
- dq_entry_t node; /* Supports a double linked list */
-
/* This is a list of ICMPV6 callbacks. Each callback represents a thread
* that is stalled, waiting for a device-specific event.
*/
Review comment:
ditto
##########
File path: net/bluetooth/bluetooth_sockif.c
##########
@@ -439,6 +439,7 @@ static int bluetooth_l2cap_bind(FAR struct socket *psock,
FAR const struct sockaddr_l2 *iaddr,
socklen_t addrlen)
{
+ FAR struct socket_conn_s *sconn = psock->s_conn;
Review comment:
should we change all sconn to conn to unity the term?
##########
File path: net/ieee802154/ieee802154.h
##########
@@ -97,14 +99,11 @@ struct ieee802154_conn_s
{
/* Common prologue of all connection structures. */
- dq_entry_t node; /* Supports a double linked list */
-
/* This is a list of IEEE 802.15.4 callbacks. Each callback represents
* a thread that is stalled, waiting for a device-specific event.
*/
Review comment:
ditto
##########
File path: net/local/local_accept.c
##########
@@ -181,8 +181,8 @@ int local_accept(FAR struct socket *psock, FAR struct sockaddr *addr,
* block.
*/
- ret = local_open_server_tx(conn,
- _SS_ISNONBLOCK(psock->s_flags));
+ ret = local_open_server_tx(
+ conn, _SS_ISNONBLOCK(conn->sconn.s_flags));
Review comment:
why change the format?
##########
File path: net/local/local_accept.c
##########
@@ -201,8 +201,8 @@ int local_accept(FAR struct socket *psock, FAR struct sockaddr *addr,
* for writing.
*/
- ret = local_open_server_rx(conn,
- _SS_ISNONBLOCK(psock->s_flags));
+ ret = local_open_server_rx(
+ conn, _SS_ISNONBLOCK(conn->sconn.s_flags));
Review comment:
ditto
##########
File path: net/udp/udp_sendto_unbuffered.c
##########
@@ -260,17 +260,26 @@ ssize_t psock_udp_sendto(FAR struct socket *psock, FAR const void *buf,
size_t len, int flags,
FAR const struct sockaddr *to, socklen_t tolen)
{
- FAR struct udp_conn_s *conn;
+ FAR struct udp_conn_s *conn = psock->s_conn;
struct sendto_s state;
int ret;
+ /* Verify that the sockfd corresponds to valid, allocated socket */
+
+ if (psock == NULL || psock->s_type != SOCK_DGRAM ||
Review comment:
why add check here?
##########
File path: net/udp/udp_sendto_buffered.c
##########
@@ -539,19 +539,23 @@ ssize_t psock_udp_sendto(FAR struct socket *psock, FAR const void *buf,
size_t len, int flags,
FAR const struct sockaddr *to, socklen_t tolen)
{
- FAR struct udp_conn_s *conn;
+ FAR struct udp_conn_s *conn = (FAR struct udp_conn_s *)psock->s_conn;
FAR struct udp_wrbuffer_s *wrb;
bool nonblock;
bool empty;
int ret = OK;
+ /* Get the underlying the UDP connection structure. */
+
+ DEBUGASSERT(conn);
Review comment:
conn = (FAR struct udp_conn_s *)psock->s_conn;
DEBUGASSERT(conn);
and revert the change at line 455
##########
File path: net/socket/socket.h
##########
@@ -95,9 +95,9 @@
# define _SO_SETERRNO(s,e) \
do \
{ \
- if (s != NULL) \
+ if (s != NULL && (s)->s_conn != NULL) \
{ \
- s->s_error = (int16_t)e; \
+ ((FAR struct socket_conn_s *)s->s_conn)->s_error = (int16_t)e; \
Review comment:
let' add local variable
##########
File path: net/socket/net_sendfile.c
##########
@@ -113,7 +114,6 @@ ssize_t psock_sendfile(FAR struct socket *psock, FAR struct file *infile,
if (psock == NULL || psock->s_conn == NULL)
Review comment:
change psock->s_conn to sconn
##########
File path: net/tcp/tcp_monitor.c
##########
@@ -161,11 +159,14 @@ static uint16_t tcp_monitor_event(FAR struct net_driver_s *dev,
/* Clear the socket error */
- _SO_SETERRNO(psock, OK);
Review comment:
let's change _SO_SETERRNO argument from socket to socket_conn_s
##########
File path: net/can/can.h
##########
@@ -75,15 +76,12 @@ struct can_conn_s
{
/* Common prologue of all connection structures. */
- dq_entry_t node; /* Supports a doubly linked list */
-
- /* This is a list of NetLink connection callbacks. Each callback
+ /* This is a list of Can connection callbacks. Each callback
* represents a thread that is stalled, waiting for a device-specific
* event.
*/
Review comment:
ditto
##########
File path: net/local/local_sockif.c
##########
@@ -517,7 +517,8 @@ static int local_connect(FAR struct socket *psock,
{
/* Verify that the socket is not already connected */
- if (_SS_ISCONNECTED(psock->s_flags))
+ if (_SS_ISCONNECTED(((FAR struct socket_conn_s *)
+ psock->s_conn)->s_flags))
Review comment:
add temp conn variable
##########
File path: net/local/local_connect.c
##########
@@ -311,8 +311,8 @@ int psock_local_connect(FAR struct socket *psock,
if (conn->lc_proto == SOCK_STREAM)
{
ret =
- local_stream_connect(client, conn,
- _SS_ISNONBLOCK(psock->s_flags));
+ local_stream_connect(
+ client, conn, _SS_ISNONBLOCK(conn->sconn.s_flags));
Review comment:
ditto
##########
File path: net/socket/accept.c
##########
@@ -147,7 +148,8 @@ int psock_accept(FAR struct socket *psock, FAR struct sockaddr *addr,
/* Mark the new socket as connected. */
- newsock->s_flags |= _SF_CONNECTED;
+ ((FAR struct socket_conn_s *)
Review comment:
sconn = newsock->s_conn;
sconn->s_flags |= _SF_CONNECTED;
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -452,24 +452,23 @@ static uint16_t sendfile_eventhandler(FAR struct net_driver_s *dev,
ssize_t tcp_sendfile(FAR struct socket *psock, FAR struct file *infile,
FAR off_t *offset, size_t count)
{
- FAR struct tcp_conn_s *conn;
+ FAR struct tcp_conn_s *conn = (FAR struct tcp_conn_s *)psock->s_conn;
struct sendfile_s state;
off_t startpos;
int ret;
+ DEBUGASSERT(conn != NULL);
Review comment:
conn = (FAR struct udp_conn_s *)psock->s_conn;
DEBUGASSERT(conn);
and revert the change at line 455
##########
File path: net/tcp/tcp_netpoll.c
##########
@@ -129,11 +129,14 @@ static uint16_t tcp_poll_eventhandler(FAR struct net_driver_s *dev,
reason = ECONNREFUSED;
}
- _SO_SETERRNO(info->psock, reason);
Review comment:
ditto
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r801229491
##########
File path: include/nuttx/net/net.h
##########
@@ -208,6 +208,10 @@ struct socket_conn_s
FAR struct devif_callback_s *list_tail;
/* Connection-specific content may follow */
+
+ /* Definitions of 8-bit socket flags */
+
+ uint8_t s_flags; /* See _SF_* definitions */
Review comment:
done
##########
File path: net/icmp/icmp.h
##########
@@ -77,14 +78,11 @@ struct icmp_conn_s
{
/* Common prologue of all connection structures. */
- dq_entry_t node; /* Supports a doubly linked list */
-
/* This is a list of ICMP callbacks. Each callback represents a thread
* that is stalled, waiting for a device-specific event.
*/
Review comment:
done
##########
File path: net/local/local_accept.c
##########
@@ -181,8 +181,8 @@ int local_accept(FAR struct socket *psock, FAR struct sockaddr *addr,
* block.
*/
- ret = local_open_server_tx(conn,
- _SS_ISNONBLOCK(psock->s_flags));
+ ret = local_open_server_tx(
+ conn, _SS_ISNONBLOCK(conn->sconn.s_flags));
Review comment:
warning by nxstyle check
##########
File path: net/bluetooth/bluetooth_sockif.c
##########
@@ -439,6 +439,7 @@ static int bluetooth_l2cap_bind(FAR struct socket *psock,
FAR const struct sockaddr_l2 *iaddr,
socklen_t addrlen)
{
+ FAR struct socket_conn_s *sconn = psock->s_conn;
Review comment:
> And let's add DEBUGASSERT(conn != NULL).
done
> should we change all sconn to conn to unity the term?
I think sconn is better, “conn” is a common naming in most files
##########
File path: net/udp/udp_sendto_unbuffered.c
##########
@@ -260,17 +260,26 @@ ssize_t psock_udp_sendto(FAR struct socket *psock, FAR const void *buf,
size_t len, int flags,
FAR const struct sockaddr *to, socklen_t tolen)
{
- FAR struct udp_conn_s *conn;
+ FAR struct udp_conn_s *conn = psock->s_conn;
struct sendto_s state;
int ret;
+ /* Verify that the sockfd corresponds to valid, allocated socket */
+
+ if (psock == NULL || psock->s_type != SOCK_DGRAM ||
Review comment:
sanity check copy from tcp
##########
File path: net/inet/inet_sockif.c
##########
@@ -718,7 +718,8 @@ static int inet_connect(FAR struct socket *psock,
{
/* Verify that the socket is not already connected */
- if (_SS_ISCONNECTED(psock->s_flags))
+ if (_SS_ISCONNECTED(((FAR struct socket_conn_s *)
+ psock->s_conn)->s_flags))
Review comment:
done
##########
File path: net/tcp/tcp_monitor.c
##########
@@ -161,11 +159,14 @@ static uint16_t tcp_monitor_event(FAR struct net_driver_s *dev,
/* Clear the socket error */
- _SO_SETERRNO(psock, OK);
Review comment:
ok, I will plan to improve the socket error handling on next patch, let us keep the current behavior first
##########
File path: net/udp/udp.h
##########
@@ -147,6 +147,10 @@ struct udp_conn_s
sq_queue_t write_q; /* Write buffering for UDP packets */
FAR struct net_driver_s *dev; /* Last device */
+
+ /* Callback instance for UDP sendto() */
+
+ FAR struct devif_callback_s *sndcb;
Review comment:
yes, which guard around line 141-150
https://github.com/apache/incubator-nuttx/pull/5434/files/dabe08a118f0d03b24d0bde0317bd2d83742eba9#diff-1597ffbc1b34e3e6e48bbcdaff385c423ab96b77e2c0ea7ba012c4c0a173094eL141-L150
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#issuecomment-1032227143
> If so, let's move these two commits to separate PRs and consider / merge them first. It is important to decompose the
> complexity of the huge patch to reduce risk of breaking / missing something. And it will be easier to catch / isolate potential
> issues if they are merged not at once.
@a-lunev I have split these 2 commits your mentioned in this PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#issuecomment-1031431962
> HI @anchao could you please describe better what are these invalid references? When it happen? What are these hooks for? BTW, since these are same logic modifications, please squash these commits.
Yes, I will prepare the demo to clarify this issue, give me some time until I mark this PR as ready for review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r804580141
##########
File path: net/rpmsg/rpmsg_sockif.c
##########
@@ -782,7 +786,7 @@ static int rpmsg_socket_accept(FAR struct socket *psock,
}
else
{
- if (_SS_ISNONBLOCK(psock->s_flags))
+ if (_SS_ISNONBLOCK(conn->sconn.s_flags))
Review comment:
@a-lunev
Please review, thank you!
https://github.com/apache/incubator-nuttx/pull/5466
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r804569151
##########
File path: net/rpmsg/rpmsg_sockif.c
##########
@@ -782,7 +786,7 @@ static int rpmsg_socket_accept(FAR struct socket *psock,
}
else
{
- if (_SS_ISNONBLOCK(psock->s_flags))
+ if (_SS_ISNONBLOCK(conn->sconn.s_flags))
Review comment:
Yes, I will prepare a fix for review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r801259270
##########
File path: net/bluetooth/bluetooth_sockif.c
##########
@@ -439,6 +439,7 @@ static int bluetooth_l2cap_bind(FAR struct socket *psock,
FAR const struct sockaddr_l2 *iaddr,
socklen_t addrlen)
{
+ FAR struct socket_conn_s *sconn = psock->s_conn;
Review comment:
> > And let's add DEBUGASSERT(conn != NULL).
>
> done
>
> > should we change all sconn to conn to unity the term?
>
> I think sconn is better, “conn” is a common naming in most files
Otherwise, there is a confusion with tcp_conn_s.sconn field that appears now in many places. I think conn is better when psock->s_conn is referred.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#issuecomment-1031493307
> HI @anchao could you please describe better what are these invalid references? When it happen? What are these hooks for? BTW, since these are same logic modifications, please squash these commits.
@acassis
I updated the comments in summary and provided 2 demos, please review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis merged pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
acassis merged pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r801706570
##########
File path: net/ieee802154/ieee802154_sockif.c
##########
@@ -375,15 +375,17 @@ static int ieee802154_bind(FAR struct socket *psock,
/* Bind a PF_IEEE802154 socket to an network device. */
- if (psock->s_type != SOCK_DGRAM)
+ if (ppsock->s_conn == NULL || sock->s_type != SOCK_DGRAM)
Review comment:
~ppsock~
psock
Or move "conn = (FAR struct ieee802154_conn_s *)psock->s_conn;" from line 384 before line 378.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#issuecomment-1035253435
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#issuecomment-1031640454
> @anchao if it is possible, please split the patch into two separate ones.
@a-lunev
These 2 issues depends on the "common connect prologue" change. I tried to separate the related commit, but which could not pass the CI check at the same time, the commit before submitted to the mainline, each protocols needs to pass the verification from tester, therefore, the relevant commit of this PR is split in detail
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r801717631
##########
File path: net/ieee802154/ieee802154_sockif.c
##########
@@ -375,15 +375,17 @@ static int ieee802154_bind(FAR struct socket *psock,
/* Bind a PF_IEEE802154 socket to an network device. */
- if (psock->s_type != SOCK_DGRAM)
+ if (ppsock->s_conn == NULL || sock->s_type != SOCK_DGRAM)
Review comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#issuecomment-1035872803
> > @anchao
> > Hmm, my automatic test failed now with this PR.
> > I can not telnet to lm3s6965-ek:discover with QEMU.
>
> spresense:rndis_smp also does not work.
@masayuki2009 @a-lunev
sorry for the break, there are some issues with the tcp monitor handling, I have a quick fix for this case:
https://github.com/apache/incubator-nuttx/pull/5462
but there is still need some work to improve further, I will do more sanity verification before update
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r801256879
##########
File path: net/bluetooth/bluetooth_sockif.c
##########
@@ -512,6 +513,7 @@ static int bluetooth_hci_bind(FAR struct socket *psock,
FAR const struct sockaddr_hci *hciaddr,
socklen_t addrlen)
{
+ FAR struct socket_conn_s *sconn = psock->s_conn;
Review comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r801211100
##########
File path: net/bluetooth/bluetooth_sockif.c
##########
@@ -512,6 +513,7 @@ static int bluetooth_hci_bind(FAR struct socket *psock,
FAR const struct sockaddr_hci *hciaddr,
socklen_t addrlen)
{
+ FAR struct socket_conn_s *sconn = psock->s_conn;
Review comment:
Let's add DEBUGASSERT(conn != NULL) (and in the rest similar places).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r801210182
##########
File path: net/bluetooth/bluetooth_sockif.c
##########
@@ -439,6 +439,7 @@ static int bluetooth_l2cap_bind(FAR struct socket *psock,
FAR const struct sockaddr_l2 *iaddr,
socklen_t addrlen)
{
+ FAR struct socket_conn_s *sconn = psock->s_conn;
Review comment:
And let's add DEBUGASSERT(conn != NULL).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r801257421
##########
File path: net/udp/udp_sendto_buffered.c
##########
@@ -394,10 +389,9 @@ static uint16_t sendto_eventhandler(FAR struct net_driver_s *dev,
FAR void *pvconn, FAR void *pvpriv,
uint16_t flags)
{
- FAR struct udp_conn_s *conn = (FAR struct udp_conn_s *)pvconn;
- FAR struct socket *psock = (FAR struct socket *)pvpriv;
+ FAR struct udp_conn_s *conn = (FAR struct udp_conn_s *)pvpriv;
Review comment:
Thanks for reminding, There are too many places to use pvconn, I do not want to add the same comment everywhere, I will plan to remove the pvconn parameter on next patch,
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 edited a comment on pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
masayuki2009 edited a comment on pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#issuecomment-1035600651
@anchao
Hmm, my automatic test failed now with this PR.
I can not telnet to lm3s6965-ek:discover with QEMU.
```
$ ~/opensource/QEMU/qemu-5.2/build/arm-softmmu/qemu-system-arm -net nic,model=stellaris -net user,hostfwd=tcp:127.0.0.1:20023-10.0.2.15:23,hostfwd=tcp:127.0.0.1:20021-10.0.2.15:21,hostfwd=tcp:127.0.0.1:25001-10.0.2.15:5001 -M lm3s6965evb -kernel nuttx -nographic
NuttShell (NSH) NuttX-10.2.0
nsh> uname -a
NuttX 10.2.0 eac13a113d Feb 11 2022 08:35:34 arm lm3s6965-ek
nsh> ps
PID GROUP PRI POLICY TYPE NPX STATE EVENT SIGMASK STACK USED FILLED COMMAND
0 0 0 FIFO Kthread N-- Ready 00000000 001000 000392 39.2% Idle Task
1 1 224 RR Kthread --- Waiting Semaphore 00000000 002016 000200 9.9% hpwork 0x200005f0
2 2 60 RR Kthread --- Waiting Semaphore 00000000 002016 000200 9.9% lpwork 0x200005fc
3 3 100 RR Task --- Running 00000000 002024 001024 50.5% nsh_main
4 4 100 RR Task --- Waiting Semaphore 00000000 002008 000432 21.5% Telnet daemon 0x200064c0
nsh> free
total used free largest nused nfree
Umem: 50496 15216 35280 35280 53 1
nsh> ifconfig
eth0 Link encap:Ethernet HWaddr 00:e0:de:ad:be:ef at UP
inet addr:10.0.0.2 DRaddr:10.0.0.1 Mask:255.255.255.0
IPv4 TCP UDP ICMP
Received 0000 0000 0000 0000
Dropped 0000 0000 0000 0000
IPv4 VHL: 0000 Frg: 0000
Checksum 0000 0000 0000 ----
TCP ACK: 0000 SYN: 0000
RST: 0000 0000
Type 0000 ---- ---- 0000
Sent 0000 0000 0000 0000
Rexmit ---- 0000 ---- ----
nsh> renew eth0
nsh> ifconfig
eth0 Link encap:Ethernet HWaddr 00:e0:de:ad:be:ef at UP
inet addr:10.0.2.15 DRaddr:10.0.2.2 Mask:255.255.255.0
IPv4 TCP UDP ICMP
Received 0002 0000 0002 0000
Dropped 0000 0000 0000 0000
IPv4 VHL: 0000 Frg: 0000
Checksum 0000 0000 0000 ----
TCP ACK: 0000 SYN: 0000
RST: 0000 0000
Type 0000 ---- ---- 0000
Sent 0002 0000 0002 0000
Rexmit ---- 0000 ---- ----
nsh>
$ telnet localhost 20023
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r801714311
##########
File path: net/ieee802154/ieee802154_sockif.c
##########
@@ -381,9 +381,11 @@ static int ieee802154_bind(FAR struct socket *psock,
return -EBADF;
}
+ conn = (FAR struct ieee802154_conn_s *)psock->s_conn;
Review comment:
Good. Let it be some future PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r804624905
##########
File path: net/rpmsg/rpmsg_sockif.c
##########
@@ -782,7 +786,7 @@ static int rpmsg_socket_accept(FAR struct socket *psock,
}
else
{
- if (_SS_ISNONBLOCK(psock->s_flags))
+ if (_SS_ISNONBLOCK(conn->sconn.s_flags))
Review comment:
Reviewed, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r801256694
##########
File path: net/tcp/tcp_close.c
##########
@@ -64,7 +64,7 @@ static uint16_t tcp_close_eventhandler(FAR struct net_driver_s *dev,
uint16_t flags)
{
FAR struct tcp_close_s *pstate = (FAR struct tcp_close_s *)pvpriv;
- FAR struct tcp_conn_s *conn = (FAR struct tcp_conn_s *)pvconn;
+ FAR struct tcp_conn_s *conn = pstate->cl_conn;
Review comment:
Thanks for reminding, There are too many places to use pvconn, I do not want to add the same comment everywhere, I will plan to remove the pvconn parameter on next patch,
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r801359853
##########
File path: net/udp/udp_sendto_unbuffered.c
##########
@@ -260,17 +260,26 @@ ssize_t psock_udp_sendto(FAR struct socket *psock, FAR const void *buf,
size_t len, int flags,
FAR const struct sockaddr *to, socklen_t tolen)
{
- FAR struct udp_conn_s *conn;
+ FAR struct udp_conn_s *conn = psock->s_conn;
Review comment:
move after line 275
##########
File path: net/tcp/tcp_connect.c
##########
@@ -219,15 +219,18 @@ static uint16_t psock_connect_eventhandler(FAR struct net_driver_s *dev,
else if ((flags & TCP_CONNECTED) != 0)
{
FAR struct socket *psock = pstate->tc_psock;
+ FAR struct socket_conn_s *sconn;
Review comment:
sconn
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r804555083
##########
File path: net/rpmsg/rpmsg_sockif.c
##########
@@ -782,7 +786,7 @@ static int rpmsg_socket_accept(FAR struct socket *psock,
}
else
{
- if (_SS_ISNONBLOCK(psock->s_flags))
+ if (_SS_ISNONBLOCK(conn->sconn.s_flags))
Review comment:
Here in "else" block the conn variable is NULL.
_SS_ISNONBLOCK(conn->sconn.s_flags) results in crash.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#issuecomment-1035646744
>@anchao
>Hmm, my automatic test failed now with this PR.
>I can not telnet to lm3s6965-ek:discover with QEMU.
spresense:rndis_smp also does not work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r801574906
##########
File path: net/ieee802154/ieee802154_sockif.c
##########
@@ -381,9 +381,11 @@ static int ieee802154_bind(FAR struct socket *psock,
return -EBADF;
}
+ conn = (FAR struct ieee802154_conn_s *)psock->s_conn;
Review comment:
There are still many places where psock->s_conn is introduced w/o DEBUGASSERT(conn != NULL). Do you think the debug asserts are not reasonable for them?
##########
File path: net/socket/socket.c
##########
@@ -129,16 +119,22 @@ int psock_socket(int domain, int type, int protocol,
psock->s_sockif = sockif;
ret = sockif->si_setup(psock, protocol);
- if (ret < 0)
+ }
+
+ if (ret >= 0)
+ {
+ FAR struct socket_conn_s *conn = psock->s_conn;
+
+ if (type & SOCK_NONBLOCK)
{
- nerr("ERROR: socket si_setup() failed: %d\n", ret);
- return ret;
+ conn->s_flags |= _SF_NONBLOCK;
}
- }
- /* The socket has been successfully initialized */
+ /* The socket has been successfully initialized */
+
+ conn->s_flags |= _SF_INITD;
+ }
Review comment:
Is nerr() lost here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r801696537
##########
File path: net/ieee802154/ieee802154_sockif.c
##########
@@ -381,9 +381,11 @@ static int ieee802154_bind(FAR struct socket *psock,
return -EBADF;
}
+ conn = (FAR struct ieee802154_conn_s *)psock->s_conn;
Review comment:
Done, it is reasonable, but the similar issue existed before my commit, this PR is not intended to resolve this issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r801791981
##########
File path: net/udp/udp_sendto_unbuffered.c
##########
@@ -260,17 +260,26 @@ ssize_t psock_udp_sendto(FAR struct socket *psock, FAR const void *buf,
size_t len, int flags,
FAR const struct sockaddr *to, socklen_t tolen)
{
- FAR struct udp_conn_s *conn;
+ FAR struct udp_conn_s *conn = psock->s_conn;
Review comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r801281895
##########
File path: net/ieee802154/ieee802154_sockif.c
##########
@@ -356,6 +356,7 @@ static int ieee802154_bind(FAR struct socket *psock,
FAR const struct sockaddr *addr,
socklen_t addrlen)
{
+ FAR struct socket_conn_s *sconn = psock->s_conn;
Review comment:
reuse conn at line 362
##########
File path: net/socket/setsockopt.c
##########
@@ -75,6 +75,8 @@ static int psock_socketlevel_option(FAR struct socket *psock, int option,
FAR const void *value,
socklen_t value_len)
{
+ FAR struct socket_conn_s *sconn = psock->s_conn;
Review comment:
sconn->conn
##########
File path: net/socket/net_sendfile.c
##########
@@ -104,6 +104,7 @@
ssize_t psock_sendfile(FAR struct socket *psock, FAR struct file *infile,
FAR off_t *offset, size_t count)
{
+ FAR struct socket_conn_s *sconn = psock->s_conn;
Review comment:
sconn->conn
##########
File path: fs/procfs/fs_procfsproc.c
##########
@@ -1177,11 +1177,11 @@ static ssize_t proc_groupfd(FAR struct proc_file_s *procfile,
if (file->f_inode && INODE_IS_SOCKET(file->f_inode))
{
FAR struct socket *socket = file->f_priv;
+ FAR struct socket_conn_s *sconn = socket->s_conn;
Review comment:
sconn->conn
##########
File path: net/inet/ipv6_getpeername.c
##########
@@ -69,6 +69,7 @@
int ipv6_getpeername(FAR struct socket *psock, FAR struct sockaddr *addr,
FAR socklen_t *addrlen)
{
+ FAR struct socket_conn_s *sconn = psock->s_conn;
Review comment:
sconn->conn
##########
File path: net/inet/ipv4_getpeername.c
##########
@@ -71,6 +71,7 @@ int ipv4_getpeername(FAR struct socket *psock, FAR struct sockaddr *addr,
{
#if defined(NET_TCP_HAVE_STACK) || defined(NET_UDP_HAVE_STACK)
FAR struct sockaddr_in *outaddr = (FAR struct sockaddr_in *)addr;
+ FAR struct socket_conn_s *sconn = psock->s_conn;
Review comment:
sconn->conn
##########
File path: net/inet/inet_sockif.c
##########
@@ -1069,6 +1071,9 @@ static int inet_poll(FAR struct socket *psock, FAR struct pollfd *fds,
static ssize_t inet_send(FAR struct socket *psock, FAR const void *buf,
size_t len, int flags)
{
+#ifdef NET_UDP_HAVE_STACK
+ FAR struct socket_conn_s *sconn = psock->s_conn;
Review comment:
sconn->conn
##########
File path: net/inet/inet_sockif.c
##########
@@ -716,9 +716,11 @@ static int inet_connect(FAR struct socket *psock,
#if defined(CONFIG_NET_TCP) && defined(NET_TCP_HAVE_STACK)
case SOCK_STREAM:
{
+ FAR struct socket_conn_s *sconn = psock->s_conn;
Review comment:
sconn->conn
##########
File path: net/netlink/netlink_sockif.c
##########
@@ -756,6 +756,7 @@ static ssize_t netlink_sendmsg(FAR struct socket *psock,
static ssize_t netlink_recvmsg(FAR struct socket *psock,
FAR struct msghdr *msg, int flags)
{
+ FAR struct socket_conn_s *sconn = psock->s_conn;
Review comment:
sconn->conn
##########
File path: net/local/local_sockif.c
##########
@@ -515,9 +515,11 @@ static int local_connect(FAR struct socket *psock,
#ifdef CONFIG_NET_LOCAL_STREAM
case SOCK_STREAM:
{
+ FAR struct socket_conn_s *sconn = psock->s_conn;
Review comment:
sconn->conn
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
a-lunev commented on pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#issuecomment-1031880176
> > @anchao if it is possible, please split the patch into two separate ones.
>
> @a-lunev
>
> These 2 issues depends on the "common connect prologue" change. I tried to separate the related commit, but which could not pass the CI check at the same time, the commit before submitted to the mainline, each protocols needs to pass the verification from tester, therefore, the relevant commit of this PR is split in detail
I have preliminary looked through the 24 commits. The following two commits seem independent of PR #5434:
commit 871650b558781434fec0430d21df913214595db8
net/socket: remove unused _SF_INITD
commit c8dcb02727b0b06746c026ba930e7167eb012abd
net/socket: remove unused _SF_CLOSED
If so, let's move these two commits to separate PRs and consider / merge them first. It is important to decompose the complexity of the huge patch to reduce risk of breaking / missing something. And it will be easier to catch / isolate potential issues if they are merged not at once.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#issuecomment-1035600651
@anchao
Hmm, my automatic test failed now with this PR.
I can not telnet to lm3s6965-ek:discover with QEMU.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r801259184
##########
File path: net/usrsock/usrsock_recvmsg.c
##########
@@ -302,7 +302,8 @@ ssize_t usrsock_recvmsg(FAR struct socket *psock, FAR struct msghdr *msg,
if (!(conn->flags & USRSOCK_EVENT_RECVFROM_AVAIL))
{
- if (_SS_ISNONBLOCK(psock->s_flags) || (flags & MSG_DONTWAIT) != 0)
+ if (_SS_ISNONBLOCK(conn->sconn.s_flags) ||
+ (flags & MSG_DONTWAIT) != 0)
Review comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r801259270
##########
File path: net/bluetooth/bluetooth_sockif.c
##########
@@ -439,6 +439,7 @@ static int bluetooth_l2cap_bind(FAR struct socket *psock,
FAR const struct sockaddr_l2 *iaddr,
socklen_t addrlen)
{
+ FAR struct socket_conn_s *sconn = psock->s_conn;
Review comment:
> > should we change all sconn to conn to unity the term?
>
> I think sconn is better, “conn” is a common naming in most files
Otherwise, there is a confusion with tcp_conn_s.sconn field that appears now in many places. I think conn is better when psock->s_conn is referred.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#issuecomment-1031400459
HI @anchao could you please describe better what are these invalid references? When it happen? What are these hooks for? BTW, since these are same logic modifications, please squash these commits.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r801211100
##########
File path: net/bluetooth/bluetooth_sockif.c
##########
@@ -512,6 +513,7 @@ static int bluetooth_hci_bind(FAR struct socket *psock,
FAR const struct sockaddr_hci *hciaddr,
socklen_t addrlen)
{
+ FAR struct socket_conn_s *sconn = psock->s_conn;
Review comment:
Let's add DEBUGASSERT(conn != NULL).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r801251928
##########
File path: net/usrsock/usrsock_sendmsg.c
##########
@@ -283,7 +283,8 @@ ssize_t usrsock_sendmsg(FAR struct socket *psock,
if (!(conn->flags & USRSOCK_EVENT_SENDTO_READY))
{
- if (_SS_ISNONBLOCK(psock->s_flags) || (flags & MSG_DONTWAIT) != 0)
+ if (_SS_ISNONBLOCK(conn->sconn.s_flags) ||
+ (flags & MSG_DONTWAIT) != 0)
Review comment:
Why not the following formatting?
```
if (_SS_ISNONBLOCK(conn->sconn.s_flags) ||
(flags & MSG_DONTWAIT) != 0)
```
##########
File path: net/tcp/tcp_close.c
##########
@@ -64,7 +64,7 @@ static uint16_t tcp_close_eventhandler(FAR struct net_driver_s *dev,
uint16_t flags)
{
FAR struct tcp_close_s *pstate = (FAR struct tcp_close_s *)pvpriv;
- FAR struct tcp_conn_s *conn = (FAR struct tcp_conn_s *)pvconn;
+ FAR struct tcp_conn_s *conn = pstate->cl_conn;
Review comment:
Let's document here:
```
/* FAR struct tcp_conn_s *conn = (FAR struct tcp_conn_s *)pvconn;
*
* Do not use pvconn argument to get the TCP connection pointer (the above
* commented line) because pvconn is normally NULL for some events like
* NETDEV_DOWN. Instead, the TCP connection pointer can be reliably
* obtained from the corresponding TCP socket.
*/
```
##########
File path: net/usrsock/usrsock_recvmsg.c
##########
@@ -302,7 +302,8 @@ ssize_t usrsock_recvmsg(FAR struct socket *psock, FAR struct msghdr *msg,
if (!(conn->flags & USRSOCK_EVENT_RECVFROM_AVAIL))
{
- if (_SS_ISNONBLOCK(psock->s_flags) || (flags & MSG_DONTWAIT) != 0)
+ if (_SS_ISNONBLOCK(conn->sconn.s_flags) ||
+ (flags & MSG_DONTWAIT) != 0)
Review comment:
Why not the following formatting?
```
if (_SS_ISNONBLOCK(conn->sconn.s_flags) ||
(flags & MSG_DONTWAIT) != 0)
```
##########
File path: net/udp/udp_sendto_buffered.c
##########
@@ -394,10 +389,9 @@ static uint16_t sendto_eventhandler(FAR struct net_driver_s *dev,
FAR void *pvconn, FAR void *pvpriv,
uint16_t flags)
{
- FAR struct udp_conn_s *conn = (FAR struct udp_conn_s *)pvconn;
- FAR struct socket *psock = (FAR struct socket *)pvpriv;
+ FAR struct udp_conn_s *conn = (FAR struct udp_conn_s *)pvpriv;
Review comment:
Let's document here:
```
/* FAR struct udp_conn_s *conn = (FAR struct udp_conn_s *)pvconn;
*
* Do not use pvconn argument to get the UDP connection pointer (the above
* commented line) because pvconn is normally NULL for some events like
* NETDEV_DOWN. Instead, the UDP connection pointer can be reliably
* obtained from the corresponding UDP socket.
*/
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5434:
URL: https://github.com/apache/incubator-nuttx/pull/5434#discussion_r801706570
##########
File path: net/ieee802154/ieee802154_sockif.c
##########
@@ -375,15 +375,17 @@ static int ieee802154_bind(FAR struct socket *psock,
/* Bind a PF_IEEE802154 socket to an network device. */
- if (psock->s_type != SOCK_DGRAM)
+ if (ppsock->s_conn == NULL || sock->s_type != SOCK_DGRAM)
Review comment:
~ppsock~
psock
Or (better) move "conn = (FAR struct ieee802154_conn_s *)psock->s_conn;" from line 384 to before line 378.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org