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/26 08:35:51 UTC
[GitHub] [incubator-nuttx] retogaeh opened a new pull request #3785: FAT32 fix skipping sectors at computing the number of free clusters
retogaeh opened a new pull request #3785:
URL: https://github.com/apache/incubator-nuttx/pull/3785
## Summary
Double increment of sector made computation skipping every second sector and to go beyond fat-table!
## Impact
Free number of cluster was incorrect. However, fsck.msdos under Linux still reports 2 more clusters to be free!
## 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] jlaitine commented on pull request #3785: FAT32 fix skipping sectors at computing the number of free clusters
Posted by GitBox <gi...@apache.org>.
jlaitine commented on pull request #3785:
URL: https://github.com/apache/incubator-nuttx/pull/3785#issuecomment-848816932
Hi, apart from finding obvious bugs, I don't really have deep knowledge of this implementation. The original code at least look wrong to me. I can't study this in more detail today, I'll have another look tomorrow if still needed.
--
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] jlaitine edited a comment on pull request #3785: FAT32 fix skipping sectors at computing the number of free clusters
Posted by GitBox <gi...@apache.org>.
jlaitine edited a comment on pull request #3785:
URL: https://github.com/apache/incubator-nuttx/pull/3785#issuecomment-849337616
> Free cluster summary wrong (131702 vs. really 131704)
> Auto-correcting.
Just quickly browsing through the code again; this also *may* be wrong:
---
/* Examine each cluster in the fat */
for (cluster = fs->fs_nclusters; cluster > 0; cluster--)
---
It looks to me that fs->fs_nclusters counts for all the clusters, including the reserved 2 ones, but those two first ones should not be counted to the free clusters. Maybe this should just read "for (cluster = fs->fs_nclusters; cluster > 2; cluster--)"
But I think the orginal patch was right.
--
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] davids5 commented on pull request #3785: FAT32 fix skipping sectors at computing the number of free clusters
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3785:
URL: https://github.com/apache/incubator-nuttx/pull/3785#issuecomment-849024691
I compared the `nfreeclusters == 131702 ` in `fat_computefreeclusters` with fsck.msdos
```
sudo fsck.msdos -v -f -n /dev/sdb1
fsck.fat 4.1 (2017-01-24)
Checking we can access the last sector of the filesystem
Boot sector contents:
System ID " "
Media byte 0xf8 (hard disk)
512 bytes per logical sector
32768 bytes per cluster
4404 reserved sectors
First FAT starts at byte 2254848 (sector 4404)
2 FATs, 32 bit entries
969728 bytes per FAT (= 1894 sectors)
Root directory start at cluster 2 (arbitrary size)
Data area starts at byte 4194304 (sector 8192)
242304 data clusters (7939817472 bytes)
63 sectors/track, 255 heads
8192 hidden sectors
15515648 sectors total
Start cluster field in VFAT long filename slot is not 0 (but 0x000a).
Auto-setting to 0.
Start cluster field in VFAT long filename slot is not 0 (but 0x001d).
Auto-setting to 0.
Start cluster field in VFAT long filename slot is not 0 (but 0x363e).
Auto-setting to 0.
Reclaiming unconnected clusters.
Checking free cluster summary.
Free cluster summary wrong (131702 vs. really 131704)
Auto-correcting.
Leaving filesystem unchanged.
/dev/sdb1: 65 files, 110600/242304 clusters
```
And the calculation makes more sense with this PR, but I am puzzled by
`Free cluster summary wrong (131702 vs. really 131704)`
We should merge this because without this commit the numbers are completely bogus (`nfreeclusters == 131458`)
--
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] acassis commented on pull request #3785: FAT32 fix skipping sectors at computing the number of free clusters
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #3785:
URL: https://github.com/apache/incubator-nuttx/pull/3785#issuecomment-848735796
@davids5 do you have unit tests to verify if it still working on PX4 with this modification?
--
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] davids5 merged pull request #3785: FAT32 fix skipping sectors at computing the number of free clusters
Posted by GitBox <gi...@apache.org>.
davids5 merged pull request #3785:
URL: https://github.com/apache/incubator-nuttx/pull/3785
--
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] jlaitine commented on pull request #3785: FAT32 fix skipping sectors at computing the number of free clusters
Posted by GitBox <gi...@apache.org>.
jlaitine commented on pull request #3785:
URL: https://github.com/apache/incubator-nuttx/pull/3785#issuecomment-849337616
> Free cluster summary wrong (131702 vs. really 131704)
> Auto-correcting.
Just quickly browsing through the code again; this also *may* be wrong:
---
/* Examine each cluster in the fat */
for (cluster = fs->fs_nclusters; cluster > 0; cluster--)
---
It looks to me that fs->fs_nclusters counts for all the clusters, including the reserved 2 ones, but those two first ones should not be counted to the free clusters. Maybe this should just read "for (cluster = fs->fs_nclusters; cluster > 2; cluster--)"
But I think the orginal patch was right.
--
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] retogaeh commented on pull request #3785: FAT32 fix skipping sectors at computing the number of free clusters
Posted by GitBox <gi...@apache.org>.
retogaeh commented on pull request #3785:
URL: https://github.com/apache/incubator-nuttx/pull/3785#issuecomment-848637775
@jlaitine If you have time, please have a look. Looks like you got an understanding of the fat32 implementation. Thanks.
--
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] davids5 commented on pull request #3785: FAT32 fix skipping sectors at computing the number of free clusters
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3785:
URL: https://github.com/apache/incubator-nuttx/pull/3785#issuecomment-848737451
@acassis Yes, Give me today to get it tested.
--
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] retogaeh commented on pull request #3785: FAT32 fix skipping sectors at computing the number of free clusters
Posted by GitBox <gi...@apache.org>.
retogaeh commented on pull request #3785:
URL: https://github.com/apache/incubator-nuttx/pull/3785#issuecomment-849357035
Unfortunately, this alone won't fix it. It actually increases the difference to 4 clusters since we are already 2 clusters short. However, I agree with you that the 2 reserved clusters might not be considered in the right way.
--
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