[pull] some sys-utils improvements

[pull] some sys-utils improvements

[Sami Kerola]

Hello,

I started to look sys-utils directory, and got obsessed by getting ipc
stuff to behave like a tool from this millennium. The whole directory
is not done, but I feel this is already big enough patch set.

I must admit that there is a lurking bug. When ipcrm tries to delete
everything, new --all switch without options, it prints fail rubbish
when there are only message queues resources. Bellow is an example how
I could see the issue.

$ ./ipcs

------ Shared Memory Segments --------
key        shmid      owner      perms      bytes      nattch     status

------ Semaphore Arrays --------
key        semid      owner      perms      nsems

------ Message Queues --------
key        msqid      owner      perms      used-bytes   messages

$ ./ipcmk -Q
Message queue id: 14581760
$ ./ipcrm --all -v
removing message queue id `14581760'
ipcrm: invalid id (14581760)

Strange enough the `invalid id' is not reported if I comment out
either shared memory or semaphore if blocks from remove all function.
And same happens when remove all explicitly specifies message queue
resource.

$ ./ipcmk -Q
Message queue id: 14614528
$ ./ipcrm --all=msg -v
removing message queue id `14614528'

I am 99.9% certainty my code is wrong, I just don't see how and advice
is very welcome. Other than this the patch set should be fairly OKish.

p.s. Persons who do not want to fetch my git branch, but are
interested to check what might be problem, here is the file:
https://raw.github.com/kerolasa/lelux-utiliteetit/sys-utils/sys-utils/ipcrm.c

--- snip
The following changes since commit 4e9b3bfda20ebbaf5d925dedad6ce8e2b678b563:

  lib,cpuset: fix compiler warning [-Wuninitialized] (2011-09-10 00:02:00 +0200)

are available in the git repository at:
  https://github.com/kerolasa/lelux-utiliteetit sys-utils

Sami Kerola (26):
      setarch: move options struct to function scope
      setarch: use program_invocation_short_name
      setarch: add version printing
      ipcmk: add long options & fix usage()
      ipcmk: remove useless code
      ipcmk: validate numeric option arguments
      ipcmk: remove camel casing
      ipcmk: include-what-you-use header check
      ipcrm: add long options
      ipcrm: exit if unknown error occurs
      ipcrm: refactor new and old main to share code
      ipcrm: include-what-you-use header check
      ipcs: add long options
      ipcs: include-what-you-use header check
      ipcs: comment & white space clean up
      pivot_root: add version & help option
      docs: mention long options in pivot_root.8
      ctrlaltdel: add version & help options
      docs: mention long options in ctrlaltdel.8
      docs: add --version to setarch.8
      docs: add long options to ipcmk.1 man page
      docs: add long options to ipcrm.1 man page
      docs: add long options to ipcs.1 man page
      ipcrm: add --all option
      ipcmk: allow high speed ipc creation
      ipcrm: add --verbose option

 sys-utils/Makefile.am  |    1 +
 sys-utils/ctrlaltdel.8 |   20 ++-
 sys-utils/ctrlaltdel.c |   39 ++++-
 sys-utils/ipcmk.1      |   52 +++---
 sys-utils/ipcmk.c      |  144 +++++++--------
 sys-utils/ipcrm.1      |   61 +++----
 sys-utils/ipcrm.c      |  491 ++++++++++++++++++++++++++++++------------------
 sys-utils/ipcs.1       |  101 ++++++----
 sys-utils/ipcs.c       |  279 ++++++++++++++-------------
 sys-utils/pivot_root.8 |    9 +-
 sys-utils/pivot_root.c |   60 +++++-
 sys-utils/setarch.8    |   21 ++-
 sys-utils/setarch.c    |   74 ++++----
 13 files changed, 788 insertions(+), 564 deletions(-)

-- 
   Sami Kerola
   http://www.iki.fi/kerolasa/

Re: [pull] some sys-utils improvements

[Davidlohr Bueso]

On Tue, 2011-09-13 at 23:13 +0200, Sami Kerola wrote:
> Hello,
> 

Hi Sami,

> I started to look sys-utils directory, and got obsessed by getting ipc
> stuff to behave like a tool from this millennium. The whole directory
> is not done, but I feel this is already big enough patch set.
> 
> I must admit that there is a lurking bug. When ipcrm tries to delete
> everything, new --all switch without options, it prints fail rubbish
> when there are only message queues resources. Bellow is an example how
> I could see the issue.
> 
> $ ./ipcs
> 
> ------ Shared Memory Segments --------
> key        shmid      owner      perms      bytes      nattch     status
> 
> ------ Semaphore Arrays --------
> key        semid      owner      perms      nsems
> 
> ------ Message Queues --------
> key        msqid      owner      perms      used-bytes   messages
> 
> $ ./ipcmk -Q
> Message queue id: 14581760
> $ ./ipcrm --all -v
> removing message queue id `14581760'
> ipcrm: invalid id (14581760)
> 
> Strange enough the `invalid id' is not reported if I comment out
> either shared memory or semaphore if blocks from remove all function.
> And same happens when remove all explicitly specifies message queue
> resource.
> 
> $ ./ipcmk -Q
> Message queue id: 14614528
> $ ./ipcrm --all=msg -v
> removing message queue id `14614528'
> 
> I am 99.9% certainty my code is wrong, I just don't see how and advice
> is very welcome. Other than this the patch set should be fairly OKish.

