Re: CVS problem with ssh

[Paul Eggert]

"Richard M. Stallman" <[EMAIL PROTECTED]> writes:

> I am not sure what job that program does.  The program is too long for
> me to read it just to try to figure that out.

The program arranges for fflush to have a nontrivial amount of
buffered data to flush.  It then calls fflush, so the buffer is then
flushed into a pipe that is in O_NONBLOCK mode.  If the fflush fails
with errno == EAGAIN, the program retries the fflush.  The program
then checks that the appropriate amount of data was actually written.

> However, if it is impossible to do this job reliably for fflush,
> we can still adapt the solution to that fact, by using
>   #define fflush undefined_fflush
> so that people can modify the relevant apps not to use fflush.

If the problem occurs with fflush, most likely it will also occur
with fwrite, fclose, etc.  fflush is merely the guinea pig here.


You suggested an alternative approach to this problem three years ago,
here:

http://lists.gnu.org/archive/html/bug-cvs/2002-07/msg00162.html

The basic idea is to use a pipe and an intervening subprocess to work
around the problem.  That is, if the application detects that a file
is in O_NOBLOCK mode, it forks a subprocess to talk to that file
directly, and the parent process replaces O_NONBLOCKed file descriptor
with a pipe to the subprocess.  The pipe will not be in O_NONBLOCK
mode, so it will work reliably with the parent process's stdio code.
The child process will run code taken from the blocking-io module that
uses low-level read, write, and select (or poll) calls to do the I/O
reliably.

I have implemented a quick shell-script approximation to this
suggestion, in which the child process is simply "cat -u".  This
script is called "ssh4cvs".  You can find ssh4cvs here:

http://lists.gnu.org/archive/html/bug-gnulib/2004-09/msg3.html

I've been using ssh4cvs with GNU Emacs 21.4 and a recent CVS version
(currently 1.12.9 as patched by Debian) since September 2004.  It
works for me.

More details about this suggestion are here:

http://lists.gnu.org/archive/html/bug-cvs/2002-08/msg00032.html

There were some objections at the time to the overhead of
the extra process, but I agree with what you wrote in July 2002:

  Please don't dismiss that simple idea because it is "inefficient"; the
  inefficiency of this will usually be insignificant compared with the
  slowness of the network, and this needs to be solved somehow.

  



> Isn't that a bug in FreeBSD?

Possibly, but appears that the same bug is in Solaris 10 and in
Debian GNU/Linux 3.1.  If the bug is that widespread, we have
to deal with it somehow.


___
Bug-cvs mailing list
Bug-cvs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-cvs

Re: CVS problem with ssh

[Paul Eggert]

Derek Price <[EMAIL PROTECTED]> writes:

>>You have to include stdio.h first, then #undef fwrite, then
>>#define fwrite.  It can be done (Emacs did it, after all), but I don't
>>recommend it.
>>
> Isn't this the standard behavior for a GNULIB module's header file? 
> This is almost exactly what unlocked-io does, after all.

Yes, that's true.  I'm starting to unbend on that recommendation, for
gnulib code anyway.  It's not something that can be done casually, and
it certainly wouldn't have worked in the gnulib context 10 or 20 years
ago (because files could not routinely be reincluded back then), but
I think it's OK nowadays.


>>As far as I can tell POSIX doesn't guarantee that repeated fflushes
>>will eventually write all the data, with no duplicates, in this
>>situation.
>
> Maybe not, but isn't this by far the implication of EAGAIN in most other
> contexts?  Couldn't do what you asked this time, try *AGAIN*?  I can
> understand the problem with fwrite and an object size > 1 since there is
> no way for the function to return a partial item count, but once stdio
> has the data, why should fflush not track what was written properly? 

Because fflush gets an error.  Common practice is to stop writing when
you get an error.

> GNULIB:
>
>1. All the reasons I want this in GLIBC.
>2. Ease of importing GLIBC updates to this code into CVS.

How does the proposed blocking-io module interact with the existing
unlocked-io module in gnulib?  It seems to me that they are
incompatible, since they both attempt to #define fwrite, for example.
This is a fairly serious problem, I'd think.  Can this be fixed
(possibly by modifying unlocked-io as well), so that the two modules
are compatible?


> Aside from bugs, this is what I would expect from most fflush
> implementations.  How about we install the module and see what sort of
> problem reports we get?

That sounds pretty drastic.  How about if we try this idea out first,
before installing it?

I just checked FreeBSD libc, and it appears that fflush fails once its
underlying write fails with EAGAIN.  So it appears that this approach
won't work under FreeBSD.  That's not a good sign.

Can you write a program to detect whether fflush reliably restarts?  I
just wrote the program below to do that, derived from your code, and
it fails for me both with Debian GNU/Linux 3.1 r0a and with Solaris
10, so that's not a good sign either.

But perhaps I'm not understanding what you're trying to do.



/* The idea of this module is to help programs cope with output
   streams that have been set nonblocking.

   To use it, simply make these definitions in your other files:

   #define printf blocking_printf
   #define fprintf blocking_fprintf
   #define vprintf blocking_vprintf
   #define vfprintf blocking_vfprintf
   #undef putchar
   #define putchar blocking_putchar
   #undef putc
   #define putc blocking_putc
   #define fputc blocking_putc
   #define puts blocking_puts
   #define fputs blocking_fputs
   #define fwrite blocking_fwrite
   #define fflush blocking_fflush

   and link with this module.  */

#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 


/* EWOULDBLOCK is not defined by POSIX, but some BSD systems will
   return it, rather than EAGAIN, for nonblocking writes.  */
#ifdef EWOULDBLOCK
# define blocking_error(err) ((err) == EWOULDBLOCK || (err) == EAGAIN)
#else
# define blocking_error(err) ((err) == EAGAIN)
#endif

