all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#10733: 24.0.93; w32 file truncation
@ 2012-02-05 22:34 Ota, Takaaki
  2012-02-05 23:51 ` Juanma Barranquero
  0 siblings, 1 reply; 27+ messages in thread
From: Ota, Takaaki @ 2012-02-05 22:34 UTC (permalink / raw)
  To: 10733

When opening a file on an NTFS file system the file opens as if the
content is truncated to size of 65375 characters.  This happens when
the file is an NTFS symbolic link which is made by mklink command of
cmd.exe.  There is no problem if the target file of the NTFS symbolic
link is smaller than this size.

-Tak

--text follows this line--
This bug report will be sent to the Bug-GNU-Emacs mailing list
and the GNU bug tracker at debbugs.gnu.org.  Please check that
the From: line contains a valid email address.  After a delay of up
to one day, you should receive an acknowledgement at that address.

Please write in English if possible, as the Emacs maintainers
usually do not have translators for other languages.

Please describe exactly what actions triggered the bug, and
the precise symptoms of the bug.  If you can, give a recipe
starting from `emacs -Q':



If Emacs crashed, and you have the Emacs process in the gdb debugger,
please include the output from the following gdb commands:
    `bt full' and `xbacktrace'.
For information about debugging Emacs, please read the file
c:/emacs/emacs-24.0.93/etc/DEBUG.


In GNU Emacs 24.0.93.1 (i386-mingw-nt6.1.7601)
 of 2012-01-29 on TAK-VAIO-Z1290
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --with-gcc (4.5) --cflags -Ic:/d/pub/emacs/include --ldflags
 -Lc:/d/pub/emacs/lib'

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: ENU
  value of $XMODIFIERS: nil
  locale-coding-system: cp1252
  default enable-multibyte-characters: t

Major mode: Fundamental

Minor modes in effect:
  tooltip-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
ESC x f i n d SPC f i SPC <return> m e m o <return> 
<next> <next> <next> <next> <next> <next> <next> <next> 
<next> <next> <next> <next> <next> <next> <next> <next> 
<next> <next> <next> <next> <next> <next> <next> <next> 
<next> <next> <next> <next> <next> <next> <next> <next> 
<next> <next> <next> <next> <next> <next> <next> <next> 
<next> <next> <next> <next> <next> <next> <next> <next> 
<next> <next> <next> <next> <next> <next> <next> <next> 
<next> <next> <next> <next> <next> <next> <next> <next> 
<next> <next> <next> <next> <next> <next> <next> <next> 
<next> <next> <next> <next> <next> <next> <next> <next> 
<next> <down> <down> <down> <down> <down> <down> <down> 
<down> <down> <down> <down> <down> <down> <down> <down> 
<down> <down> <down> <down> <down> <down> <down> <down> 
<down> <down> <down> <down> <down> <down> <down> <down> 
<down> <down> <down> <down> <down> <down> <down> <down> 
<down> <down> <down> ESC : ( p o i n t ) <return> ESC 
: r e p o r t SPC C-g ESC x r e p o r SPC e m SPC SPC 
<return>

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
You can run the command `find-file' with C-x C-f
scroll-up-command: End of buffer [21 times]
byte-code: End of buffer [21 times]
65375 (#o177537, #xff5f)
Quit

Load-path shadows:
None found.

Features:
(shadow sort gnus-util mail-extr message format-spec rfc822 mml easymenu
mml-sec mm-decode mm-bodies mm-encode mail-parse rfc2231 rfc2047 rfc2045
ietf-drums mm-util mail-prsvr mailabbrev mail-utils gmm-utils mailheader
emacsbug time-date tooltip ediff-hook vc-hooks lisp-float-type mwheel
dos-w32 disp-table ls-lisp w32-win w32-vars tool-bar dnd fontset image
fringe lisp-mode register page menu-bar rfn-eshadow timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core frame cham
georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese hebrew greek romanian slovak czech european ethiopic
indian cyrillic chinese case-table epa-hook jka-cmpr-hook help simple
abbrev minibuffer button faces cus-face files text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote make-network-process multi-tty emacs)






^ permalink raw reply	[flat|nested] 27+ messages in thread

* bug#10733: 24.0.93; w32 file truncation
  2012-02-05 22:34 bug#10733: 24.0.93; w32 file truncation Ota, Takaaki
@ 2012-02-05 23:51 ` Juanma Barranquero
  2012-02-06  0:16   ` Ota, Takaaki
  2012-02-06  0:20   ` Óscar Fuentes
  0 siblings, 2 replies; 27+ messages in thread
From: Juanma Barranquero @ 2012-02-05 23:51 UTC (permalink / raw)
  To: Ota, Takaaki; +Cc: 10733

> When opening a file on an NTFS file system the file opens as if the
> content is truncated to size of 65375 characters.  This happens when
> the file is an NTFS symbolic link which is made by mklink command of
> cmd.exe.  There is no problem if the target file of the NTFS symbolic
> link is smaller than this size.

Can you please give a step-by-step recipe, starting from "emacs -Q"?

Thanks,

    Juanma





^ permalink raw reply	[flat|nested] 27+ messages in thread

* bug#10733: 24.0.93; w32 file truncation
  2012-02-05 23:51 ` Juanma Barranquero
@ 2012-02-06  0:16   ` Ota, Takaaki
  2012-02-06  4:05     ` Eli Zaretskii
  2012-02-06  0:20   ` Óscar Fuentes
  1 sibling, 1 reply; 27+ messages in thread
From: Ota, Takaaki @ 2012-02-06  0:16 UTC (permalink / raw)
  To: lekktu; +Cc: 10733

Yes, I can do that.

1. Have a text file that has more than 65375 characters.  Let's call
   this text file test.txt.

2. Run cmd.exe and change directory to where test.txt is located.

3. Then execute the next command in the cmd.

  mklink link.txt test.txt

4. Now run emacs as "emacs -Q link.txt" and do M-> (end-of-buffer).

5. Find that you are not seeing the end of the file but somewhere
   around 65375 character location and the rest of the file is not
   visible.

-Tak

Sun, 5 Feb 2012 15:51:40 -0800: Juanma Barranquero <lekktu@gmail.com> wrote:

> > When opening a file on an NTFS file system the file opens as if the
> > content is truncated to size of 65375 characters.  This happens when
> > the file is an NTFS symbolic link which is made by mklink command of
> > cmd.exe.  There is no problem if the target file of the NTFS symbolic
> > link is smaller than this size.
> 
> Can you please give a step-by-step recipe, starting from "emacs -Q"?
> 
> Thanks,
> 
>     Juanma
> 






^ permalink raw reply	[flat|nested] 27+ messages in thread

* bug#10733: 24.0.93; w32 file truncation
  2012-02-05 23:51 ` Juanma Barranquero
  2012-02-06  0:16   ` Ota, Takaaki
@ 2012-02-06  0:20   ` Óscar Fuentes
  1 sibling, 0 replies; 27+ messages in thread
From: Óscar Fuentes @ 2012-02-06  0:20 UTC (permalink / raw)
  To: Juanma Barranquero; +Cc: Ota, Takaaki, 10733

Juanma Barranquero <lekktu@gmail.com> writes:

>> When opening a file on an NTFS file system the file opens as if the
>> content is truncated to size of 65375 characters.  This happens when
>> the file is an NTFS symbolic link which is made by mklink command of
>> cmd.exe.  There is no problem if the target file of the NTFS symbolic
>> link is smaller than this size.
>
> Can you please give a step-by-step recipe, starting from "emacs -Q"?

I can reproduce the problem on Windows 7 64 bits:

* Run a cmd shell as Administrator (mklink is a privileged command).

* Navigate to a directory with a text file larger than 64 KB (let's
  suppose that the file is named foo.txt).

* mklink bar.txt foo.txt.

* runemacs -Q bar.txt

* The file appears truncated in Emacs.

Very odd.





^ permalink raw reply	[flat|nested] 27+ messages in thread

* bug#10733: 24.0.93; w32 file truncation
  2012-02-06  0:16   ` Ota, Takaaki