Please try the following patch, it fixed it for me. In the case when the
errors were shown, the msgctl(2) was returning 0 (success), so errno
shouldn't be checked. Remember, this isn't reseted afterwards, so we are
running into bogus messages (the msgid's are being correctly deleted).

Thanks,
Davidlohr

>From 952d6100c28505005c0a335e3e64f384bd941859 Mon Sep 17 00:00:00 2001
From: Davidlohr Bueso <dave@gnu.org>
Date: Wed, 14 Sep 2011 10:17:15 -0300
Subject: [PATCH] ipcrm: check IPC syscalls

It's not enough to check errno for errors as the variable is not reset, we also need to check the last syscall return value to verify a problem.
This addresses bogus msgqueue errors when deleting keys.

Signed-off-by: Davidlohr Bueso <dave@gnu.org>
---
 sys-utils/ipcrm.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/sys-utils/ipcrm.c b/sys-utils/ipcrm.c
index a8d6623..c794cbe 100644
--- a/sys-utils/ipcrm.c
+++ b/sys-utils/ipcrm.c
@@ -65,6 +65,7 @@ static void __attribute__ ((__noreturn__)) usage(FILE * out)
 
 int remove_id(int type, int iskey, int id)
 {
+	int ret;
 	char *errmsg;
 	/* needed to delete semaphores */
 	union semun arg;
@@ -75,24 +76,24 @@ int remove_id(int type, int iskey, int id)
 	case SHM:
 		if (verbose)
 			printf(_("removing shared memory segment id `%d'\n"), id);
-		shmctl(id, IPC_RMID, NULL);
+		ret = shmctl(id, IPC_RMID, NULL);
 		break;
 	case MSG:
 		if (verbose)
 			printf(_("removing message queue id `%d'\n"), id);
-		msgctl(id, IPC_RMID, NULL);
+		ret = msgctl(id, IPC_RMID, NULL);
 		break;
 	case SEM:
 		if (verbose)
 			printf(_("removing semaphore id `%d'\n"), id);
-		semctl(id, 0, IPC_RMID, arg);
+		ret = semctl(id, 0, IPC_RMID, arg);
 		break;
 	default:
 		errx(EXIT_FAILURE, "impossible occurred");
 	}
 
 	/* how did the removal go? */
-	switch (errno) {
+	switch (errno && ret) {
 	case 0:
 		return 0;
 	case EACCES:
-- 
1.7.4.1

Re: [pull] some sys-utils improvements

[Karel Zak]

On Wed, Sep 14, 2011 at 10:21:53AM -0300, Davidlohr Bueso wrote:
> On Tue, 2011-09-13 at 23:13 +0200, Sami Kerola wrote:
> Please try the following patch, it fixed it for me. In the case when the
> errors were shown, the msgctl(2) was returning 0 (success), so errno
> shouldn't be checked. Remember, this isn't reseted afterwards, so we are
> running into bogus messages (the msgid's are being correctly deleted).
> 
> Thanks,
> Davidlohr
> 
> From 952d6100c28505005c0a335e3e64f384bd941859 Mon Sep 17 00:00:00 2001
> From: Davidlohr Bueso <dave@gnu.org>
> Date: Wed, 14 Sep 2011 10:17:15 -0300
> Subject: [PATCH] ipcrm: check IPC syscalls
> 
> It's not enough to check errno for errors as the variable is not reset, we also need to check the last syscall return value to verify a problem.
> This addresses bogus msgqueue errors when deleting keys.
> 
> Signed-off-by: Davidlohr Bueso <dave@gnu.org>
> ---
>  sys-utils/ipcrm.c |    9 +++++----
>  1 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/sys-utils/ipcrm.c b/sys-utils/ipcrm.c
> index a8d6623..c794cbe 100644
> --- a/sys-utils/ipcrm.c
> +++ b/sys-utils/ipcrm.c
> @@ -65,6 +65,7 @@ static void __attribute__ ((__noreturn__)) usage(FILE * out)
>  
>  int remove_id(int type, int iskey, int id)
>  {
> +	int ret;
>  	char *errmsg;
>  	/* needed to delete semaphores */
>  	union semun arg;
> @@ -75,24 +76,24 @@ int remove_id(int type, int iskey, int id)
>  	case SHM:
>  		if (verbose)
>  			printf(_("removing shared memory segment id `%d'\n"), id);
> -		shmctl(id, IPC_RMID, NULL);
> +		ret = shmctl(id, IPC_RMID, NULL);
>  		break;
>  	case MSG:
>  		if (verbose)
>  			printf(_("removing message queue id `%d'\n"), id);
> -		msgctl(id, IPC_RMID, NULL);
> +		ret = msgctl(id, IPC_RMID, NULL);
>  		break;
>  	case SEM:
>  		if (verbose)
>  			printf(_("removing semaphore id `%d'\n"), id);
> -		semctl(id, 0, IPC_RMID, arg);
> +		ret = semctl(id, 0, IPC_RMID, arg);
>  		break;
>  	default:
>  		errx(EXIT_FAILURE, "impossible occurred");
>  	}
>  
>  	/* how did the removal go? */
> -	switch (errno) {
> +	switch (errno && ret) {

 so true/false only ... why we need the "case EACCES" (etc.)?

>  	case 0:
>  		return 0;
>  	case EACCES:

 why we cannot use:

  if (ret < 0) 
    switch(errno) {
      case EACCES:
         ...
         break;

      ....
  }

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [pull] some sys-utils improvements

[Davidlohr Bueso]

On Wed, 2011-09-14 at 18:35 +0200, Karel Zak wrote:
> On Wed, Sep 14, 2011 at 10:21:53AM -0300, Davidlohr Bueso wrote:
> > On Tue, 2011-09-13 at 23:13 +0200, Sami Kerola wrote:
> > Please try the following patch, it fixed it for me. In the case when the
> > errors were shown, the msgctl(2) was returning 0 (success), so errno
> > shouldn't be checked. Remember, this isn't reseted afterwards, so we are
> > running into bogus messages (the msgid's are being correctly deleted).
> > 
> > Thanks,
> > Davidlohr
> > 
> > From 952d6100c28505005c0a335e3e64f384bd941859 Mon Sep 17 00:00:00 2001
> > From: Davidlohr Bueso <dave@gnu.org>
> > Date: Wed, 14 Sep 2011 10:17:15 -0300
> > Subject: [PATCH] ipcrm: check IPC syscalls
> > 
> > It's not enough to check errno for errors as the variable is not reset, we also need to check the last syscall return value to verify a problem.
> > This addresses bogus msgqueue errors when deleting keys.
> > 
> > Signed-off-by: Davidlohr Bueso <dave@gnu.org>
> > ---
> >  sys-utils/ipcrm.c |    9 +++++----
> >  1 files changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/sys-utils/ipcrm.c b/sys-utils/ipcrm.c
> > index a8d6623..c794cbe 100644
> > --- a/sys-utils/ipcrm.c
> > +++ b/sys-utils/ipcrm.c
> > @@ -65,6 +65,7 @@ static void __attribute__ ((__noreturn__)) usage(FILE * out)
> >  
> >  int remove_id(int type, int iskey, int id)
> >  {
> > +	int ret;
> >  	char *errmsg;
> >  	/* needed to delete semaphores */
> >  	union semun arg;
> > @@ -75,24 +76,24 @@ int remove_id(int type, int iskey, int id)
> >  	case SHM:
> >  		if (verbose)
> >  			printf(_("removing shared memory segment id `%d'\n"), id);
> > -		shmctl(id, IPC_RMID, NULL);
> > +		ret = shmctl(id, IPC_RMID, NULL);
> >  		break;
> >  	case MSG:
> >  		if (verbose)
> >  			printf(_("removing message queue id `%d'\n"), id);
> > -		msgctl(id, IPC_RMID, NULL);
> > +		ret = msgctl(id, IPC_RMID, NULL);
> >  		break;
> >  	case SEM:
> >  		if (verbose)
> >  			printf(_("removing semaphore id `%d'\n"), id);
> > -		semctl(id, 0, IPC_RMID, arg);
> > +		ret = semctl(id, 0, IPC_RMID, arg);
> >  		break;
> >  	default:
> >  		errx(EXIT_FAILURE, "impossible occurred");
> >  	}
> >  
> >  	/* how did the removal go? */
> > -	switch (errno) {
> > +	switch (errno && ret) {
> 
>  so true/false only ... why we need the "case EACCES" (etc.)?
> 
> >  	case 0:
> >  		return 0;
> >  	case EACCES:
> 
>  why we cannot use:
> 
>   if (ret < 0) 
>     switch(errno) {
>       case EACCES:
>          ...
>          break;
> 
>       ....
>   }

Well, yes, the case 0 didn't make much sense, otherwise I don't really
care how we evaluate ret/errno. Is the below patch ok with you?

From: Davidlohr Bueso <dave@gnu.org>
Date: Wed, 14 Sep 2011 14:02:15 -0300
Subject: [PATCH] ipcrm: check IPC syscalls

It's not enough to check errno for errors as the variable is not reset, we also need to check the last syscall return value to verify a problem.
This addresses bogus msgqueue errors when deleting keys.

Signed-off-by: Davidlohr Bueso <dave@gnu.org>
---
 sys-utils/ipcrm.c |   51 ++++++++++++++++++++++++++-------------------------
 1 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/sys-utils/ipcrm.c b/sys-utils/ipcrm.c
index a8d6623..a176fc7 100644
--- a/sys-utils/ipcrm.c
+++ b/sys-utils/ipcrm.c
@@ -65,6 +65,7 @@ static void __attribute__ ((__noreturn__)) usage(FILE * out)
 
 int remove_id(int type, int iskey, int id)
 {
+	int ret;
 	char *errmsg;
 	/* needed to delete semaphores */
 	union semun arg;
@@ -75,46 +76,46 @@ int remove_id(int type, int iskey, int id)
 	case SHM:
 		if (verbose)
 			printf(_("removing shared memory segment id `%d'\n"), id);
-		shmctl(id, IPC_RMID, NULL);
+		ret = shmctl(id, IPC_RMID, NULL);
 		break;
 	case MSG:
 		if (verbose)
 			printf(_("removing message queue id `%d'\n"), id);
-		msgctl(id, IPC_RMID, NULL);
+		ret = msgctl(id, IPC_RMID, NULL);
 		break;
 	case SEM:
 		if (verbose)
 			printf(_("removing semaphore id `%d'\n"), id);
-		semctl(id, 0, IPC_RMID, arg);
+		ret = semctl(id, 0, IPC_RMID, arg);
 		break;
 	default:
 		errx(EXIT_FAILURE, "impossible occurred");
 	}
 
 	/* how did the removal go? */
-	switch (errno) {
-	case 0:
-		return 0;
-	case EACCES:
-	case EPERM:
-		errmsg = iskey ? _("permission denied for key")
-		    : _("permission denied for id");
-		break;
-	case EINVAL:
-		errmsg = iskey ? _("invalid key")
-		    : _("invalid id");
-		break;
-	case EIDRM:
-		errmsg = iskey ? _("already removed key")
-		    : _("already removed id");
-		break;
-	default:
-		if (iskey)
-			err(EXIT_FAILURE, _("key failed"));
-		else
-			err(EXIT_FAILURE, _("id failed"));
+	if (ret < 0) {
+		switch (errno) {
+		case EACCES:
+		case EPERM:
+			errmsg = iskey ? _("permission denied for key")
+				: _("permission denied for id");
+			break;
+		case EINVAL:
+			errmsg = iskey ? _("invalid key")
+				: _("invalid id");
+			break;
+		case EIDRM:
+			errmsg = iskey ? _("already removed key")
+				: _("already removed id");
+			break;
+		default:
+			if (iskey)
+				err(EXIT_FAILURE, _("key failed"));
+			else
+				err(EXIT_FAILURE, _("id failed"));
+		}
+		warnx("%s (%d)", errmsg, id);
 	}
-	warnx("%s (%d)", errmsg, id);
 	return 1;
 }
 
-- 
1.7.4.1

Re: [pull] some sys-utils improvements

[Karel Zak]

On Tue, Sep 13, 2011 at 11:13:57PM +0200, Sami Kerola wrote:
> I started to look sys-utils directory, and got obsessed by getting ipc
> stuff to behave like a tool from this millennium. The whole directory
> is not done, but I feel this is already big enough patch set.

 Some notes about USAGE_*:

* USAGE_BEGIN_TAIL is unnecessary if we know that USAGE_MAN_TAIL is
  always on new line, we can use \n at the begin of the
  USAGE_MAN_TAIL:
    
      "\nFor more details see %s.\n"

* I don't like the current

     #define USAGE_MAN_TAIL   _("For more details see %s.\n")

 macro. It's bad manner to use hidden argument(s) for macros, it would
 be better to have:

     #define USAGE_MAN_TAIL(_man)   _("\nFor more details see %s.\n"), _man


* how we want to use USAGE_HELP and USAGE_VERSION macros? The strings
  in the macros are the same for all programs, it means that there is
  the same space between "-h, --help" and "display this help and exit"
  in all programs. For example:

   printf(_("-x, --extra-mega-cool <file>    enable the best feature for <file>\n");
   printf(USAGE_HELP);
   printf(USAGE_USAGE);

   -x, --extra-mega-cool <file>    enable the best feature for <file>
   -h, --help     display this help and exit
   -V, --version  output version information and exit

 mess, maybe we need an extra line between program specific options
 and the common options:

   printf(_("-x, --extra-mega-cool <file>    enable the best feature for <file>\n");
   printf(USAGE_SEPARATOR);
   printf(USAGE_HELP);
   printf(USAGE_USAGE);
   printf(USAGE_MAN_TAIL("foo(1)");

 output:

   -x, --extra-mega-cool <file>    enable the best feature for <file>

   -h, --help     display this help and exit
   -V, --version  output version information and exit

   For more details see foo(1).
  
 it also means that --help and --version should be at the end of the
 options list.

>       setarch: move options struct to function scope
>       setarch: use program_invocation_short_name
>       setarch: add version printing
>       ipcmk: add long options & fix usage()
>       ipcmk: remove useless code
>       ipcmk: validate numeric option arguments
>       ipcmk: remove camel casing
>       ipcmk: include-what-you-use header check

 Note that in the 2.20 release we have introduced a new bug by
 include-what-you-use patch. It's necessary to test it with
 (at least) --disable-nls.

 The ideal solution would be to add something like tools/checkbuilds
 to test different ./configure scenarios (enable/disable NLS,
 slang/ncurses/ncursesw, ...). Now it's possible that not all
 scenarios are tested now. We have to test it, because people are too
 lazy to test -rc releases...

>       ipcrm: add long options
>       ipcrm: exit if unknown error occurs

 Please, don't use "else" after non-return functions. For example:

  if (x)
     non_return_func()
  else
     ...


 Heee.. smatch should be improved to detect this code :-)

>       ipcrm: refactor new and old main to share code

 Please:

    - check strtoul() result. See strutils.c

    - if possible, then use only one line for  var = E ? x : y;  it's
      unnecessary to move ": y" to another line.

    - don't use "error" as variable name, glibc provides such function.

    - btw, do we have regression test for ipcrm? :-)

>       ipcrm: include-what-you-use header check
>       ipcs: add long options
>       ipcs: include-what-you-use header check
>       ipcs: comment & white space clean up
>       pivot_root: add version & help option
>       docs: mention long options in pivot_root.8

 Hmm... there should be also switch_root(8) in SEE ALSO section. 

>       ctrlaltdel: add version & help options
>       docs: mention long options in ctrlaltdel.8
>       docs: add --version to setarch.8
>       docs: add long options to ipcmk.1 man page
>       docs: add long options to ipcrm.1 man page
>       docs: add long options to ipcs.1 man page
>       ipcrm: add --all option
>       ipcmk: allow high speed ipc creation
>       ipcrm: add --verbose option
...
>  13 files changed, 788 insertions(+), 564 deletions(-)

 Thanks!

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [pull] some sys-utils improvements

[Sami Kerola]

On Wed, Sep 14, 2011 at 19:04, Davidlohr Bueso <dave@gnu.org> wrote:

Thanks Dave. Now when I look the fix it's obvious. The patch is
added to my branch, with tiny modifications

* The if shorthands are put to one line.
* After the patch function always returned as it would have failed, which now
  is fixed.

On Wed, Sep 14, 2011 at 19:46, Karel Zak <kzak@redhat.com> wrote:
> On Tue, Sep 13, 2011 at 11:13:57PM +0200, Sami Kerola wrote:
>> I started to look sys-utils directory, and got obsessed by getting ipc
>> stuff to behave like a tool from this millennium. The whole directory
>> is not done, but I feel this is already big enough patch set.
>
>  Some notes about USAGE_*:
>
> * USAGE_BEGIN_TAIL is unnecessary if we know that USAGE_MAN_TAIL is
>  always on new line, we can use \n at the begin of the
>  USAGE_MAN_TAIL:
>
>      "\nFor more details see %s.\n"

When writing that to usage howto I thought there could be an executable which
does not have manual page. That was a misconception; everything has manual page,
and if something does not it will be created. I'll scrap the
USAGE_BEGIN_TAIL.

> * I don't like the current
>
>     #define USAGE_MAN_TAIL   _("For more details see %s.\n")
>
>  macro. It's bad manner to use hidden argument(s) for macros, it would
>  be better to have:
>
>     #define USAGE_MAN_TAIL(_man)   _("\nFor more details see %s.\n"), _man

Corrected.

> * how we want to use USAGE_HELP and USAGE_VERSION macros? The strings
>  in the macros are the same for all programs, it means that there is
>  the same space between "-h, --help" and "display this help and exit"
>  in all programs. For example:
>
>   printf(_("-x, --extra-mega-cool <file>    enable the best feature for <file>\n");
>   printf(USAGE_HELP);
>   printf(USAGE_USAGE);
>
>   -x, --extra-mega-cool <file>    enable the best feature for <file>
>   -h, --help     display this help and exit
>   -V, --version  output version information and exit
>
>  mess, maybe we need an extra line between program specific options
>  and the common options:
>
>   printf(_("-x, --extra-mega-cool <file>    enable the best feature for <file>\n");
>   printf(USAGE_SEPARATOR);
>   printf(USAGE_HELP);
>   printf(USAGE_USAGE);
>   printf(USAGE_MAN_TAIL("foo(1)");
>
>  output:
>
>   -x, --extra-mega-cool <file>    enable the best feature for <file>
>
>   -h, --help     display this help and exit
>   -V, --version  output version information and exit
>
>   For more details see foo(1).
>
>  it also means that --help and --version should be at the end of the
>  options list.

The usage howto is already telling --help and --version are expected to be
last in list, so that should be OK. The separator in between specific and
'common' options is added to howto and code. Obvious good thing about the
separator is that if we end up not liking it it's trivial to make
ineffective.

>>       setarch: move options struct to function scope
>>       setarch: use program_invocation_short_name
>>       setarch: add version printing
>>       ipcmk: add long options & fix usage()
>>       ipcmk: remove useless code
>>       ipcmk: validate numeric option arguments
>>       ipcmk: remove camel casing
>>       ipcmk: include-what-you-use header check
>
>  Note that in the 2.20 release we have introduced a new bug by
>  include-what-you-use patch. It's necessary to test it with
>  (at least) --disable-nls.
>
>  The ideal solution would be to add something like tools/checkbuilds
>  to test different ./configure scenarios (enable/disable NLS,
>  slang/ncurses/ncursesw, ...). Now it's possible that not all
>  scenarios are tested now. We have to test it, because people are too
>  lazy to test -rc releases...

Problem being is that the include-what-you-use gives very good hints and
false positives, so a tiny slip with copy and paste will result to a such
regressions what happen earlier.

Maybe the best way to deal the headers is to catch whether a file has
includes which are dealt some smart way in util-linux/include files. AFAIK
the list such includes at least following.

err.h
langinfo.h
libintl.h
linux/version.h
locale.h
paths.h
stdint.h
sys/ioccom.h
sys/mkdev.h
wchar.h
wctype.h

My suggestion is to expand utility we alrady have

tools/checkincludes.pl

If no-one else will pickup making the utility more useful I will do that at
some day. BTW I'll stop doing the header clean ups until the check is done.

>>       ipcrm: add long options
>>       ipcrm: exit if unknown error occurs
>
>  Please, don't use "else" after non-return functions. For example:
>
>  if (x)
>     non_return_func()
>  else
>     ...

Corrected & the principle is added to howto-contribute.

>  Heee.. smatch should be improved to detect this code :-)
>
>>       ipcrm: refactor new and old main to share code
>
>  Please:
>
>    - check strtoul() result. See strutils.c

Corrected.

>    - if possible, then use only one line for  var = E ? x : y;  it's
>      unnecessary to move ": y" to another line.

Corrected & the principle is added to howto-contribute.

>    - don't use "error" as variable name, glibc provides such function.

Corrected.

>    - btw, do we have regression test for ipcrm? :-)

We don't yet. I added this to my list of things to do. Initial thought about
the test is that is should not rely on ipcs to validate anything, because it
should test that utility. Separate similar utility is needed. Secondly the
iprm testing should not be destructive, which rules out remove all testing
unless 'I know what I am doing' switch is used.

>>       ipcrm: include-what-you-use header check
>>       ipcs: add long options
>>       ipcs: include-what-you-use header check
>>       ipcs: comment & white space clean up
>>       pivot_root: add version & help option
>>       docs: mention long options in pivot_root.8
>
>  Hmm... there should be also switch_root(8) in SEE ALSO section.

Added.

The request-pull bellow updates the previous I sent.

The following changes since commit 4e9b3bfda20ebbaf5d925dedad6ce8e2b678b563:

  lib,cpuset: fix compiler warning [-Wuninitialized] (2011-09-10 00:02:00 +0200)

are available in the git repository at:
  https://github.com/kerolasa/lelux-utiliteetit sys-utils

Davidlohr Bueso (1):
      ipcrm: check IPC syscalls

Sami Kerola (28):
      setarch: move options struct to function scope
      setarch: use program_invocation_short_name
      setarch: add version printing
      ipcmk: add long options & fix usage()
      ipcmk: remove useless code
      ipcmk: validate numeric option arguments
      ipcmk: remove camel casing
      ipcmk: include-what-you-use header check
      ipcrm: add long options
      ipcrm: exit if unknown error occurs
      ipcrm: refactor new and old main to share code
      ipcrm: include-what-you-use header check
      ipcs: add long options
      ipcs: include-what-you-use header check
      ipcs: comment & white space clean up
      pivot_root: add version & help option
      docs: mention long options in pivot_root.8
      ctrlaltdel: add version & help options
      docs: mention long options in ctrlaltdel.8
      docs: add --version to setarch.8
      docs: add long options to ipcmk.1 man page
      docs: add long options to ipcrm.1 man page
      docs: add long options to ipcs.1 man page
      ipcrm: add --all option
      ipcmk: allow high speed ipc creation
      ipcrm: add --verbose option
      build-sys: fixes to USAGE_* macros
      docs: add non-return function and if shorthand tips

 Documentation/howto-contribute.txt     |   18 ++
 Documentation/howto-usage-function.txt |    6 +-
 include/c.h                            |    4 +-
 sys-utils/Makefile.am                  |    2 +
 sys-utils/arch.c                       |    5 +-
 sys-utils/ctrlaltdel.8                 |   20 +-
 sys-utils/ctrlaltdel.c                 |   39 +++-
 sys-utils/ipcmk.1                      |   52 ++--
 sys-utils/ipcmk.c                      |  144 ++++-----
 sys-utils/ipcrm.1                      |   65 ++---
 sys-utils/ipcrm.c                      |  515 ++++++++++++++++++++------------
 sys-utils/ipcs.1                       |  101 ++++---
 sys-utils/ipcs.c                       |  278 +++++++++---------
 sys-utils/pivot_root.8                 |   10 +-
 sys-utils/pivot_root.c                 |   60 +++-
 sys-utils/setarch.8                    |   21 +-
 sys-utils/setarch.c                    |   74 +++---
 17 files changed, 841 insertions(+), 573 deletions(-)

-- 
   Sami Kerola
   http://www.iki.fi/kerolasa/

Re: [pull] some sys-utils improvements

[Karel Zak]

On Sat, Sep 17, 2011 at 03:10:39PM +0200, Sami Kerola wrote:
> >   printf(_("-x, --extra-mega-cool <file>    enable the best feature for <file>\n");
> >   printf(USAGE_SEPARATOR);
> >   printf(USAGE_HELP);
> >   printf(USAGE_USAGE);
> >   printf(USAGE_MAN_TAIL("foo(1)");
[...]
> The usage howto is already telling --help and --version are expected to be
> last in list, so that should be OK. The separator in between specific and
> 'common' options is added to howto and code. Obvious good thing about the
> separator is that if we end up not liking it it's trivial to make
> ineffective.

 Note that the separator should not be used after USAGE_OPTIONS (I
 have fixed this in the code).

[...]

> My suggestion is to expand utility we alrady have
> 
> tools/checkincludes.pl

 no problem, go ahead.

>  Documentation/howto-contribute.txt     |   18 ++
>  Documentation/howto-usage-function.txt |    6 +-
>  include/c.h                            |    4 +-
>  sys-utils/Makefile.am                  |    2 +
>  sys-utils/arch.c                       |    5 +-
>  sys-utils/ctrlaltdel.8                 |   20 +-
>  sys-utils/ctrlaltdel.c                 |   39 +++-
>  sys-utils/ipcmk.1                      |   52 ++--
>  sys-utils/ipcmk.c                      |  144 ++++-----
>  sys-utils/ipcrm.1                      |   65 ++---
>  sys-utils/ipcrm.c                      |  515 ++++++++++++++++++++------------
>  sys-utils/ipcs.1                       |  101 ++++---
>  sys-utils/ipcs.c                       |  278 +++++++++---------
>  sys-utils/pivot_root.8                 |   10 +-
>  sys-utils/pivot_root.c                 |   60 +++-
>  sys-utils/setarch.8                    |   21 +-
>  sys-utils/setarch.c                    |   74 +++---
>  17 files changed, 841 insertions(+), 573 deletions(-)

 Applied, thanks.

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com