Re: more gcc warnings

[Paul Eggert]

Eric Blake <[EMAIL PROTECTED]> writes:

> Of course, this is cygwin-specific; it would need accompanying
> autoconf magic and #ifdef'ery to install it only on platforms with
> O_BINARY and where freopen(NULL) doesn't work, without causing
> compile errors on other platforms.

Thanks.  Can you write the Autoconf part?

Also, how is the copyright papers thing going?  This is a nontrivial
change, I'm afraid.


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

Re: more gcc warnings

[Eric Blake]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

According to Paul Eggert on 7/15/2005 4:10 PM:
> 
> For coreutils we don't need to worry about this.  We can assume that
> if freopen (NULL, ...) is being called, then the call is either
> freopen (NULL, "rb", stdin) or freopen (NULL, "wb", stdout).

With that simplification, here goes a version of rpl_freopen that lets CVS
head work on cygwin.  Of course, this is cygwin-specific; it would need
accompanying autoconf magic and #ifdef'ery to install it only on platforms
with O_BINARY and where freopen(NULL) doesn't work, without causing
compile errors on other platforms.

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

/* Like freopen, but when NAME is NULL, ensure that it is only used as
   rpl_freopen (NULL, "rb", stdin) or rpl_freopen (NULL, "wb", stdout),
   to force those streams to binary.  Any other MODE or FILE for a NULL
   NAME closes FILE and fails with EBADF.  */

FILE *
rpl_freopen (const char *name, const char *mode, FILE *file)
{
  int fd;
  if (name)
return freopen (name, mode, file);
  if (! mode)
{
  errno = EINVAL;
  return NULL;
}
  fd = fileno (file);
  if ((fd == STDIN_FILENO && ! strcmp (mode, "rb"))
  || (fd == STDOUT_FILENO && ! strcmp (mode, "wb")))
{
  if (! isatty (fd))
setmode (fd, O_BINARY);
  return file;
}
  fclose (file);
  errno = EBADF;
  return NULL;
}

> 
>>Even if newlib (the provider of freopen() for both cygwin and mingw)
>>updates freopen() to actually implement file access changes where
>>possible, we still need to deal with the fact that a failure will
>>close the underlying file descriptor.
> 
> 
> Is failure still possible under DOS, under the above assumptions?  If
> not, then let's not worry about it.  Otherwise, is it OK if we simply
> ignore the failure, and let later uses of the stream report an error?

I don't know whether DJGPP (the DOS port) uses newlib, or does something
else for freopen, or if it has bit-rotted in enough other places in
coreutils CVS to make it worth worrying about.  If someone is actively
maintaining that port, they will let us know soon enough; otherwise I
don't think it is worth worrying about.  The only problem I see with
ignoring the error is that freopen() is required to close its stream on
error, and that the documentation of fclose() states that operations on a
closed FILE* result in undefined behavior; but you are right that in
general, a closed stream will gracefully cause further errors that will be
detected by the existing code rather than crashing the system.

- --
Life is short - so eat dessert first!

Eric Blake [EMAIL PROTECTED]
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.1 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFC2Q8Q84KuGfSFAYARAo3YAJ95iMsv08tM6w+W+IyeMAwj3/01YACghLwJ
gkS3P7rCQfVpKC+cqHPbW5I=
=H7xC
-END PGP SIGNATURE-


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

Re: more gcc warnings

[Paul Eggert]

Eric Blake <[EMAIL PROTECTED]> writes:

> If all that changed was the addition or subtraction of O_APPEND,

For coreutils we don't need to worry about this.  We can assume that
if freopen (NULL, ...) is being called, then the call is either
freopen (NULL, "rb", stdin) or freopen (NULL, "wb", stdout).

> On cygwin, fcntl(F_SETFL) currently doesn't change binary vs. text,
> so that requires setmode(O_BINARY) if the mode included 'b', and it
> would be easy to add our !isatty filter here.

Good, that makes it sound easy.

> But because we do not know whether mode "w" would have opened the
> current file in binary or text mode,

Surely we don't need to worry about that.  We can simply invoke
setmode to change the mode to O_BINARY if the file is not a terminal.

> if (O_BINARY)
>   {
> FILE *tmp = freopen (NULL, "rb", stdin);
> if (tmp) stdin = tmp;
>   }

No, that won't work: portable code can't assign to stdin.  But I don't
think we need to worry about this if we adopt the solution mentioned
above.

> Even if newlib (the provider of freopen() for both cygwin and mingw)
> updates freopen() to actually implement file access changes where
> possible, we still need to deal with the fact that a failure will
> close the underlying file descriptor.

Is failure still possible under DOS, under the above assumptions?  If
not, then let's not worry about it.  Otherwise, is it OK if we simply
ignore the failure, and let later uses of the stream report an error?

> Hopefully we are safe so long as we limit ourselves to just use
> freopen(NULL, "rb", stdin), freopen(NULL, "wb", stdout), and
> freopen(non_null, any_mode, any_file), as is the current case in CVS.

Yes, that's the basic idea.


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

Re: more gcc warnings

[Eric Blake]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

According to Paul Eggert on 7/14/2005 6:03 PM:
> 
> So we don't have to worry about this problem with traditional Unix
> implementations; only with DOS-like ones.
> 
> Is it easy to write an freopen wrapper for cygwin?  And have it do the
> isatty check as well?  That way, we can migrate the "&& ! isatty
> (fileno (stream))" part out of the mailine code.
> 

