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 2021/05/12 23:00:20 UTC
[GitHub] [incubator-nuttx] masayuki2009 opened a new pull request #3707: Fix NFS over TCP
masayuki2009 opened a new pull request #3707:
URL: https://github.com/apache/incubator-nuttx/pull/3707
## Summary
- This PR consists of the following commits to fix NFS over TCP
- commit 1: fs: nfs: Fix rpcclnt_send() in TCP mode
- I noticed that NFS over TCP does not work with Ubuntu 18.04
- Finally, I found that the record marking packet should not
be sent separately
- This commit fixes this issue
- commit 2: fs: nfs: Fix to read a large packet in TCP mode
- I noticed that receiving a read large packet (e.g. 2KB) in TCP
mode does not work correctly
- Actually, rpcclnt_receive() only received up to MSS
- This commit fixes this issue
- commit 3: fs: nfs: Do not bind to a local port in TCP mode
- Since binding to a local port is not necessary in TCP mode,
it should be removed
- commit 4: boards: sabre-6quad: Update netnsh/netnsh_smp defconfigs
- commit 5: boards: spresense: Update rndis/rndis_smp/wifi/wifi_smp defconfigs
## Impact
- TCP mode only
## Testing
- Tested with NFS server on Ubuntu 18.04 (x86_64)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3707: Fix NFS over TCP
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3707:
URL: https://github.com/apache/incubator-nuttx/pull/3707#issuecomment-840270588
> Hmm, build error happened with sim/posix_spawn.
> I think the error does not relate to this PR.
>
> ```
> ====================================================================================
> Configuration/Tool: sim/posix_spawn
> ------------------------------------------------------------------------------------
> Cleaning...
> Configuring...
> Building NuttX...
> machine/sim/arch_setjmp.S: Assembler messages:
> machine/sim/arch_setjmp.S:101: Error: operand type mismatch for `jmp'
> make[1]: *** [Makefile:130: bin/arch_setjmp.o] Error 1
> stdio/lib_libvsprintf.c: In function 'vsprintf_internal':
> Error: stdio/lib_libvsprintf.c:405:19: error: duplicate case value
> 405 | case sizeof(unsigned long long):
> | ^~~~
> ```
Should be fixed by: https://github.com/apache/incubator-nuttx/pull/3706
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on a change in pull request #3707: Fix NFS over TCP
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #3707:
URL: https://github.com/apache/incubator-nuttx/pull/3707#discussion_r632127702
##########
File path: fs/nfs/rpc_clnt.c
##########
@@ -371,13 +395,22 @@ static int rpcclnt_receive(FAR struct rpcclnt *rpc,
resplen = mark;
}
- error = psock_recv(&rpc->rc_so, reply, resplen, 0);
+repeat:
+ error = psock_recv(&rpc->rc_so, reply + offset, resplen, 0);
if (error < 0)
{
ferr("ERROR: psock_recv response failed: %d\n", error);
return error;
}
+ resplen -= error;
+ offset += error;
+
+ if (rpc->rc_sotype == SOCK_STREAM && resplen)
+ {
+ goto repeat;
+ }
Review comment:
@gustavonihei
Thanks for the suggestion.
I will change the code based on the https://cwiki.apache.org/confluence/display/NUTTX/Coding+Standard#dowhile
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on a change in pull request #3707: Fix NFS over TCP
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #3707:
URL: https://github.com/apache/incubator-nuttx/pull/3707#discussion_r631740659
##########
File path: fs/nfs/rpc_clnt.c
##########
@@ -297,20 +297,26 @@ static int rpcclnt_socket(FAR struct rpcclnt *rpc, in_port_t rport)
static int rpcclnt_send(FAR struct rpcclnt *rpc,
FAR void *call, int reqlen)
{
- uint32_t mark;
int ret = OK;
- /* Send the record marking(RM) for stream only */
-
if (rpc->rc_sotype == SOCK_STREAM)
{
- mark = txdr_unsigned(0x80000000 | reqlen);
- ret = psock_send(&rpc->rc_so, &mark, sizeof(mark), 0);
- if (ret < 0)
- {
- ferr("ERROR: psock_send mark failed: %d\n", ret);
- return ret;
- }
+ /* Prepare the fragment header for stream only
+ * NOTE: Sending a separate packet does not work with Linux host
+ */
+
+ FAR struct rpc_call_header *ch = (FAR struct rpc_call_header *)call;
+
+ /* NOTE: subtract the fragment header size in the header */
+
+ ch->rp_fh = txdr_unsigned(0x80000000 | (reqlen - 4));
Review comment:
@xiaoxiang781216
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3707: Fix NFS over TCP
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3707:
URL: https://github.com/apache/incubator-nuttx/pull/3707#discussion_r631520750
##########
File path: fs/nfs/rpc_clnt.c
##########
@@ -297,29 +299,47 @@ static int rpcclnt_socket(FAR struct rpcclnt *rpc, in_port_t rport)
static int rpcclnt_send(FAR struct rpcclnt *rpc,
FAR void *call, int reqlen)
{
+ FAR uint8_t *pkt = NULL;
uint32_t mark;
int ret = OK;
- /* Send the record marking(RM) for stream only */
+ /* Prepare the record marking(RM) for stream only
+ * NOTE: Sending a separate packet does not work with Linux host
+ */
if (rpc->rc_sotype == SOCK_STREAM)
{
- mark = txdr_unsigned(0x80000000 | reqlen);
- ret = psock_send(&rpc->rc_so, &mark, sizeof(mark), 0);
- if (ret < 0)
+ pkt = (uint8_t *)kmm_malloc(sizeof(mark) + reqlen);
+
+ if (NULL == pkt)
{
ferr("ERROR: psock_send mark failed: %d\n", ret);
- return ret;
+ return -ENOMEM;
}
+
+ mark = txdr_unsigned(0x80000000 | reqlen);
+ memcpy(pkt, &mark, sizeof(mark));
}
- /* Send the call message
+ /* Send a RPC message
+ * For TCP, the record marking + call message will be sent
+ * For UDP, call message will be sent
*
* On success, psock_send returns the number of bytes sent;
* On failure, it returns a negated errno value.
*/
- ret = psock_send(&rpc->rc_so, call, reqlen, 0);
+ if (pkt)
+ {
+ memcpy(pkt + sizeof(mark), call, reqlen);
+ ret = psock_send(&rpc->rc_so, pkt, sizeof(mark) + reqlen, 0);
Review comment:
it's better to call psock_sendmsg instead to avoid kmalloc in every transaction.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3707: Fix NFS over TCP
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3707:
URL: https://github.com/apache/incubator-nuttx/pull/3707#discussion_r632142229
##########
File path: fs/nfs/rpc_clnt.c
##########
@@ -371,13 +395,22 @@ static int rpcclnt_receive(FAR struct rpcclnt *rpc,
resplen = mark;
}
- error = psock_recv(&rpc->rc_so, reply, resplen, 0);
+repeat:
+ error = psock_recv(&rpc->rc_so, reply + offset, resplen, 0);
if (error < 0)
{
ferr("ERROR: psock_recv response failed: %d\n", error);
return error;
}
+ resplen -= error;
+ offset += error;
+
+ if (rpc->rc_sotype == SOCK_STREAM && resplen)
+ {
+ goto repeat;
+ }
Review comment:
> @gustavonihei
>
> Thanks for the suggestion.
> I will change the code based on the https://cwiki.apache.org/confluence/display/NUTTX/Coding+Standard#dowhile
@masayuki2009 I didn't notice I gave a non-compliant suggestion! :rofl:
Thanks for pointing out!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3707: Fix NFS over TCP
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3707:
URL: https://github.com/apache/incubator-nuttx/pull/3707#discussion_r631878319
##########
File path: fs/nfs/rpc_clnt.c
##########
@@ -241,6 +241,11 @@ static int rpcclnt_socket(FAR struct rpcclnt *rpc, in_port_t rport)
goto bad;
}
+ if (rpc->rc_sotype == SOCK_STREAM)
+ {
+ goto connect;
Review comment:
According to the coding stardard, `goto` should only be used for exception handling:
https://cwiki.apache.org/confluence/display/NUTTX/Coding+Standard#goto
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3707: Fix NFS over TCP
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3707:
URL: https://github.com/apache/incubator-nuttx/pull/3707#discussion_r631880604
##########
File path: fs/nfs/rpc_clnt.c
##########
@@ -371,13 +395,22 @@ static int rpcclnt_receive(FAR struct rpcclnt *rpc,
resplen = mark;
}
- error = psock_recv(&rpc->rc_so, reply, resplen, 0);
+repeat:
+ error = psock_recv(&rpc->rc_so, reply + offset, resplen, 0);
if (error < 0)
{
ferr("ERROR: psock_recv response failed: %d\n", error);
return error;
}
+ resplen -= error;
+ offset += error;
+
+ if (rpc->rc_sotype == SOCK_STREAM && resplen)
+ {
+ goto repeat;
+ }
Review comment:
According to the coding stardard, goto should only be used for exception handling:
https://cwiki.apache.org/confluence/display/NUTTX/Coding+Standard#goto
```suggestion
do {
error = psock_recv(&rpc->rc_so, reply + offset, resplen, 0);
if (error < 0)
{
ferr("ERROR: psock_recv response failed: %d\n", error);
return error;
}
resplen -= error;
offset += error;
} while (rpc->rc_sotype == SOCK_STREAM && resplen != 0);
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on pull request #3707: Fix NFS over TCP
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #3707:
URL: https://github.com/apache/incubator-nuttx/pull/3707#issuecomment-840158060
Hmm, build error happened with sim/posix_spawn.
I think the error does not relate to this PR.
```
====================================================================================
Configuration/Tool: sim/posix_spawn
------------------------------------------------------------------------------------
Cleaning...
Configuring...
Building NuttX...
machine/sim/arch_setjmp.S: Assembler messages:
machine/sim/arch_setjmp.S:101: Error: operand type mismatch for `jmp'
make[1]: *** [Makefile:130: bin/arch_setjmp.o] Error 1
stdio/lib_libvsprintf.c: In function 'vsprintf_internal':
Error: stdio/lib_libvsprintf.c:405:19: error: duplicate case value
405 | case sizeof(unsigned long long):
| ^~~~
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei merged pull request #3707: Fix NFS over TCP
Posted by GitBox <gi...@apache.org>.
gustavonihei merged pull request #3707:
URL: https://github.com/apache/incubator-nuttx/pull/3707
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3707: Fix NFS over TCP
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3707:
URL: https://github.com/apache/incubator-nuttx/pull/3707#discussion_r631680331
##########
File path: fs/nfs/rpc_clnt.c
##########
@@ -297,20 +297,26 @@ static int rpcclnt_socket(FAR struct rpcclnt *rpc, in_port_t rport)
static int rpcclnt_send(FAR struct rpcclnt *rpc,
FAR void *call, int reqlen)
{
- uint32_t mark;
int ret = OK;
- /* Send the record marking(RM) for stream only */
-
if (rpc->rc_sotype == SOCK_STREAM)
{
- mark = txdr_unsigned(0x80000000 | reqlen);
- ret = psock_send(&rpc->rc_so, &mark, sizeof(mark), 0);
- if (ret < 0)
- {
- ferr("ERROR: psock_send mark failed: %d\n", ret);
- return ret;
- }
+ /* Prepare the fragment header for stream only
+ * NOTE: Sending a separate packet does not work with Linux host
+ */
+
+ FAR struct rpc_call_header *ch = (FAR struct rpc_call_header *)call;
+
+ /* NOTE: subtract the fragment header size in the header */
+
+ ch->rp_fh = txdr_unsigned(0x80000000 | (reqlen - 4));
Review comment:
a little bit complex, why not directly use psock_sendmsg + iovec for tcp?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on a change in pull request #3707: Fix NFS over TCP
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #3707:
URL: https://github.com/apache/incubator-nuttx/pull/3707#discussion_r631571627
##########
File path: fs/nfs/rpc_clnt.c
##########
@@ -297,29 +299,47 @@ static int rpcclnt_socket(FAR struct rpcclnt *rpc, in_port_t rport)
static int rpcclnt_send(FAR struct rpcclnt *rpc,
FAR void *call, int reqlen)
{
+ FAR uint8_t *pkt = NULL;
uint32_t mark;
int ret = OK;
- /* Send the record marking(RM) for stream only */
+ /* Prepare the record marking(RM) for stream only
+ * NOTE: Sending a separate packet does not work with Linux host
+ */
if (rpc->rc_sotype == SOCK_STREAM)
{
- mark = txdr_unsigned(0x80000000 | reqlen);
- ret = psock_send(&rpc->rc_so, &mark, sizeof(mark), 0);
- if (ret < 0)
+ pkt = (uint8_t *)kmm_malloc(sizeof(mark) + reqlen);
+
+ if (NULL == pkt)
{
ferr("ERROR: psock_send mark failed: %d\n", ret);
- return ret;
+ return -ENOMEM;
}
+
+ mark = txdr_unsigned(0x80000000 | reqlen);
+ memcpy(pkt, &mark, sizeof(mark));
}
- /* Send the call message
+ /* Send a RPC message
+ * For TCP, the record marking + call message will be sent
+ * For UDP, call message will be sent
*
* On success, psock_send returns the number of bytes sent;
* On failure, it returns a negated errno value.
*/
- ret = psock_send(&rpc->rc_so, call, reqlen, 0);
+ if (pkt)
+ {
+ memcpy(pkt + sizeof(mark), call, reqlen);
+ ret = psock_send(&rpc->rc_so, pkt, sizeof(mark) + reqlen, 0);
Review comment:
@xiaoxiang781216
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on a change in pull request #3707: Fix NFS over TCP
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #3707:
URL: https://github.com/apache/incubator-nuttx/pull/3707#discussion_r632134444
##########
File path: fs/nfs/rpc_clnt.c
##########
@@ -241,6 +241,11 @@ static int rpcclnt_socket(FAR struct rpcclnt *rpc, in_port_t rport)
goto bad;
}
+ if (rpc->rc_sotype == SOCK_STREAM)
+ {
+ goto connect;
Review comment:
Done
##########
File path: fs/nfs/rpc_clnt.c
##########
@@ -371,13 +395,22 @@ static int rpcclnt_receive(FAR struct rpcclnt *rpc,
resplen = mark;
}
- error = psock_recv(&rpc->rc_so, reply, resplen, 0);
+repeat:
+ error = psock_recv(&rpc->rc_so, reply + offset, resplen, 0);
if (error < 0)
{
ferr("ERROR: psock_recv response failed: %d\n", error);
return error;
}
+ resplen -= error;
+ offset += error;
+
+ if (rpc->rc_sotype == SOCK_STREAM && resplen)
+ {
+ goto repeat;
+ }
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on a change in pull request #3707: Fix NFS over TCP
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #3707:
URL: https://github.com/apache/incubator-nuttx/pull/3707#discussion_r631525622
##########
File path: fs/nfs/rpc_clnt.c
##########
@@ -297,29 +299,47 @@ static int rpcclnt_socket(FAR struct rpcclnt *rpc, in_port_t rport)
static int rpcclnt_send(FAR struct rpcclnt *rpc,
FAR void *call, int reqlen)
{
+ FAR uint8_t *pkt = NULL;
uint32_t mark;
int ret = OK;
- /* Send the record marking(RM) for stream only */
+ /* Prepare the record marking(RM) for stream only
+ * NOTE: Sending a separate packet does not work with Linux host
+ */
if (rpc->rc_sotype == SOCK_STREAM)
{
- mark = txdr_unsigned(0x80000000 | reqlen);
- ret = psock_send(&rpc->rc_so, &mark, sizeof(mark), 0);
- if (ret < 0)
+ pkt = (uint8_t *)kmm_malloc(sizeof(mark) + reqlen);
+
+ if (NULL == pkt)
{
ferr("ERROR: psock_send mark failed: %d\n", ret);
- return ret;
+ return -ENOMEM;
}
+
+ mark = txdr_unsigned(0x80000000 | reqlen);
+ memcpy(pkt, &mark, sizeof(mark));
}
- /* Send the call message
+ /* Send a RPC message
+ * For TCP, the record marking + call message will be sent
+ * For UDP, call message will be sent
*
* On success, psock_send returns the number of bytes sent;
* On failure, it returns a negated errno value.
*/
- ret = psock_send(&rpc->rc_so, call, reqlen, 0);
+ if (pkt)
+ {
+ memcpy(pkt + sizeof(mark), call, reqlen);
+ ret = psock_send(&rpc->rc_so, pkt, sizeof(mark) + reqlen, 0);
Review comment:
@xiaoxiang781216
I thought the same thing but I found that we need to modify the existing data structure which might give an impact.
Anyway, I will try to investigate again.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on a change in pull request #3707: Fix NFS over TCP
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #3707:
URL: https://github.com/apache/incubator-nuttx/pull/3707#discussion_r631712689
##########
File path: fs/nfs/rpc_clnt.c
##########
@@ -297,20 +297,26 @@ static int rpcclnt_socket(FAR struct rpcclnt *rpc, in_port_t rport)
static int rpcclnt_send(FAR struct rpcclnt *rpc,
FAR void *call, int reqlen)
{
- uint32_t mark;
int ret = OK;
- /* Send the record marking(RM) for stream only */
-
if (rpc->rc_sotype == SOCK_STREAM)
{
- mark = txdr_unsigned(0x80000000 | reqlen);
- ret = psock_send(&rpc->rc_so, &mark, sizeof(mark), 0);
- if (ret < 0)
- {
- ferr("ERROR: psock_send mark failed: %d\n", ret);
- return ret;
- }
+ /* Prepare the fragment header for stream only
+ * NOTE: Sending a separate packet does not work with Linux host
+ */
+
+ FAR struct rpc_call_header *ch = (FAR struct rpc_call_header *)call;
+
+ /* NOTE: subtract the fragment header size in the header */
+
+ ch->rp_fh = txdr_unsigned(0x80000000 | (reqlen - 4));
Review comment:
@xiaoxiang781216
OK. Let me reconsider the code.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #3707: Fix NFS over TCP
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3707:
URL: https://github.com/apache/incubator-nuttx/pull/3707#discussion_r631880604
##########
File path: fs/nfs/rpc_clnt.c
##########
@@ -371,13 +395,22 @@ static int rpcclnt_receive(FAR struct rpcclnt *rpc,
resplen = mark;
}
- error = psock_recv(&rpc->rc_so, reply, resplen, 0);
+repeat:
+ error = psock_recv(&rpc->rc_so, reply + offset, resplen, 0);
if (error < 0)
{
ferr("ERROR: psock_recv response failed: %d\n", error);
return error;
}
+ resplen -= error;
+ offset += error;
+
+ if (rpc->rc_sotype == SOCK_STREAM && resplen)
+ {
+ goto repeat;
+ }
Review comment:
According to the coding stardard, `goto` should only be used for exception handling:
https://cwiki.apache.org/confluence/display/NUTTX/Coding+Standard#goto
```suggestion
do {
error = psock_recv(&rpc->rc_so, reply + offset, resplen, 0);
if (error < 0)
{
ferr("ERROR: psock_recv response failed: %d\n", error);
return error;
}
resplen -= error;
offset += error;
} while (rpc->rc_sotype == SOCK_STREAM && resplen != 0);
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org