Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes to use lfs_filebd on windows platforms #643

Merged

Conversation

m8ddin
Copy link

@m8ddin m8ddin commented Feb 13, 2022

There are two issues, when using the file-based block device emulation
on Windows Platforms:

  1. There is no fsync implementation available. This needs to be mapped
    to a Windows-specific FlushFileBuffers system call.
  2. The block device file needs to be opened as binary file (O_BINARY)
    The corresponding flag is not required for Linux.

There are two issues, when using the file-based block device emulation
on Windows Platforms:
1. There is no fsync implementation available. This needs to be mapped
   to a Windows-specific FlushFileBuffers system call.
2. The block device file needs to be opened as binary file (O_BINARY)
	   The corresponding flag is not required for Linux.
@geky
Copy link
Member

geky commented Feb 20, 2022

Hi @m8ddin, thanks for the PR.

A request, could these be changed to explicit ifdef statements?

#ifdef _WIN32
bd->fd = open(path, O_RDWR | O_CREAT | O_BINARY, 0666);
#else
bd->fd = open(path, O_RDWR | O_CREAT, 0666);
#endif

This would just help future work to know what exact idiosyncrasies are required for each platform.


I also want to highlight that we can't easily test these platform specific changes in CI, so future regressions are possible. This unfortunately may break in the future without warning.

@m8ddin
Copy link
Author

m8ddin commented Feb 20, 2022

Hi @geky !
Thanks for reviewing the PR so quickly! Of course, I can add explicit ifdefs. Will add another commit today.

@geky
Copy link
Member

geky commented Feb 20, 2022

Hi @m8ddin, thanks for the update.

Sorry, I meant that each platform specific function should be an ifdef, both the O_BINARY and FlushFileBuffers, instead of aliasing fsync. Consider what would happen if Windows adds fsync in the future (or more likely fsync is provided by mingw or some other toolchain on Windows).

#ifdef _WIN32
int err = FlushFileBuffers((HANDLE)_get_osfhandle(bd->fd)) ? 0 : -1;
#else
int err = fsync(bd->fd);
#endif

@geky geky added needs minor version new functionality only allowed in minor versions next minor labels Feb 20, 2022
@m8ddin
Copy link
Author

m8ddin commented Feb 20, 2022

Makes sense 👍

@geky
Copy link
Member

geky commented Feb 22, 2022

Thanks for this! I will bring this in next minor release, which shouldn't be too far away.

@geky geky added this to the v2.5 milestone Apr 9, 2022
@geky geky changed the base branch from master to devel April 9, 2022 17:42
@geky geky changed the base branch from devel to squash/fix-filebd-windows April 10, 2022 18:16
@geky geky added v2.5 and removed needs minor version new functionality only allowed in minor versions labels Apr 10, 2022
@geky geky merged commit a12e213 into littlefs-project:squash/fix-filebd-windows Apr 11, 2022
geky added a commit that referenced this pull request Apr 11, 2022
Fixes to use lfs_filebd on windows platforms
@geky geky mentioned this pull request Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants