Re: -Wvla vs dcnpgettext_expr's VLA decl

[Simon Josefsson]

Jim Meyering  writes:

> I can live with the status quo, but it would make -Wvla more useful
> not to put such constructs in commonly-included header files.

FWIW, I find these definitions from gettext.h are rarely needed, so
unless you really need them, considering doing something like this:

http://git.savannah.gnu.org/cgit/libidn.git/tree/gl/override/lib/gettext.h.diff

/Simon

Re: manywarnings and -f options

[Simon Josefsson]

Jim Meyering  writes:

> Eric Blake wrote:
>> On 12/03/2011 09:00 AM, Simon Josefsson wrote:
>>> What does -funit-at-a-time really do?  My gcc 4.4 manual says:
>>>
>>> `-funit-at-a-time'
>>>  This option is left for compatibility reasons. `-funit-at-a-time'
>>>  has no effect, while `-fno-unit-at-a-time' implies
>>>  `-fno-toplevel-reorder' and `-fno-section-anchors'.
>>>
>>>  Enabled by default.
>>
>> That's the case for 4.4 and later.  But in gcc 4.3, it was not
>> unconditionally enabled, and as I said earlier, at least coreutils ran
>> into a situation where gcc 4.3. failed to compile at -Werror because
>> -Wdisabled-optimization warned that -fno-unit-at-a-time was required,
>> which warning turned into an error.
>>
>> At this point, gcc 4.3 is slowly phasing out; most Linux distros and
>> Cygwin have moved on to newer compilers, where the problem is less
>> likely to happen.
>
> IMHO, we should treat --enable-gcc-warnings as something that must work
> well with the latest stable version of gcc (currently 4.6) and recent
> glibc headers.  Trying to accommodate older versions of gcc does not seem
> worthwhile.  Just tell people who use old versions of gcc not to use
> --enable-gcc-warnings, or even detect that and turn it off automatically.

I think this is a good approach: I wouldn't want workarounds for issues
in old gcc in manywarnings.m4.  Manywarnings is a maintainer tool, and
maintainers can be required to have newer tools than users, so
manywarnings could require more recent tools.  However, personally I
still use gcc 4.4 on my primary development machine, so if it isn't
difficult to support it, I'd prefer that.

/Simon

Re: manywarnings and -f options

[Simon Josefsson]

Eric Blake  writes:

> On 12/03/2011 09:00 AM, Simon Josefsson wrote:
>> What does -funit-at-a-time really do?  My gcc 4.4 manual says:
>> 
>> `-funit-at-a-time'
>>  This option is left for compatibility reasons. `-funit-at-a-time'
>>  has no effect, while `-fno-unit-at-a-time' implies
>>  `-fno-toplevel-reorder' and `-fno-section-anchors'.
>> 
>>  Enabled by default.
>
> That's the case for 4.4 and later.  But in gcc 4.3, it was not
> unconditionally enabled, and as I said earlier, at least coreutils ran
> into a situation where gcc 4.3. failed to compile at -Werror because
> -Wdisabled-optimization warned that -fno-unit-at-a-time was required,
> which warning turned into an error.
>
> At this point, gcc 4.3 is slowly phasing out; most Linux distros and
> Cygwin have moved on to newer compilers, where the problem is less
> likely to happen.

Agreed -- so couldn't the answer to this situation be "don't use
--enable-gcc-warnings"?  I realize we may run into a similar situation
in the future though, and I don't have a good idea on how to resolve
that.  We could deal with it on a case-by-case basis.

>> The manual for -ipa-pure-const says:
>> 
>> `-fipa-pure-const'
>>  Discover which functions are pure or constant.  Enabled by default
>>  at `-O' and higher.
>> 
>> That seems harmless and as far I understand would not modify code
>> generation.
>
> It doesn't modify code generation, but it DOES modify warning
> generation, and in a way that negatively interacts with libtool:
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=10197

I see this problem from time to time as well, but I believe it is really
a libtool problem that it hides messages, and not a problem with the
triggers of that problem.  It is especially problematic when building
code that has a lot of '#if PIC' or '#if !PIC' in it.