I tried starting an implementation of rpl_freopen, and it gets complicated
fast (but I think it is doable at least when you are not changing
read/write mode).  If name is not NULL, just return the result of the real
freopen.  From there, use fileno() to convert to an fd (or fail with
EBADF, remembering to call fclose() on all failure paths), and
fcntl(F_GETFL) to see the current mode.  Then you have to parse all 15
valid modes to see which flags to set (or fail with EINVAL).

If all that changed was the addition or subtraction of O_APPEND,
fcntl(F_SETFL) does the trick (while reusing the former values of status
flags such as O_SYNC and O_NONBLOCK).  On cygwin, fcntl(F_SETFL) currently
doesn't change binary vs. text, so that requires setmode(O_BINARY) if the
mode included 'b', and it would be easy to add our !isatty filter here.
But because we do not know whether mode "w" would have opened the current
file in binary or text mode, the best we can do is leave the mode alone
(which means that once rpl_freopen converts a file descriptor to binary,
it cannot restore it back to its default), or else fail with EBADF.  I
don't think we need to support nonstandard modes such as "wt" to force
text mode.

Furthermore, if the requested mode changes the file access, between
O_RDONLY, O_WRONLY, or O_RDWR, then fcntl() is inadequate to perform the
change.  Using fdopen() would handle this, except that it creates a new
FILE* rather than reusing the existing parameter f as freopen requires.
For now, the strictest behavior is to fail with EBADF if the freopen tries
to change the file access mode, which means that freopen(NULL, "rb",
stdin) will fail if stdin was previously opened "r+".  But in reality, it
is probably better that if the new access mode is more restrictive, then
we succeed without changing the mode (for the example of converting stdin
from "r+" to "rb", it means that if we mistakenly call putc() after
rpl_freopen(), it will succeed instead of fail).  Still, we are forced to
return EBADF if the request was to add an access mode.  Also, when mode
includes 'w', we would have to call ftruncate().

Now if we don't mind changing POSIX semantics, by leaving f unchanged (and
open) on failure and returning a different FILE* on success than f, then
it is a simple matter of making rpl_freopen defer to fdopen (fileno (f),
mode).  If the fdopen fails, we just return NULL, and the paradigm becomes:

if (O_BINARY)
  {
FILE *tmp = freopen (NULL, "rb", stdin);
if (tmp) stdin = tmp;
  }

That way, a failure in freopen does not corrupt stdin, which is essential
if the rest of the utility is to stand a chance of proper operation.  With
the current CVS code, the EFAULT failure of cygwin's freopen is being
ignored, but cygwin closed stdin in the attempt, so that all future file
operations fail with EBADF:
$ echo hi | coreutils/src/tr a b
coreutils/src/tr: read error: Bad file descriptor

Hmm, maybe it is simpler, after all, to stick with the nonstandard setmode
(fileno (f), O_BINARY) than to try to get a working freopen, or making
freopen semantics change from the standards to fit our needs and hoping
that we don't ever add a future use that would be broken by our changed
semantics.  Even if newlib (the provider of freopen() for both cygwin and
mingw) updates freopen() to actually implement file access changes where
possible, we still need to deal with the fact that a failure will close
the underlying file descriptor.

Hopefully we are safe so long as we limit ourselves to just use
freopen(NULL, "rb", stdin), freopen(NULL, "wb", stdout), and
freopen(non_null, any_mode, any_file), as is the current case in CVS.

- --
Life is short - so eat dessert first!

Eric Blake [EMAIL PROTECTED]
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.1 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFC17wY84KuGfSFAYARAgqCAJsFM6SAiEbcsbZnp7nWBvTrEBZ6XwCfW2xA
MnFdFx9H6RE4vlx4+w4PWJ4=
=3i7n
-END PGP SIGNATURE-


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

Re: more gcc warnings

[Paul Eggert]

Eric Blake <[EMAIL PROTECTED]> writes:

> As it is, you will need a wrapper for freopen.  At least Solaris 8 and cygwin 
> 1.5.18 choke on the program below,

Solaris 8 won't choke with the way coreutils invokes it, since it
always looks something like this:

  if (O_BINARY && ! isatty (STDIN_FILENO))
freopen (NULL, "rb", stdin);

and O_BINARY is zero in Solaris 8.

So we don't have to worry about this problem with traditional Unix
implementations; only with DOS-like ones.

> has broken coreutils CVS head on cygwin until we have a replacement freopen 
> that can handle a NULL filename when the platform won't.

Is it easy to write an freopen wrapper for cygwin?  And have it do the
isatty check as well?  That way, we can migrate the "&& ! isatty
(fileno (stream))" part out of the mailine code.


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

Re: more gcc warnings

[Eric Blake]

Paul Eggert  CS.UCLA.EDU> writes:
> 
> I dislike all that isatty stuff -- is there some way that we could
> easily remove it from the mainline sources, and put it in config.h or
> somewhere we we don't have to see it?  For example, can we replace this:
> 
>   if (O_BINARY && ! isatty (STDIN_FILENO))
> freopen (NULL, "rb", stdin);
> 
> with this:
> 
>   if (O_BINARY)
> freopen (NULL, "rb", stdin);

