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/07/11 13:08:44 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request #4120: net/tcp: tcp_sendfile need restore the file location at the end
xiaoxiang781216 opened a new pull request #4120:
URL: https://github.com/apache/incubator-nuttx/pull/4120
## Summary
quote from https://man7.org/linux/man-pages/man2/sendfile.2.html:
If offset is not NULL, then it points to a variable holding the
file offset from which sendfile() will start reading data from
in_fd. When sendfile() returns, this variable will be set to the
offset of the byte following the last byte that was read. If
offset is not NULL, then sendfile() does not modify the file
offset of in_fd; otherwise the file offset is adjusted to reflect
the number of bytes read from in_fd.
If offset is NULL, then data will be read from in_fd starting at
the file offset, and the file offset will be updated by the call.
The change also align with the implementation at:
libs/libc/misc/lib_sendfile.c
## Impact
Make sendfile more confirm to the standard
## Testing
--
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] yamt commented on pull request #4120: net/tcp: tcp_sendfile need restore the file location at the end
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #4120:
URL: https://github.com/apache/incubator-nuttx/pull/4120#issuecomment-878070779
> @yamt I open a new issue(#4122) to track this problem. The problem exist before this patch, let's fix it by a new patch?
ok
--
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 pull request #4120: net/tcp: tcp_sendfile need restore the file location at the end
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4120:
URL: https://github.com/apache/incubator-nuttx/pull/4120#issuecomment-878011411
Oh, yes I look at the wrong location.
--
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] yamt commented on a change in pull request #4120: net/tcp: tcp_sendfile need restore the file location at the end
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #4120:
URL: https://github.com/apache/incubator-nuttx/pull/4120#discussion_r667558531
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -304,15 +303,6 @@ static uint16_t sendfile_eventhandler(FAR struct net_driver_s *dev,
* happen until the polling cycle completes).
*/
- ret = file_seek(pstate->snd_file,
- pstate->snd_foffset + pstate->snd_sent, SEEK_SET);
- if (ret < 0)
- {
- nerr("ERROR: Failed to lseek: %d\n", ret);
- pstate->snd_sent = ret;
- goto end_wait;
- }
-
Review comment:
i guess this breaks retransmission.
--
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] yamt commented on pull request #4120: net/tcp: tcp_sendfile need restore the file location at the end
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #4120:
URL: https://github.com/apache/incubator-nuttx/pull/4120#issuecomment-878005960
> > btw, file i/o with net_lock held looks quite dangerous to me.
> > do we have a defined locking order between net_lock and filesystem locks?
>
> All file i/o either doesn't hold net_lock, or temporarily net_unlock. Could you point out the place I missed?
isn't sendfile_eventhandler called with the lock held?
--
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] davids5 merged pull request #4120: net/tcp: tcp_sendfile need restore the file location at the end
Posted by GitBox <gi...@apache.org>.
davids5 merged pull request #4120:
URL: https://github.com/apache/incubator-nuttx/pull/4120
--
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] yamt commented on pull request #4120: net/tcp: tcp_sendfile need restore the file location at the end
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #4120:
URL: https://github.com/apache/incubator-nuttx/pull/4120#issuecomment-877889261
btw, file i/o with net_lock held looks quite dangerous to me.
do we have a defined locking order between net_lock and filesystem locks?
--
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 pull request #4120: net/tcp: tcp_sendfile need restore the file location at the end
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4120:
URL: https://github.com/apache/incubator-nuttx/pull/4120#issuecomment-878001404
> btw, file i/o with net_lock held looks quite dangerous to me.
> do we have a defined locking order between net_lock and filesystem locks?
All file i/o either doesn't hold net_lock, or temporarily net_unlock. Could you point out the place I missed?
--
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 #4120: net/tcp: tcp_sendfile need restore the file location at the end
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4120:
URL: https://github.com/apache/incubator-nuttx/pull/4120#discussion_r667588226
##########
File path: net/tcp/tcp_sendfile.c
##########
@@ -304,15 +303,6 @@ static uint16_t sendfile_eventhandler(FAR struct net_driver_s *dev,
* happen until the polling cycle completes).
*/
- ret = file_seek(pstate->snd_file,
- pstate->snd_foffset + pstate->snd_sent, SEEK_SET);
- if (ret < 0)
- {
- nerr("ERROR: Failed to lseek: %d\n", ret);
- pstate->snd_sent = ret;
- goto end_wait;
- }
-
Review comment:
Yes, you are right. Please review the new version 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.
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 pull request #4120: net/tcp: tcp_sendfile need restore the file location at the end
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4120:
URL: https://github.com/apache/incubator-nuttx/pull/4120#issuecomment-878034252
@yamt I open a new issue(https://github.com/apache/incubator-nuttx/issues/4122) to track this problem. The problem exist before this patch, let's fix it by a new 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