/Simon

Re: manywarnings and -f options

[Jim Meyering]

Simon Josefsson wrote:
> Jim Meyering  writes:
>> Eric Blake wrote:
>>> On 12/03/2011 09:00 AM, Simon Josefsson wrote:
 What does -funit-at-a-time really do?  My gcc 4.4 manual says:

 `-funit-at-a-time'
  This option is left for compatibility reasons. `-funit-at-a-time'
  has no effect, while `-fno-unit-at-a-time' implies
  `-fno-toplevel-reorder' and `-fno-section-anchors'.

  Enabled by default.
>>>
>>> That's the case for 4.4 and later.  But in gcc 4.3, it was not
>>> unconditionally enabled, and as I said earlier, at least coreutils ran
>>> into a situation where gcc 4.3. failed to compile at -Werror because
>>> -Wdisabled-optimization warned that -fno-unit-at-a-time was required,
>>> which warning turned into an error.
>>>
>>> At this point, gcc 4.3 is slowly phasing out; most Linux distros and
>>> Cygwin have moved on to newer compilers, where the problem is less
>>> likely to happen.
>>
>> IMHO, we should treat --enable-gcc-warnings as something that must work
>> well with the latest stable version of gcc (currently 4.6) and recent
>> glibc headers.  Trying to accommodate older versions of gcc does not seem
>> worthwhile.  Just tell people who use old versions of gcc not to use
>> --enable-gcc-warnings, or even detect that and turn it off automatically.
>
> I think this is a good approach: I wouldn't want workarounds for issues
> in old gcc in manywarnings.m4.  Manywarnings is a maintainer tool, and
> maintainers can be required to have newer tools than users, so
> manywarnings could require more recent tools.  However, personally I
> still use gcc 4.4 on my primary development machine, so if it isn't
> difficult to support it, I'd prefer that.

That is reasonable, since you'll be motivated to address any problem
that is specific to your aging, er... "stable" environment ;-)

Besides, if it just-works even with gcc-4.4, we'll avoid at least
a few bug reports.

Re: -Wvla vs dcnpgettext_expr's VLA decl

[Jim Meyering]

Simon Josefsson wrote:

> Jim Meyering  writes:
>
>> I can live with the status quo, but it would make -Wvla more useful
>> not to put such constructs in commonly-included header files.
>
> FWIW, I find these definitions from gettext.h are rarely needed, so
> unless you really need them, considering doing something like this:
>
> http://git.savannah.gnu.org/cgit/libidn.git/tree/gl/override/lib/gettext.h.diff

Good idea.  Thanks.
That works for coreutils and lets me reenable -Wvla.

mktemp: not found

[Bruno Haible]

Hi Jim,

On Minix 3.1.8, I get this spurious output from the test-verify.sh test:

  mktemp: not found
  PASS: test-verify.sh

This patch fixes it. OK to apply?


2011-12-04  Bruno Haible  

tests: Avoid spurious error message on platforms without mktemp program.
* tests/init.sh (mktempd_): Run mktemp in a subshell.

--- tests/init.sh.orig  Sun Dec  4 14:55:00 2011
+++ tests/init.sh   Sun Dec  4 14:54:51 2011
@@ -521,7 +521,7 @@
   esac
 
   # First, try to use mktemp.
-  d=`unset TMPDIR; mktemp -d -t -p "$destdir_" "$template_" 2>/dev/null` \
+  d=`unset TMPDIR; (mktemp -d -t -p "$destdir_" "$template_") 2>/dev/null` \
 || fail=1
 
   # The resulting name must be in the specified directory.

-- 
In memoriam Fred Hampton 

Re: -Wvla vs dcnpgettext_expr's VLA decl

[Bruno Haible]