@ 2012-02-06  4:05     ` Eli Zaretskii
  2012-02-06  5:25       ` Óscar Fuentes
  0 siblings, 1 reply; 27+ messages in thread
From: Eli Zaretskii @ 2012-02-06  4:05 UTC (permalink / raw)
  To: Ota, Takaaki; +Cc: lekktu, 10733

> Date: Sun, 5 Feb 2012 16:16:23 -0800
> From: "Ota, Takaaki" <Takaaki.Ota@am.sony.com>
> Cc: 10733@debbugs.gnu.org
> 
> Yes, I can do that.
> 
> 1. Have a text file that has more than 65375 characters.  Let's call
>    this text file test.txt.
> 
> 2. Run cmd.exe and change directory to where test.txt is located.
> 
> 3. Then execute the next command in the cmd.
> 
>   mklink link.txt test.txt
> 
> 4. Now run emacs as "emacs -Q link.txt" and do M-> (end-of-buffer).
> 
> 5. Find that you are not seeing the end of the file but somewhere
>    around 65375 character location and the rest of the file is not
>    visible.

Emacs on Windows doesn't support symlinks; they are new with Vista and
Windows 7.  Someone needs to write the code to support that, before
this can be considered a bug (as opposed to a feature request).

What you describe seems to indicate that insert-file-contents only
reads the first 64KB portion of the file, and then bails out.  Running
Emacs under debugger and reporting where it bails out of the reading
loop might help coming up with some band-aid solution.  But full
support of symlink requires specific support code, that just isn't
there at this time.





^ permalink raw reply	[flat|nested] 27+ messages in thread

* bug#10733: 24.0.93; w32 file truncation
  2012-02-06  4:05     ` Eli Zaretskii
@ 2012-02-06  5:25       ` Óscar Fuentes
  2012-02-06 16:16         ` Óscar Fuentes
  2012-02-06 16:55         ` Eli Zaretskii
  0 siblings, 2 replies; 27+ messages in thread
From: Óscar Fuentes @ 2012-02-06  5:25 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: lekktu, Ota, Takaaki, 10733

Eli Zaretskii <eliz@gnu.org> writes:

[snip]

> Emacs on Windows doesn't support symlinks; they are new with Vista and
> Windows 7.  Someone needs to write the code to support that, before
> this can be considered a bug (as opposed to a feature request).
>
> What you describe seems to indicate that insert-file-contents only
> reads the first 64KB portion of the file, and then bails out.  Running
> Emacs under debugger and reporting where it bails out of the reading
> loop might help coming up with some band-aid solution.  But full
> support of symlink requires specific support code, that just isn't
> there at this time.

NTFS symlinks are transparent to applications. Emacs doesn't need to
know that it is accessing a symlink to correctly read and save the file.

I bet it is a bug on the CRT (the stat call that retrieves the file
size, to be precise). Maybe it is a MinGW thing. It would be interesting
to hear reports from people using an Emacs compiled with a modern
version of Visual Studio, or even know if people using the most recent
MinGW CRT release can duplicate the problem.

This is a very serious bug (potential data loss). I'll put this on my
TODO list and hopefully run a debug session on a Windows 7 machine next
week. Meanwhile, if anyone can provide the info mentioned on the
previous paragraph, it would be helpful.





^ permalink raw reply	[flat|nested] 27+ messages in thread

* bug#10733: 24.0.93; w32 file truncation
  2012-02-06  5:25       ` Óscar Fuentes
@ 2012-02-06 16:16         ` Óscar Fuentes
  2012-02-06 17:21           ` Eli Zaretskii
  2012-02-06 16:55         ` Eli Zaretskii
  1 sibling, 1 reply; 27+ messages in thread
From: Óscar Fuentes @ 2012-02-06 16:16 UTC (permalink / raw)
  To: 10733; +Cc: lekktu, Ota, Takaaki

Óscar Fuentes <ofv@wanadoo.es> writes:

> I bet it is a bug on the CRT (the stat call that retrieves the file
> size, to be precise). Maybe it is a MinGW thing.

No, it is an Emacs thing. `stat' is defined in lib-src/ntlib.c,
overriding the MSVCRT implementation, which accounts for symlinks, while
Emacs' does not.

Before the definition of `stat' on lib-src/ntlib.c there is this
comment:

/* We need this because nt/inc/sys/stat.h defines struct stat that is
   incompatible with the MS run-time libraries.  */

That looks like an understatement. Actually, we need our own stat
function and struct because the `struct stat' that Emacs uses is
incompatible with the one defined in MSVCRT, right?

The obvious fix does not seem difficult, although ugly and
verbose. OTOH, I'll like to remove the Emacs reimplementation of `stat',
but that looks more cumbersome. How much time we have until the release?

BTW, the obvious fix may require some care for not breaking Emacs
support on MS Windows versions prior to XP. We still support Windows 9x,
don't we?





^ permalink raw reply	[flat|nested] 27+ messages in thread

* bug#10733: 24.0.93; w32 file truncation
  2012-02-06  5:25       ` Óscar Fuentes
  2012-02-06 16:16         ` Óscar Fuentes
@ 2012-02-06 16:55         ` Eli Zaretskii
  1 sibling, 0 replies; 27+ messages in thread
From: Eli Zaretskii @ 2012-02-06 16:55 UTC (permalink / raw)
  To: Óscar Fuentes; +Cc: lekktu, Takaaki.Ota, 10733

> From: Óscar Fuentes <ofv@wanadoo.es>
> Cc: "Ota\, Takaaki" <Takaaki.Ota@am.sony.com>,  lekktu@gmail.com,  10733@debbugs.gnu.org
> Date: Mon, 06 Feb 2012 06:25:32 +0100
> 
> NTFS symlinks are transparent to applications.

Well, evidently, they aren't.

> Emacs doesn't need to know that it is accessing a symlink to
> correctly read and save the file.

Evidently, it does.






^ permalink raw reply	[flat|nested] 27+ messages in thread

* bug#10733: 24.0.93; w32 file truncation
  2012-02-06 16:16         ` Óscar Fuentes
@ 2012-02-06 17:21           ` Eli Zaretskii
  2012-02-06 17:58             ` Óscar Fuentes
  0 siblings, 1 reply; 27+ messages in thread
From: Eli Zaretskii @ 2012-02-06 17:21 UTC (permalink / raw)
  To: Óscar Fuentes; +Cc: lekktu, Takaaki.Ota, 10733

> From: Óscar Fuentes <ofv@wanadoo.es>
> Cc: Eli Zaretskii <eliz@gnu.org>,  lekktu@gmail.com,  "Ota\, Takaaki" <Takaaki.Ota@am.sony.com>
> Date: Mon, 06 Feb 2012 17:16:52 +0100
> 
> Óscar Fuentes <ofv@wanadoo.es> writes:
> 
> > I bet it is a bug on the CRT (the stat call that retrieves the file
> > size, to be precise). Maybe it is a MinGW thing.
> 
> No, it is an Emacs thing. `stat' is defined in lib-src/ntlib.c,
> overriding the MSVCRT implementation, which accounts for symlinks, while
> Emacs' does not.

Can you tell the details, please?  Specifically, what would it take to
"account for symlinks" in our implementation of `stat'?  You did say
symlinks were supposed to be transparent.

> Before the definition of `stat' on lib-src/ntlib.c there is this
> comment:
> 
> /* We need this because nt/inc/sys/stat.h defines struct stat that is
>    incompatible with the MS run-time libraries.  */
> 
> That looks like an understatement. Actually, we need our own stat
> function and struct because the `struct stat' that Emacs uses is
> incompatible with the one defined in MSVCRT, right?

