phobos commit, revision 1560


user: rsinfu

msg:
Fixed bugzilla 4188: std.file.remove throws Exception on success.

cenforce() used getErrno(), not GetLastError(), for errors happened in Win32 API.

http://www.dsource.org/projects/phobos/changeset/1560

On 5/26/2010 7:21 AM, dsource.org wrote:
> phobos commit, revision 1560
> 
> 
> user: rsinfu
> 
> msg:
> Fixed bugzilla 4188: std.file.remove throws Exception on success.
> 
> cenforce() used getErrno(), not GetLastError(), for errors happened in Win32 API.
> 
> http://www.dsource.org/projects/phobos/changeset/1560

This change seems wrong.

Is there a use of cenforce that's wrong?  Does the inclusion of getErrno need to be conditional on something?

Pure removal of the call to getErrno removes important error text that's useful in many contexts.

- Brad

Brad Roberts <braddr at puremagic.com> wrote:
> On 5/26/2010 7:21 AM, dsource.org wrote:
> > phobos commit, revision 1560
> > 
> > 
> > user: rsinfu
> > 
> > msg:
> > Fixed bugzilla 4188: std.file.remove throws Exception on success.
> > 
> > cenforce() used getErrno(), not GetLastError(), for errors happened in Win32 API.
> > 
> > http://www.dsource.org/projects/phobos/changeset/1560
> 
> This change seems wrong.
> 
> Is there a use of cenforce that's wrong?  Does the inclusion of getErrno need to be conditional on something?
> 
> Pure removal of the call to getErrno removes important error text that's useful in many contexts.
> 
> - Brad

It's correct.  getErrno (GetLastError) is automatically called as a
default argument.

Let's look inside the fixed cenforce():
--------------------
private T cenforce(T, string file = __FILE__, uint line = __LINE__)
(T condition, lazy const(char)[] name)
{
    if (!condition)
    {
        throw new FileException(
            text("In ", file, "(", line, "), data file ", name));
    }
    return condition;
}
--------------------
It throws FileException with a single string argument.  The called constructor is this:
--------------------
class FileException : Exception
{
    ...
    version(Windows) this(string name, uint errno = GetLastError)
    {
        this(name, sysErrorString(errno));
        this.errno = errno;
    }
    version(Posix) this(in char[] name, uint errno = .getErrno)
    {
        auto s = strerror(errno);
        this(name, to!string(s));
        this.errno = errno;
    }
}
--------------------
Note the default argument.  It utilizes appropriate error number on the compiled platform, and the constructor converts it to error message. So, the pure removal of getErrno works.  Or rather, passing getErrno() is definitely wrong on Windows.

Actually, FileExceptions are thrown in this way in other places of std/file.d.  For example:
--------------------
version(Windows) void rename(in char[] from, in char[] to)
{
    enforce(useWfuncs
            ? MoveFileW(std.utf.toUTF16z(from), std.utf.toUTF16z(to))
            : MoveFileA(toMBSz(from), toMBSz(to)),
            new FileException(
                text("Attempting to rename file ", from, " to ",
                        to)));
}
--------------------

I verified it, tested the code on Posix and Win32.  I got correct message on error.  Then I commited the code to the repository.

I'm sorry if my commit message caused you to misunderstand.  The commit message might be too short; I should have explained these things in the commit message.


Shin

I remember seeing what looked like a bug in remove() when working on the Windows port of rdmd. I don't know if it is related to what you just changed.

I think I got the problem figured out, but I never committed a fix.

First, create a file and remove it. Should be no exception, worked last time I saw it.

Next, create a file, open it in another process (I used a directory and cmd.exe), then remove it. Now, this is where I saw it throw an exception saying "success". I figure the problem was that the Windows function returned something saying "file in use, but delete is queued up", which enforce saw as an error, but GetLastError called success.

Probably a different bug than what you looked at, but maybe not.

