GNOME Bugzilla – Bug 674214
g_rename (on Windows) will only work for closed files
Last modified: 2018-05-24 14:02:42 UTC
Created attachment 212164 [details] 3 x functions to modify the behaviour of g_rename() (Windows only) I'm using glib-win32, version 2.24. In that version, the functions g_open(), g_creat() and g_fopen() defer to _wopen(), _wcreat() and _wfopen() respectively. This is very similar to the corresponding arrangement for Linux. Unfortunately however, those Windows functions do not support renaming a file whilst it's open. As a result, g_rename() behaves differently on the Windows platform compared to its Linux behaviour, where files can be renamed - even while there are file handles still open. To get around this restriction I've re-written those 3 functions to use CreateFile() on the Windows platform. I've managed to keep the API exactly the same as it was previously - but the flags and attributes expected by CreateFile() are totally different from _wopen() etc. So I've needed to convert the incoming flags and attributes to their appropriate counterparts (which makes the code look very longwinded). I've been using the modified code for several weeks and am very pleased with the result. However, it would benefit from someone else reviewing my changes. Two pairs of eyes are always better than one! I'd like to offer my revisions to be considered for adoption. Please note that there was no need to modify g_rename() itself. I've attached a small 'C' file which includes the new versions of those 3 functions in their entirety. If accepted, they'd need to replace the functions already in gstdio.c With thanks. John Emmas
Hi John, What you should submit here is a patch that applies cleanly with todays git master. Submitting a file including three functions means that whoever tries to build your modification needs to patch their sources manually. You can create a patch for glib in the following way: a.) Make modifications to your clone of glib git master b.) git commit -a (commit your changes locally, log changes) c.) git format-patch HEAD^ (create the actual patch) Since I don't know for sure that someone is in charge of win32, it could be helpful to provide a short test case which shows that your patch fixes the issue (see it break with current git, see it fixed with your patches). The test case would probably be best integrated in glib/tests/Makefile.am so that it can run with other tests in the suite every time `make check' is run (however just a standalone test that anyone can compile would still be great). In any case I don't maintain glib (and I can't say that I can really grasp the gravity of the internal file handle changes you've made), I think we lost our long time win32 guy and I'm just saying that with an easy to apply patch and a test case to prove that the issues are fixed, the patch will have better chances of landing cleanly and quickly. Do all the glib tests pass on your win32 machine when your run 'make check' ?
On 20 Apr 2012, at 18:11, glib (bugzilla.gnome.org) wrote: " Do all the glib tests pass on your win32 machine when your run 'make check' ? " Hi Tristan. I'm not actually using 'make' (I'm building with Visual C++) but yes, all the tests still pass apart from 'g_test_trap_fork' which says it isn't implemented on windows. " What you should submit here is a patch that applies cleanly with todays git master. Submitting a file including three functions means that whoever tries to build your modification needs to patch their sources manually. You can create a patch for glib in the following way: a.) Make modifications to your clone of glib git master b.) git commit -a (commit your changes locally, log changes) c.) git format-patch HEAD^ (create the actual patch) " I'd help if I could but I'm not running git at present (it'll happen sometime this year but not immediately). Having said that, in my current glib version (2.24) the functions 'g_open()' 'g_creat()' and 'g_fopen()' are little more than wrappers around their system counterparts. There's only a few lines of code in each function. If that's still true in the current version it should be pretty simple for someone to patch the file manually. Only 3 functions need to be replaced and they're all in gstdio.c. All that's needed is to delete (or comment out) the existing functions and try my versions in their place. It can't be more than a couple of minutes work. And you could even add a new test to validate that an open file can now be renamed.! John
Created attachment 222587 [details] [review] Proposed patch As the submitter didn't supply a patch in the right format I decided to translate it into the correct format as it fixes a Win32-specific issue I'm also experiencing with glib. I also fixed two small memleaks which were in the original proposed files
Created attachment 222589 [details] [review] Proposed patch (with two memleaks resolved) Apparently I uploaded the copy which still had the two memory leaks... Here's a version of the patch which should be memleak-free
Did my / Erik's patch ever make it into the official source code? I guess not because I made a git clone a few days ago and discovered that gstdio.c is still using the original functions (_wcreat() etc).
Sorry to bump this again. Did these fixes somehow get forgotten about or was the patch unacceptable for some reason? If the changes were unacceptable, I suppose I'll have to apply the patch locally but I don't want to do that if things are already 'in the pipeline'. John
I applied the patch locally this morning but in doing so, I discovered a small typo at line 231. The line:- errno - EINVAL; should of course read:- errno = EINVAL;
I totally agree with the direction this patch is going to take glib. Whenever you can get away with using W32 API instead of MS' half-assed CRT implementation, use W32 API, as it's often more powerful (also, it's versioning information is more complete and easier to check). The code looks sane at the first glance.
Created attachment 271071 [details] [review] Updated patch This updated version of the patch contains a fix for the typo mentioned earlier and various markup improvements
I'm a bit skeptical towards this patch, as it introduces a lot of code that might break down in certain cases. Also it fixes one issue, but sacrifices functionality for it which might not be desirable. If I didn't overlook anything there's at least one issue: 1. The patch looks for the binary modifier at the end (i.e. 'w+b') while the ISO C specification allows changed ordering (e.g. 'wb+'). Furthermore there could be compatibility issues: 2. Some (maybe most) implementations choose to ignore additional characters in the mode argument (which is also suggested in the ISO C specification). The current patch fails if (strlen(mode) > 3) but also if an unknown character is appended to a shorter modifier. 2b. Some implementations offer many more specifiers (even Microsoft's implementation does) which are impossible to use with this patch.
FWIW in Mixbus and Ardour we've been using this patch since 2012 and it hasn't broken anything up to now. I also mentioned (in comment #2) that glib's tests were all still passing, except for one which isn't relevant on Windows. Is that no longer the case? There's a wider issue here though... As LRN mentioned, a couple of posts ago, Microsoft's CRT implementation is particularly weak in the area of file handling (for example, the limitation of only 2,000 open files across all running apps). Most users will never notice this but by it's very nature, a product like Ardour or Mixbus (a DAW) will often need to hold thousands of files open simultaneously. If someone could figure out a way to achieve this (without breaking compatibility for the non-Windows platforms) it would be a major step forward for glib.
I tend to agree with Eduard. The current code is not handling all filemodes definitely. As we are talking about a patch which has been originated 5 years and 2 major OS releases ago: Are we sure that we still have the original issue aka renaming a file whilst it's open? Could we test it to see that the problem still exist today using the original (unpatched) GLIB implementation (which uses _wopen, _wfopen etc)?
Sorry guys but I'm moving house today and I'll be offline for a few days now.
I have tested over the weekend with the current OS (W10) and MS CRT: it is the same behaviour, if you open file with _wopen(), _wcreat() and _wfopen(), it opens in exclusive mode, and you cannot rename the files using g_rename()/ MoveFileExW . I see two possible paths: 1. Either we accept these limitations of the underlying CRT, and use the existing GLib code without the proposed patch. That would mean that FWIW and Mixbus would be in a difficult situation to find alternative solution within in the respective programs. 2. Take the existing initial patch with CreateFile(), amd make it more robust and fool-proof to work with additional filemodes and check for other potential pitfalls. Also this means that g_rename should be reviewd/changed as well.
For the longer term, is there maybe a third way forward..? g_open() / g_creat() etc currently return an int. Maybe they should return an opaque type which would be still int for the non-Windows platforms. On Windows, it could still be technically int - but in practice, it could be considered as being HANDLE (void* IIRC). I'm sure this could be achieved without too much disruption to the API and it would solve a lot of significant problems, apart from this g_rename() one. For example, I mentioned earlier that for the conventional (DOS style) handles, Windows has a limit of only 2,048 open handles across all running apps. I do appreciate that 2,048 might seem like a lot of open files but I can promise you, it isn't. Applications like Mixbus typically need to keep thousands of files open simultaneously. And I'm sorry guys, but glib has proven to be a real handicap here. It's very much preventing us from building a 'pro' version of Mixbus :-(
@John Emmas: just out of curiosity: have you tested these changes in 64-bit version of glib? Or a more broader question: Mixbus and Ardour: are they using the 32-bit glib or do they have a 64-bit version using 64-bit glib?
Peter:- I personally haven't tested a 64-bit build (it's 5 years since I first reported this issue) but yes, Mixbus now gets built for Windows in both 32-bit and 64-bit flavours. AFAIK my original modifications are still getting used in both versions and I'm pretty sure nobody's reported any problems. For Windows though, I still think the best way forward would be if we could find a way to stop using the DOS-style functions (such as 'open' etc). On Windows those functions have other serious limitations, apart from just the rename() one...
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/539.