No, you are missing the point of that comment.  lib-src/ntlib.c is not
compiled into Emacs, it's compiled into lib-src programs.
Theoretically, since those programs don't need anything fancy from
`stat', they could use the stock MSVCRT implementation.  But because
these programs are compiled with -I../nt/inc, the compiler picks up
the definition of `struct stat' that is used by Emacs, and because of
this incompatibility lib-src programs cannot use the MSVCRT
implementation of `stat'.

> The obvious fix does not seem difficult, although ugly and
> verbose.

Can you please describe the problem, in addition to what you propose
to be a solution?

> I'll like to remove the Emacs reimplementation of `stat'

That is a non-starter.  The private implementation of `stat' is needed
to support Posix features, such as meaningful inode numbers, UID and
GID, etc.  You won't find anything close to that in MSVCRT.  Quite a
few parts in Emacs expect those features.

> How much time we have until the release?

We cannot afford to make such a change before the release, no matter
how far away is it, even if I'd agree to that (which I don't).  `stat'
is too central to Emacs operation to make such changes at this time.

> BTW, the obvious fix may require some care for not breaking Emacs
> support on MS Windows versions prior to XP. We still support Windows 9x,
> don't we?

Yes, we do.  In fact, Emacs 24.1 will again work on Windows 9X, after
it turned out that 23.x (and perhaps also 22.x) didn't.






^ permalink raw reply	[flat|nested] 27+ messages in thread

* bug#10733: 24.0.93; w32 file truncation
  2012-02-06 17:21           ` Eli Zaretskii
@ 2012-02-06 17:58             ` Óscar Fuentes
  2012-02-06 18:37               ` Eli Zaretskii
  0 siblings, 1 reply; 27+ messages in thread
From: Óscar Fuentes @ 2012-02-06 17:58 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: lekktu, Takaaki.Ota, 10733

Eli Zaretskii <eliz@gnu.org> writes:

>> > I bet it is a bug on the CRT (the stat call that retrieves the file
>> > size, to be precise). Maybe it is a MinGW thing.
>> 
>> No, it is an Emacs thing. `stat' is defined in lib-src/ntlib.c,
>> overriding the MSVCRT implementation, which accounts for symlinks, while
>> Emacs' does not.
>
> Can you tell the details, please?  Specifically, what would it take to
> "account for symlinks" in our implementation of `stat'?  You did say
> symlinks were supposed to be transparent.

Transparent for applications, yes. The CRT is not an application. If you
replace CRT functions with your own, the transparency goes away.

>> Before the definition of `stat' on lib-src/ntlib.c there is this
>> comment:
>> 
>> /* We need this because nt/inc/sys/stat.h defines struct stat that is
>>    incompatible with the MS run-time libraries.  */
>> 
>> That looks like an understatement. Actually, we need our own stat
>> function and struct because the `struct stat' that Emacs uses is
>> incompatible with the one defined in MSVCRT, right?
>
> No, you are missing the point of that comment.  lib-src/ntlib.c is not
> compiled into Emacs, it's compiled into lib-src programs.
> Theoretically, since those programs don't need anything fancy from
> `stat', they could use the stock MSVCRT implementation.  But because
> these programs are compiled with -I../nt/inc, the compiler picks up
> the definition of `struct stat' that is used by Emacs, and because of
> this incompatibility lib-src programs cannot use the MSVCRT
> implementation of `stat'.

I think you are saying essentially the same as I do but with very
different words.

OTOH, you are saying "lib-src/ntlib.c is not compiled into Emacs, it's
compiled into lib-src programs." and the key hypotheses made by me here
is that Emacs uses the `stat' definition on lib-src/ntlib.c. Is that
correct?

>> The obvious fix does not seem difficult, although ugly and
>> verbose.
>
> Can you please describe the problem, in addition to what you propose
> to be a solution?

Symlinks are detected and handled specially on MSVCRT's stat. In
aessence, for symlinks it uses fstat. It all amounts to 15 lines of
simple code. But that's not directly transposable to Emacs' stat because
we need to access and translate the MSVCRT `struct stat' that `fstat'
creates to the Emacs' `struct stat'. That final part is the "ugly and
verbose" part.

>> I'll like to remove the Emacs reimplementation of `stat'
>
> That is a non-starter.  The private implementation of `stat' is needed
> to support Posix features, such as meaningful inode numbers, UID and
> GID, etc.  You won't find anything close to that in MSVCRT.  Quite a
> few parts in Emacs expect those features.
>
>> How much time we have until the release?
>
> We cannot afford to make such a change before the release, no matter
> how far away is it, even if I'd agree to that (which I don't).  `stat'
> is too central to Emacs operation to make such changes at this time.

Such change would be conceptually straightforward and quite safe. It
amounts to using MSVCRT `stat' and translating its `struct stat' to
Emacs' `struct stat'. Instead of defining our own `stat' and `struct
stat' we define `emacs_w32_stat' and `struct emacs_w32_stat' and do

#if windows
#define stat emacs_w32_stat
#endif

so no changes are required outside lib-src/ntlib.c and nt/sys/stat.h.

I don't get your mention to inode numbers, UID and GID. The
implementation on ntlib.c simply does

  buf->st_ino = 0;

and I see no references to UID and GID. Maybe those are obtained
elsewhere. As far as I can see a tranlation from MSVCRT's `struct stat'
to Emacs' is possible.

>> BTW, the obvious fix may require some care for not breaking Emacs
>> support on MS Windows versions prior to XP. We still support Windows 9x,
>> don't we?
>
> Yes, we do.  In fact, Emacs 24.1 will again work on Windows 9X, after
> it turned out that 23.x (and perhaps also 22.x) didn't.

So we are reintroducing support for a legacy OS after being de facto
removed for several years. Curious.





^ permalink raw reply	[flat|nested] 27+ messages in thread

* bug#10733: 24.0.93; w32 file truncation
  2012-02-06 17:58             ` Óscar Fuentes
@ 2012-02-06 18:37               ` Eli Zaretskii
  2012-02-06 20:19                 ` Ota, Takaaki
  2012-02-06 23:27                 ` Óscar Fuentes
  0 siblings, 2 replies; 27+ messages in thread
From: Eli Zaretskii @ 2012-02-06 18:37 UTC (permalink / raw)
  To: Óscar Fuentes; +Cc: lekktu, Takaaki.Ota, 10733

> From: Óscar Fuentes <ofv@wanadoo.es>
> Cc: lekktu@gmail.com,  Takaaki.Ota@am.sony.com,  10733@debbugs.gnu.org
> Date: Mon, 06 Feb 2012 18:58:15 +0100
> 
> >> /* We need this because nt/inc/sys/stat.h defines struct stat that is
> >>    incompatible with the MS run-time libraries.  */
> >> 
> >> That looks like an understatement. Actually, we need our own stat
> >> function and struct because the `struct stat' that Emacs uses is
> >> incompatible with the one defined in MSVCRT, right?
> >
> > No, you are missing the point of that comment.  lib-src/ntlib.c is not
> > compiled into Emacs, it's compiled into lib-src programs.
> > Theoretically, since those programs don't need anything fancy from
> > `stat', they could use the stock MSVCRT implementation.  But because
> > these programs are compiled with -I../nt/inc, the compiler picks up
> > the definition of `struct stat' that is used by Emacs, and because of
> > this incompatibility lib-src programs cannot use the MSVCRT
> > implementation of `stat'.
> 
> I think you are saying essentially the same as I do but with very
> different words.

No, I'm not.

> OTOH, you are saying "lib-src/ntlib.c is not compiled into Emacs, it's
> compiled into lib-src programs." and the key hypotheses made by me here
> is that Emacs uses the `stat' definition on lib-src/ntlib.c. Is that
> correct?

No.  The `stat' Emacs uses is in w32.c.  What you see on
lib-src/ntlib.c is just a minimal subset of what we have in w32.c.

> Symlinks are detected and handled specially on MSVCRT's stat. In
> aessence, for symlinks it uses fstat.

But fstat probably calls GetFileInformationByHandle under the hood,
and we already call that function in w32.c:stat.  So maybe the fix is
not as ugly and inelegant as you thought.