On 5/27/10, Shin Fujishiro <rsinfu at gmail.com> wrote:
> Brad Roberts <braddr at puremagic.com> wrote:
>> On 5/26/2010 7:21 AM, dsource.org wrote:
>> > phobos commit, revision 1560
>> >
>> >
>> > user: rsinfu
>> >
>> > msg:
>> > Fixed bugzilla 4188: std.file.remove throws Exception on success.
>> >
>> > cenforce() used getErrno(), not GetLastError(), for errors happened in
>> > Win32 API.
>> >
>> > http://www.dsource.org/projects/phobos/changeset/1560
>>
>> This change seems wrong.
>>
>> Is there a use of cenforce that's wrong?  Does the inclusion of getErrno
>> need to
>> be conditional on something?
>>
>> Pure removal of the call to getErrno removes important error text that's
>> useful
>> in many contexts.
>>
>> - Brad
>
> It's correct.  getErrno (GetLastError) is automatically called as a
> default argument.
>
> Let's look inside the fixed cenforce():
> --------------------
> private T cenforce(T, string file = __FILE__, uint line = __LINE__)
> (T condition, lazy const(char)[] name)
> {
>     if (!condition)
>     {
>         throw new FileException(
>             text("In ", file, "(", line, "), data file ", name));
>     }
>     return condition;
> }
> --------------------
> It throws FileException with a single string argument.  The called constructor is this:
> --------------------
> class FileException : Exception
> {
>     ...
>     version(Windows) this(string name, uint errno = GetLastError)
>     {
>         this(name, sysErrorString(errno));
>         this.errno = errno;
>     }
>     version(Posix) this(in char[] name, uint errno = .getErrno)
>     {
>         auto s = strerror(errno);
>         this(name, to!string(s));
>         this.errno = errno;
>     }
> }
> --------------------
> Note the default argument.  It utilizes appropriate error number on the compiled platform, and the constructor converts it to error message. So, the pure removal of getErrno works.  Or rather, passing getErrno() is definitely wrong on Windows.
>
> Actually, FileExceptions are thrown in this way in other places of std/file.d.  For example:
> --------------------
> version(Windows) void rename(in char[] from, in char[] to)
> {
>     enforce(useWfuncs
>             ? MoveFileW(std.utf.toUTF16z(from), std.utf.toUTF16z(to))
>             : MoveFileA(toMBSz(from), toMBSz(to)),
>             new FileException(
>                 text("Attempting to rename file ", from, " to ",
>                         to)));
> }
> --------------------
>
> I verified it, tested the code on Posix and Win32.  I got correct message on error.  Then I commited the code to the repository.
>
> I'm sorry if my commit message caused you to misunderstand.  The commit message might be too short; I should have explained these things in the commit message.
>
>
> Shin
> _______________________________________________
> phobos mailing list
> phobos at puremagic.com
> http://lists.puremagic.com/mailman/listinfo/phobos
>

Adam Ruppe <destructionator at gmail.com> wrote:
> I remember seeing what looked like a bug in remove() when working on the Windows port of rdmd. I don't know if it is related to what you just changed.
> 
> I think I got the problem figured out, but I never committed a fix.
> 
> First, create a file and remove it. Should be no exception, worked last time I saw it.
> 
> Next, create a file, open it in another process (I used a directory and cmd.exe), then remove it. Now, this is where I saw it throw an exception saying "success". I figure the problem was that the Windows function returned something saying "file in use, but delete is queued up", which enforce saw as an error, but GetLastError called success.
> 
> Probably a different bug than what you looked at, but maybe not.
> 

Verified, it's the same bug.  A wrong error message was produced.

If one attempts to remove opened file, DeleteFile fails and sets win32 error code ERROR_SHARING_VIOLATION (32).  But cenforce used errno (instead of GetLastError) for producing win32 error message.  errno was zero, so the "success" error message was produced*.

* zero == win32 error code ERROR_SUCCESS

Shin