As it is, you will need a wrapper for freopen.  At least Solaris 8 and cygwin 
1.5.18 choke on the program below, dying with EFAULT instead of changing the 
mode of stdin as POSIX requires.  While I like your change stylistically, it 
has broken coreutils CVS head on cygwin until we have a replacement freopen 
that can handle a NULL filename when the platform won't.

#include 
#include 
int main(void)
{
   FILE* f = freopen (NULL, "rb", stdin); /* Ensure that stdin is binary */
   printf ("file is %s, errno %d:%s\n", f ? "good" : "null", errno,
   strerror(errno));
   return 0;
}

$ uname -a
CYGWIN_NT-5.0 eblake 1.5.19s(0.132/4/2) 20050705 16:21:45 i686 unknown unknown 
Cygwin
$ ./foo
file is null, errno 14:Bad address

$ uname -a
SunOS perth 5.8 Generic_108528-15 sun4u sparc SUNW,Sun-Blade-100 Solaris
$ ./foo
file is null, errno 14:Bad address

--
Eric Blake





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

Re: text vs. binary [Re: more gcc warnings]

[Bob Proulx]

Paul Eggert wrote:
> [EMAIL PROTECTED] (Eric Blake) writes:
> > nohup - POSIX requires that stdout from utility may to go to nohup.out, so
> > nohup.out should probably be opened in same mode as nohup's stdout (if it
> > exists)
> 
> nohup.out is currently opened in text mode.  Let's just leave that
> alone, unless someone really needs it.  (I doubt whether they will.)

Since nohup.out is expected to be the terminal output of the command
it makes the most sense to me if it is text mode since it is most
likely to always be text.  It seems very unlikely any normal case
would not be text.

Bob


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

Re: text vs. binary [Re: more gcc warnings]

[Paul Eggert]

[EMAIL PROTECTED] (Eric Blake) writes:

> http://lists.gnu.org/archive/html/bug-coreutils/2005-05/msg00136.html

I just looked at that, and don't really follow all the DOS stuff, but
have these comments anyway:

> cat - POSIX requires binary input and output, and this already has -B option 
> to fine-tune mode

I've removed -B.  It didn't really work on DOS, as far as I could see.
And it wasn't portable (it didn't work on GNU/Linux).

> head - POSIX requires text input, but compare to tail -c on binary input

Currently "head" runs in binary mode (which means, in DOS, that it
uses binary mode except if stdin/stdout is a tty).

> nohup - POSIX requires that stdout from utility may to go to nohup.out, so
> nohup.out should probably be opened in same mode as nohup's stdout (if it
> exists)

nohup.out is currently opened in text mode.  Let's just leave that
alone, unless someone really needs it.  (I doubt whether they will.)

> I've thought
> more about fcntl(fd, F_SETFL, flags), where the nice gnulib behavior
> would be:
> neither O_BINARY nor O_TEXT: leave mode unchanged
> O_BINARY: if not tty, force binary mode
> O_TEXT: if not tty, force text mode
> O_BINARY | O_TEXT: if not tty, swap mode

My kneejerk reaction is that sounds pretty confusing.  But perhaps I
don't see the application area.

>> and have a wrapper on MS-DOS that defines freopen to not do anything
>> if the first argument is NULL, the second ends in "b", and the third a
>> standard tty?
>
> Since POSIX states that "w+b" and "wb+" are synonyms, for example,

OK, use 'contains "b"' instead of 'ends in "b"'.

> I'd like to see a deprecation period rather than outright removal,
> where -B is undocumented, and using it evokes a warning but
> is otherwise a no-op (rather than a fatal unrecognized option).

You could talk me into that, yes.

> Are there non-standard hosts out there that don't treat "ab" as
> a synonym to "a" per POSIX