> > We cannot afford to make such a change before the release, no matter
> > how far away is it, even if I'd agree to that (which I don't).  `stat'
> > is too central to Emacs operation to make such changes at this time.
> 
> Such change would be conceptually straightforward and quite safe. It
> amounts to using MSVCRT `stat' and translating its `struct stat' to
> Emacs' `struct stat'. Instead of defining our own `stat' and `struct
> stat' we define `emacs_w32_stat' and `struct emacs_w32_stat' and do
> 
> #if windows
> #define stat emacs_w32_stat
> #endif

This doesn't work.  I don't remember all the details at the moment,
but it's not a coincidence we define `stat' in w32.c, not `sys_stat',
as we do with other functions.  One immediate problem with this is
that we also have our own implementation of `fstat' on w32.c, and they
must both share the same `struct stat'.  It gets really ugly, believe
me.

Anyway, I don't think we need to call MSVCRT's `fstat' to fix this,
see above.

> I don't get your mention to inode numbers, UID and GID. The
> implementation on ntlib.c simply does
> 
>   buf->st_ino = 0;
> 
> and I see no references to UID and GID.

See above: you are looking at the wrong `stat'.  The one that caused
all this is on w32.c.

> >> BTW, the obvious fix may require some care for not breaking Emacs
> >> support on MS Windows versions prior to XP. We still support Windows 9x,
> >> don't we?
> >
> > Yes, we do.  In fact, Emacs 24.1 will again work on Windows 9X, after
> > it turned out that 23.x (and perhaps also 22.x) didn't.
> 
> So we are reintroducing support for a legacy OS after being de facto
> removed for several years. Curious.

It wasn't a "removal", it was a tricky bug.

Anyway, see this discussion:

  http://lists.gnu.org/archive/html/emacs-devel/2011-07/msg00785.html
  http://lists.gnu.org/archive/html/emacs-devel/2011-07/msg00827.html






^ permalink raw reply	[flat|nested] 27+ messages in thread

* bug#10733: 24.0.93; w32 file truncation
  2012-02-06 18:37               ` Eli Zaretskii
@ 2012-02-06 20:19                 ` Ota, Takaaki
  2012-02-06 20:23                   ` Lennart Borgman
                                     ` (2 more replies)
  2012-02-06 23:27                 ` Óscar Fuentes
  1 sibling, 3 replies; 27+ messages in thread
From: Ota, Takaaki @ 2012-02-06 20:19 UTC (permalink / raw)
  To: eliz; +Cc: ofv, lekktu, 10733

For your information:

(file-attributes "memo")
(nil 1 544 513 (20269 7834) (20269 7834) (20269 7834) 0 "-rw-rw-rw-" nil (8448 8 . 22758) (62004 . 15649))
(file-attributes "memo.old")
(nil 1 544 513 (19686 63524) (20262 20973) (19686 63524) 233153 "-r--r--r--" nil (512 5 . 24351) (62004 . 15649))

where "memo" is the NTFS symlink and "memo.old" is a real file.  I
don't know how the size 0 on symlink side is translated into 64K.

-Tak

Mon, 6 Feb 2012 10:37:56 -0800: Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Óscar Fuentes <ofv@wanadoo.es>
> > Cc: lekktu@gmail.com,  Takaaki.Ota@am.sony.com,  10733@debbugs.gnu.org
> > Date: Mon, 06 Feb 2012 18:58:15 +0100
> > 
> > >> /* We need this because nt/inc/sys/stat.h defines struct stat that is
> > >>    incompatible with the MS run-time libraries.  */
> > >> 
> > >> That looks like an understatement. Actually, we need our own stat
> > >> function and struct because the `struct stat' that Emacs uses is
> > >> incompatible with the one defined in MSVCRT, right?
> > >
> > > No, you are missing the point of that comment.  lib-src/ntlib.c is not
> > > compiled into Emacs, it's compiled into lib-src programs.
> > > Theoretically, since those programs don't need anything fancy from
> > > `stat', they could use the stock MSVCRT implementation.  But because
> > > these programs are compiled with -I../nt/inc, the compiler picks up
> > > the definition of `struct stat' that is used by Emacs, and because of
> > > this incompatibility lib-src programs cannot use the MSVCRT
> > > implementation of `stat'.
> > 
> > I think you are saying essentially the same as I do but with very
> > different words.
> 
> No, I'm not.
> 
> > OTOH, you are saying "lib-src/ntlib.c is not compiled into Emacs, it's
> > compiled into lib-src programs." and the key hypotheses made by me here
> > is that Emacs uses the `stat' definition on lib-src/ntlib.c. Is that
> > correct?
> 
> No.  The `stat' Emacs uses is in w32.c.  What you see on
> lib-src/ntlib.c is just a minimal subset of what we have in w32.c.
> 
> > Symlinks are detected and handled specially on MSVCRT's stat. In
> > aessence, for symlinks it uses fstat.
> 
> But fstat probably calls GetFileInformationByHandle under the hood,
> and we already call that function in w32.c:stat.  So maybe the fix is
> not as ugly and inelegant as you thought.
> 
> > > We cannot afford to make such a change before the release, no matter
> > > how far away is it, even if I'd agree to that (which I don't).  `stat'
> > > is too central to Emacs operation to make such changes at this time.
> > 
> > Such change would be conceptually straightforward and quite safe. It
> > amounts to using MSVCRT `stat' and translating its `struct stat' to
> > Emacs' `struct stat'. Instead of defining our own `stat' and `struct
> > stat' we define `emacs_w32_stat' and `struct emacs_w32_stat' and do
> > 
> > #if windows
> > #define stat emacs_w32_stat
> > #endif
> 
> This doesn't work.  I don't remember all the details at the moment,
> but it's not a coincidence we define `stat' in w32.c, not `sys_stat',
> as we do with other functions.  One immediate problem with this is
> that we also have our own implementation of `fstat' on w32.c, and they
> must both share the same `struct stat'.  It gets really ugly, believe
> me.
> 
> Anyway, I don't think we need to call MSVCRT's `fstat' to fix this,
> see above.
> 
> > I don't get your mention to inode numbers, UID and GID. The
> > implementation on ntlib.c simply does
> > 
> >   buf->st_ino = 0;
> > 
> > and I see no references to UID and GID.
> 
> See above: you are looking at the wrong `stat'.  The one that caused
> all this is on w32.c.
> 
> > >> BTW, the obvious fix may require some care for not breaking Emacs
> > >> support on MS Windows versions prior to XP. We still support Windows 9x,
> > >> don't we?
> > >
> > > Yes, we do.  In fact, Emacs 24.1 will again work on Windows 9X, after
> > > it turned out that 23.x (and perhaps also 22.x) didn't.
> > 
> > So we are reintroducing support for a legacy OS after being de facto
> > removed for several years. Curious.
> 
> It wasn't a "removal", it was a tricky bug.
> 
> Anyway, see this discussion:
> 
>   http://lists.gnu.org/archive/html/emacs-devel/2011-07/msg00785.html
>   http://lists.gnu.org/archive/html/emacs-devel/2011-07/msg00827.html
> 






^ permalink raw reply	[flat|nested] 27+ messages in thread

* bug#10733: 24.0.93; w32 file truncation
  2012-02-06 20:19                 ` Ota, Takaaki
@ 2012-02-06 20:23                   ` Lennart Borgman
  2012-02-06 21:11                     ` Eli Zaretskii
  2012-02-06 20:24                   ` Lars Ingebrigtsen
  2012-02-06 21:08                   ` Eli Zaretskii
  2 siblings, 1 reply; 27+ messages in thread
From: Lennart Borgman @ 2012-02-06 20:23 UTC (permalink / raw)
  To: Ota, Takaaki; +Cc: ofv, lekktu, 10733

To me it seems quite scary that Emacs has its own implementation of
such low level functions.





^ permalink raw reply	[flat|nested] 27+ messages in thread

* bug#10733: 24.0.93; w32 file truncation
  2012-02-06 20:19                 ` Ota, Takaaki
  2012-02-06 20:23                   ` Lennart Borgman
@ 2012-02-06 20:24                   ` Lars Ingebrigtsen
  2012-02-06 21:09                     ` Eli Zaretskii
  2012-02-06 21:08                   ` Eli Zaretskii
  2 siblings, 1 reply; 27+ messages in thread