Jim Meyering wrote:
> >> Do we have a guarantee that that array dimension is reasonable?
> >
> > Yes. While msgctxt and msgid normally rarely exceed 1 KB (because
> 
> That sounds like convention.  Is there a guarantee?

No, there is no guarantee. It's the programmer's responsibility to pass
only sensible arguments to this function.

> What I was wondering is whether there were some guard, say
> in all callers, that would prevent VLA abuse.

No, there isn't. The msgid and msgctxt are meant to be literal strings.
Even malicious abusers of a program cannot turn string literals into
multi-megabyte monsters that would lead to stack overflow.

Bruno
-- 
In memoriam Fred Hampton 

Re: [PATCH 2/4] Add a new sethostname module

[Bruno Haible]

Ben Walton wrote:
> --- a/doc/glibc-functions/sethostname.texi
> +++ b/doc/glibc-functions/sethostname.texi
> @@ -2,18 +2,24 @@
>  @subsection @code{sethostname}
>  @findex sethostname
>  
> -Gnulib module: ---
> +Gnulib module: sethostname
>  
>  Portability problems fixed by Gnulib:
>  @itemize
> -@end itemize
> -
> -Portability problems not fixed by Gnulib:
> -@itemize
>  @item
>  This function is missing on some platforms:
>  Minix 3.1.8, Cygwin, mingw, MSVC 9, Interix 3.5, BeOS.
>  @item
>  This function is not declared on some platforms:
>  AIX 7.1, OSF/1 5.1, Solaris 10.
> +@item
> +On Solaris 10, the first argument is @code{char *} instead of
> +@code{const char *} and the second parameter is @code{int} instead of
> +@code{size_t}.
> +@item
> +On some platforms the Gnulib replacement always fails with ENOSYS.
> +@end itemize
> +
> +Portability problems not fixed by Gnulib:
> +@itemize
>  @end itemize

On Solaris 11-2011, I'm getting a gcc warning at test-sethostname1.c:23. Which
means, the problem with the declaration's parametere types is not fixed on this
version of Solaris. I'm fixing the doc:


2011-12-04  Bruno Haible  

sethostname: Fix documentation.
* doc/glibc-functions/sethostname.texi: Move the Solaris problem to the
"not fixed" section.

--- doc/glibc-functions/sethostname.texi.orig   Sun Dec  4 15:45:57 2011
+++ doc/glibc-functions/sethostname.texiSun Dec  4 15:43:42 2011
@@ -13,12 +13,12 @@
 @item
 This function is not declared on some platforms:
 AIX 7.1, OSF/1 5.1, Solaris 10.
-@item
-On Solaris 10, the first argument is @code{char *} instead of
-@code{const char *} and the second parameter is @code{int} instead of
-@code{size_t}.
 @end itemize
 
 Portability problems not fixed by Gnulib:
 @itemize
+@item
+On Solaris 11 2010-11, the first argument is @code{char *} instead of
+@code{const char *} and the second parameter is @code{int} instead of
+@code{size_t}.
 @end itemize
-- 
In memoriam Fred Hampton 

Re: [PATCH 4/4] Add a test suite for the sethostname module

[Bruno Haible]

Ben Walton wrote:
> Provide a module that tests the functionality of sethostname().

I have verified that the test 2 is skipped for non-root users and succeeds
for root users on Linux/glibc 2.11, MacOS X 10.5, Solaris 11 2010-11,
Minix 3.1.8, and that the hostname is correctly restored when the test
terminates.

Bruno
-- 
In memoriam Fred Hampton 

Re: [PATCH 2/4] Add a new sethostname module

[Ben Walton]

Excerpts from Bruno Haible's message of Sat Dec 03 08:09:23 -0500 2011:

Hi Bruno,

> Thanks, I'm applying this for you. I trust that you will follow through
> with the paperwork.

Yes, I'll ensure that gets completed.

> I'm applying a couple of followup tweaks.

Thanks for the tweaks...sorry for requiring them of you still.

I've also noted the reasons for the tweaks and will attempt to provide
better patches in the future.

Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

Re: [PATCH 4/4] Add a test suite for the sethostname module

[Ben Walton]

Excerpts from Bruno Haible's message of Sat Dec 03 08:50:37 -0500 2011:

Hi Bruno,

> > +#if !HAVE_GETEUID
> > +# define geteuid() ((uid_t) -1)
> > +#endif
> 
> Wow! You have thought at many things. You did it better than if I had
> written this test.

I saw a similar trick somewhere else in gnulib (although I can't find
it now)

In one of the subsequent tweaks to this test, you restored a space
between the function name and the emtpy () at the call site for
geteuid.  I realize I let some foo() slip through so it's
understandable that you were doing a bunch of corrections.  Will this
not break the use of the macro for platforms lacking geteuid though?

If so, the following patch should fix this.

>From 7b0c5cb00ba3a294ce1919fded4dab2816919c03 Mon Sep 17 00:00:00 2001
From: Ben Walton 
Date: Sun, 4 Dec 2011 10:54:31 -0500
Subject: [PATCH] Tweak test-sethostname2 to intentionally violate coding style

We must violate the coding style in test-sethostname2.c in order to
allow the preprocessor to substitute a value for geteuid() on
platforms that lack it.

Signed-off-by: Ben Walton 
---
 ChangeLog |7 +++
 tests/test-sethostname2.c |5 -
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 0bd5d06..d9a38b1 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2011-12-04  Ben Walton  
+
+   sethostname tests: Intentionally violate style
+   * tests/set-sethostname2.c: If geteuid isn't defined, we rely on
+   the macro geteuid() being replaced by the preprocessor.  Remove
+   the space betwen function name and () so that this can happen.
+
 2011-12-04  Bruno Haible  
 
sethostname: Fix documentation.
diff --git a/tests/test-sethostname2.c b/tests/test-sethostname2.c
index 51f92ae..a791726 100644
--- a/tests/test-sethostname2.c
+++ b/tests/test-sethostname2.c
@@ -48,7 +48,10 @@ main (int argc, char *argv[] _GL_UNUSED)
  consider things like CAP_SYS_ADMIN (linux) or PRIV_SYS_ADMIN
  (solaris), etc.  systems without a working geteuid (mingw, MSVC
  9) will always skip this test. */
-  if (geteuid () != 0)
+  /* NOTE: the missing space between function name and () is
+ intentional here so that in the event geteuid is derived from the
+ macro above it will still work. */
+  if (geteuid() != 0)
 {
   fprintf (stderr, "Skipping test: insufficient permissions.\n");
   return 77;
-- 
1.7.4.1

> Oops, that looks like a tab again.

I did install a .dir-locals.el on one system and forgot to copy it to
other places where I was testing this.  Would a .dir-locals.el (and
vim equivalent) file be something that might be included in the
project?  Something that defined the indentation and C style rules but
nothing else?  I suspect not, but I'm asking just in case.

Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

Re: [PATCH 4/4] Add a test suite for the sethostname module

[Bruno Haible]

Ben Walton wrote:
> In one of the subsequent tweaks to this test, you restored a space
> between the function name and the emtpy () at the call site for
> geteuid.  I realize I let some foo() slip through so it's
> understandable that you were doing a bunch of corrections.  Will this
> not break the use of the macro for platforms lacking geteuid though?
> ...
> -  if (geteuid () != 0)
> +  /* NOTE: the missing space between function name and () is
> + intentional here so that in the event geteuid is derived from the
> + macro above it will still work. */
> +  if (geteuid() != 0)

You have been misunderstanding how the C and C++ macro expansion process
works. The only place where a space before the open parenthesis is not
allowed is in the *definition* of a macro with arguments, like here:

# define geteuid() ((uid_t) -1)

NOT

# define geteuid () ((uid_t) -1)

For details, please see in ISO C:
http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf
section 6.10.3.

Bruno
-- 
In memoriam Fred Hampton 

Re: avoiding tabs in Emacs

[Bruno Haible]

Ben Walton wrote:
> > Oops, that looks like a tab again.
> 
> I did install a .dir-locals.el on one system and forgot to copy it to
> other places where I was testing this.  Would a .dir-locals.el (and
> vim equivalent) file be something that might be included in the
> project?  Something that defined the indentation and C style rules but
> nothing else?

What do the Emacs users think about it? Is this the right way to go?

Bruno
-- 
In memoriam Fred Hampton 

Re: mktemp: not found

[Jim Meyering]

Bruno Haible wrote:
> On Minix 3.1.8, I get this spurious output from the test-verify.sh test:
>
>   mktemp: not found
>   PASS: test-verify.sh
>
> This patch fixes it. OK to apply?
>
> 2011-12-04  Bruno Haible  
>
>   tests: Avoid spurious error message on platforms without mktemp program.
>   * tests/init.sh (mktempd_): Run mktemp in a subshell.
>
> --- tests/init.sh.origSun Dec  4 14:55:00 2011
> +++ tests/init.sh Sun Dec  4 14:54:51 2011
> @@ -521,7 +521,7 @@
>esac
>
># First, try to use mktemp.
> -  d=`unset TMPDIR; mktemp -d -t -p "$destdir_" "$template_" 2>/dev/null` \
> +  d=`unset TMPDIR; (mktemp -d -t -p "$destdir_" "$template_") 2>/dev/null` \

Thanks.  That would avoid the spurious output.
How about using { ...; } instead?

  d=`unset TMPDIR; { mktemp -d -t -p "$destdir_" "$template_"; } 2>/dev/null` \

That should do the same without the cost of a sub-shell.

Re: mktemp: not found

[Bruce Korb]

On 12/04/11 11:57, Jim Meyering wrote:

   d=`unset TMPDIR; { mktemp -d -t -p "$destdir_" "$template_"; } 2>/dev/null` \

That should do the same without the cost of a sub-shell.


Or:
  d=`
exec 2>/dev/null
unset TMPDIR
mktemp -d -t -p "$destdir_" "$template_"
  `

as we discovered recently, there are some commands (builtin's, okay)
where the output goes to stderr before it gets redirected.
So get your fingers in the habit of not using overly long lines
and redirecting junk before the command in question.  :)

Re: mktemp: not found

[Bruno Haible]

Jim Meyering wrote:
> How about using { ...; } instead?
> 
>   d=`unset TMPDIR; { mktemp -d -t -p "$destdir_" "$template_"; } 2>/dev/null` 
> \
> 
> That should do the same without the cost of a sub-shell.

Yes, this works too. Applied, with this ChangeLog entry:


2011-12-04  Bruno Haible  
Jim Meyering  

tests: Avoid spurious error message on platforms without mktemp program.
* tests/init.sh (mktempd_): Run mktemp in a subcommand.

-- 
In memoriam Fred Hampton 

Re: sethostname

[Bruno Haible]

Let me add support for sethostname() on Windows platforms.
Unlike on Unix, the hostname change becomes effective (outside the
Control Panel) only after a reboot.


2011-12-04  Bruno Haible  

sethostname: Port to Windows platforms.
* lib/sethostname.c: Provide an alternate implementation for Windows
platforms.
* tests/test-sethostname2.c (geteuid): Redefine on Cygwin.
(main): Skip the test if sethostname() fails with EPERM. On Windows
platforms, don't check the result of gethostname().

--- lib/sethostname.c.orig  Mon Dec  5 03:43:38 2011
+++ lib/sethostname.c   Mon Dec  5 03:34:51 2011
@@ -19,6 +19,7 @@
 
 #include 
 
+#if !((defined _WIN32 || defined __WIN32__) || defined __CYGWIN__)
 /* Unix API.  */
 
 /* Specification.  */
@@ -82,3 +83,80 @@
   return -1;
 #endif
 }
+
+#else
+/* Native Windows API.  Also used on Cygwin.  */
+
+/* Ensure that  declares SetComputerNameEx.  */
+#undef _WIN32_WINNT
+#define _WIN32_WINNT 0x500
+
+#define WIN32_LEAN_AND_MEAN
+
+/* Specification.  */
+#include 
+
+#include 
+#include 
+#include 
+
+#include 
+/* The mingw header files don't define GetComputerNameEx, SetComputerNameEx.  
*/
+#ifndef GetComputerNameEx
+# define GetComputerNameEx GetComputerNameExA
+#endif
+#ifndef SetComputerNameEx
+# define SetComputerNameEx SetComputerNameExA
+#endif
+
+/* Set up to LEN chars of NAME as system hostname.
+   Return 0 if ok, set errno and return -1 on error. */
+
+int
+sethostname (const char *name, size_t len)
+{
+  char name_asciz[HOST_NAME_MAX + 1];
+  char old_name[HOST_NAME_MAX + 1];
+  DWORD old_name_len;
+
+  /* Ensure the string isn't too long.  glibc does allow setting an
+ empty hostname so no point in enforcing a lower bound. */
+  if (len > HOST_NAME_MAX)
+{
+  errno = EINVAL;
+  return -1;
+}
+
+  /* Prepare a NUL-terminated copy of name.  */
+  memcpy (name_asciz, name, len);
+  name_asciz[len] = '\0';
+
+  /* Save the old NetBIOS name.  */
+  old_name_len = sizeof (old_name) - 1;
+  if (! GetComputerNameEx (ComputerNamePhysicalNetBIOS,
+   old_name, &old_name_len))
+old_name_len = 0;
+
+  /* Set both the NetBIOS and the first part of the IP / DNS name.  */
+  if (! SetComputerNameEx (ComputerNamePhysicalNetBIOS, name_asciz))
+{
+  errno = (GetLastError () == ERROR_ACCESS_DENIED ? EPERM : EINVAL);
+  return -1;
+}
+  if (! SetComputerNameEx (ComputerNamePhysicalDnsHostname, name_asciz))
+{
+  errno = (GetLastError () == ERROR_ACCESS_DENIED ? EPERM : EINVAL);
+  /* Restore the old NetBIOS name.  */
+  if (old_name_len > 0)
+{
+  old_name[old_name_len] = '\0';
+  SetComputerNameEx (ComputerNamePhysicalNetBIOS, old_name);
+}
+  return -1;
+}
+
+  /* Note that the new host name becomes effective only after a reboot!  */
+  return 0;
+}
+
+#endif
--- tests/test-sethostname2.c.orig  Mon Dec  5 03:43:38 2011
+++ tests/test-sethostname2.c   Mon Dec  5 03:43:33 2011
@@ -31,8 +31,10 @@
 
 #define TESTHOSTNAME "gnulib-hostname"
 
-/* mingw and MSVC 9 lack geteuid, so setup a dummy value.  */
-#if !HAVE_GETEUID
+/* mingw and MSVC 9 lack geteuid, so setup a dummy value.
+   On Cygwin, geteuid() may return non-zero even for user accounts with
+   administrator privileges, so use a dummy value as well.  */
+#if !HAVE_GETEUID || defined __CYGWIN__
 # define geteuid() 0
 #endif
 
@@ -72,6 +74,11 @@
"Skipping test: sethostname is not really implemented.\n");
   return 77;
 }
+  else if (rcs == -1 && errno == EPERM)
+{
+  fprintf (stderr, "Skipping test: insufficient permissions.\n");
+  return 77;
+}
   else
 {
   fprintf (stderr, "error setting valid hostname.\n");
@@ -82,6 +89,10 @@
 {
   ASSERT (gethostname (newname, sizeof (newname)) == 0);
 
+  /* On Windows, a hostname change becomes effective only after
+ a reboot.  */
+#if !((defined _WIN32 || defined __WIN32__) || defined __CYGWIN__)
+
   /* if we don't get back what we put in, there is no need to
  restore the original name as we will assume it was not
  properly changed. */
@@ -90,6 +101,7 @@
   fprintf (stderr, "set/get comparison failed.\n");
   return 1;
 }
+#endif
 }
 
   /* glibc does allow setting a zero length name, so the lower bound

-- 
In memoriam Fred Hampton