Re: [pull] some sys-utils improvements

[Karel Zak <kzak@redhat.com>]

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