From: Lars Ingebrigtsen @ 2012-02-06 20:24 UTC (permalink / raw)
  To: Ota, Takaaki; +Cc: ofv, lekktu, 10733

"Ota, Takaaki" <Takaaki.Ota@am.sony.com> writes:

> where "memo" is the NTFS symlink and "memo.old" is a real file.  I
> don't know how the size 0 on symlink side is translated into 64K.

If Emacs doesn't know the size of the file (i.e., the fs says that the
file is 0 bytes long), then Emacs will only read the first 64K of the
file.  This was discussed in the bug report about how /proc files are
truncated by Emacs.

The fix proposed there (i.e., "read until you get to eof") would
probably fix this, too.

-- 
(domestic pets only, the antidote for overdose, milk.)
  http://lars.ingebrigtsen.no  *  Sent from my Rome





^ permalink raw reply	[flat|nested] 27+ messages in thread

* bug#10733: 24.0.93; w32 file truncation
  2012-02-06 20:19                 ` Ota, Takaaki
  2012-02-06 20:23                   ` Lennart Borgman
  2012-02-06 20:24                   ` Lars Ingebrigtsen
@ 2012-02-06 21:08                   ` Eli Zaretskii
  2 siblings, 0 replies; 27+ messages in thread
From: Eli Zaretskii @ 2012-02-06 21:08 UTC (permalink / raw)
  To: Ota, Takaaki; +Cc: ofv, lekktu, 10733

> Date: Mon, 6 Feb 2012 12:19:53 -0800
> CC: <ofv@wanadoo.es>, <lekktu@gmail.com>, <10733@debbugs.gnu.org>
> From: "Ota, Takaaki" <Takaaki.Ota@am.sony.com>
> 
> For your information:
> 
> (file-attributes "memo")
> (nil 1 544 513 (20269 7834) (20269 7834) (20269 7834) 0 "-rw-rw-rw-" nil (8448 8 . 22758) (62004 . 15649))
> (file-attributes "memo.old")
> (nil 1 544 513 (19686 63524) (20262 20973) (19686 63524) 233153 "-r--r--r--" nil (512 5 . 24351) (62004 . 15649))
> 
> where "memo" is the NTFS symlink and "memo.old" is a real file.  I
> don't know how the size 0 on symlink side is translated into 64K.

insert-file-contents reads files in 64KB chunks.  So it reads only one
chunk.





^ permalink raw reply	[flat|nested] 27+ messages in thread

* bug#10733: 24.0.93; w32 file truncation
  2012-02-06 20:24                   ` Lars Ingebrigtsen
@ 2012-02-06 21:09                     ` Eli Zaretskii
  0 siblings, 0 replies; 27+ messages in thread
From: Eli Zaretskii @ 2012-02-06 21:09 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: ofv, lekktu, Takaaki.Ota, 10733

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: <eliz@gnu.org>,  ofv@wanadoo.es,  lekktu@gmail.com,  10733@debbugs.gnu.org
> Date: Mon, 06 Feb 2012 21:24:04 +0100
> 
> "Ota, Takaaki" <Takaaki.Ota@am.sony.com> writes:
> 
> > where "memo" is the NTFS symlink and "memo.old" is a real file.  I
> > don't know how the size 0 on symlink side is translated into 64K.
> 
> If Emacs doesn't know the size of the file (i.e., the fs says that the
> file is 0 bytes long), then Emacs will only read the first 64K of the
> file.  This was discussed in the bug report about how /proc files are
> truncated by Emacs.
> 
> The fix proposed there (i.e., "read until you get to eof") would
> probably fix this, too.

That's a band-aid at best.  If Emacs doesn't know the size of the file
that is the target of the symlink and its other attributes, other
places will break.

What is needed is to resolve the link, i.e. do the equivalent of
`lstat'.





^ permalink raw reply	[flat|nested] 27+ messages in thread

* bug#10733: 24.0.93; w32 file truncation
  2012-02-06 20:23                   ` Lennart Borgman
@ 2012-02-06 21:11                     ` Eli Zaretskii
  2012-02-06 21:20                       ` Lennart Borgman
  0 siblings, 1 reply; 27+ messages in thread
From: Eli Zaretskii @ 2012-02-06 21:11 UTC (permalink / raw)
  To: Lennart Borgman; +Cc: ofv, lekktu, Takaaki.Ota, 10733

> From: Lennart Borgman <lennart.borgman@gmail.com>
> Date: Mon, 6 Feb 2012 21:23:20 +0100
> Cc: eliz@gnu.org, ofv@wanadoo.es, lekktu@gmail.com, 10733@debbugs.gnu.org
> 
> To me it seems quite scary that Emacs has its own implementation of
> such low level functions.

Don't worry: `stat' is a low-level function on Posix platforms, but
not on Windows.

w32.c is full of such "low-level" functions, reimplemented to be more
Posix like, because MSVCRT does a very poor job in that regard, and
Emacs expects Posix-compliance in quite a few places.





^ permalink raw reply	[flat|nested] 27+ messages in thread

* bug#10733: 24.0.93; w32 file truncation
  2012-02-06 21:11                     ` Eli Zaretskii
@ 2012-02-06 21:20                       ` Lennart Borgman
  2012-02-06 21:31                         ` Eli Zaretskii
  0 siblings, 1 reply; 27+ messages in thread
From: Lennart Borgman @ 2012-02-06 21:20 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: ofv, lekktu, Takaaki.Ota, 10733

On Mon, Feb 6, 2012 at 22:11, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Lennart Borgman <lennart.borgman@gmail.com>
>> Date: Mon, 6 Feb 2012 21:23:20 +0100
>> Cc: eliz@gnu.org, ofv@wanadoo.es, lekktu@gmail.com, 10733@debbugs.gnu.org
>>
>> To me it seems quite scary that Emacs has its own implementation of
>> such low level functions.
>
> Don't worry: `stat' is a low-level function on Posix platforms, but
> not on Windows.

That is not what worries me. This is reimplementation is surely necessary.

What worries me is the bypassing of MSVCRT. I think that perhaps it is
better to reimplement on top of that. But that might be just me.





^ permalink raw reply	[flat|nested] 27+ messages in thread

* bug#10733: 24.0.93; w32 file truncation
  2012-02-06 21:20                       ` Lennart Borgman
@ 2012-02-06 21:31                         ` Eli Zaretskii
  2012-02-06 21:35                           ` Lennart Borgman
  0 siblings, 1 reply; 27+ messages in thread
From: Eli Zaretskii @ 2012-02-06 21:31 UTC (permalink / raw)
  To: Lennart Borgman; +Cc: ofv, lekktu, Takaaki.Ota, 10733

> From: Lennart Borgman <lennart.borgman@gmail.com>
> Date: Mon, 6 Feb 2012 22:20:31 +0100
> Cc: Takaaki.Ota@am.sony.com, ofv@wanadoo.es, lekktu@gmail.com, 
> 	10733@debbugs.gnu.org
> 
> What worries me is the bypassing of MSVCRT.

Why?  What do you think `stat' in MSVCRT does?  (You can find the
sources on the Internet.)  It calls some of the same Win32 APIs, and
then fills `struct stat'.  There''s nothing magic about that, and
nothing to be afraid of.

> I think that perhaps it is better to reimplement on top of that.

You can't.  To get the missing information you must go to lower-level
APIs.





^ permalink raw reply	[flat|nested] 27+ messages in thread

* bug#10733: 24.0.93; w32 file truncation
  2012-02-06 21:31                         ` Eli Zaretskii
