Willing to debug bug #3542 (23.0.94; File access via UNC path slow again under Windows)

[Eli Zaretskii]

> From: Mathias Dahl <mathias.dahl@...>
> Date: Thu, 9 Jul 2009 13:53:17 +0200
> Cc: Eli Zaretskii <eliz@...>, emacs-devel@...
>
> I suggest we change the default value of this variable to nil (or make
> the code faster).

The code is written to effectively behave as if the variable was nil
for remote filesystems.  It just didn't handle UNC file names as
remote.  Now it does.

Making the code faster (even beyond this bugfix) is the plan.
Changing the default to nil isn't.

Thanks for your help in finding the bug and fixing it.

[Chong Yidong]

Eli Zaretskii <eliz@...> writes:

> Right.  We didn't treat UNC file names as remote, it's as simple as
> that.
>
> I installed the patch below on the trunk.
>
> Stefan and Yidong, is it okay to install on the release branch as
> well?
>
> 2009-07-09  Eli Zaretskii  <eliz@...>
>
> * w32.c (stat): Treat UNC file names as residing on remote
> drives.  (Bug#3542)

Looks OK to me.

[Eli Zaretskii]

> From: Chong Yidong <cyd@...>
> Cc: Jason Rumney <jasonr@...>, emacs-devel@..., drew.adams@...,
>         mathias.dahl@...
> Date: Thu, 09 Jul 2009 17:33:51 -0400
>
> Eli Zaretskii <eliz@...> writes:
>
> > Right.  We didn't treat UNC file names as remote, it's as simple as
> > that.
> >
> > I installed the patch below on the trunk.
> >
> > Stefan and Yidong, is it okay to install on the release branch as
> > well?
> >
> > 2009-07-09  Eli Zaretskii  <eliz@...>
> >
> > * w32.c (stat): Treat UNC file names as residing on remote
> > drives.  (Bug#3542)
>
> Looks OK to me.

Thanks, committed to the branch as well.

[Stefan Monnier]

> Stefan and Yidong, is it okay to install on the release branch as
> well?

Fine by me as well.
BTW, could you factor out your "remoteness test" and make it available
to file-remote-p?


        Stefan

[Eli Zaretskii]

> From: Stefan Monnier <monnier@...>
> Cc: Jason Rumney <jasonr@...>,  emacs-devel@...,  drew.adams@...,  mathias.dahl@...
> Date: Sat, 11 Jul 2009 15:17:29 -0400
>
> > Stefan and Yidong, is it okay to install on the release branch as
> > well?
>
> Fine by me as well.

Thanks.

> BTW, could you factor out your "remoteness test" and make it available
> to file-remote-p?

Already done by today's commits to the trunk.  The function is called
is_slow_fs, and returns non-zero if its argument resides on a
filesystem deemed slow.

I'm not sure this is what you want for file-remote-p.  Perhaps you
only want files on remote (a.k.a. networked) filesystems.  There's
code to do that as well (in w32.c:logon_network_drive), so if you
want, I can make it a separate routine.

[Stefan Monnier]

> Already done by today's commits to the trunk.  The function is called
> is_slow_fs, and returns non-zero if its argument resides on a
> filesystem deemed slow.
> I'm not sure this is what you want for file-remote-p.  Perhaps you
> only want files on remote (a.k.a. networked) filesystems.  There's

I'm not sure I understand the difference.  Could you give an example of
a filesystem that's slow but not "remote (a.k.a. networked)"?


        Stefan


PS: the criterion for file-remote-p is (C-h f file-remote-p):
"...A file is considered "remote" if accessing it is likely to be slower or
less reliable than accessing local files..."

[Andreas Schwab-2]

Stefan Monnier <monnier@...> writes:

> I'm not sure I understand the difference.  Could you give an example of
> a filesystem that's slow but not "remote (a.k.a. networked)"?

Filesystems aren't slow, devices are.

Andreas.

--
Andreas Schwab, schwab@...
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

[Andreas Schwab-2]

Eli Zaretskii <eliz@...> writes:

> Already done by today's commits to the trunk.  The function is called
> is_slow_fs, and returns non-zero if its argument resides on a
> filesystem deemed slow.

Btw, Lisp strings are guaranteed to be NUL terminated, no need to create
a copy to use is as a C string.

Andreas.

--
Andreas Schwab, schwab@...
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

[Eli Zaretskii]

> From: Stefan Monnier <monnier@...>
> Cc: mathias.dahl@...,  emacs-devel@...,  drew.adams@...,  jasonr@...
> Date: Mon, 13 Jul 2009 08:17:08 -0400
>
> > Already done by today's commits to the trunk.  The function is called
> > is_slow_fs, and returns non-zero if its argument resides on a
> > filesystem deemed slow.
> > I'm not sure this is what you want for file-remote-p.  Perhaps you
> > only want files on remote (a.k.a. networked) filesystems.  There's
>
> I'm not sure I understand the difference.  Could you give an example of
> a filesystem that's slow but not "remote (a.k.a. networked)"?

The code in is_slow_fs considers only fixed disks and RAM disks not
``slow''.  Other types, which include CDs and removable media such as
USB memory sticks, are considered ``slow''.

> PS: the criterion for file-remote-p is (C-h f file-remote-p):
> "...A file is considered "remote" if accessing it is likely to be slower or
> less reliable than accessing local files..."

Right, but I remember a prolonged discussion about that which IIRC
ended in controversy.  That's why I asked.

[Eli Zaretskii]

> From: Andreas Schwab <schwab@...>
> Cc: Eli Zaretskii <eliz@...>, emacs-devel@..., jasonr@...,
>         drew.adams@..., mathias.dahl@...
> Date: Mon, 13 Jul 2009 15:38:02 +0200
>
> Stefan Monnier <monnier@...> writes:
>
> > I'm not sure I understand the difference.  Could you give an example of
> > a filesystem that's slow but not "remote (a.k.a. networked)"?
>
> Filesystems aren't slow, devices are.

Well, filesystems _shouldn't_ be slow, though some are ;-)

But yes, I agree that ``filesystem'' is a kind of misnomer, although
some filesystems are associated with very specific devices.

[Eli Zaretskii]

> From: Andreas Schwab <schwab@...>
> Cc: Stefan Monnier <monnier@...>, emacs-devel@...,
>         jasonr@..., drew.adams@..., mathias.dahl@...
> Date: Mon, 13 Jul 2009 15:57:48 +0200
>
> Btw, Lisp strings are guaranteed to be NUL terminated, no need to create
> a copy to use is as a C string.

Really?  I will have to look at the code.  Did I dream or at some
point in the past they weren't guaranteed to be null-terminated?

[Andreas Schwab-2]

Eli Zaretskii <eliz@...> writes:

>> From: Andreas Schwab <schwab@...>
>> Cc: Stefan Monnier <monnier@...>, emacs-devel@...,
>>         jasonr@..., drew.adams@..., mathias.dahl@...
>> Date: Mon, 13 Jul 2009 15:57:48 +0200
>>
>> Btw, Lisp strings are guaranteed to be NUL terminated, no need to create
>> a copy to use is as a C string.
>
> Really?  I will have to look at the code.

See allocate_string_data.

> Did I dream or at some point in the past they weren't guaranteed to
> be null-terminated?

Not in the last 18 years. :-)

Andreas.

--
Andreas Schwab, schwab@...
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

[Eli Zaretskii]

> From: Andreas Schwab <schwab@...>
> Cc: emacs-devel@...
> Date: Mon, 13 Jul 2009 21:39:20 +0200
>
> See allocate_string_data.

Aha.  But it sounds like it's not just me who is confused.  Here's
just two examples:

  From doc.c:

      strp = SDATA (string);
      while (strp < SDATA (string) + SBYTES (string))

  (why not "while *strp"?)

  From fileio.c:

      nm = (unsigned char *) alloca (SBYTES (filename) + 1);
      bcopy (SDATA (filename), nm, SBYTES (filename) + 1);

  (why +1? it potentially accesses memory beyond end of `filename's
  contents)

Anyway, thanks.  I will fix the code I committed.

[Andreas Schwab-2]

Eli Zaretskii <eliz@...> writes:

> Aha.  But it sounds like it's not just me who is confused.  Here's
> just two examples:
>
>   From doc.c:
>
>       strp = SDATA (string);
>       while (strp < SDATA (string) + SBYTES (string))
>
>   (why not "while *strp"?)

Just because strings are always NUL terminated does not mean that every
NUL terminates a string.

Andreas.

--
Andreas Schwab, schwab@...
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

[Chong Yidong]

Andreas Schwab <schwab@...> writes:

> Just because strings are always NUL terminated does not mean that every
> NUL terminates a string.

Yes, and in fact this struck recently---see Bug#3772.

[YAMAMOTO Mitsuharu]

>>>>> On Mon, 13 Jul 2009 23:13:20 +0300, Eli Zaretskii <eliz@...> said:

> Aha.  But it sounds like it's not just me who is confused.  Here's
> just two examples:

>   From fileio.c:

>       nm = (unsigned char *) alloca (SBYTES (filename) + 1);
>       bcopy (SDATA (filename), nm, SBYTES (filename) + 1);

>   (why +1? it potentially accesses memory beyond end of `filename's
>   contents)

It is necessary because SBYTES does not count the extra NUL character.
E.g., SBYTES (empty_unibyte_string) == 0, and
*(SDATA (empty_unibyte_string)) == '\0'.

                                     YAMAMOTO Mitsuharu
                                mituharu@...

[Stefan Monnier]

>> > Already done by today's commits to the trunk.  The function is called
>> > is_slow_fs, and returns non-zero if its argument resides on a
>> > filesystem deemed slow.
>> > I'm not sure this is what you want for file-remote-p.  Perhaps you
>> > only want files on remote (a.k.a. networked) filesystems.  There's
>>
>> I'm not sure I understand the difference.  Could you give an example of
>> a filesystem that's slow but not "remote (a.k.a. networked)"?

> The code in is_slow_fs considers only fixed disks and RAM disks not
> ``slow''.  Other types, which include CDs and removable media such as
> USB memory sticks, are considered ``slow''.

Hmm... removable media... I think it should still be considered as
non-remote, although accessing a USB stick after you removed it (but
without unmounting it) may hang just like accessing a dead NFS server.


        Stefan

[Stefan Monnier]

> Aha.  But it sounds like it's not just me who is confused.  Here's
> just two examples:

>   From doc.c:

>       strp = SDATA (string);
>       while (strp < SDATA (string) + SBYTES (string))

>   (why not "while *strp"?)

As said Andreas, this would stop at the first NUL, which may appear
within the string.

>   From fileio.c:
>       nm = (unsigned char *) alloca (SBYTES (filename) + 1);
>       bcopy (SDATA (filename), nm, SBYTES (filename) + 1);
>   (why +1? it potentially accesses memory beyond end of `filename's
>   contents)

The +1 is precisely used to make sure we copy the terminating NUL.


        Stefan

[Eli Zaretskii]

> From: Stefan Monnier <monnier@...>
> Cc: Andreas Schwab <schwab@...>,  emacs-devel@...
> Date: Mon, 13 Jul 2009 20:54:14 -0400
>
> > Aha.  But it sounds like it's not just me who is confused.  Here's
> > just two examples:
>
> >   From doc.c:
>
> >       strp = SDATA (string);
> >       while (strp < SDATA (string) + SBYTES (string))
>
> >   (why not "while *strp"?)
>
> As said Andreas, this would stop at the first NUL, which may appear
> within the string.

But then a gazillion other places are buggy: we _do_ use SDATA(str) as
a C string, and pass it to functions that will stop examining the
string on the first null.  A random example:

  d = opendir (SDATA (Fdirectory_file_name (encoded_dir)));

What am I missing?

> >   From fileio.c:
> >       nm = (unsigned char *) alloca (SBYTES (filename) + 1);
> >       bcopy (SDATA (filename), nm, SBYTES (filename) + 1);
> >   (why +1? it potentially accesses memory beyond end of `filename's
> >   contents)
>
> The +1 is precisely used to make sure we copy the terminating NUL.

That's not my reading of allocate_string_data.  Are you sure?

Anyway, if that's true, then again we have bugs in other places.  Like
this one:

  directory_nbytes = SBYTES (directory);
  if (directory_nbytes == 0
      || !IS_ANY_SEP (SREF (directory, directory_nbytes - 1)))
    needsep = 1;
  [...]
                  int nbytes = len + directory_nbytes + needsep;
                  fullname = make_uninit_multibyte_string (nbytes, nbytes);
                  bcopy (SDATA (directory), SDATA (fullname),
                         directory_nbytes);

[Miles Bader-2]

Eli Zaretskii <eliz@...> writes:
>> As said Andreas, this would stop at the first NUL, which may appear
>> within the string.
>
> But then a gazillion other places are buggy: we _do_ use SDATA(str) as
> a C string, and pass it to functions that will stop examining the
> string on the first null.  A random example:
>
>   d = opendir (SDATA (Fdirectory_file_name (encoded_dir)));

That's a limitation of opendir (and posix filenames in general), and
there's absolutely nothing we can do it (so there's no point in worrying
about that case), but we shouldn't introduce new limitations, since for
many other uses, NUL-containing strings are fine.

-Miles

--
Scriptures, n. The sacred books of our holy religion, as distinguished from
the false and profane writings on which all other faiths are based.