Bugs item #3302830, was opened at 2011-05-16 05:05
Message generated for change (Comment added) made by cstrauss
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: Closed
Resolution: Accepted
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: Cesar Strauss (cstrauss)
Date: 2012-11-29 14:18
Message:
The fix for this issue was released with MSYS runtime 1.0.18,
----------------------------------------------------------------------
Comment By: Cesar Strauss (cstrauss)
Date: 2011-10-12 12:04
Message:
The patch is now in CVS.
I modified it to use a separate handle to monitor the root directory, The
idea is to keep the handle open until /etc is created, instead of getting a
new handle every time a change happens in the root.
Thanks,
Cesar
----------------------------------------------------------------------
Comment By: Cesar Strauss (cstrauss)
Date: 2011-08-24 18:04
Message:
Thanks for the reminder and sorry for the delay. I'll include it in the
next release.
Regards,
Cesar
----------------------------------------------------------------------
Comment By: Earnie Boyd (earnie)
Date: 2011-08-24 05:15
Message:
You can ask but the decision of inclusion is still Cesar's to make. He is
the current maintainer.
----------------------------------------------------------------------
Comment By: Fabian Greffrath (fabian_deb)
Date: 2011-08-24 02: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 05: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 05: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 07:07
Message:
Add some debug_printf and quit gracefully if / is untraceable.
----------------------------------------------------------------------
Comment By: Earnie Boyd (earnie)
Date: 2011-05-27 06: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 05: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 07: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 07: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 05: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 07: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 05: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 01: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-19 15: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-19 15: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 01: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 00: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-18 15: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 04: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
|