@ 2012-02-06 21:35                           ` Lennart Borgman
  0 siblings, 0 replies; 27+ messages in thread
From: Lennart Borgman @ 2012-02-06 21:35 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: ofv, lekktu, Takaaki.Ota, 10733

On Mon, Feb 6, 2012 at 22:31, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Lennart Borgman <lennart.borgman@gmail.com>
>> Date: Mon, 6 Feb 2012 22:20:31 +0100
>> Cc: Takaaki.Ota@am.sony.com, ofv@wanadoo.es, lekktu@gmail.com,
>>       10733@debbugs.gnu.org
>>
>> What worries me is the bypassing of MSVCRT.
>
> Why?  What do you think `stat' in MSVCRT does?  (You can find the
> sources on the Internet.)  It calls some of the same Win32 APIs, and
> then fills `struct stat'.  There''s nothing magic about that, and
> nothing to be afraid of.

Ok, I see. But what seems to have happened now is that changes in
these calls that are implemented in MSVCRT are not mirrored in the
Emacs reimplementation. (And we are aware that it is a bit serious.)

>> I think that perhaps it is better to reimplement on top of that.
>
> You can't.  To get the missing information you must go to lower-level
> APIs.

Do you say that it would slow down the Emacs implementation to get
this information with new calls (beside those potentially going
through MSVCRT)? Then I agree.





^ permalink raw reply	[flat|nested] 27+ messages in thread

* bug#10733: 24.0.93; w32 file truncation
  2012-02-06 18:37               ` Eli Zaretskii
  2012-02-06 20:19                 ` Ota, Takaaki
@ 2012-02-06 23:27                 ` Óscar Fuentes
  2012-02-06 23:43                   ` Ota, Takaaki
  2012-02-07  4:02                   ` Eli Zaretskii
  1 sibling, 2 replies; 27+ messages in thread
From: Óscar Fuentes @ 2012-02-06 23:27 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: lekktu, Takaaki.Ota, 10733

Eli Zaretskii <eliz@gnu.org> writes:

[snip]

> No.  The `stat' Emacs uses is in w32.c.  What you see on
> lib-src/ntlib.c is just a minimal subset of what we have in w32.c.

That's important. Thanks.

>> Symlinks are detected and handled specially on MSVCRT's stat. In
>> aessence, for symlinks it uses fstat.
>
> But fstat probably calls GetFileInformationByHandle under the hood,
> and we already call that function in w32.c:stat.  So maybe the fix is
> not as ugly and inelegant as you thought.

Yup. This patch fixes the problem:

diff --git a/src/w32.c b/src/w32.c
index f610a36..035e1f2 100644
--- a/src/w32.c
+++ b/src/w32.c
@@ -3444,6 +3444,29 @@ stat (const char * path, struct stat * buf)
 	}
     }
 
+  buf->st_size = 0;
+
+  if ((wfd.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) &&
+      (wfd.dwReserved0 == IO_REPARSE_TAG_SYMLINK))
+    {
+      HANDLE fh;
+      BY_HANDLE_FILE_INFORMATION info;
+
+      if ((fh = CreateFile (name, 0, 0, NULL, OPEN_EXISTING,
+                            FILE_FLAG_BACKUP_SEMANTICS, NULL))
+          && GetFileInformationByHandle (fh, &info))
+        {
+          buf->st_size = info.nFileSizeHigh;
+          buf->st_size <<= 32;
+          buf->st_size += info.nFileSizeLow;
+        }
+      else
+        {
+          errno = ENOENT;
+          return -1;
+        }
+    }
+
   if (!(NILP (Vw32_get_true_file_attributes)
 	|| (EQ (Vw32_get_true_file_attributes, Qlocal) && is_slow_fs (name)))
       /* No access rights required to get info.  */
@@ -3532,9 +3555,12 @@ stat (const char * path, struct stat * buf)
   buf->st_dev = volume_info.serialnum;
   buf->st_rdev = volume_info.serialnum;
 
-  buf->st_size = wfd.nFileSizeHigh;
-  buf->st_size <<= 32;
-  buf->st_size += wfd.nFileSizeLow;
+  if (! buf->st_size)
+    {
+      buf->st_size = wfd.nFileSizeHigh;
+      buf->st_size <<= 32;
+      buf->st_size += wfd.nFileSizeLow;
+    }
 
   /* Convert timestamps to Unix format. */
   buf->st_mtime = convert_time (wfd.ftLastWriteTime);


Maybe it can be integrated in the

if (!(NILP(Vw32_get_true_file_attributes) ...

hence reusing the calls to CreateFile and GetFileInformationByHandle and
shortening the patch, but as I don't know what
Vw32_get_true_file_attributes does, preferread to follow the safe way.

However, we still have another implementation on ntlib.c.

And the fix is just for the size. Don't know if there are other
attributes suffer from the same problem. Possibly the right thing is to
do what MSVCRT does:

if is-symlink?
  use fstat
fi





^ permalink raw reply related	[flat|nested] 27+ messages in thread

* bug#10733: 24.0.93; w32 file truncation
  2012-02-06 23:27                 ` Óscar Fuentes
