You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "dry-75 (via GitHub)" <gi...@apache.org> on 2024/07/30 00:32:36 UTC
[PR] nsh_dd: fix lseek return check. [nuttx-apps]
dry-75 opened a new pull request, #2456:
URL: https://github.com/apache/nuttx-apps/pull/2456
- As was merged in commit 1852731df87e5895e299b74aabdb1984b7e3f795 on dd_main.c: lseek returns -1 on error.
Should be consistent in nsh_ddcmd.c and nsh_main.c.
## Summary
## Impact
## 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
Re: [PR] nsh_dd: fix lseek return check. [nuttx-apps]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #2456:
URL: https://github.com/apache/nuttx-apps/pull/2456#discussion_r1699706498
##########
nshlib/nsh_ddcmd.c:
##########
@@ -385,7 +385,7 @@ int cmd_dd(FAR struct nsh_vtbl_s *vtbl, int argc, FAR char **argv)
if (dd.skip)
{
ret = lseek(dd.infd, dd.skip * dd.sectsize, SEEK_SET);
- if (ret < -1)
+ if (ret < 0)
Review Comment:
from https://pubs.opengroup.org/onlinepubs/009695399/functions/lseek.html, the return value represents the absolute offset after seeking, why do we need consider the returned value may be a negative value in success case?
--
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
Re: [PR] nsh_dd: fix lseek return check. [nuttx-apps]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #2456:
URL: https://github.com/apache/nuttx-apps/pull/2456
--
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
Re: [PR] nsh_dd: fix lseek return check. [nuttx-apps]
Posted by "yamt (via GitHub)" <gi...@apache.org>.
yamt commented on code in PR #2456:
URL: https://github.com/apache/nuttx-apps/pull/2456#discussion_r1696278236
##########
nshlib/nsh_ddcmd.c:
##########
@@ -385,7 +385,7 @@ int cmd_dd(FAR struct nsh_vtbl_s *vtbl, int argc, FAR char **argv)
if (dd.skip)
{
ret = lseek(dd.infd, dd.skip * dd.sectsize, SEEK_SET);
- if (ret < -1)
+ if (ret < 0)
Review Comment:
FS_LARGEFILE is not just pedantic i suspect.
--
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
Re: [PR] nsh_dd: fix lseek return check. [nuttx-apps]
Posted by "dry-75 (via GitHub)" <gi...@apache.org>.
dry-75 commented on code in PR #2456:
URL: https://github.com/apache/nuttx-apps/pull/2456#discussion_r1699920300
##########
nshlib/nsh_ddcmd.c:
##########
@@ -385,7 +385,7 @@ int cmd_dd(FAR struct nsh_vtbl_s *vtbl, int argc, FAR char **argv)
if (dd.skip)
{
ret = lseek(dd.infd, dd.skip * dd.sectsize, SEEK_SET);
- if (ret < -1)
+ if (ret < 0)
Review Comment:
What Yamamoto seems to assert ( but he didn't specifically linked to it) is that in the old if not outdated POSIX - see under Rationale in your link provided @xiaoxiang781216
"...The POSIX.1-1990 standard did not specifically prohibit lseek() from returning a negative offset..." etc.
In same URL, and all way to updated POSIX 2024 page of lseek, that Rationalte section is under header / page section titled "_The following sections are informative_". One way to read that is, Information FYI nice to know history.
But at the same time, the return value para does not still explicitly state that the offset from beginning of the file cannot be negative.
> RETURN VALUE
Upon successful completion, the resulting offset, as measured in bytes from the beginning of the file, shall be returned. Otherwise, -1 shall be returned, errno shall be set to indicate the error, and the file offset shall remain unchanged.
To which POSIX whole of NuttX conforms I do not know, and if this special case if of any use in current base. My guess is if this suggested change would work for most cases and only breaks when using some files which return negative other than -1 to signal error , well they are broken.
I can submit this change, otherwise if anyone is interested to dig more into this I would have to pass as I would like to focus on other things.
I found the comment/discussion informative however, as I always used lseek with closed eyes wrt. to the possibility of it returning a negative while not in error. (But then again, all use cases I recall using it, It would never make sense for it to return a valid negative value which would not signal and error, whatever it would be actually).
--
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
Re: [PR] nsh_dd: fix lseek return check. [nuttx-apps]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #2456:
URL: https://github.com/apache/nuttx-apps/pull/2456#issuecomment-2257378152
@dry-75 could you rebase the patch to the latest mainline? ci broken is fixed by https://github.com/apache/nuttx-apps/pull/2455.
--
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
Re: [PR] nsh_dd: fix lseek return check. [nuttx-apps]
Posted by "dry-75 (via GitHub)" <gi...@apache.org>.
dry-75 commented on code in PR #2456:
URL: https://github.com/apache/nuttx-apps/pull/2456#discussion_r1699429154
##########
nshlib/nsh_ddcmd.c:
##########
@@ -385,7 +385,7 @@ int cmd_dd(FAR struct nsh_vtbl_s *vtbl, int argc, FAR char **argv)
if (dd.skip)
{
ret = lseek(dd.infd, dd.skip * dd.sectsize, SEEK_SET);
- if (ret < -1)
+ if (ret < 0)
Review Comment:
type off_t is good, but isn't better to keep check < 0 ? to flag any negative as error.
--
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
Re: [PR] nsh_dd: fix lseek return check. [nuttx-apps]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #2456:
URL: https://github.com/apache/nuttx-apps/pull/2456#discussion_r1699286875
##########
nshlib/nsh_ddcmd.c:
##########
@@ -385,7 +385,7 @@ int cmd_dd(FAR struct nsh_vtbl_s *vtbl, int argc, FAR char **argv)
if (dd.skip)
{
ret = lseek(dd.infd, dd.skip * dd.sectsize, SEEK_SET);
- if (ret < -1)
+ if (ret < 0)
Review Comment:
@dry-75 could you provide a patch to fix the type mismatch 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
Re: [PR] nsh_dd: fix lseek return check. [nuttx-apps]
Posted by "dry-75 (via GitHub)" <gi...@apache.org>.
dry-75 commented on PR #2456:
URL: https://github.com/apache/nuttx-apps/pull/2456#issuecomment-2257434356
@xiaoxiang781216 rebased.
--
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
Re: [PR] nsh_dd: fix lseek return check. [nuttx-apps]
Posted by "dry-75 (via GitHub)" <gi...@apache.org>.
dry-75 commented on code in PR #2456:
URL: https://github.com/apache/nuttx-apps/pull/2456#discussion_r1696265228
##########
nshlib/nsh_ddcmd.c:
##########
@@ -385,7 +385,7 @@ int cmd_dd(FAR struct nsh_vtbl_s *vtbl, int argc, FAR char **argv)
if (dd.skip)
{
ret = lseek(dd.infd, dd.skip * dd.sectsize, SEEK_SET);
- if (ret < -1)
+ if (ret < 0)
Review Comment:
That would be, pedantic, but perhaps then leave it for next common fix PR : updating both dds
--
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
Re: [PR] nsh_dd: fix lseek return check. [nuttx-apps]
Posted by "yamt (via GitHub)" <gi...@apache.org>.
yamt commented on code in PR #2456:
URL: https://github.com/apache/nuttx-apps/pull/2456#discussion_r1696249637
##########
nshlib/nsh_ddcmd.c:
##########
@@ -385,7 +385,7 @@ int cmd_dd(FAR struct nsh_vtbl_s *vtbl, int argc, FAR char **argv)
if (dd.skip)
{
ret = lseek(dd.infd, dd.skip * dd.sectsize, SEEK_SET);
- if (ret < -1)
+ if (ret < 0)
Review Comment:
well, lseek returns an off_t, which can be 64-bit where "int ret" is usually 32-bit.
it's better to do:
```c
off_t off = lseek(...);
if (off == (off_t)-1) {
....
}
```
--
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
Re: [PR] nsh_dd: fix lseek return check. [nuttx-apps]
Posted by "dry-75 (via GitHub)" <gi...@apache.org>.
dry-75 commented on code in PR #2456:
URL: https://github.com/apache/nuttx-apps/pull/2456#discussion_r1699920300
##########
nshlib/nsh_ddcmd.c:
##########
@@ -385,7 +385,7 @@ int cmd_dd(FAR struct nsh_vtbl_s *vtbl, int argc, FAR char **argv)
if (dd.skip)
{
ret = lseek(dd.infd, dd.skip * dd.sectsize, SEEK_SET);
- if (ret < -1)
+ if (ret < 0)
Review Comment:
What Yamamoto seems to assert ( but he didn't specifically linked to it) is that in the old if not outdated POSIX - see under Rationale in your link provided @xiaoxiang781216
"...The POSIX.1-1990 standard did not specifically prohibit lseek() from returning a negative offset..." etc.
In same URL, and all way to updated POSIX 2024 page of lseek, that Rationale section is under header / page section titled "_The following sections are informative_". One way to read that is, Information FYI nice to know history.
But at the same time, the return value para does not still explicitly state that the offset from beginning of the file cannot be negative.
> RETURN VALUE
Upon successful completion, the resulting offset, as measured in bytes from the beginning of the file, shall be returned. Otherwise, -1 shall be returned, errno shall be set to indicate the error, and the file offset shall remain unchanged.
To which POSIX whole of NuttX conforms I do not know, and if this special case if of any use in current base. My guess is if this suggested change would work for most cases and only breaks when using some files which return negative other than -1 to signal error , well they are broken.
I can submit this change, otherwise if anyone is interested to dig more into this I would have to pass as I would like to focus on other things.
I found the comment/discussion informative however, as I always used lseek with closed eyes wrt. to the possibility of it returning a negative while not in error. (But then again, all use cases I recall using it, It would never make sense for it to return a valid negative value which would not signal and error, whatever it would be actually).
--
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
Re: [PR] nsh_dd: fix lseek return check. [nuttx-apps]
Posted by "yamt (via GitHub)" <gi...@apache.org>.
yamt commented on code in PR #2456:
URL: https://github.com/apache/nuttx-apps/pull/2456#discussion_r1699875814
##########
nshlib/nsh_ddcmd.c:
##########
@@ -385,7 +385,7 @@ int cmd_dd(FAR struct nsh_vtbl_s *vtbl, int argc, FAR char **argv)
if (dd.skip)
{
ret = lseek(dd.infd, dd.skip * dd.sectsize, SEEK_SET);
- if (ret < -1)
+ if (ret < 0)
Review Comment:
> from https://pubs.opengroup.org/onlinepubs/009695399/functions/lseek.html, the return value represents the absolute offset after seeking, why do we need consider the returned value may be a negative value in success case?
you can do eg. `lseek(fd, -100, SEEK_SET)`.
for example, you may use negative offsets for /dev/kmem style character devices.
note that on some platforms negative addresses are normal.
--
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
Re: [PR] nsh_dd: fix lseek return check. [nuttx-apps]
Posted by "yamt (via GitHub)" <gi...@apache.org>.
yamt commented on code in PR #2456:
URL: https://github.com/apache/nuttx-apps/pull/2456#discussion_r1699516546
##########
nshlib/nsh_ddcmd.c:
##########
@@ -385,7 +385,7 @@ int cmd_dd(FAR struct nsh_vtbl_s *vtbl, int argc, FAR char **argv)
if (dd.skip)
{
ret = lseek(dd.infd, dd.skip * dd.sectsize, SEEK_SET);
- if (ret < -1)
+ if (ret < 0)
Review Comment:
a negative offset is a negative offset.
it's normal for some kind of files.
(i don't know if nuttx has such files or not though.
i don't know if it's relevant to this particular usage of lseek either.)
ie. a more pedantic check would be:
```
errno = 0;
off_t off = lseek(...);
if (off == (off_t)-1 && errno != 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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nsh_dd: fix lseek return check. [nuttx-apps]
Posted by "yamt (via GitHub)" <gi...@apache.org>.
yamt commented on code in PR #2456:
URL: https://github.com/apache/nuttx-apps/pull/2456#discussion_r1700148514
##########
nshlib/nsh_ddcmd.c:
##########
@@ -385,7 +385,7 @@ int cmd_dd(FAR struct nsh_vtbl_s *vtbl, int argc, FAR char **argv)
if (dd.skip)
{
ret = lseek(dd.infd, dd.skip * dd.sectsize, SEEK_SET);
- if (ret < -1)
+ if (ret < 0)
Review Comment:
> as far as I know, nuttx doesn't have file system support the negative absolute position.
is that case, i guess it doesn't really matter.
(except that it's still a good practice to write portable 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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] nsh_dd: fix lseek return check. [nuttx-apps]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #2456:
URL: https://github.com/apache/nuttx-apps/pull/2456#discussion_r1700129204
##########
nshlib/nsh_ddcmd.c:
##########
@@ -385,7 +385,7 @@ int cmd_dd(FAR struct nsh_vtbl_s *vtbl, int argc, FAR char **argv)
if (dd.skip)
{
ret = lseek(dd.infd, dd.skip * dd.sectsize, SEEK_SET);
- if (ret < -1)
+ if (ret < 0)
Review Comment:
> > from https://pubs.opengroup.org/onlinepubs/009695399/functions/lseek.html, the return value represents the absolute offset after seeking, why do we need consider the returned value may be a negative value in success case?
>
> you can do eg. `lseek(fd, -100, SEEK_SET)`.
>
> for example, you may use negative offsets for /dev/kmem style character devices. note that on some platforms negative addresses are normal.
as far as I know, nuttx doesn't have file system support the negative absolute position.
--
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
Re: [PR] nsh_dd: fix lseek return check. [nuttx-apps]
Posted by "dry-75 (via GitHub)" <gi...@apache.org>.
dry-75 commented on code in PR #2456:
URL: https://github.com/apache/nuttx-apps/pull/2456#discussion_r1699928317
##########
nshlib/nsh_ddcmd.c:
##########
@@ -385,7 +385,7 @@ int cmd_dd(FAR struct nsh_vtbl_s *vtbl, int argc, FAR char **argv)
if (dd.skip)
{
ret = lseek(dd.infd, dd.skip * dd.sectsize, SEEK_SET);
- if (ret < -1)
+ if (ret < 0)
Review Comment:
Btw, searching for word "absolute" on the page in your linked URL, nothing comes up. No absolute on any revision I checked of posix's lseek.
--
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