Yes.  At least, such hosts used to exist.  Maybe we needn't worry
about them now.  I suppose I can rip out the (O_BINARY ? "rb" : r")
stuff.  (Though this does save a byte on POSIX hosts.  :-)


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

text vs. binary [Re: more gcc warnings]

[Eric Blake]

> I've been putting off cleaning up this mess, but I found the time this
> weekend to give it a start.  I installed the following patch.  It
> avoids the use of setmode and  entirely, and it cleans up some
> of the inconsistencies in the code (in some cases, they were bugs that
> even infected the GNU/Linux version -- ouch!).

Thanks for starting this task.

> 
> Since I don't use DOS, someone with some expertise in that area will
> have to double-check it.

I'm willing to check it on cygwin.  This is a partial review, I will probably
have more comments as I get more time to actually comple and test
some of the changes.  It would also be nice if someone could give anon
CVS a kick; it has been stuck since the 5th.

I would also like to compare your work to my email on the subject a
couple of months ago:
http://lists.gnu.org/archive/html/bug-coreutils/2005-05/msg00136.html

> 
> I dislike all that isatty stuff -- is there some way that we could
> easily remove it from the mainline sources, and put it in config.h or
> somewhere we we don't have to see it?  For example, can we replace this:
> 
>   if (O_BINARY && ! isatty (STDIN_FILENO))
> freopen (NULL, "rb", stdin);
> 
> with this:
> 
>   if (O_BINARY)
> freopen (NULL, "rb", stdin);

Sounds like a good candidate for a gnulib module.  Also, I've thought
more about fcntl(fd, F_SETFL, flags), where the nice gnulib behavior
would be:
neither O_BINARY nor O_TEXT: leave mode unchanged
O_BINARY: if not tty, force binary mode
O_TEXT: if not tty, force text mode
O_BINARY | O_TEXT: if not tty, swap mode

> 
> and have a wrapper on MS-DOS that defines freopen to not do anything
> if the first argument is NULL, the second ends in "b", and the third a
> standard tty?

Since POSIX states that "w+b" and "wb+" are synonyms, for example,
we should be a little more careful than just checking for ends in 'b'.  Or
is there a coding style that states which of the two we should prefer?

> Index: src/cat.c
> @@ -553,9 +543,6 @@ main (int argc, char **argv)
>  {"show-ends", no_argument, NULL, 'E'},
>  {"show-tabs", no_argument, NULL, 'T'},
>  {"show-all", no_argument, NULL, 'A'},
> -#if O_BINARY
> -{"binary", no_argument, NULL, 'B'},
> -#endif

I'd like to see a deprecation period rather than outright removal,
where -B is undocumented, and using it evokes a warning but
is otherwise a no-op (rather than a fatal unrecognized option).

> Index: src/tac.c
> Index: src/tee.c
> @@ -140,7 +140,10 @@ tee (int nfiles, const char **files)
>ssize_t bytes_read;
>int i;
>bool ok = true;
> -  const char *mode_string = (append ? "a" : "w");
> +  char const *mode_string =
> +(O_BINARY
> + ? (append ? "ab" : "wb")
> + : (append ? "a" : "w"));

Are there non-standard hosts out there that don't treat "ab" as
a synonym to "a" per POSIX, and if so, should we cater to them
with a gnulib module?  It is much more readable to write this as:

char const *mode_string = (append ? "ab" : "wb");

--
Eric Blake




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

Re: more gcc warnings

[Paul Eggert]

[EMAIL PROTECTED] (Eric Blake) writes:

> cygwin's headers are poorly documented; I often resort to grepping
> /usr/include.  setmode() is in , along with get_osfhandle and
> a redundant declaration of access().

I've been putting off cleaning up this mess, but I found the time this
weekend to give it a start.  I installed the following patch.  It
avoids the use of setmode and  entirely, and it cleans up some
of the inconsistencies in the code (in some cases, they were bugs that
even infected the GNU/Linux version -- ouch!).

Since I don't use DOS, someone with some expertise in that area will
have to double-check it.

I dislike all that isatty stuff -- is there some way that we could
easily remove it from the mainline sources, and put it in config.h or
somewhere we we don't have to see it?  For example, can we replace this:

  if (O_BINARY && ! isatty (STDIN_FILENO))
freopen (NULL, "rb", stdin);

with this:

  if (O_BINARY)
freopen (NULL, "rb", stdin);

and have a wrapper on MS-DOS that defines freopen to not do anything
if the first argument is NULL, the second ends in "b", and the third a
standard tty?


2005-07-11  Paul Eggert  <[EMAIL PROTECTED]>

* NEWS: Binary input and output are now implemented more consistently.
These changes affect only platforms like MS-DOS that distinguish
between binary and text files.
* doc/coreutils.texi (cat invocation): Remove -B or --binary option
(available on MS-DOS-like platforms only).  Explain when text and
binary mode are used now.
(md5sum invocation): -b actually does have an effect on Unix: it
causes "*" to be output.  Explain when text and binary mode are
used now.
* src/cat.c (usage, main, long_options) [O_BINARY]:
Remove support for -B.  Use same rules as other programs to decide
whether to use binary I/O, except that the -bensAE options always
select text mode.
* src/cat.c (main): Avoid setmode; use POSIX-specified routines instead.
* src/cksum.c (cksum): Likewise.
* src/head.c (head_lines, head_file): Likewise.
* src/od.c (open_next_file): Likewise.
* src/split.c (main): Likewise.
* src/sum.c (bsd_sum_file, sysv_sym_file): Likewise.
* src/tac.c (copy_to_temp, tac_file, main): Likewise.
* src/tail.c (tail_bytes, tail_lines, tail_file, main): Likewise.
* src/tee.c (tee): Likewise.
* src/tr.c (main): Likewise.
* src/wc.c (wc): Likewise.
* src/copy.c (copy_reg): Always copy in binary mode.
* src/expand.c (expand): Always copy in text mode.  POSIX says
the input and output must be text.
* src/unexpand.c (unexpand): Likewise.
* src/head.c (elide_tail_bytes_file, elide_tail_lines_file, head_bytes):
(head_lines, head_file): Always use binary mode except for std tty.
* src/md5sum.c (usage): Clarify whether text or binary is the default.
(split_3, main): BINARY is now a 3-way value.  All uses changed.
(digest_file): Likewise.  Clear *BINARY if we determine the file
to be text.  All uses changed.
(main): Don't report a file to be binary if we actually read it
as text in MS-DOS, because it was a terminal.
* src/shred.c (wipefile): Always use binary mode.  Clearly this
never worked right on DOS!
* src/system.h (setmode, fileno): Remove; no longer needed, we think.
(SET_MODE, SET_BINARY, SET_BINARY2): Remove.
[defined __DJGPP__]: Don't include  or .
* src/wc.c (wc_file): FILE might be null now.
(main): Simplify code a bit, so that fewer places need the
setmode fixes.

Index: NEWS
===
RCS file: /fetish/cu/NEWS,v
retrieving revision 1.300
diff -p -u -r1.300 NEWS
--- NEWS3 Jul 2005 16:52:09 -   1.300
+++ NEWS11 Jul 2005 18:08:34 -
@@ -48,6 +48,31 @@ GNU coreutils NEWS  
   "Utility Syntax Guidelines" in the Minutes of the January 2005
   Meeting .
 
+** Binary input and output are now implemented more consistently.
+  These changes affect only platforms like MS-DOS that distinguish
+  between binary and text files.
+
+  The following programs now always use text input/output:
+
+expand unexpand
+
+  The following programs now always use binary input/output to copy data:
+
+cp install mv shred
+
+  The following programs now always use binary input/output to copy
+  data, except for stdin and stdout when it is a terminal.
+
+head tac tail tee tr
+(cat behaves similarly, unless one of the options -bensAE is used.)
+
+  cat's --binary or -B option has been removed.  It existed only on
+  MS-DOS-like platforms, and didn't work as documented there.
+
+  md5sum and sha1sum now obey the -b or --binary option, even if
+  standard input is a terminal, 

Re: more gcc warnings

[Eric Blake]

> > One more that I hadn't paid attention to - for systems with O_BINARY, the
> > macro SET_BINARY was using setmode() without a prototype,
> 
> Which include file declares setmode()?  Where is this documented?
> I looked in the cygwin web site without much luck.

cygwin's headers are poorly documented; I often resort to grepping
/usr/include.  setmode() is in , along with get_osfhandle and
a redundant declaration of access().

> 
> > Should  be included in "system.h"
> 
> I'd rather not.  In fact, I'd rather system.h included less than it
> already does.
> 
> > should we redo this patch to just fix cksum.c to include 
> 
> Sorry, I don't follow.  cksum.c already includes .

Oh, it was because of the blind #define fileno _fileno, that I was getting
the warning for an implicit definition.  Cygwin properly declares fileno()
in , but does not declare _fileno.

> 
> > -# ifndef __DJGPP__
> > +# if defined __CYGWIN__
> > +#  include 
> > +#  include 
> > +# elif !defined __DJGPP__
> 
> What is ?  Why does both it and  need to be included?

 is not needed, but  is needed for setmode().

--
Eric Blake




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

Re: more gcc warnings

[Paul Eggert]

Eric Blake <[EMAIL PROTECTED]> writes:

> One more that I hadn't paid attention to - for systems with O_BINARY, the
> macro SET_BINARY was using setmode() without a prototype,

Which include file declares setmode()?  Where is this documented?
I looked in the cygwin web site without much luck.

> Should  be included in "system.h"

I'd rather not.  In fact, I'd rather system.h included less than it
already does.

> should we redo this patch to just fix cksum.c to include 

Sorry, I don't follow.  cksum.c already includes .

> On an unrelated question, I noticed that some files have copyright lines
> that don't match GNU coding standards.

We should fix that, at least for files that we maintain ourselves.

> -# ifndef __DJGPP__
> +# if defined __CYGWIN__
> +#  include 
> +#  include 
> +# elif !defined __DJGPP__

What is ?  Why does both it and  need to be included?

While looking at this I did notice that a lot of files include 
that don't need to.  I installed this:

2005-07-09  Paul Eggert  <[EMAIL PROTECTED]>

* src/comm.c, src/csplit.c, src/dd.c, src/join.c, src/md5sum.c:
* src/pr.c, src/sort.c, src/tee.c:
Don't include stdio.h; no longer needed.

Index: src/comm.c
===
RCS file: /fetish/cu/src/comm.c,v
retrieving revision 1.84
diff -p -u -r1.84 comm.c
--- src/comm.c  3 Jul 2005 07:16:23 -   1.84
+++ src/comm.c  9 Jul 2005 21:56:50 -
@@ -19,7 +19,6 @@
 
 #include 
 
-#include 
 #include 
 #include 
 #include "system.h"
Index: src/csplit.c
===
RCS file: /fetish/cu/src/csplit.c,v
retrieving revision 1.141
diff -p -u -r1.141 csplit.c
--- src/csplit.c3 Jul 2005 07:17:12 -   1.141
+++ src/csplit.c9 Jul 2005 21:56:51 -
@@ -20,7 +20,6 @@
 
 #include 
 
-#include 
 #include 
 #include 
 #include 
Index: src/dd.c
===
RCS file: /fetish/cu/src/dd.c,v
retrieving revision 1.184
diff -p -u -r1.184 dd.c
--- src/dd.c3 Jul 2005 07:17:36 -   1.184
+++ src/dd.c9 Jul 2005 21:56:51 -
@@ -18,7 +18,6 @@
 /* Written by Paul Rubin, David MacKenzie, and Stuart Kemp. */
 
 #include 
-#include 
 
 #define SWAB_ALIGN_OFFSET 2
 
Index: src/join.c
===
RCS file: /fetish/cu/src/join.c,v
retrieving revision 1.141
diff -p -u -r1.141 join.c
--- src/join.c  3 Jul 2005 07:17:58 -   1.141
+++ src/join.c  9 Jul 2005 21:56:51 -
@@ -19,7 +19,6 @@
 
 #include 
 
-#include 
 #include 
 #include 
 #include 
Index: src/md5sum.c
===
RCS file: /fetish/cu/src/md5sum.c,v
retrieving revision 1.136
diff -p -u -r1.136 md5sum.c
--- src/md5sum.c3 Jul 2005 07:18:21 -   1.136
+++ src/md5sum.c9 Jul 2005 21:56:51 -
@@ -20,7 +20,6 @@
 #include 
 
 #include 
-#include 
 #include 
 
 #include "system.h"
Index: src/pr.c
===
RCS file: /fetish/cu/src/pr.c,v
retrieving revision 1.152
diff -p -u -r1.152 pr.c
--- src/pr.c3 Jul 2005 07:19:06 -   1.152
+++ src/pr.c9 Jul 2005 21:56:51 -
@@ -311,7 +311,6 @@
 
 #include 
 
-#include 
 #include 
 #include 
 #include "system.h"
Index: src/sort.c
===
RCS file: /fetish/cu/src/sort.c,v
retrieving revision 1.319
diff -p -u -r1.319 sort.c
--- src/sort.c  3 Jul 2005 07:20:04 -   1.319
+++ src/sort.c  9 Jul 2005 21:56:52 -
@@ -26,7 +26,6 @@
 #include 
 #include 
 #include 
-#include 
 #include "system.h"
 #include "error.h"
 #include "hard-locale.h"
Index: src/tee.c
===
RCS file: /fetish/cu/src/tee.c,v
retrieving revision 1.81
diff -p -u -r1.81 tee.c
--- src/tee.c   3 Jul 2005 07:22:50 -   1.81
+++ src/tee.c   9 Jul 2005 21:56:52 -
@@ -18,7 +18,6 @@
 /* Mike Parker, Richard M. Stallman, and David MacKenzie */
 
 #include 
-#include 
 #include 
 #include 
 #include 


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

Re: more gcc warnings

[Eric Blake]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

According to Paul Eggert on 7/9/2005 1:40 AM:
> 
>>Does the following patch fix your problems with Cygwin?

It hasn't hit anonymous CVS yet, but manual application of the patch from
your email silenced gcc -Wall, so the problem has been addressed.  Thanks
for your help!

> Clearly the stropt.h -> stropts.h fix was needed, so I installed that
> patch.  If you still have problems with Cygwin please let us know.
> 

One more that I hadn't paid attention to - for systems with O_BINARY, the
macro SET_BINARY was using setmode() without a prototype, and cksum.c
calls SET_BINARY while using fileno() without a prototype.  This fixes it
for cygwin, but leaves other O_BINARY hosts (such as mingw) as is unless a
maintainer for those ports chimes in.  Should  be included in
"system.h", adding bulk to all the *.c files even though it only helps
one, or should we redo this patch to just fix cksum.c to include 
(perhaps conditioned on O_BINARY which implies the use of fileno)?  There
are a couple of other programs (such as expand.c) that only call fileno
inside the SET_BINARY macro, but at least they already include .

On an unrelated question, I noticed that some files have copyright lines
that don't match GNU coding standards.  For example, ls.c has

Copyright (C) 85, 88, 90, 91, 1995-2005 Free Software Foundation, Inc.

http://www.gnu.org/prep/maintain/maintain.html#Copyright-Notices states
"Do not abbreviate the year list using a range; for instance, do not write
`1996--1998'; instead, write `1996, 1997, 1998'. Do write each relevant
year as a four-digit number. In the normal course of maintenance, you may
come across copyright notices which omit the century, as in `1996, 97,
98'?change these to include the century. However, there is no need to
systematically change the notice in every old file."

2005-07-09  Eric Blake  <[EMAIL PROTECTED]>

* src/system.h [O_BINARY && __CYGWIN__]: Include actual headers
for setmode and fileno.

Index: src/system.h
===
RCS file: /cvsroot/coreutils/coreutils/src/system.h,v
retrieving revision 1.132
diff -u -p -r1.132 system.h
- --- src/system.h6 Jul 2005 09:34:09 -   1.132
+++ src/system.h9 Jul 2005 12:34:13 -
@@ -217,7 +217,10 @@ initialize_exit_failure (int status)
 #endif

 #if O_BINARY
- -# ifndef __DJGPP__
+# if defined __CYGWIN__
+#  include 
+#  include 
+# elif !defined __DJGPP__
 #  define setmode _setmode
 #  define fileno(_fp) _fileno (_fp)
 # endif /* not DJGPP */

- --
Life is short - so eat dessert first!

Eric Blake [EMAIL PROTECTED]
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.0 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFCz8lM84KuGfSFAYARAvvlAJ9RX/Ndg1elQiDsoXfD7yiCI+alwgCdFN8Y
FoGlwtOEx8VxFaRsCUeCux4=
=UUqW
-END PGP SIGNATURE-


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

Re: more gcc warnings

[Paul Eggert]

Paul Eggert <[EMAIL PROTECTED]> writes:

> Does the following patch fix your problems with Cygwin?

Clearly the stropt.h -> stropts.h fix was needed, so I installed that
patch.  If you still have problems with Cygwin please let us know.


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

Re: more gcc warnings

[Paul Eggert]

Does the following patch fix your problems with Cygwin?  (I haven't
had time to write a changelog entry for it.)  Most likely it won't
work on some older hosts, but we can cross that bridge if we come to
it.


Index: configure.ac
===
RCS file: /fetish/cu/configure.ac,v
retrieving revision 1.58
diff -p -u -r1.58 configure.ac
--- configure.ac8 Jul 2005 22:53:50 -   1.58
+++ configure.ac9 Jul 2005 00:09:22 -
@@ -50,7 +50,6 @@ AC_CHECK_FUNCS(gethostid,
 
 gl_MACROS
 
-AC_HEADER_TIOCGWINSZ()
 gl_WINSIZE_IN_PTEM
 
 AC_MSG_CHECKING(whether localtime caches TZ)
Index: m4/jm-macros.m4
===
RCS file: /fetish/cu/m4/jm-macros.m4,v
retrieving revision 1.225
diff -p -u -r1.225 jm-macros.m4
--- m4/jm-macros.m4 8 Jul 2005 22:56:17 -   1.225
+++ m4/jm-macros.m4 9 Jul 2005 00:09:22 -
@@ -196,7 +196,7 @@ AC_DEFUN([gl_CHECK_ALL_HEADERS],
 stdlib.h \
 stdint.h \
 string.h \
-stropt.h \
+stropts.h \
 sys/filsys.h \
 sys/fs/s5param.h \
 sys/fs_types.h \
Index: src/cat.c
===
RCS file: /fetish/cu/src/cat.c,v
retrieving revision 1.103
diff -p -u -r1.103 cat.c
--- src/cat.c   8 Jul 2005 22:54:15 -   1.103
+++ src/cat.c   9 Jul 2005 00:09:23 -
@@ -27,12 +27,14 @@
 #include 
 #include 
 #include 
-#if HAVE_STROPT_H
-# include 
+
+#if HAVE_STROPTS_H
+# include 
 #endif
-#if HAVE_FIONREAD_IN_SYS_IOCTL
+#if HAVE_SYS_IOCTL_H
 # include 
 #endif
+
 #include "system.h"
 #include "error.h"
 #include "full-write.h"
Index: src/ls.c
===
RCS file: /fetish/cu/src/ls.c,v
retrieving revision 1.393
diff -p -u -r1.393 ls.c
--- src/ls.c8 Jul 2005 22:54:40 -   1.393
+++ src/ls.c9 Jul 2005 00:09:23 -
@@ -42,8 +42,10 @@
 #if HAVE_TERMIOS_H
 # include 
 #endif
-
-#ifdef GWINSZ_IN_SYS_IOCTL
+#if HAVE_STROPTS_H
+# include 
+#endif
+#if HAVE_SYS_IOCTL_H
 # include 
 #endif
 
@@ -60,10 +62,6 @@
 #include 
 #include 
 
-#if HAVE_STROPT_H
-# include 
-#endif
-
 /* Use SA_NOCLDSTOP as a proxy for whether the sigaction machinery is
present.  */
 #ifndef SA_NOCLDSTOP
Index: src/stty.c
===
RCS file: /fetish/cu/src/stty.c,v
retrieving revision 1.137
diff -p -u -r1.137 stty.c
--- src/stty.c  8 Jul 2005 22:54:40 -   1.137
+++ src/stty.c  9 Jul 2005 00:09:23 -
@@ -36,28 +36,28 @@
 
 #include 
 #include 
+
 #if HAVE_TERMIOS_H
 # include 
 #endif
-#ifdef GWINSZ_IN_SYS_IOCTL
+#if HAVE_STROPTS_H
+# include 
+#endif
+#ifdef HAVE_SYS_IOCTL_H
 # include 
 #endif
+
 #ifdef WINSIZE_IN_PTEM
 # include 
 # include 
 #endif
 #ifdef GWINSZ_IN_SYS_PTY
-# include 
 # include 
 # include 
 #endif
 #include 
 #include 
 
-#if HAVE_STROPT_H
-# include 
-#endif
-
 #include "system.h"
 #include "error.h"
 #include "fd-reopen.h"



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

Re: more gcc warnings

[Paul Eggert]

[EMAIL PROTECTED] (Eric Blake) writes:

> ls.c and stty.c use ioctl without including ,
> triggering a warning about implicit declarations.  Even worse, since
> ioctl is a varargs function, this is undefined C (luckily, it
> compiles and links okay on cygwin).

Thanks for reporting this.  POSIX says that  declares ioctl,
so let's try including that instead.

> id.c calls error (which ultimately gets to the printf family) with a format
> %u for uid_t and gid_t.

%lu should be good enough; id.c uses that elsewhere.  If we run into
hosts where uid_t and gid_t are wider than long int, then we can worry
about it later.  (Such hosts would not conform to pre-2001 POSIX.)

I installed this patch:

2005-07-08  Paul Eggert  <[EMAIL PROTECTED]>
 
Fix porting problems reported by Eric Blake.

* configure.ac: Add check for HAVE_FIONREAD_IN_SYS_IOCTL.
* m4/jm-macros.m4 (gl_CHECK_ALL_HEADERS): Check for stropt.h.
* src/cat.c, src/ls.c, src/stty.c: Include stropt.h if available,
because POSIX says that's where ioctl is declared.
* src/cat.c: Use HAVE_FIONREAD_IN_SYS_IOCTL instead of _POSIX_SOURCE
to decide whether to include .

* src/id.c (print_user): Don't assume uid fits in unsigned int.
(print_group): Likewise, for gid.

Index: configure.ac
===
RCS file: /fetish/cu/configure.ac,v
retrieving revision 1.57
diff -p -u -r1.57 configure.ac
--- configure.ac18 May 2005 19:27:39 -  1.57
+++ configure.ac8 Jul 2005 22:49:47 -
@@ -244,6 +244,12 @@ if test $jm_cv_sys_tiocgwinsz_needs_term
[Define if your system defines TIOCGWINSZ in sys/pty.h.])
 fi
 
+# For src/cat.c.
+AC_CHECK_DECL([FIONREAD],
+  [AC_DEFINE([HAVE_FIONREAD_IN_SYS_IOCTL], 1,
+ [Define to 1 if  defines FIONREAD.])],
+  [], [#include ])
+
 # For src/kill.c.
 AC_CHECK_DECLS([strsignal, sys_siglist, _sys_siglist, __sys_siglist], , ,
   [AC_INCLUDES_DEFAULT
Index: m4/jm-macros.m4
===
RCS file: /fetish/cu/m4/jm-macros.m4,v
retrieving revision 1.224
diff -p -u -r1.224 jm-macros.m4
--- m4/jm-macros.m4 3 Jul 2005 09:30:00 -   1.224
+++ m4/jm-macros.m4 8 Jul 2005 22:49:47 -
@@ -196,6 +196,7 @@ AC_DEFUN([gl_CHECK_ALL_HEADERS],
 stdlib.h \
 stdint.h \
 string.h \
+stropt.h \
 sys/filsys.h \
 sys/fs/s5param.h \
 sys/fs_types.h \
Index: src/cat.c
===
RCS file: /fetish/cu/src/cat.c,v
retrieving revision 1.102
diff -p -u -r1.102 cat.c
--- src/cat.c   16 Jun 2005 21:40:43 -  1.102
+++ src/cat.c   8 Jul 2005 22:49:48 -
@@ -27,7 +27,10 @@
 #include 
 #include 
 #include 
-#ifndef _POSIX_SOURCE
+#if HAVE_STROPT_H
+# include 
+#endif
+#if HAVE_FIONREAD_IN_SYS_IOCTL
 # include 
 #endif
 #include "system.h"
Index: src/id.c
===
RCS file: /fetish/cu/src/id.c,v
retrieving revision 1.85
diff -p -u -r1.85 id.c
--- src/id.c30 May 2005 07:33:00 -  1.85
+++ src/id.c8 Jul 2005 22:49:48 -
@@ -202,7 +202,8 @@ print_user (uid_t uid)
   pwd = getpwuid (uid);
   if (pwd == NULL)
{
- error (0, 0, _("cannot find name for user ID %u"), uid);
+ error (0, 0, _("cannot find name for user ID %lu"),
+(unsigned long int) uid);
  ok = false;
}
 }
@@ -225,7 +226,8 @@ print_group (gid_t gid)
   grp = getgrgid (gid);
   if (grp == NULL)
{
- error (0, 0, _("cannot find name for group ID %u"), gid);
+ error (0, 0, _("cannot find name for group ID %lu"),
+(unsigned long int) gid);
  ok = false;
}
 }
Index: src/ls.c
===
RCS file: /fetish/cu/src/ls.c,v
retrieving revision 1.392
diff -p -u -r1.392 ls.c
--- src/ls.c3 Jul 2005 09:31:19 -   1.392
+++ src/ls.c8 Jul 2005 22:49:48 -
@@ -60,6 +60,10 @@
 #include 
 #include 
 
+#if HAVE_STROPT_H
+# include 
+#endif
+
 /* Use SA_NOCLDSTOP as a proxy for whether the sigaction machinery is
present.  */
 #ifndef SA_NOCLDSTOP
Index: src/stty.c
===
RCS file: /fetish/cu/src/stty.c,v
retrieving revision 1.136
diff -p -u -r1.136 stty.c
--- src/stty.c  3 Jul 2005 07:21:03 -   1.136
+++ src/stty.c  8 Jul 2005 22:49:48 -
@@ -54,6 +54,10 @@
 #include 
 #include 
 
+#if HAVE_STROPT_H
+# include 
+#endif
+
 #include "system.h"
 #include "error.h"
 #include "fd-reopen.h"


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

Re: more gcc warnings

[Eric Blake]

> [EMAIL PROTECTED] (Eric Blake) writes:
> 
> > ls.c and stty.c use ioctl without including ,
> > triggering a warning about implicit declarations.  Even worse, since
> > ioctl is a varargs function, this is undefined C (luckily, it
> > compiles and links okay on cygwin).
> 
> Thanks for reporting this.  POSIX says that  declares ioctl,
> so let's try including that instead.

POSIX spells it  (not ), and only on platforms that
support STREAMS.  Cygwin is not one of them, and has no ,
so your patch will not help cygwin.  Furthermore, cygwin's 
is pretty wimpy - it declares JUST ioctl and 3 #defines WINDOWS_*.  So
this part of the patch needs a total rework to fix your misspelling and to
still find a solution that supports cygwin.

> 
> > id.c calls error (which ultimately gets to the printf family) with a format
> > %u for uid_t and gid_t.
> 
> %lu should be good enough; id.c uses that elsewhere.  If we run into
> hosts where uid_t and gid_t are wider than long int, then we can worry
> about it later.  (Such hosts would not conform to pre-2001 POSIX.)

This part looked fine.

--
Eric Blake




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