@ 2012-02-06 23:43                   ` Ota, Takaaki
  2012-02-07  0:07                     ` Óscar Fuentes
  2012-02-07  4:02                   ` Eli Zaretskii
  1 sibling, 1 reply; 27+ messages in thread
From: Ota, Takaaki @ 2012-02-06 23:43 UTC (permalink / raw)
  To: ofv; +Cc: lekktu, 10733

Don't you need to dispose fh properly?

-Tak

Mon, 6 Feb 2012 15:27:07 -0800: Óscar Fuentes <ofv@wanadoo.es> wrote:

> Eli Zaretskii <eliz@gnu.org> writes:
> 
> [snip]
> 
> > No.  The `stat' Emacs uses is in w32.c.  What you see on
> > lib-src/ntlib.c is just a minimal subset of what we have in w32.c.
> 
> That's important. Thanks.
> 
> >> Symlinks are detected and handled specially on MSVCRT's stat. In
> >> aessence, for symlinks it uses fstat.
> >
> > But fstat probably calls GetFileInformationByHandle under the hood,
> > and we already call that function in w32.c:stat.  So maybe the fix is
> > not as ugly and inelegant as you thought.
> 
> Yup. This patch fixes the problem:
> 
> diff --git a/src/w32.c b/src/w32.c
> index f610a36..035e1f2 100644
> --- a/src/w32.c
> +++ b/src/w32.c
> @@ -3444,6 +3444,29 @@ stat (const char * path, struct stat * buf)
>  	}
>      }
>  
> +  buf->st_size = 0;
> +
> +  if ((wfd.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) &&
> +      (wfd.dwReserved0 == IO_REPARSE_TAG_SYMLINK))
> +    {
> +      HANDLE fh;
> +      BY_HANDLE_FILE_INFORMATION info;
> +
> +      if ((fh = CreateFile (name, 0, 0, NULL, OPEN_EXISTING,
> +                            FILE_FLAG_BACKUP_SEMANTICS, NULL))
> +          && GetFileInformationByHandle (fh, &info))
> +        {
> +          buf->st_size = info.nFileSizeHigh;
> +          buf->st_size <<= 32;
> +          buf->st_size += info.nFileSizeLow;
> +        }
> +      else
> +        {
> +          errno = ENOENT;
> +          return -1;
> +        }
> +    }
> +
>    if (!(NILP (Vw32_get_true_file_attributes)
>  	|| (EQ (Vw32_get_true_file_attributes, Qlocal) && is_slow_fs (name)))
>        /* No access rights required to get info.  */
> @@ -3532,9 +3555,12 @@ stat (const char * path, struct stat * buf)
>    buf->st_dev = volume_info.serialnum;
>    buf->st_rdev = volume_info.serialnum;
>  
> -  buf->st_size = wfd.nFileSizeHigh;
> -  buf->st_size <<= 32;
> -  buf->st_size += wfd.nFileSizeLow;
> +  if (! buf->st_size)
> +    {
> +      buf->st_size = wfd.nFileSizeHigh;
> +      buf->st_size <<= 32;
> +      buf->st_size += wfd.nFileSizeLow;
> +    }
>  
>    /* Convert timestamps to Unix format. */
>    buf->st_mtime = convert_time (wfd.ftLastWriteTime);
> 
> 
> Maybe it can be integrated in the
> 
> if (!(NILP(Vw32_get_true_file_attributes) ...
> 
> hence reusing the calls to CreateFile and GetFileInformationByHandle and
> shortening the patch, but as I don't know what
> Vw32_get_true_file_attributes does, preferread to follow the safe way.
> 
> However, we still have another implementation on ntlib.c.
> 
> And the fix is just for the size. Don't know if there are other
> attributes suffer from the same problem. Possibly the right thing is to
> do what MSVCRT does:
> 
> if is-symlink?
>   use fstat
> fi
> 






^ permalink raw reply	[flat|nested] 27+ messages in thread

* bug#10733: 24.0.93; w32 file truncation
  2012-02-06 23:43                   ` Ota, Takaaki
@ 2012-02-07  0:07                     ` Óscar Fuentes
  0 siblings, 0 replies; 27+ messages in thread
From: Óscar Fuentes @ 2012-02-07  0:07 UTC (permalink / raw)
  To: Ota, Takaaki; +Cc: ofv, lekktu, 10733

"Ota, Takaaki" <Takaaki.Ota@am.sony.com> writes:

> Don't you need to dispose fh properly?

Yes. And check that it works for directories too.

The patch is not intended as a definitive fix, but as a data point on
the discussion.





^ permalink raw reply	[flat|nested] 27+ messages in thread

* bug#10733: 24.0.93; w32 file truncation
  2012-02-06 23:27                 ` Óscar Fuentes
  2012-02-06 23:43                   ` Ota, Takaaki
@ 2012-02-07  4:02                   ` Eli Zaretskii
  2012-02-07  5:22                     ` Óscar Fuentes
  1 sibling, 1 reply; 27+ messages in thread
From: Eli Zaretskii @ 2012-02-07  4:02 UTC (permalink / raw)
  To: Óscar Fuentes; +Cc: lekktu, Takaaki.Ota, 10733

> From: Óscar Fuentes <ofv@wanadoo.es>
> Cc: lekktu@gmail.com,  Takaaki.Ota@am.sony.com,  10733@debbugs.gnu.org
> Date: Tue, 07 Feb 2012 00:27:07 +0100
> 
> >> Symlinks are detected and handled specially on MSVCRT's stat. In
> >> aessence, for symlinks it uses fstat.
> >
> > But fstat probably calls GetFileInformationByHandle under the hood,
> > and we already call that function in w32.c:stat.  So maybe the fix is
> > not as ugly and inelegant as you thought.
> 
> Yup. This patch fixes the problem:

Thanks.

> Maybe it can be integrated in the
> 
> if (!(NILP(Vw32_get_true_file_attributes) ...
> 
> hence reusing the calls to CreateFile and GetFileInformationByHandle and
> shortening the patch, but as I don't know what
> Vw32_get_true_file_attributes does, preferread to follow the safe way.

You did right: w32-get-true-file-attributes can be set by the user to
nil, if she wants her file ops faster.

> And the fix is just for the size. Don't know if there are other
> attributes suffer from the same problem. Possibly the right thing is to
> do what MSVCRT does:
> 
> if is-symlink?
>   use fstat
> fi

Since fstat is also reimplemented, I'd rather do what it does inline.

For that, we need to know which other attributes are reported
different.  Or maybe just test for the reparse point up front and do
all the work for the target instead.






^ permalink raw reply	[flat|nested] 27+ messages in thread

* bug#10733: 24.0.93; w32 file truncation
  2012-02-07  4:02                   ` Eli Zaretskii
@ 2012-02-07  5:22                     ` Óscar Fuentes
  2012-02-07 17:35                       ` Eli Zaretskii
  0 siblings, 1 reply; 27+ messages in thread
From: Óscar Fuentes @ 2012-02-07  5:22 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: lekktu, Takaaki.Ota, 10733

Eli Zaretskii <eliz@gnu.org> writes:

>> Maybe it can be integrated in the
>> 
>> if (!(NILP(Vw32_get_true_file_attributes) ...
>> 
>> hence reusing the calls to CreateFile and GetFileInformationByHandle and
>> shortening the patch, but as I don't know what
>> Vw32_get_true_file_attributes does, preferread to follow the safe way.
>
> You did right: w32-get-true-file-attributes can be set by the user to
> nil, if she wants her file ops faster.

I was thinking on something like

diff --git a/src/w32.c b/src/w32.c
index 3d3d334..418be63 100644
--- a/src/w32.c
+++ b/src/w32.c
@@ -3447,8 +3447,12 @@ stat (const char * path, struct stat * buf)
 	}
     }
 
-  if (!(NILP (Vw32_get_true_file_attributes)
-	|| (EQ (Vw32_get_true_file_attributes, Qlocal) && is_slow_fs (name)))
+  buf->st_size = 0;
+
+  if ((!(NILP (Vw32_get_true_file_attributes)
+         || (EQ (Vw32_get_true_file_attributes, Qlocal) && is_slow_fs (name)))
+       || ((wfd.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) &&
+           (wfd.dwReserved0 == IO_REPARSE_TAG_SYMLINK)))
       /* No access rights required to get info.  */
       && (fh = CreateFile (name, 0, 0, NULL, OPEN_EXISTING,
 			   FILE_FLAG_BACKUP_SEMANTICS, NULL))
@@ -3461,6 +3465,9 @@ stat (const char * path, struct stat * buf)
 
       if (GetFileInformationByHandle (fh, &info))
 	{
+          buf->st_size = info.nFileSizeHigh;
+          buf->st_size <<= 32;
+          buf->st_size += info.nFileSizeLow;
 	  buf->st_nlink = info.nNumberOfLinks;
 	  /* Might as well use file index to fake inode values, but this
 	     is not guaranteed to be unique unless we keep a handle open
@@ -3535,9 +3542,12 @@ stat (const char * path, struct stat * buf)
   buf->st_dev = volume_info.serialnum;
   buf->st_rdev = volume_info.serialnum;
 
-  buf->st_size = wfd.nFileSizeHigh;
-  buf->st_size <<= 32;
-  buf->st_size += wfd.nFileSizeLow;
+  if (!buf->st_size)
+    {
+      buf->st_size = wfd.nFileSizeHigh;
+      buf->st_size <<= 32;
+      buf->st_size += wfd.nFileSizeLow;
+    }
 
   /* Convert timestamps to Unix format. */
   buf->st_mtime = convert_time (wfd.ftLastWriteTime);


That is not as robust, though, because if GetFileInformationByHandle
fails the function does not return with an error code.

>> And the fix is just for the size. Don't know if there are other
>> attributes suffer from the same problem. Possibly the right thing is to
>> do what MSVCRT does:
>> 
>> if is-symlink?
>>   use fstat
>> fi
>
> Since fstat is also reimplemented, I'd rather do what it does inline.
>
> For that, we need to know which other attributes are reported
> different.  Or maybe just test for the reparse point up front and do
> all the work for the target instead.

Since Emacs' fstat reimplementation is based on
GetFileInformationByHandle, and that the handle points to the linked
file (CreateFile follows the link unless told otherwise), we should be
safe delegating all work to `fstat' when a symlink is detected on `stat'
(the executable bit must be setted on `stat', but that's no problem.)





^ permalink raw reply related	[flat|nested] 27+ messages in thread

* bug#10733: 24.0.93; w32 file truncation
  2012-02-07  5:22                     ` Óscar Fuentes
@ 2012-02-07 17:35                       ` Eli Zaretskii
  2012-08-03 10:59                         ` Eli Zaretskii
  0 siblings, 1 reply; 27+ messages in thread
From: Eli Zaretskii @ 2012-02-07 17:35 UTC (permalink / raw)
  To: Óscar Fuentes; +Cc: lekktu, Takaaki.Ota, 10733

> From: Óscar Fuentes <ofv@wanadoo.es>
> Cc: lekktu@gmail.com,  Takaaki.Ota@am.sony.com,  10733@debbugs.gnu.org
> Date: Tue, 07 Feb 2012 06:22:12 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Maybe it can be integrated in the
> >> 
> >> if (!(NILP(Vw32_get_true_file_attributes) ...
> >> 
> >> hence reusing the calls to CreateFile and GetFileInformationByHandle and
> >> shortening the patch, but as I don't know what
> >> Vw32_get_true_file_attributes does, preferread to follow the safe way.
> >
> > You did right: w32-get-true-file-attributes can be set by the user to
> > nil, if she wants her file ops faster.
> 
> I was thinking on something like
> 
> diff --git a/src/w32.c b/src/w32.c
> index 3d3d334..418be63 100644
> --- a/src/w32.c
> +++ b/src/w32.c
> @@ -3447,8 +3447,12 @@ stat (const char * path, struct stat * buf)
>  	}
>      }
>  
> -  if (!(NILP (Vw32_get_true_file_attributes)
> -	|| (EQ (Vw32_get_true_file_attributes, Qlocal) && is_slow_fs (name)))
> +  buf->st_size = 0;
> +
> +  if ((!(NILP (Vw32_get_true_file_attributes)
> +         || (EQ (Vw32_get_true_file_attributes, Qlocal) && is_slow_fs (name)))
> +       || ((wfd.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) &&
> +           (wfd.dwReserved0 == IO_REPARSE_TAG_SYMLINK)))

Then what made you hesitate?  This approach looks fine to me.

> >> if is-symlink?
> >>   use fstat
> >> fi
> >
> > Since fstat is also reimplemented, I'd rather do what it does inline.
> >
> > For that, we need to know which other attributes are reported
> > different.  Or maybe just test for the reparse point up front and do
> > all the work for the target instead.
> 
> Since Emacs' fstat reimplementation is based on
> GetFileInformationByHandle, and that the handle points to the linked
> file (CreateFile follows the link unless told otherwise), we should be
> safe delegating all work to `fstat' when a symlink is detected on `stat'
> (the executable bit must be setted on `stat', but that's no problem.)

Please compare w32.c's `fstat' with `stat', and you will see that the
former does much less than the latter, even with information that can
be gotten by the handle.  To go the way you suggest, we need first to
make `fstat' a proper subset of `stat'.  (I never had time to do it,
and since `fstat' is used much less that `stat' in Emacs, more
important jobs came first.)






^ permalink raw reply	[flat|nested] 27+ messages in thread

* bug#10733: 24.0.93; w32 file truncation
  2012-02-07 17:35                       ` Eli Zaretskii
@ 2012-08-03 10:59                         ` Eli Zaretskii
  0 siblings, 0 replies; 27+ messages in thread
From: Eli Zaretskii @ 2012-08-03 10:59 UTC (permalink / raw)
  To: ofv, lekktu, Takaaki.Ota; +Cc: 10733-done

> Date: Tue, 07 Feb 2012 19:35:25 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: lekktu@gmail.com, Takaaki.Ota@am.sony.com, 10733@debbugs.gnu.org
> 
> > From: Óscar Fuentes <ofv@wanadoo.es>
> > Cc: lekktu@gmail.com,  Takaaki.Ota@am.sony.com,  10733@debbugs.gnu.org
> > Date: Tue, 07 Feb 2012 06:22:12 +0100
> > 
> > Eli Zaretskii <eliz@gnu.org> writes:
> > 
> > >> Maybe it can be integrated in the
> > >> 
> > >> if (!(NILP(Vw32_get_true_file_attributes) ...
> > >> 
> > >> hence reusing the calls to CreateFile and GetFileInformationByHandle and
> > >> shortening the patch, but as I don't know what
> > >> Vw32_get_true_file_attributes does, preferread to follow the safe way.
> > >
> > > You did right: w32-get-true-file-attributes can be set by the user to
> > > nil, if she wants her file ops faster.
> > 
> > I was thinking on something like
> > 
> > diff --git a/src/w32.c b/src/w32.c
> > index 3d3d334..418be63 100644
> > --- a/src/w32.c
> > +++ b/src/w32.c
> > @@ -3447,8 +3447,12 @@ stat (const char * path, struct stat * buf)
> >  	}
> >      }
> >  
> > -  if (!(NILP (Vw32_get_true_file_attributes)
> > -	|| (EQ (Vw32_get_true_file_attributes, Qlocal) && is_slow_fs (name)))
> > +  buf->st_size = 0;
> > +
> > +  if ((!(NILP (Vw32_get_true_file_attributes)
> > +         || (EQ (Vw32_get_true_file_attributes, Qlocal) && is_slow_fs (name)))
> > +       || ((wfd.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) &&
> > +           (wfd.dwReserved0 == IO_REPARSE_TAG_SYMLINK)))
> 
> Then what made you hesitate?  This approach looks fine to me.
> 
> > >> if is-symlink?
> > >>   use fstat
> > >> fi
> > >
> > > Since fstat is also reimplemented, I'd rather do what it does inline.
> > >
> > > For that, we need to know which other attributes are reported
> > > different.  Or maybe just test for the reparse point up front and do
> > > all the work for the target instead.
> > 
> > Since Emacs' fstat reimplementation is based on
> > GetFileInformationByHandle, and that the handle points to the linked
> > file (CreateFile follows the link unless told otherwise), we should be
> > safe delegating all work to `fstat' when a symlink is detected on `stat'
> > (the executable bit must be setted on `stat', but that's no problem.)
> 
> Please compare w32.c's `fstat' with `stat', and you will see that the
> former does much less than the latter, even with information that can
> be gotten by the handle.  To go the way you suggest, we need first to
> make `fstat' a proper subset of `stat'.  (I never had time to do it,
> and since `fstat' is used much less that `stat' in Emacs, more
> important jobs came first.)

Since trunk revision 109416 adds full support for symlinks on
MS-Windows platforms, and fixes this bug as a side effect, I'm closing
this bug.






^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2012-08-03 10:59 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-02-05 22:34 bug#10733: 24.0.93; w32 file truncation Ota, Takaaki
2012-02-05 23:51 ` Juanma Barranquero
2012-02-06  0:16   ` Ota, Takaaki
2012-02-06  4:05     ` Eli Zaretskii
2012-02-06  5:25       ` Óscar Fuentes
2012-02-06 16:16         ` Óscar Fuentes
2012-02-06 17:21           ` Eli Zaretskii
2012-02-06 17:58             ` Óscar Fuentes
2012-02-06 18:37               ` Eli Zaretskii
2012-02-06 20:19                 ` Ota, Takaaki
2012-02-06 20:23                   ` Lennart Borgman
2012-02-06 21:11                     ` Eli Zaretskii
2012-02-06 21:20                       ` Lennart Borgman
2012-02-06 21:31                         ` Eli Zaretskii
2012-02-06 21:35                           ` Lennart Borgman
2012-02-06 20:24                   ` Lars Ingebrigtsen
2012-02-06 21:09                     ` Eli Zaretskii
2012-02-06 21:08                   ` Eli Zaretskii
2012-02-06 23:27                 ` Óscar Fuentes
2012-02-06 23:43                   ` Ota, Takaaki
2012-02-07  0:07                     ` Óscar Fuentes
2012-02-07  4:02                   ` Eli Zaretskii
2012-02-07  5:22                     ` Óscar Fuentes
2012-02-07 17:35                       ` Eli Zaretskii
2012-08-03 10:59                         ` Eli Zaretskii
2012-02-06 16:55         ` Eli Zaretskii
2012-02-06  0:20   ` Óscar Fuentes

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.