Menu
▾
▴
[MinGW-notify] [ mingw-Bugs-3302830 ] Terminates silently if /etc is missing
|
From: SF/projects/mingw n. l. <min...@li...> - 2011-08-24 09:43:46
|
Bugs item #3302830, was opened at 2011-05-16 14:05 Message generated for change (Comment added) made by fabian_deb You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=102435&aid=3302830&group_id=2435 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: MSYS Group: None Status: Open Resolution: None Priority: 5 Private: No Submitted By: Fabian Greffrath (fabian_deb) Assigned to: Cesar Strauss (cstrauss) Summary: Terminates silently if /etc is missing Initial Comment: Hi, currently, if one tries to create a "portable" MSYS by copying all the .exe and .dll files from the c:\mingw\msys\1.0\bin directory to e.g. a flash drive, the MSYS programs may terminate silently without any output and error messages. The reason is that the ../etc directory (relative to msys-1.0.dll) is missing. There is a thread in msys-1.0.dll called read_mounts_thread that watches for changes in /etc and calls ExitProcess (1) if the directory does not exist (or is unreadable or whatever INVALID_HANDLE_VALUE may mean). However, I don't see why the missing /etc directory should cause the whole library to exit! I mean, nothing inside the /etc directory is strictly required as creating an entire empty one will fix the issue. IMHO, as long as /etc/fstab is missing anyway, a missing /etc directory and an empty one should be handled the same by the library. I therefore suggest to replace the call to "ExitProcess (1)" in line 1668 of file source\winsup\cygwin\path.cc in the msysCORE sources by "return (0)" and thus simply closing the thread instead of the whole process if the /etc directory is not readable. Thanks for considering! - Fabian ---------------------------------------------------------------------- >Comment By: Fabian Greffrath (fabian_deb) Date: 2011-08-24 11:43 Message: > The final decision will be Cesar's, but it looks reasonable to me. Caesar hasn't commented on this issue and the patch for three months now. May I ask if the patch has a chance to get applied in the next MSYS release? ---------------------------------------------------------------------- Comment By: Fabian Greffrath (fabian_deb) Date: 2011-06-01 14:58 Message: > 1) You may expect a quicker turn-around if you provide a ChangeLog Done in path.cc_loop3+changelog.patch. > (If you can > restore the files you've already deleted from this ticket, that would be > appreciated). Sorry, but I was so convinced of my final patch that I in turn deleted all the previous revisions. But I'll keep it in mind for future patches. ;) ---------------------------------------------------------------------- Comment By: Keith Marshall (keithmarshall) Date: 2011-06-01 14:41 Message: The final decision will be Cesar's, but it looks reasonable to me. However, I do have two comments: 1) You may expect a quicker turn-around if you provide a ChangeLog -- not as a diff, but may be in the diff file; see the example I've attached to https://sourceforge.net/tracker/?func=detail&aid=3310148&group_id=2435&atid=102435 2) Please DON'T delete previous versions of your patches. The history of how you got to your final submission may be a valuable aid in the review process, and the maintainer may eventually adopt a solution based on some combination of the techniques you've tried along the way. (If you can restore the files you've already deleted from this ticket, that would be appreciated). ---------------------------------------------------------------------- Comment By: Fabian Greffrath (fabian_deb) Date: 2011-05-27 16:07 Message: Add some debug_printf and quit gracefully if / is untraceable. ---------------------------------------------------------------------- Comment By: Earnie Boyd (earnie) Date: 2011-05-27 15:30 Message: This last patch certainly looks cleaner. You might want to leave a debug_printf in though. It is only active (via preprocessor macros) if debugging is defined and provides information to strace. ---------------------------------------------------------------------- Comment By: Fabian Greffrath (fabian_deb) Date: 2011-05-27 14:30 Message: I have simplified and reworked the patch to use only one file handle, see path.cc_loop2.patch. It should maybe error out if FindFirstChangeNotification(info->RootPath) returns INVALID_HANDLE_VALUE, but this can be easily added after an initial review. ---------------------------------------------------------------------- Comment By: Fabian Greffrath (fabian_deb) Date: 2011-05-24 16:28 Message: > Patience goes a long way toward appreciation of others time when dealing in > Open Source software remediation and maintenance Sure, this was merely meant as a friendly reminder. I should have added a smiley or so. ;) ---------------------------------------------------------------------- Comment By: Earnie Boyd (earnie) Date: 2011-05-24 16:00 Message: Patience goes a long way toward appreciation of others time when dealing in Open Source software remediation and maintenance; while not neglecting the providers of patches is as much appreciated by the provider. As soon as we get a minute of round tuit we can give a review. I know Keith is unavailable ATM and Cesar may be as well. ---------------------------------------------------------------------- Comment By: Fabian Greffrath (fabian_deb) Date: 2011-05-24 14:49 Message: I'd very much appreciate a review of the path.cc_loop.patch that I submitted. ---------------------------------------------------------------------- Comment By: Fabian Greffrath (fabian_deb) Date: 2011-05-20 16:33 Message: > No, I like Cesar's idea better and modify what is watched to the root / > directory and once /etc is created switch back to watching /etc. Alright, I have implemented this in path.cc_loop.patch. It may need some polish, but at least it works as expected. :) ---------------------------------------------------------------------- Comment By: Earnie Boyd (earnie) Date: 2011-05-20 14:37 Message: No, I like Cesar's idea better and modify what is watched to the root / directory and once /etc is created switch back to watching /etc. ---------------------------------------------------------------------- Comment By: Fabian Greffrath (fabian_deb) Date: 2011-05-20 10:03 Message: > Or to simply mkdir() the missing /etc > directory, and add an empty /etc/fstab file? I think creating only the /etc directory should be sufficient, the thread just needs something that it can actually watch. The code would become rather ugly, however, since we would have to run the same FindFirstChangeNotification call again and check its return value after the mkdir(). So earnie, do you suggest to apply my original patch and just return from the thread when /etc is not watchable? ---------------------------------------------------------------------- Comment By: Cesar Strauss (cstrauss) Date: 2011-05-20 00:40 Message: > Will it consume any more CPU cycles than the existing loop on > FindNextChangeNotification()? The existing loop has a blocking call to WaitForSingleObject. It should consume cycles only when the directory contents actually changes. > If looping on FindFirstChangeNotification() is unacceptable, would it be > possible to make it block? Indeed. I suggest using FindFirstChangeNotification / FindNextChangeNotification / WaitForSingleObject on the PARENT directory, waiting for the creation of the etc directory itself. Cesar ---------------------------------------------------------------------- Comment By: Earnie Boyd (earnie) Date: 2011-05-20 00:07 Message: Your original patch, the thread will only remain closed for the initial session. Once /etc is created and the session exited and restarted the thread will remain open. Silently creating /etc could have undesirable effects as well, such as not being able to create the directory. Is it worth the overhead to check it in the thread, no, the check for /etc existing should be in the dll initialization code so that it only happens once during dll startup. ---------------------------------------------------------------------- Comment By: Keith Marshall (keithmarshall) Date: 2011-05-19 10:39 Message: >> Put FindFirstChangeNotification into a loop and repeat >> it until ../etc is finally accessible, then proceed to the loop around >> FindNextChangeNotification as usual. > > Won't this loop consume CPU cycles? Will it consume any more CPU cycles than the existing loop on FindNextChangeNotification()? > I'd rather use your original suggestion. I concur with Fabian's conclusion that the loss of (mount table) functionality which that incurs would be undesirable -- although certainly better than the current (silently abort) behaviour. If looping on FindFirstChangeNotification() is unacceptable, would it be possible to make it block? Or to simply mkdir() the missing /etc directory, and add an empty /etc/fstab file? (Perhaps with a warning diagnostic to this effect)? ---------------------------------------------------------------------- Comment By: Fabian Greffrath (fabian_deb) Date: 2011-05-19 09:21 Message: > Won't this loop consume CPU cycles? I'd rather use your original > suggestion. The original loop around FindNextChangeNotification does consume CPU cycles as well. The main advantage against my first suggestion is that you can add an /etc/fstab file, even if it was initially missing when the process was started, and then newly added mounts are still handled as expected. This is impossible with my first suggestion, which closes the thread for the whole runtime of the process if there is no /etc/fstab at start. - Fabian ---------------------------------------------------------------------- Comment By: Cesar Strauss (cstrauss) Date: 2011-05-19 00:17 Message: > Put FindFirstChangeNotification into a loop and repeat > it until ../etc is finally accessible, then proceed to the loop around > FindNextChangeNotification as usual. Won't this loop consume CPU cycles? I'd rather use your original suggestion. Regards, Cesar ---------------------------------------------------------------------- Comment By: Fabian Greffrath (fabian_deb) Date: 2011-05-18 13:35 Message: Just in case you didn't like the naive approach that my first patch took, here is another one: Put FindFirstChangeNotification into a loop and repeat it until ../etc is finally accessible, then proceed to the loop around FindNextChangeNotification as usual. I have tested this patch and it actually works as expected! I can start bash from a directory that contains nothing more than bash.exe itself and the needed shared libraries, i.e. my patched msys-1.0.dll and stock msys-iconv-2.dll, msys-intl-8.dll and msys-regex-1.dll. There is no etc directory in the directory above. With the stock msys-1.0.dll, bash just quits silently, but with my patch it actually runs. If I also copy over mount and msysmnt.exe, I can run mount to monitor the mount points. As long as there is no ../etc directory, mount will complain about a missing /etc/fstab and only show the usual mount points, e.g. c: -> /c. As soon as I create the etc directory in the directory above and add an entry to its fstab file, this new mount point is instantly available in bash as well (i.e. the loop around FindNextChangeNotification has successfully been entered). If I blank out the ../etc/fstab file again, the just added mount point instantly disappears. Removing the ../etc/fstab file or the ../etc directory is not possible ("access denied"), though, but I guess that is expected due to the watching thread and not a side effect of my patch. I'd be glad to provide more information or examples if needed! Cheers, Fabian ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=102435&aid=3302830&group_id=2435 |
