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/03/28 10:38:36 UTC
[GitHub] [incubator-nuttx] Donny9 opened a new pull request #5883: lseek: use type:off_t for return value
Donny9 opened a new pull request #5883:
URL: https://github.com/apache/incubator-nuttx/pull/5883
## Summary
lseek: use type:off_t for return value
Signed-off-by: Jiuzhu Dong <do...@xiaomi.com>
## Impact
N/A
## Testing
daily test
--
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 edited a comment on pull request #5883: lseek: use type:off_t for return value
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5883:
URL: https://github.com/apache/incubator-nuttx/pull/5883#issuecomment-1080561058
> But still change in `file_seek` and `nx_seek` seems not to be needed. I was confused by seen those lines.
you mean the type change or the cast change? The first change is required to avoid losing the high 32bits, the second is optional but since the type change to off_t which make the cast become no-op and then better to remove the cast to clean up 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.
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] pkarashchenko commented on pull request #5883: lseek: use type:off_t for return value
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5883:
URL: https://github.com/apache/incubator-nuttx/pull/5883#issuecomment-1080538000
But still change in `file_seek` and `nx_seek` seems not to be needed. I was confused by seen those lines.
--
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 #5883: lseek: use type:off_t for return value
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5883:
URL: https://github.com/apache/incubator-nuttx/pull/5883#issuecomment-1080561058
> But still change in `file_seek` and `nx_seek` seems not to be needed. I was confused by seen those lines.
you mean the type change or the cast change? The first change is required to avoid losing the high 32bits, the second is optional but since the type change to off_t which make the cast become no-op.
--
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 merged pull request #5883: lseek: use type:off_t for return value
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #5883:
URL: https://github.com/apache/incubator-nuttx/pull/5883
--
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] pkarashchenko commented on pull request #5883: lseek: use type:off_t for return value
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5883:
URL: https://github.com/apache/incubator-nuttx/pull/5883#issuecomment-1080494007
IMO this is a zero value change. With max optimization the impact will be none, but with debug optimization the stack usage will increase if `defined(CONFIG_FS_LARGEFILE) && defined(CONFIG_HAVE_LONG_LONG)` because `off_t` will become `int64_t`.
--
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 #5883: lseek: use type:off_t for return value
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5883:
URL: https://github.com/apache/incubator-nuttx/pull/5883#issuecomment-1080532855
It isn't uncommon to see file bigger than 4GB in IoT devices especially for multimedia files.
--
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] pkarashchenko commented on pull request #5883: lseek: use type:off_t for return value
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5883:
URL: https://github.com/apache/incubator-nuttx/pull/5883#issuecomment-1080585193
> > But still change in `file_seek` and `nx_seek` seems not to be needed. I was confused by seen those lines.
>
> you mean the type change or the cast change? The first change is required to avoid losing the high 32bits, the second is optional but since the type change to off_t which make the cast become no-op and then better to remove the cast to clean up the code.
The seek case is a bit messy in NuttX code and this change is definitely needed. Here is what I'm talking about:
1. I would prefer `newpos = -EINVAL;` -> `return newpos;` in `drivers/bch/bchdev_driver.c` instead of change `ret` from `int` to `off_t`. If we do this then we can remove `newpos` at all from the `bch_seek` and use `ret` everywhere there.
2. Change in `nx_seek` is optional and I think that having a cast in `return ret;` is equivalent to having a cast in `ret = fs_getfilep(fd, &filep);`, but `off_t ret;` use 4 more bytes of stack (without optimization of course).
3. Change in `file_seek` is good, but in general here comes the "messy things". The code `ret = inode->u.i_ops->seek(filep, offset, whence);` -> `if (ret < 0)` -> `return ret;` and still keeps execution assuming that `f_pos;` is updated and `return filep->f_pos;` even if `inode->u.i_ops->seek` returned `>= 0`, but for example in `drivers/sensors/adxl372.c` the `adxl372_seek` does not update `filep->f_pos` and just returns `priv->seek_address`. The same style is used in many other drivers. So most probably we just need to `return inode->u.i_ops->seek(filep, offset, whence);` and eliminate `ret` at all.
--
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] pkarashchenko commented on pull request #5883: lseek: use type:off_t for return value
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5883:
URL: https://github.com/apache/incubator-nuttx/pull/5883#issuecomment-1080536341
> It isn't uncommon to see file bigger than 4GB in IoT devices especially for multimedia files.
My mistake again. Thank you for pointing this out. Had to press "Expand down" button few times to see `ret = newpos;`.
--
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 #5883: lseek: use type:off_t for return value
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5883:
URL: https://github.com/apache/incubator-nuttx/pull/5883#issuecomment-1080671378
@pkarashchenko how about you create a patch after this PR get merged? It's better to do the change step by step.
--
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] pkarashchenko commented on pull request #5883: lseek: use type:off_t for return value
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5883:
URL: https://github.com/apache/incubator-nuttx/pull/5883#issuecomment-1080723575
Yes. Sure
--
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