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

In container posix semantics #141

Open
wants to merge 3 commits into
base: msys2-3.4.6
Choose a base branch
from

Conversation

YO4
Copy link
Contributor

@YO4 YO4 commented Mar 7, 2023

#59 complains that remove does not work in docker volume folder.
That seems to mounted folder does not support *_POSIX_SEMANTICS functions in hyper-v container.
https://ci.appveyor.com/project/YO4/test-msys2-in-container/builds/46432262

Since it is unclear how to determine in advance whether posix semantics will work, fall back to the conventional method when an error occurs.

Functions affected are unlink(), rename(), and their derivatives.

YO4 added 2 commits March 4, 2023 21:47
Deleting files returns STATUS_INVALID_PARAMETE on a bind mounted file system in hyper-v container with FILE_DISPOSITION_POSIX_SEMANTICS.
Therefore fall back to default method.

This code is suggeted by dscho and I change it more simple.
Renaming files returns STATUS_INVALID_PARAMETE on a bind mounted file system in hyper-v container with FILE_RENAME_POSIX_SEMANTICS.

Disable the use_posix_semantics flag and retry.
@jeremyd2019
Copy link
Member

I assume this issue also exists in upstream Cygwin. It might make sense to send this patch upstream to cygwin-patches@cygwin.com and get their opinions/ideas, and hopefully get something applied there first.

@AJDurant
Copy link

AJDurant commented Mar 8, 2023

Since it is unclear how to determine in advance whether posix semantics will work, fall back to the conventional method when an error occurs.

This is the thing I was stuck on, your solution seems neat to me.

@YO4 YO4 force-pushed the in-container-posix-semantics branch from eb30798 to a796047 Compare March 16, 2023 13:51
@YO4 YO4 force-pushed the in-container-posix-semantics branch from a796047 to 749d3a2 Compare March 17, 2023 10:34
@YO4
Copy link
Contributor Author

YO4 commented Mar 17, 2023

I updated test https://ci.appveyor.com/project/YO4/test-msys2-in-container/builds/46532757
and posted patches to cygwin-patches.

@dscho
Copy link
Collaborator

dscho commented Mar 20, 2023

For lurkers, the Cygwin contribution can be seen here.

@YO4
Copy link
Contributor Author

YO4 commented Mar 24, 2023

Now this patches are merged into upstream/master, 527dd1b 7666e24 87ab6c7
And they added making a decision in advance with FILE_SUPPORTS_OPEN_BY_FILE_ID fe2545e

@dscho
Copy link
Collaborator

dscho commented Mar 27, 2023

@YO4 personally, I'd love to see the changes backported early into the MSYS2 runtime (once the jury has decided on FILE_SUPPORTS_OPEN_BY_FILE_ID that is, and the patches have stabilized).

@YO4
Copy link
Contributor Author

YO4 commented Apr 14, 2023

Hmmm, I saw https://github.com/msys2/msys2-runtime/tags and mistakenly assumed that msys2-runtime would not be updated until cygwin's next release.
If msys2-runtime can be released with a new pkgrel before the next release of cygwin, I would like to see this PR merged.
At this time, this patch does not include a decision on FILE_SUPPORTS_OPEN_BY_FILE_ID, but it is preferable to be able to complete the process with a retry rather than have it fail.

@YO4 YO4 marked this pull request as ready for review April 14, 2023 14:55
@dscho
Copy link
Collaborator

dscho commented May 26, 2023

If msys2-runtime can be released with a new pkgrel before the next release of cygwin, I would like to see this PR merged.

I see that the upstream version is slightly different: 87ab6c7#diff-bf890865130314cb545fec4788e41d9daa5dd8832a21435c6556619d5578a8aaL740-R744 does not look like 749d3a2#diff-bf890865130314cb545fec4788e41d9daa5dd8832a21435c6556619d5578a8aaR739-R741

Other than that, I am totally in favor of merging it and incrementing the pkgrel.

Copy link

@AJDurant AJDurant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@YO4 can you make the change that @dscho mentioned in #141 (comment) so this can get merged?

Comment on lines 738 to +741
goto out;
debug_printf ("NtSetInformationFile (%S, FileDispositionInformationEx)"
" returns %y with posix semantics. Disable it and retry.",
pc.get_nt_native_path (), status);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
goto out;
debug_printf ("NtSetInformationFile (%S, FileDispositionInformationEx)"
" returns %y with posix semantics. Disable it and retry.",
pc.get_nt_native_path (), status);
{
debug_printf ("NtSetInformationFile returns %y "
"with posix semantics. Disable it and retry.", status);
goto out;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants