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 17:30:27 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5434: net/tcp/udp: remove psock hook to avoid invalid reference
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