After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 674214 - g_rename (on Windows) will only work for closed files
g_rename (on Windows) will only work for closed files
Status: RESOLVED OBSOLETE
Product: glib
Classification: Platform
Component: win32
2.24.x
Other Windows
: Normal enhancement
: ---
Assigned To: gtk-win32 maintainers
gtk-win32 maintainers
Depends on:
Blocks:
 
 
Reported: 2012-04-16 18:08 UTC by John Emmas
Modified: 2018-05-24 14:02 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
3 x functions to modify the behaviour of g_rename() (Windows only) (10.84 KB, text/plain)
2012-04-16 18:08 UTC, John Emmas
  Details
Proposed patch (9.44 KB, patch)
2012-08-27 21:24 UTC, Erik van Pienbroek
none Details | Review
Proposed patch (with two memleaks resolved) (9.22 KB, patch)
2012-08-27 21:31 UTC, Erik van Pienbroek
none Details | Review
Updated patch (9.28 KB, patch)
2014-03-06 07:42 UTC, Erik van Pienbroek
none Details | Review

Description John Emmas 2012-04-16 18:08:23 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
Comment 1 Tristan Van Berkom 2012-04-20 17:11:14 UTC
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' ?
Comment 2 John Emmas 2012-04-21 13:20:47 UTC
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
Comment 3 Erik van Pienbroek 2012-08-27 21:24:56 UTC
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
Comment 4 Erik van Pienbroek 2012-08-27 21:31:46 UTC
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
Comment 5 John E 2012-11-27 16:23:13 UTC
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).
Comment 6 John E 2012-12-07 09:11:53 UTC
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
Comment 7 John E 2012-12-14 10:52:09 UTC
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;
Comment 8 LRN 2014-03-06 07:33:18 UTC
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.
Comment 9 Erik van Pienbroek 2014-03-06 07:42:18 UTC
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
Comment 10 Eduard Braun 2017-04-21 20:33:19 UTC
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.
Comment 11 John E 2017-04-22 06:48:49 UTC
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.
Comment 12 peterbudai 2017-04-27 07:24:53 UTC
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)?
Comment 13 John Emmas 2017-04-27 09:02:25 UTC
Sorry guys but I'm moving house today and I'll be offline for a few days now.
Comment 14 peterbudai 2017-05-02 11:16:30 UTC
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.
Comment 15 John Emmas 2017-05-12 08:08:49 UTC
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 :-(
Comment 16 peterbudai 2017-05-28 12:56:13 UTC
@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?
Comment 17 John Emmas 2017-05-28 14:26:50 UTC
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...
Comment 18 GNOME Infrastructure Team 2018-05-24 14:02:42 UTC
-- 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.