int
blocking_fwrite (const void *buffer, size_t size, size_t count, FILE *stream)
{
  size_t count_left = count;
  size_t count_written = 0;
  int desc = fileno (stream);

  if (size == 0 || count == 0)
return 0;

  /* Handle the case where SIZE * COUNT overflows.
 (The comparison is designed to be conservative.)  */
  if (size > 1 && (double)size * (double)count > SIZE_MAX / 4)
{
  const char *p = (const char *) buffer;
  for (count_written = 0; count_written < count; count_written++)
{
  if (blocking_fwrite (p, sizeof *p, size, stream) < size)
break;
  p += size;
}
  return count_written;
}

  /* When SIZE is > 1, convert to the byte-wise case
 since that is the only way fwrite undertakes not to repeat
 writing part of the data.  */
  if (size > 1)
{
  size_t total = size * count;
  unsigned int value = blocking_fwrite (buffer, sizeof 1, total,
stream);
  /* It is ok, according to POSIX, if VALUE is not a multiple
 of SIZE, because fwrite in that case can write part of an object.  */
  return value / size;
}

  while (1)
{
  int written;
  fd_set set;

  written = fwrite (buffer, 1, count_left, stream);
  count_written += written;
  if (written == count_left)
break;
  if (blocking_error (errno))
break;

  /* Wait for space to be available.  */
  FD_ZERO (&set);
  FD_SET (desc, &set);
  select (desc + 1, NULL, &

Re: [bug-gnulib] New GNULIB glob module?

[Paul Eggert]

Derek Price <[EMAIL PROTECTED]> writes:

> It may be.  It looks like the change was intentional
> (http://sources.redhat.com/cgi-bin/cvsweb.cgi/libc/sysdeps/generic/glob.c?rev=1.52&content-type=text/x-cvsweb-markup&cvsroot=glibc),
> but I still disagree.

I agree with you.  Historically, the "*" pattern in the shell has
always matched dangling links.  Otherwise "rm *" wouldn't work as
desired.

> I ran your test program on NetBSD 1.6.1 and it does return the
> broken symlink.

OK, let's argue that BSD is right and Solaris is wrong, and glibc
should be fixed.  If necessary we can take this to the POSIX committee.
In the mean time we can submit the three patches.  I'll take a look
at doing that.


___
Bug-cvs mailing list
Bug-cvs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-cvs

Re: [bug-gnulib] New GNULIB glob module?

[Paul Eggert]

Derek Price <[EMAIL PROTECTED]> writes:

>1. Corrects an incorrect check for a successful return from
>   getlogin_r to assume only 0 means success, per the POSIX2 spec:
>   
> .
>2. Moves the check for GLOB_MARK directory status (and the append of
>   `/') into glob_in_dir, where it is more efficient than performing
>   a second pass and sometimes calling stat a second time on each
>   file or directory.  All calls to stat are avoided when
>   dirent->d_type is available.  No call to realloc of the directory
>   name is ever necessary since room for the slash can be allocated
>   in the first pass.

These changes sound reasonable, though we should submit them as
separate patches.

Is (2) independent of (3)?  (Please see below for why this is important.)


>3. Ignores broken links only when GLOB_ONLYDIR is set.  With glibc
>   versions 2.3.3 through 2.3.5, the following in an empty directory
>   would return nothing:
>
>   ln -s doesnt-exist linkname
>   glob ("*", ...)
>
>   This fix syncs with the comments in the file, syncs with the
>   POSIX2 spec, restores the pre-glibc-2.3.3 behavior, and simply
>   makes more sense - why should `ls *' fail to list broken links?

This change sounds controversial to me.  glibc 2.3.5 behaves similarly
to Solaris 8 and to Solaris 10 -- I just checked, with the following
program and with the working directory containing only a dangling
symlink:

  #include 
  #include 

  int
  main (void)
  {
glob_t g;
int r = glob ("*", 0, NULL, &g);
int i;

if (r != 0)
  {
fprintf (stderr, "glob failed (%s)\n",
 r == GLOB_NOMATCH ? "GLOB_NOMATCH"
 : r == GLOB_NOSPACE ? "GLOB_NOSPACE"
 : "other glob failure");
return 1;
  }

for (i = 0; i < g.gl_pathc; i++)
  puts (g.gl_pathv[i]);
return 0;
  }

Solaris 8 and 10 both report "glob failed (GLOB_NOMATCH)".

Let's separate (3) into a separate patch and think about it more
carefully before submitting it.

Have you investigated with FreeBSD glob does?  It seems to use
gl_lstat, which our glob doesn't.  That's odd.  What's the point of
having a gl_lstat if it's never used?


___
Bug-cvs mailing list
Bug-cvs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-cvs

Re: [bug-gnulib] New GNULIB glob module?

[Paul Eggert]

Derek Price <[EMAIL PROTECTED]> writes:

> After that I will write up a ChangeLog entry for the glob-glibc2gnulib
> diff and submit our changes back to the glibc team, unless someone here
> who is used to working with them would like to take a go at the actual
> submission part.  Perhaps it would be smoother if someone already known
> to the glibc team introduced me and this patch?

Yes, probably.  I'm willing to have a go at it.

I suggest submitting two patches.

(1) the part that makes it work with gnulib.
(2) the bug fix.

It's OK for (2) to assume that (1) has already been applied.

Each patch should have its own Changelog entry.

> Is the "definitive" version of shared files usually in glibc,
> gnulib, or wherever someone last remembered to merge when this
> happens?

It's supposed to be in glibc, but we're not as good at keeping glibc
up to date as we should be.


___
Bug-cvs mailing list
Bug-cvs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-cvs

Re: [bug-gnulib] New GNULIB glob module?

[Paul Eggert]

Derek Price <[EMAIL PROTECTED]> writes:

> Fair enough, but why undo the change to glob.m4?  Shouldn't I just
> change the target of the AC_DEFINE from MISSING_SYS_CDEFS_H to _SYS_CDEFS_H?

Yes, you're right.

Sorry, I'd forgotten the trick that I had suggested.  (This suggests
that it deserves a nice fat comment.  :-)


___
Bug-cvs mailing list
Bug-cvs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-cvs

Re: [bug-gnulib] New GNULIB glob module?

[Paul Eggert]

Derek Price <[EMAIL PROTECTED]> writes:

> I chose the _SYS_CDEFS_H route since it seemed simplest to me, though I
> chose to name the macro `MISSING_SYS_CDEFS_H'.

Sorry, that's not right, since it fails in the following scenario:

  #define MISSING_SYS_CDEFS_H 27
  #include 

in an ordinary program that is not using gnulib, but is using glibc.
MISSING_SYS_CDEFS_H is part of the user's namespace, and should not affect
how  works.

Let's just use _SYS_CDEFS_H.  We can then remove all mention of
MISSING_SYS_CDEFS_H from glob_.h and undo the most recent change to
glob.m4.  This should be OK: if we run into some non-glibc that has
this problem it will probably use some other include file anyway.

> You told me it was safe to make this assumption originally.

Sorry, I hadn't thought through the implications of merging this back
into glibc.


___
Bug-cvs mailing list
Bug-cvs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-cvs

Re: [bug-gnulib] New GNULIB glob module?

[Paul Eggert]

Derek Price <[EMAIL PROTECTED]> writes:

> +#else
> +/* Is there really a case where the getlogin or getlogin_r proto can come 
> from
> +   somewhere other than ?  */
> +# ifdef HAVE_GETLOGIN_R
> +extern int getlogin_r (char *, size_t);
> +# else
> +extern char *getlogin (void);
> +# endif
> +#endif

This comment is somewhat odd, and suggests that you're as troubled by the
code as I am.  How about if you replace the above by the following:

   #if ! HAVE_DECL_GETLOGIN_R
   # if ! HAVE_DECL_GETLOGIN
   char *getlogin (void);
   # endif
   static int
   getlogin_r (char *name, size_t size)
   {
 char *n = getlogin ();
 if (n)
   {
 size_t nlen = strlen (n);
 if (nlen < size)
   {
 memcpy (name, n, nlen + 1);
 return 0;
   }
 errno = ERANGE;
   }
 return -1;
   }
   #endif

and modify the rest of the code to assume that getlogin_r works, and
modify glob.m4 to check for whether getlogin and getlogin_r are
declared, rather than whether they exist.  Or, perhaps better, you can
create a getlogin_r module with the above code, and have the glob
module depend on that.

> +# ifdef HAVE___POSIX_GETPWNAM_R
> +   /* Solaris.  */
> +#  define getpwnam_r(name, bufp, buf, len, res) \
> +__posix_getpwnam_r (name, bufp, buf, len, res)
> +# endif

I don't see why this is needed.  The Solaris include files already
contain the magic necessary to convert getpwnam_r to
__posix_getpwnam_r, right?  Or is it because that depends on
_POSIX_PTHREAD_SEMANTICS?  If so, why not define that, as follows?  I
don't see why any gnulib-using application would want the nonstandard
Solaris versions of the *_r functions.

--- extensions.m4.~1.6.~2005-02-23 05:49:36 -0800
+++ extensions.m4   2005-05-24 12:35:48 -0700
@@ -21,6 +21,10 @@ AC_DEFUN([gl_USE_SYSTEM_EXTENSIONS], [
 [/* Enable extensions on Solaris.  */
 #ifndef __EXTENSIONS__
 # undef __EXTENSIONS__
+#endif
+#ifndef _POSIX_PTHREAD_SEMANTICS
+# undef _POSIX_PTHREAD_SEMANTICS
 #endif])
   AC_DEFINE([__EXTENSIONS__])
+  AC_DEFINE([_POSIX_PTHREAD_SEMANTICS])
 ])


> +#if defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE
>  # define HAVE_D_TYPE 1
> -#endif

At this point it would be simpler and clearer if we removed these
lines, and removed all uses of HAVE_D_TYPE.  We can simply substitute
"#ifdef _DIRENT_HAVE_D_TYPE" for "#ifdef HAVE_D_TYPE" later.
(HAVE_STRUCT_DIRENT_D_TYPE isn't relevant for that ifdef.)


Now turning to glob_.h:

> +#if defined _LIBC || HAVE_SYS_CDEFS_H
> +# include 
> +#endif

Ouch.  I just noticed that this isn't right for the case where glob.h
is installed into /usr/include/glob.h as part of a typical C library
installation.  First, _LIBC won't be defined there.  Second, glibc
can't infringe on the user's name space by depending on HAVE_SYS_CDEFS_H.

One way to work around this is to rewrite it as follows:

#ifndef _SYS_CDEFS_H
# include 
#endif

where glob.m4 defines _SYS_CDEFS_H if sys/cdefs.h is missing.
This differs from the usual gnulib convention, but it might
be simplest.

Another possibility is to do "mkdir sys; echo '' >sys/cdefs.h" if
 is missing, so that "#include " always
works.  This is a bit yucky on the build side, though it simplifies
the source code.


> +/* We need `size_t' for the following definitions.  */
> +#ifdef _LIBC
> +__BEGIN_DECLS

The comment seems a bit misplaced now.

How about if we build on my previous suggestion above, as follows?

#ifndef _SYS_CDEFS_H
# include 
#endif
#ifndef __BEGIN_DECLS
# define __BEGIN_DECLS
# define __END_DECLS
# define __THROW
#endif

and remove the existing "define __THROW".  This makes it clearer that
these three macros are defined by sys/cdefs.h.  Then we can keep the
__BEGIN_DECLS, __THROW, etc. stuff just the way that glibc has it.


> +#if defined _LIBC && defined __USE_LARGEFILE64
> +# ifdef __USE_GNU
> +struct stat64;
> +# endif

This isn't right, since glob.h can't depend on _LIBC (as mentioned above).
Please replace "defined _LIBC" with "!defined GLOB_PREFIX", everywhere
that you see _LIBC used (other than the changes already mentioned above).

> -# if defined __GNUC__ && __GNUC__ >= 2

This doesn't look right, since when installed as a part of glibc, the
header must be useable by GCC 1, or by non-GCC compilers.  Shouldn't
we leave this stuff in?  You might want to change it to

  # if defined __GNUC__ && __GNUC__ >= 2 && !defined GLOB_PREFIX

> -#if __USE_FILE_OFFSET64 && __GNUC__ < 2
> -# define glob glob64
> -# define globfree globfree64

Similarly, this code should be left in, with the first line changed to

  #if __USE_FILE_OFFSET64 && __GNUC__ < 2 && !defined GLOB_PREFIX

> -#if !defined __USE_FILE_OFFSET64 || __GNUC__ < 2
> +#if !defined _LIBC || !defined __USE_FILE_OFFSET64

Similarly, this should be:

  #if !defined __USE_FILE_OFFSET64 || __GNUC__ < 2 || defined GLOB_PREFIX


___
Bug-cvs mailing list
Bug-cvs@gnu.org
http://lists.gnu.o

Re: [bug-gnulib] New GNULIB glob module?

[Paul Eggert]

Derek Price <[EMAIL PROTECTED]> writes:

> all the HAVE_.*64 stuff in glob.c shouldn't be necessary when !_LIBC.

Yes, that's correct; it should just invoke stat, opendir, etc., without
worrying about their 64-bit variants.  I thought it already did that?
If not, where does it not do it?

> Also, this particular bit in glob_.h would need to be used when !_LIBC
> as well, since user-space programs compiling against the glob.h system
> header from glibc would want glob64 used as compile-time magic, correct?

Sorry, you've lost me here (I don't know what "this particular bit"
is).  If !_LIBC, then glob_.h should simply declare glob and globfree,
using the standard (i.e., non-64-bit) type names.  These functions
will use 64-bit types if they are available, but the user code will
too and the user code won't need to worry about this.  I had thought
that this is what the code did already.


> Please excuse me if this is way off, I don't have much experience at the
> 64-bit file offset stuff, myself.

The basic idea is that GNU applications never need access to names
like stat64, open64, etc.  They just invoke stat and open.

The only reason for stat64 and open64 is for support of mixed
applications, where some of the code uses 32-bit interfaces and other
code uses 64-bit.  This is useful when you don't have access to the
source of legacy code that knows only about 32-bit interfaces.  But
GNU programmers have access to the source code, so we can simply
recompile it to use 64-bit interfaces uniformly, which we can then
simply call via their usual names stat, open, etc.


> One thing that leads me to believe I may be wrong is that it looks
> like the glob64 from glibc 2.3.5 is only a stub function.

I don't remember the details, but I vaguely recall that glob64 was a
mistake.  Let's not worry about it for gnulib.


___
Bug-cvs mailing list
Bug-cvs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-cvs

Re: [bug-gnulib] New GNULIB glob module?

[Paul Eggert]

Derek Price <[EMAIL PROTECTED]> writes:

>>But let's step back a second. Why are we worried about building
>>gnulib glob.c under glibc? It will never happen, right? So perhaps
>>we needn't worry about this problem at all.
>
> Won't it?  I thought the idea was that when you and I settled on
> something that looked good I would attempt to send it back to the libc
> folks so that the files wouldn't need to be maintained separately.

Yes, that's the idea, but that's a different scenario.  In that
scenario we are building glob.c as part of glibc, so _LIBC is 1.  I am
talking about the scenario where glob.c is part of a user program
(i.e., glob.c is imported from gnulib), and where the C library is
glibc, but we are compiling glob.c anyway.  Will that ever happen?

Answering my own question: yes it will, in general, because perhaps
the gnulib version fixes a bug that's in the glibc version, and the
user wants the gnulib version.  So we should worry about this.
And (as far as I can see) the simplest fix is to do this:

#if defined _LIBC || HAVE_SYS_CDEFS_H
# include 
#endif

with the appropriate change to glob.m4 to define HAVE_SYS_CDEFS_H.


___
Bug-cvs mailing list
Bug-cvs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-cvs

Re: [bug-gnulib] New GNULIB glob module?

[Paul Eggert]

Derek Price <[EMAIL PROTECTED]> writes:

>>This seems a bit brittle.  Why not simply try to compile this program?
>>
>>  #include 
>>  char a[_GNU_GLOB_INTERFACE_VERSION == 1 ? 1 : -1];
>
> Because I like to avoid runtime tests if I can avoid it, since they
> cannot be used when cross-compiling.

The code that I proposed does not have any runtime tests, since the
array is allocated statically.  Tests like this are often used during
cross-compiling.

> it should be pretty safe to grep for "gnu_glob_version = 1", unless you
> are worried about some future CPP padding with spaces before the EOL?

Yes, that sort of thing.  cpp can even insert newlines if it wants to.
It's best to avoid cpp tricks if there is an easy substitute, which
there is here.

> There is one change I know I made that might have an effect.  I added
> the "!defined _LIBC" to the following block because I was unsure what
> __REDIRECT_NTH was supposed to be doing

I think that one's OK.

> Fixed.  It's still listed in srclist.txt as "gpl" however, since the
> other glibc modules were.  Why is that?

That tells the import-from-glibc module to use the GPL in the CVS
repository.  It's fine.

More messages to follow, since I need to look at the code again.


___
Bug-cvs mailing list
Bug-cvs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-cvs

Re: [bug-gnulib] New GNULIB glob module?

[Paul Eggert]

Derek Price <[EMAIL PROTECTED]> writes:

>>Why do we need to include  here?  All we need is size_t,
>>right?  And stddef.h gives us that.
>
> If I don't, I get the following error:
>
> In file included from glob.c:23:
> glob.h:107: error: syntax error before "struct"
> In file included from /usr/include/errno.h:36,
>  from glob.c:25:
> /usr/include/bits/errno.h:38: error: syntax error before "extern"

Sorry, I can't debug that, since I don't have the exact version you do.

In the latest version you sent, we have code that looks like this:


   #ifdef _LIBC
   # include 
   #else
   # include 
   # include 
   # undef __size_t
   # define __size_t size_t
   #endif

   __BEGIN_DECLS

First, already we have something bogus: that __BEGIN_DECLS.  It must
be protected by "#ifdef _LIBC", since random C environments don't have
it.  Similarly for __END_DECLS.

Second, that  ought to be unnecessary.  We need only
size_t, and  provides that.  If we need  for
some other reason on your platform, let's figure out why that is, and
attack its root source rather than the symptoms.  (The rest of this
message does that, I think.)

> Hrm.  Tracing this a little farther, it is only , or even
> the ,

OK, here's what's happening, I think.  Every glibc header includes
 (and thus ) first thing, and this defines a
whole bunch of stuff.  If you attempt to declare libc-related stuff
without including  first, bad things happen.

The simplest fix would be to do something like this:

#if defined _LIBC || HAVE_SYS_CDEFS_H
# include 
#endif

with the appropriate change to glob.m4.

But let's step back a second.  Why are we worried about building
gnulib glob.c under glibc?  It will never happen, right?  So perhaps
we needn't worry about this problem at all.


> I copied and pasted this comment and the #undef block
> from the glob.c file.  It was previously placed just prior to the
> #include of .  This looked the better place for it since I
> assumed that we wouldn't want applications using the GNULIB glob module
> to issue lots of redefinition warnings.

OK, but in general this problem cannot be fixed in glob_.h.  If the
application includes our glob.h and then the buggy system files, it
will still lose big-time.

I tracked down these #undefs to this patch:

http://sourceware.org/cgi-bin/cvsweb.cgi/libc/posix/Attic/glob.c.diff?r1=1.20&r2=1.21&cvsroot=glibc

which has this ChangeLog entry in glibc:

Thu Jan 21 20:12:25 1993  Roland McGrath  ([EMAIL PROTECTED])

* posix/glob.c: Put #includes of  and  after
all system includes, in case one of them has conflicting defns of
FNM_* or GLOB_*, so we will redefine.  #undef FNM_* and GLOB_*
before including our headers.

I have heard of this problem occurring with  -- indeed,
 documents that it occurs with HP-UX A.08.07 -- but it's
not clear to me that the problem every actually occurred with
.  Perhaps Roland made the change for  simply as a
precaution, because it had happened with .

Also, the problem, if it occurred with glob.h, was a long time ago,
and perhaps we need not worry about it any more.  HP-UX 8.07 was
released in 1992, and HP hasn't supported it for many years.  I doubt
whether people are building new GNU software for it any more.

With that in mind, how about if we simply remove those #undef's?  If
the problem recurs we can put them back in again.


___
Bug-cvs mailing list
Bug-cvs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-cvs

Re: [bug-gnulib] New GNULIB glob module?

[Paul Eggert]

Derek Price <[EMAIL PROTECTED]> writes:

> Are you sure?  You asked me to restore similar parens around bit-ands
> back at several other locations despite other work that changed the
> lines, in an earlier email.  Not that I disagree now.  I actually prefer
> the version without the unnecessary parens around the  bit-and.  I just
> think we should be consistent.

It's a minor point, but expressions like (a & b && c) are assumed to
be confusing, as they depend on obscure precedence rules, whereas
expressions like (a & b ? c : d) are not confusing in the same way:
there's only one way to parse them, even if you have forgotten the
precedence.


___
Bug-cvs mailing list
Bug-cvs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-cvs

Re: [bug-gnulib] New GNULIB glob module?

[Paul Eggert]

One other remark about glob.c.  You should remove this comment, as
it's obsolete now:

/* Comment out all this code if we are using the GNU C Library, and are not
   actually compiling the library itself.  This code is part of the GNU C
   Library, but also included in many other GNU distributions.  Compiling
   and linking in this code is a waste when using the GNU C library
   (especially if it is a shared library).  Rather than having every GNU
   program understand `configure --with-gnu-libc' and omit the object files,
   it is simpler to just do this in the source for each such file.  */


Now for comments on m4/glob.m4 and modules/glob.  This finishes up
this round of reviewing.

gl_GLOB_SUBSTITUTE and gl_GLOB both invoke gl_PREREQ_GLOB.  This
is overkill, surely.  I'd say only the former needs to invoke it.

> +  AC_CHECK_HEADERS([glob.h gnu-versions.h], [], [GLOB_H=glob.h])
> +
> +  if test -z "$GLOB_H"; then
> +AC_EGREP_CPP([gl_gnu_glob_version = 1$],
> +[#include 
> +gl_gnu_glob_version = _GNU_GLOB_INTERFACE_VERSION],
> +  [], [GLOB_H=glob.h])
> +  fi

This seems a bit brittle.  Why not simply try to compile this program?

  #include 
  char a[_GNU_GLOB_INTERFACE_VERSION == 1 ? 1 : -1];

> +  AC_CHECK_HEADERS_ONCE([dirent.h sys/ndir.h sys/dir.h ndir.h])dnl

Substitute AC_REQUIRE([AC_HEADER_DIRENT]) for this line.

> +  AC_CHECK_FUNCS_ONCE([dirent64 getlogin_r getpwnam_r stat64])dnl

The other three functions are fine, but: dirent64 is a function?  I
thought it was a structure tag.

Surely you mean to use AC_CHECK_TYPE([struct dirent64]), and for the
code to to refer to HAVE_STRUCT_DIRENT64 rather than HAVE_DIRENT64?

Come to think of it, how does the code currently work here?  In the
normal case (where the maintainer has not used configure's
--disable-largefile option, and where the system supports a 64-bit
file interface) we want glob to use the 64-bit file interface.  Is
that what is happening here?  I can't quite follow it.


> +++ modules/glob  13 May 2005 16:23:46 -
> @@ -0,0 +1,43 @@
> +Description:
> +Search for files and directories with paths matching a pattern.
> +
> +Depends-on:
> +alloca
> +extensions
> +fnmatch
> +mempcpy
> +realloc

I don't see why this module depends on the realloc module.  It doesn't
ever realloc anything to size zero.

> +License:
> +??? LGPL ???

Yes, it's LGPL.


___
Bug-cvs mailing list
Bug-cvs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-cvs

Re: [bug-gnulib] New GNULIB glob module?

[Paul Eggert]

Now for commentary on glob.h.

Derek Price <[EMAIL PROTECTED]> writes:

> --- ../glibc-2.3.5/posix/glob.h   2004-09-16 20:55:15.0 -0400
> +++ lib/glob_.h   2005-05-13 12:21:39.0 -0400
> @@ -19,29 +19,48 @@
>  #ifndef  _GLOB_H
>  #define  _GLOB_H 1
>  
> -#include 
> +#ifdef _LIBC
> +# include 
> +#else
> +# include 
> +# include 
> +# undef __size_t
> +# define __size_t size_t
> +#endif

Why do we need to include  here?  All we need is size_t,
right?  And stddef.h gives us that.

> +/* Some system libraries erroneously define these.  */
> +#undef   GLOB_ERR
> ...

Which system libraries are these?  Perhaps a comment?

> +# define getopt __GLOB_ID (getopt)

Surely this line is spurious and can be removed.

> -#if !defined __USE_FILE_OFFSET64 || __GNUC__ < 2
> +#if !defined _LIBC || !defined __USE_FILE_OFFSET64 || __GNUC__ < 2

Can't we remove at least the "|| __GNUC__ < 2" here?  glibc assumes
recent versions of GCC.

>  extern void __REDIRECT_NTH (globfree, (glob_t *__pglob), globfree64);
>  #endif
> -
>  #ifdef __USE_LARGEFILE64
>  extern int glob64 (__const char *__restrict __pattern, int __flags,
>  int (*__errfunc) (__const char *, int),

Let's omit this change; it's a different matter.


___
Bug-cvs mailing list
Bug-cvs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-cvs

Re: [bug-gnulib] New GNULIB glob module?

[Paul Eggert]

Derek Price <[EMAIL PROTECTED]> writes:

>  /* Enable GNU extensions in glob.h.  */
> -#ifndef _GNU_SOURCE
> +#if defined _LIBC && !defined _GNU_SOURCE
>  # define _GNU_SOURCE 1
>  #endif

I just checked the glibc source file include/libc-symbols.h, and it
defines both _LIBC and _GNU_SOURCE.  So this stuff isn't needed at
all; please remove these lines from glob.c instead.

> +   HAVE_STRUCT_DIRENT_D_TYPE is defined by GNULIB's glob.m4 when the same
> +   member is found.  */

A bit wordy; please rephrase to "HAVE_STRUCT_DIRENT_D_TYPE plays the
same rule in gnulib."

> -  qsort ((__ptr_t) &pglob->gl_pathv[oldcount],
> +  qsort ((void *) &pglob->gl_pathv[oldcount],

You can remove the cast entirely, right?

> -   free ((__ptr_t) pglob->gl_pathv[pglob->gl_offs + i]);
> -  free ((__ptr_t) pglob->gl_pathv);
> +   free ((void *) pglob->gl_pathv[pglob->gl_offs + i]);
> +  free ((void *) pglob->gl_pathv);

Similarly, these casts aren't needed.

-   free ((__ptr_t) array[--i]);
+   free ((void *) array[--i]);

Nor these.

> - free ((__ptr_t) array[--i]);
> + free ((void *) array[--i]);

Nor this one.

> +   p = mempcpy ((void *) new->name, name, len);

Nor this one.

> - free ((__ptr_t) names->name);
> + free ((void *) names->name);

Nor this.


> -  return (((flags & GLOB_ALTDIRFUNC)
> -? (*pglob->gl_stat) (fullname, &st)
> -: __stat64 (fullname, &st64)) == 0);
> +  return ((flags & GLOB_ALTDIRFUNC)
> +   ? (*pglob->gl_stat) (fullname, &st) == 0 && S_ISDIR (st.st_mode)
> +   : __stat64 (fullname, &st64) == 0 && S_ISDIR (st64.st_mode));

As long as you're reparenthesizing you might as well get rid of
the unnecessary ones around (flags & GLOB_ALTDIRFUNC).

> +   new->name = malloc (len + 1
> +   + ((flags & GLOB_MARK) && isdir));

This line is 80 columns; should probably shrink it to 78, e.g., via.

  new->name =
malloc (...);

glob.c is looking pretty good now.  I gotta run now, but I'll look at
glob.h later today I hope.


___
Bug-cvs mailing list
Bug-cvs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-cvs

Re: [bug-gnulib] New GNULIB glob module?

[Paul Eggert]

Derek Price <[EMAIL PROTECTED]> writes:

> We've been removing those sort of constructs from CVS as part of the
> move away from K&R support.  IIRC, I was told that typecasts to and from
> (void *) were necessary on some really old systems, but that I could be
> confident that we wouldn't encounter any C89 systems that required it. 

That's correct.

> Perhaps this is simply legacy code related to the __ptr_t stuff and
> could be removed as well?

Yes, quite possibly.  How about if we simply assume C89-or-later for
now.  If someone runs into a problem with a bogus masquerading
compiler we can fix it later.

> why does that preclude a #include  in glob.h?

Because in glibc #include  isn't supposed to define irrelevant
identifiers like wchar_t (which  does define).

It's OK for #include  to define these identifiers in gnulib,
though.  Partly because we can't avoid it, and partly because we're
not as persnickety anyway.

I'll review the rest of your message later (sorry, too many meetings today!).


___
Bug-cvs mailing list
Bug-cvs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-cvs

Re: [bug-gnulib] New GNULIB glob module?

[Paul Eggert]

Derek Price <[EMAIL PROTECTED]> writes:

>>First, what is "__ptr_t"?  Shouldn't it be replaced by "void *"
>>uniformly?
>
> This is one of the items I didn't touch since it didn't raise any
> compiler warnings here.  It looks like it is mostly being used as a void
> *, though I'm not sure why since there are a few direct void * decls. 

My guess is that __ptr_t is a glibc thing, and is present to allow
ports to pre-C89 hosts that lacked support for void *.  We no longer
worry about such hosts, so we can simply use void *.

> It looks like there are a few unnecessary typecasts

Possibly they're needed for people who use C++ compilers to
compile C code?

> It's okay to assume C89 or better for glibc?  I knew it was correct for
> GNULIB but I was unsure about glibc.

Yes, it's OK.

> Otherwise, my correction is as you specified, but if we can assume C89
> in glob.c, why can't we assume C89 here and just #include ?

glob.h is not supposed to infringe on the user namespace when it's
part of glibc.  However, it's OK (and to some extent unavoidable) for
glob.h to do so when it's part of user code.

> -#if defined HAVE_UNISTD_H || defined _LIBC
> +#if HAVE_UNISTD_H || _LIBC

I'd omit this change; it's not necessary and it just complicates
the job of evaluating the patch.

>  # ifdef HAVE_VMSDIR_H
> +   /* Should this be imported from glibc?  */
>  #  include "vmsdir.h"

glibc doesn't have a vmsdir.h.  This area should be fixed up but this
is a separate matter; I'd omit this change for now.

> +# define CONVERT_D_TYPE(d64, d32)

This macro isn't used anywhere and can be removed.

> +# define MUST_BE(d, t)   ((d)->d_type == (t))
> +# define MAY_BE_LINK(d)  (MUST_BE((d), DT_UNKNOWN) || MUST_BE((d), 
> DT_LNK))
> +# define MAY_BE_DIR(d)   (MUST_BE((d), DT_DIR) || MAY_BE_LINK (d))

This is a bit hard to follow, I'm afraid, and the names are
contributing to the confusion.  We should use "MIGHT" instead of
"MAY", to avoid the classic "may" ambiguity.  How about if we rename
MUST_BE to DIRENT_MUST_BE, MAY_BE_LINK to DIRENT_MIGHT_BE_LINK, and
MAY_BE_DIR to DIRENT_MIGHT_BE_DIR?  We also refrain from having the
MAY_BE macros call MUST_BE.  Something like this:

/* True if the directory entry D must be of type T.  */  
# define DIRENT_MUST_BE(d, t) ((d)->d_type == (t))

/* True if the directory entry D might be a symbolic link.  */
# define DIRENT_MIGHT_BE_SYMLINK(d) \
((d)->d_type == DT_LNK || (d)->d_type == DT_UNKNOWN)

/* True if the directory entry D might be a directory.  */
# define DIRENT_MIGHT_BE_DIR(d) \
((d)->d_type == DT_DIR || DIRENT_MIGHT_BE_SYMLINK (d))

> -#if _LIBC
> +#if _LIBC || HAVE_FUNCTION_DIRENT64
>  # define HAVE_DIRENT64   1

I don't see why this change is needed.  The .m4 file sets
HAVE_DIRENT64.

> +/* GNULIB includes that shouldn't be used when copiling LIBC.  */

I'd remove this comment; it's obvious.

> +/* Correct versions of these headers all come from GNULIB when missing or
> + * broken.
> + */

Likewise.  Also, please use GNU style for comments (no leading-*).

  #ifdef HAVE_FUNCTION_STAT64
  /* Assume the struct stat64 type.  */
  # define HAVE_STAT64  1
  #endif

The m4 file defines HAVE_STAT64, so this entire code block should be
removed.

>  #ifndef HAVE_STAT64
> +  /* Use stat() instead.  */
>  # define __stat64(fname, buf) __stat (fname, buf)

I'd omit this change; the comment is obvious.

> +# define stat64 stat

I don't see why this #define is needed; doesn't the code use __stat64
uniformly?

> +#elif !_LIBC

Generally the code uses "defined _LIBC" rather than "_LIBC".  It's not
universally true, but I'd stick to the strong-majority style when adding
new code.

> +  /* GNULIB needs the usual versions.  */
> +# define __stat64 stat64

First, lose the comment (it's obvious); second, all the #define's of
__stat64 should be kept near each other.

> +  /* GNULIB needs the usual versions.  */

Again, the comment isn't needed.

> +# define __stat stat
> +# define __alloca alloca
> +# define __readdir readdir
> +# define __readdir64 readdir64

Perhaps these should be put next to the __stat64 define?

> -#include 

The Gnulib tradition is to include the interface file (here, )
first, immediately after including .  Could you please do
that here, too?

> -#ifdef HAVE_GETLOGIN_R
> +#ifdef HAVE_FUNCTION_GETLOGIN_R

This change shouldn't be needed.

> -
> +
> +
> +
>  static const char *next_brace_sub (const char *begin, int flags) __THROW;

This change shouldn't be needed.

> -#   if defined HAVE_GETLOGIN_R || defined _LIBC
> +#   if defined HAVE_FUNCTION_GETLOGIN_R || defined _LIBC

This change shouldn't be needed.

> -#   if defined HAVE_GETPWNAM_R || defined _LIBC
> +#   if defined HAVE_FUNCTION_GETPWNAM_R || defined _LIBC

Nor this.

> -#  if defined HAVE_GETPWNAM_R || defined _LIBC
> +#  if defined HAVE_FUNCTION_GETPWNAM_R || defined _LIBC

Nor this.

> -#endif
> +#endif /* !defined _LIBC || !defined NO_GLOB_PATTERN_P */

Nor this.

> -static int
> -link_exists_p (cons

Re: [bug-gnulib] New GNULIB glob module?

[Paul Eggert]

Derek Price <[EMAIL PROTECTED]> writes:

> I needed a portable version of glob for CVS, so I imported the glob
> module from glibc 2.3.5 into a GNULIB format.

Wow!  Nice project.

> Mostly I kept the glibc version intact, though I managed to simplify
> some portability cruft immensely by replacing huge chunks of code
> with #includes of GNULIB modules.

Ideally we should write code that can be folded back into glibc,
identical to the gnulib version.  That should be doable here, but some
more hacking is needed.

First, what is "__ptr_t"?  Shouldn't it be replaced by "void *"
uniformly?

> -/* AIX requires this to be the first thing in the file.  */
> -#if defined _AIX && !defined __GNUC__
> - #pragma alloca
> -#endif

This change is good, since glibc doesn't need that code and gnulib
doesn't either (any more).

> -#if defined HAVE_LIMITS_H || defined __GNU_LIBRARY__
> -# include 
> -#endif
> +#include 

This sort of change is good, since gnulib assumes C89 or better now.

> -#define GLOB_INTERFACE_VERSION 1
> -#if !defined _LIBC && defined __GNU_LIBRARY__ && __GNU_LIBRARY__ > 1
> -# include 
> -# if _GNU_GLOB_INTERFACE_VERSION == GLOB_INTERFACE_VERSION
> -#  define ELIDE_CODE
> -# endif
> -#endif

This removal looks reasonable, except shouldn't glob.m4 check that
_GNU_GLOB_INTERFACE_VERSION is 1?  It can do that as a compile-time
check.

> -#if defined HAVE_UNISTD_H || defined _LIBC
> +#if HAVE_HEADER_UNISTD_H || _LIBC

This sort of change doesn't look right.  Why not leave the code alone?
Nothing ever defines HAVE_HEADER_UNISTD_H.  There are several other
instances of this sort of thing, e.g., HAVE_HEADER_SYS_NDIR_H,
HAVE_DIRENT64.

> -#if _LIBC
> -# define HAVE_DIRENT64   1
> +#if defined _DIRENT_HAVE_D_TYPE && !defined HAVE_DIRENT_D_TYPE
> +# define HAVE_DIRENT_D_TYPE  1
>  #endif

This should use HAVE_STRUCT_DIRENT_D_TYPE, from d-type.m4.  Also,
don't see why you need to change the spelling of HAVE_D_TYPE.  Please
try to leave the identifiers the same, so that the glibc maintainers
have an easier time of following the patch.

> -#ifdef _LIBC
> -# include 
> -# undef strdup
> -# define strdup(str) __strdup (str)
> -# define sysconf(id) __sysconf (id)
> -# define closedir(dir) __closedir (dir)
> -# define opendir(name) __opendir (name)
> -# define readdir(str) __readdir64 (str)
> -# define getpwnam_r(name, bufp, buf, len, res) \
> -   __getpwnam_r (name, bufp, buf, len, res)
> -# ifndef __stat64
> -#  define __stat64(fname, buf) __xstat64 (_STAT_VER, fname, buf)
> -# endif
> -# define HAVE_STAT64 1
> -#endif

Please leave this sort of code in, so that the source code can be
identical in libc.

> +#include "mempcpy.h"
> +#include "stat-macros.h"
> +#include "strdup.h"

These includes need to be protected by #ifndef _LIBC.

> +int glob_pattern_p (const char *pattern, int quote);

I don't see why this declaration is needed in glob.c.
Isn't it in glob_.h?

> -   if (__glob_pattern_p (drive_spec, !(flags & GLOB_NOESCAPE)))
> +   if (glob_pattern_p (drive_spec, !(flags & GLOB_NOESCAPE)))

I'd leave the code alone here, to minimize the libc changes.
You can put a "#define __glob_pattern_p glob_pattern_p" inside
Similarly for __stat64, etc.

> -  if (flags & GLOB_MARK)
> -{
> -  /* Append slashes to directory names.  */
> -  size_t i;

Why remove this feature?  Is there any harm to leaving it alone?

>  int
> -__glob_pattern_p (pattern, quote)
> - const char *pattern;
> - int quote;
> +glob_pattern_p (const char *pattern, int quote)

This sort of change is good.

> -# ifdef _LIBC
> -weak_alias (__glob_pattern_p, glob_pattern_p)
> -# endif

We can leave the code alone here; it doesn't hurt.

>struct stat st;
> +# ifdef HAVE_FUNCTION_STAT64
>struct stat64 st64;
> +# endif

This sort of thing is awkward.  How about if you remove the
"# define st64 st" line?  Then you can omit this sort of change.

> -  return (((flags & GLOB_ALTDIRFUNC)
> -? (*pglob->gl_stat) (fullname, &st)
> -: __stat64 (fullname, &st64)) == 0);
> +  return flags & GLOB_ALTDIRFUNC
> +  ? (*pglob->gl_stat) (fullname, &st) == 0 && S_ISDIR (st.st_mode)
> +  : stat64 (fullname, &st64) == 0 && S_ISDIR (st64.st_mode);

Please stick with the original indenting style and names.

Why did you insert the && S_ISDIR here?  Is this a bug fix?  It's not
clear from the patch.  (Also, you need a ChangeLog entry suitable for
sending this patch to the glibc folks.)

> +#ifdef HAVE_DIRENT_D_TYPE
> +# define MAY_BE_LINK(d) (d->d_type == DT_UNKNOWN || d->d_type == DT_LNK)
> +# define MAY_BE_DIR(d)   (d->d_type == DT_DIR || MAY_BE_LINK (d))

Parenthesize (d) and move these definitions to an early part of the
program, and put in an else-part, e.g.:

#if HAVE_D_TYPE
# define MAY_BE_LINK(d) ((d)->d_type == DT_UNKNOWN || (d)->d_type == DT_LNK)
# define MAY_BE_DIR(d)  ((d)->d_type == DT_DIR || MAY_BE_LINK (d))
# define MUST_BE(d, t)  ((d)->d_type == (t))
#else
# define MAY_BE_LINK(d) true
# define MAY_BE_D

Re: [bug-gnulib] Re: getopt and Solaris 10

[Paul Eggert]

Derek Price <[EMAIL PROTECTED]> writes:

> Revised patch attached.

Thanks; I installed the following slightly-different patch.

2005-05-10  Derek Price  <[EMAIL PROTECTED]>

* getopt.m4 (gl_GETOPT): Check for Solaris 10 bug, not decl, when
possible.

--- getopt.m4   6 May 2005 01:04:20 -   1.9
+++ getopt.m4   10 May 2005 19:11:00 -  1.10
@@ -1,4 +1,4 @@
-# getopt.m4 serial 8
+# getopt.m4 serial 9
 dnl Copyright (C) 2002, 2003, 2004, 2005 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -39,8 +39,27 @@ AC_DEFUN([gl_GETOPT],
 dnl Solaris 10 getopt doesn't handle `+' as a leading character in an
 dnl option string (as of 2005-05-05).
 if test -z "$GETOPT_H"; then
-  AC_CHECK_DECL([getopt_clip], [GETOPT_H=getopt.h], [],
-   [#include ])
+  AC_CACHE_CHECK([for working GNU getopt function], 
[gl_cv_func_gnu_getopt],
+  [AC_RUN_IFELSE(
+[AC_LANG_PROGRAM([#include ],
+  [[
+char *myargv[3];
+myargv[0] = "conftest";
+myargv[1] = "-+";
+myargv[2] = 0;
+return getopt (2, myargv, "+a") != '?';
+   ]])],
+[gl_cv_func_gnu_getopt=yes],
+[gl_cv_func_gnu_getopt=no],
+[dnl cross compiling - pessimistically guess based on decls
+ dnl Solaris 10 getopt doesn't handle `+' as a leading character in an
+ dnl option string (as of 2005-05-05).
+ AC_CHECK_DECL([getopt_clip],
+  [gl_cv_func_gnu_getopt=no], [gl_cv_func_gnu_getopt=yes],
+  [#include ])])])
+  if test "$gl_cv_func_gnu_getopt" = "no"; then
+   GETOPT_H=getopt.h
+  fi
 fi
 
 if test -n "$GETOPT_H"; then


___
Bug-cvs mailing list
Bug-cvs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-cvs

Re: [bug-gnulib] Re: getopt and Solaris 10

[Paul Eggert]

Derek Price <[EMAIL PROTECTED]> writes:


> + myargv[[0]] = "conftest";
> + myargv[[1]] = "-+";

This doesn't null-terminate myargv.

But I still don't get why the change is needed.  It sounds like you're
assuming Solaris 11 getopt might get fixed?  But even in that case,
the current code will work, right, since it will use GNU getopt?  So
this is just an optimization for the hypothetical case if Solaris 11
getopt gets fixed?  In that case perhaps we should wait for Solaris 11
and test it before installing this patch, as it's more likely to cause
a bug than to fix one.


___
Bug-cvs mailing list
Bug-cvs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-cvs

Re: [bug-gnulib] Re: [PATCH] mmap-anon.m4: use proper macro & condition

[Paul Eggert]

"Mark D. Baushke" <[EMAIL PROTECTED]> writes:

> I have not seen any discussion or commit on this patch suggested by
> Moriyoshi Koizumi <[EMAIL PROTECTED]>

Sorry, I didn't see that.  The patch seems obvious so I installed it.
Thanks.


___
Bug-cvs mailing list
Bug-cvs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-cvs

Re: [bug-gnulib] Re: getopt and Solaris 10

[Paul Eggert]

Derek Price <[EMAIL PROTECTED]> writes:

> I prefer door #2.  Trivial patch attached:

Thanks, but I'd rather use AC_CHECK_DECL, so I installed this instead,
into both coreutils and gnulib.  Does it work?

2005-05-05  Paul Eggert  <[EMAIL PROTECTED]>

* lib/getopt.m4 (gl_GETOPT): Check for Solaris 10 getopt, and
avoid needless checks.

--- getopt.m4.~1.8.~2005-01-23 00:06:57 -0800
+++ getopt.m4   2005-05-05 17:53:53 -0700
@@ -1,5 +1,5 @@
-# getopt.m4 serial 7
-dnl Copyright (C) 2002, 2003, 2004 Free Software Foundation, Inc.
+# getopt.m4 serial 8
+dnl Copyright (C) 2002, 2003, 2004, 2005 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
 dnl with or without modifications, as long as this notice is preserved.
@@ -26,11 +26,22 @@ AC_DEFUN([gl_GETOPT],
   if test -z "$GETOPT_H"; then
 GETOPT_H=
 AC_CHECK_HEADERS([getopt.h], [], [GETOPT_H=getopt.h])
-AC_CHECK_FUNCS([getopt_long_only], [], [GETOPT_H=getopt.h])
+if test -z "$GETOPT_H"; then
+  AC_CHECK_FUNCS([getopt_long_only], [], [GETOPT_H=getopt.h])
+fi
 
 dnl BSD getopt_long uses an incompatible method to reset option processing,
 dnl and (as of 2004-10-15) mishandles optional option-arguments.
-AC_CHECK_DECL([optreset], [GETOPT_H=getopt.h], [], [#include ])
+if test -z "$GETOPT_H"; then
+  AC_CHECK_DECL([optreset], [GETOPT_H=getopt.h], [], [#include ])
+fi
+
+dnl Solaris 10 getopt doesn't handle `+' as a leading character in an
+dnl option string (as of 2005-05-05).
+if test -z "$GETOPT_H"; then
+  AC_CHECK_DECL([getopt_clip], [GETOPT_H=getopt.h], [],
+   [#include ])
+fi
 
 if test -n "$GETOPT_H"; then
   gl_GETOPT_SUBSTITUTE


___
Bug-cvs mailing list
Bug-cvs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-cvs

Re: Bad interaction between CVS and ssh due to libc

[Paul Eggert]

> From: [EMAIL PROTECTED] (Paul Jarc)
> Date: Fri, 09 Aug 2002 14:25:20 -0400
> 
> Derek Robert Price <[EMAIL PROTECTED]> wrote:
> > I assume they have different file table entries in the kernel
> > because stderr can be set to nonblock without doing the same to
> > stdout.  I need to know when the two file descriptors point to the
> > same file table entry (i.e. $ cvs  2>&1).
> 
> Ah.  So frob the flags on one of them and see if the other one changes
> too; then restore the original flags.  This still isn't perfect, for
> the same reason that the problem exists: another process with a copy
> of the same descriptor could be messing around with flags at the same
> time.  But that seems unlikely in the cases where the problem appears.

Since (as you mention) this optimization isn't safe, then I wouldn't
do it.  For this particular application, it's just an optimization to
check for the same file table entries; it's OK to omit the
optimization if it isn't safe.

In contrast, the st_dev+st_ino optimization is safe, since it doesn't
make any assumptions about what other processes are doing.


___
Bug-cvs mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/bug-cvs

Re: Bad interaction between CVS and ssh due to libc

[Paul Eggert]

This message follows up on a bug-cvs thread which can be viewed at:
http://groups.google.com/groups?th=e4df2fdc1f4f4950

> Date: Thu, 25 Jul 2002 12:07:16 -0600 (MDT)
> From: Richard Stallman <[EMAIL PROTECTED]>

> If all stdio output functions handle EAGAIN by sleeping for a short
> time and trying again, most user programs will be happy with the
> results.

This would contradict POSIX 1003.1-2001, which requires that fputc
must fail with EAGAIN in this situation, and that printf etc. must
operate as if by calling fputc.  I expect that there are some programs
that rely on this behavior.  So, if we need better support for stdio
to nonblocking file descriptors, I think it should be added as an
extension, not as an incompatible change.

In this case, though, I'd rather modify CVS to use a portable solution
as suggested below.  This will let CVS work now, even on older glibc
implementations, as well as on non-glibc implementations.


> Do you see another approach that really solves the problem?

You were on the right track in
http://mail.gnu.org/pipermail/bug-cvs/2002-July/009014.html
when you wrote:

> how about making CVS pipe the output through `cat' when
> it invokes a subprocess?  That should be easy.

I would improve on this suggestion as follows.  CVS should
automatically create a "cat"-like process when it is about to invoke
$CVS_RSH, if it detects that the problem might occur.  That is, if the
CVS client detects that stderr and stdout point to the same file, and
that the O_NONBLOCK flag might make a difference with that file (i.e.,
that the file is not a regular file), then the CVS client should
create an extra process to read bytes from $CVS_RSH's stderr and write
bytes to stderr.  That way, there will be no problem if $CVS_RSH sets
the O_NONBLOCK option on $CVS_RSH's stderr.

This "cat"-like process need not invoke the "cat" command -- it can
just read from one file descriptor and write to the other, without
execing any program.

It may sound inefficient to have an extra "cat"-like process, but it
doesn't cost much in practice.  For example, GNU tar sometimes creates
a "cat"-like process when it invokes "gzip" as a subcommand, since GNU
tar needs to reblock the output when writing to tape devices.  This
works fine and almost nobody notices the overhead.  And it's only 50
or 100 lines of source code in GNU tar, so it's not a major
maintenance problem either.

http://mail.gnu.org/pipermail/bug-cvs/2002-July/009175.html
suggests that Derek Robert Price was working on a solution along these
lines.  I think this is a good idea.

I see also that Larry Jones objected in
http://mail.gnu.org/pipermail/bug-cvs/2002-July/009176.html
that Price's solution would be "unnecessary overhead in the vast
majority of cases", but if CVS limits the solution to just the cases
described above, there won't be much overhead in practice.
And anyway, it's better for CVS to not lose data, even if there is a
bit more overhead in doing so.

___
Bug-cvs mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/bug-cvs

Re: Bug in patch 2.5.4 or its documentation

[Paul Eggert]

> From: "Derek R. Price" <[EMAIL PROTECTED]>
> Date: Thu, 08 Feb 2001 14:07:03 -0500
> 
> As near as I can read the man page, files should be created by patch
> if the original is listed as /dev/null or is empty and has a
> creation time of the Epoch (1970-01-01 00:00:00 UTC),

Yes, that's correct.

> regardless of
> whether --posix has been specified (it claims --posix will cause
> patch to conform to the POSIX.2 specification).

This part is not correct, as of POSIX 1003.1-2001.  The new POSIX spec
requires that the file must exist for it to be patched.  This is not
very reasonable, but it's what the spec says.

>  [dprice@empress temp]$ echo a line >tmp
>  [dprice@empress temp]$ diff -cN /dev/null tmp >tmp.diff
>  [dprice@empress temp]$ cd test
>  [dprice@empress test]$ patch --posix -p0 <../tmp.diff
>  can't find file to patch at input line 3

This is the correct behavior.  If you remove the "--posix" though,
it should work as you expected.  That was a bug in "patch" that is
(finally!) fixed in the latest test version, which you can get from:

ftp://alpha.gnu.org/gnu/patch/patch-2.5.6.tar.gz

Thanks for your bug report.

___
Bug-cvs mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/bug-cvs

Re: CVS/Diff merge defect. (fwd)

[Paul Eggert]

This message follows up on an old Diff bug report by Kevin Pearson

and proposed patch by Karl Tomlinson
.

> Date: Tue, 15 Aug 2000 16:33:50 +1200
> From: Karl Tomlinson <[EMAIL PROTECTED]>
> 
> I include patches for version 2.7 of diffutils to use OLDFILE as the
> common file when a merge or edscript is requested of diff3.  I think the
> --inhibit-hunk-merge option of 2.7.2 is no longer required.

Thanks for the analysis and patch.  I fixed the problem in a somewhat
different way, and you can see the result in the latest test version
of GNU diffutils, in:

ftp://alpha.gnu.org/gnu/diffutils/diffutils-2.7.10.tar.gz

(The fix has actually been present since diffutils test version 2.7.3
 in November, but I forgot to drop you a line then.)

> I can submit patches for cvs also but I'll wait to see if there is
> feedback on this.

Sorry about the late feedback, and thanks again for the fix.  (Also
thanks to Jim Meyering who kept bugging me to release a fixed version
of diffutils.  :-)

___
Bug-cvs mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/bug-cvs

CVS 1.11.1p1 port to POSIX 1003.1-2001 hosts

[Paul Eggert]

The new POSIX standard is now official (IEEE Std 1003.1-2001), and it
has removed support for some obsolete utility options that CVS uses in
a few places.  Basically, the new POSIX has removed digit-string
options (e.g., "tail -1") and options beginning with "+" (e.g., "sort
+1").  I'm using an experimental environment that insists on the new
standard, so I tend to run into these problems before other people do.

Here is a proposed patch to port to POSIX 1003.1-2001 hosts.

2002-02-24  Paul Eggert  <[EMAIL PROTECTED]>

Port to POSIX 1003.1-2001 hosts.

* doc/cvs.texinfo, src/sanity.sh: Don't use head -1.
* FAQ: Don't use sort +0.1.
* BUGS: Don't use diff -2.
* contrib/cvs2vendor.sh, contrib/rcs2sccs.sh, contrib/sccs2rcs.in:
Don't assume that sort +0 works.

===
RCS file: doc/cvs.texinfo,v
retrieving revision 1.11.1.1
retrieving revision 1.11.1.1.0.1
diff -pu -r1.11.1.1 -r1.11.1.1.0.1
--- doc/cvs.texinfo 2001/04/24 18:14:52 1.11.1.1
+++ doc/cvs.texinfo 2002/02/24 08:14:54 1.11.1.1.0.1
@@ -11959,7 +11959,7 @@ evaluate the log message.
 #  Verify that the log message contains a valid bugid
 #  on the first line.
 #
-if head -1 < $1 | grep '^BugId:[ ]*[0-9][0-9]*$' > /dev/null; then
+if sed q < $1 | grep '^BugId:[ ]*[0-9][0-9]*$' > /dev/null; then
 exit 0
 else
 echo "No BugId found."
@@ -12077,7 +12077,7 @@ edit the log message.
 if [ "x$EDITOR" = "x" ]; then EDITOR=vi; fi
 if [ "x$CVSEDITOR" = "x" ]; then CVSEDITOR=$EDITOR; fi
 $CVSEDITOR $1
-until head -1|grep '^BugId:[ ]*[0-9][0-9]*$' < $1
+until sed q|grep '^BugId:[ ]*[0-9][0-9]*$' < $1
 do  echo -n  "No BugId found.  Edit again? ([y]/n)"
 read ans
 case $@{ans@} in
===
RCS file: FAQ,v
retrieving revision 1.11.1.1
retrieving revision 1.11.1.1.0.1
diff -pu -r1.11.1.1 -r1.11.1.1.0.1
--- FAQ 2000/08/30 01:16:32 1.11.1.1
+++ FAQ 2002/02/24 08:14:54 1.11.1.1.0.1
@@ -4353,7 +4353,7 @@ [EMAIL PROTECTED]

You should be able to run:

-   sort +0.1 ${dir1}/history ${dir2}/history > history
+   sort -k 1.2 ${dir1}/history ${dir2}/history > history

If you "diff" a standard history file before and after such a sort,
you might see other differences caused by garbage (split lines, nulls,
===
RCS file: BUGS,v
retrieving revision 1.11.1.1
retrieving revision 1.11.1.1.0.1
diff -pu -r1.11.1.1 -r1.11.1.1.0.1
--- BUGS1999/05/13 22:49:24 1.11.1.1
+++ BUGS2002/02/24 08:14:54 1.11.1.1.0.1
@@ -101,7 +101,7 @@ file's description.
   Date: Sat, 25 Feb 1995 17:01:15 -0500
   
   mycroft@duality [1]; cd /usr/src/lib/libc
-  mycroft@duality [1]; cvs diff -c2 '-D1 day ago' -Dnow
+  mycroft@duality [1]; cvs diff -C2 '-D1 day ago' -Dnow
   cvs server: Diffing .
   cvs server: Diffing DB
   cvs [server aborted]: could not chdir to DB: No such file or directory
===
RCS file: src/sanity.sh,v
retrieving revision 1.11.1.1
retrieving revision 1.11.1.1.0.1
diff -pu -r1.11.1.1 -r1.11.1.1.0.1
--- src/sanity.sh   2001/04/25 22:30:56 1.11.1.1
+++ src/sanity.sh   2002/02/24 08:14:54 1.11.1.1.0.1
@@ -14210,7 +14210,7 @@ ${PROG} [a-z]*: Rebuilding administrativ
  # Now test verifymsg
  cat >${TESTDIR}/vscript < /dev/null; then
+if sed q < \$1 | grep '^BugId:[ ]*[0-9][0-9]*$' > /dev/null; then
 exit 0
 else
 echo "No BugId found."
@@ -18607,7 +18607,7 @@ done"
  dotest tag8k-16 "$testcvs -Q tag $t-a $file" ''
 
  # Extract the author value.
- name=`sed -n 's/.*;   author \([^;]*\);.*/\1/p' 
${CVSROOT_DIRNAME}/$module/$file,v|head -1`
+ name=`sed -n 's/.*;   author \([^;]*\);.*/\1/p' 
+${CVSROOT_DIRNAME}/$module/$file,v|sed q`
 
  # Form a suffix string of length (16 - length($name)).
  # CAREFUL: this will lose if $name is longer than 16.
===
RCS file: contrib/rcs2sccs.sh,v
retrieving revision 1.11.1.1
retrieving revision 1.11.1.1.0.1
diff -pu -r1.11.1.1 -r1.11.1.1.0.1
--- contrib/rcs2sccs.sh 1997/02/12 15:35:07 1.11.1.1
+++ contrib/rcs2sccs.sh 2002/02/24 08:14:54 1.11.1.1.0.1
@@ -42,13 +42,17 @@ cp $tmpfile $sedfile
 
 # Loop over every RCS file in RCS dir
 #
+if sort -k 1,1 /dev/null 2>/dev/null
+then sort_each_field='-k 1 -k 2 -k 3 -k 4 -k 5 -k 6 -k 7 -k 8 -k 9'
+else sort_each_field='+0

Re: CVS/Diff merge defect. (fwd)

[Paul Eggert]

   Date: Thu, 15 Jun 2000 13:52:29 -0600 (MDT)
   From: Kevin Pearson <[EMAIL PROTECTED]>

   This seemed to be related to the problem that was discovered by Noah
   Friedman <[EMAIL PROTECTED]> and patched by Paul Eggert
   <[EMAIL PROTECTED]> back in November of 1996.  Paul's patch he
   in-effect turned off shift_boundaries (which optimizes the diff by
   merging hunks if possible.)  This fixed the problem described by Noah.

As I mentioned in that message, that patch was a quick hack.  What I
hope is a better fix can be found in the latest test version of diffutils, at:
ftp://alpha.gnu.org/gnu/diffutils/diffutils-2.7.2.tar.gz
It adds a new option inhibit_hunk_merge which turns off hunk merging,
which is causing most of the problem.

Even this better fix is not ideal, because what we want is a true
3-way diff, that can inspect all three input files simultaneously and
minimize the resulting output.  But that's a long story.

   To test diff3, run the the command:
 diff3 -E -am testcase0 testcase0-1.1 testcase0-1.1.2.2
   and
 diff3 -E -am testcase1 testcase1-1.2 testcase0-1.1.2.1

I think you may be onto a bug here (even with diffutils 2.7.2), but I
couldn't reproduce the problem with these test cases.  Your patch to
diffutils 2.7 made no difference in the output of the first command.
For the second command, you didn't supply testcase1-1.2 or
testcase0-1.1.2.1 in your message, so I created them with:

co -p1.2 testcase1 >testcase1-1.2
co -p1.1.2.1 testcase0 >testcase0-1.1.2.1

After doing this, I found no difference in executing the second
command between diffutils 2.7, 2.7 with your fix, and 2.7.2.

The second command seemed a bit odd, as I would have expected the last
file to be testcase1-1.1.2.1 instead of testcase0-1.1.2.1.  So I also
tried running "diff3 -E -am testcase1 testcase1-1.2 testcase1-1.1.2.1"
but also got the same results with all three versions of diff.

So I must be doing something wrong in trying to reproduce the bug.
Can you help me out in trying to reproduce it, with diffutils only?
Thanks.

Re: Wrong ISO 8601 date format in CVS Id, Date and Log identifier

[Paul Eggert]

>  It would seem the use
>  of "/" is a "left-over" from using the American date format, which is
>  MM/DD/YY.

No, it's actually a leftover from RCS, which originally used the date
format YY/MM/DD.  This is not the American date format: it was
designed by W. Tichy (a German :-) in the early 1980s and I believe it
predates ISO 8601.  As RCS maintainer I extended the format to
/MM/DD in the early 1990s.  I stuck with the "/" for backward
compatibility, as some RCS date-parsing software back then didn't
allow "-".

I wouldn't object if the date format were changed to ISO 8601 now,
though of course there should be an option to leave it the way it is.