util-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/6] pull: renice changes
@ 2014-09-06 11:24 Sami Kerola
  2014-09-06 11:24 ` [PATCH 1/6] renice: reorder functions to avoid need of function prototype Sami Kerola
                   ` (7 more replies)
  0 siblings, 8 replies; 15+ messages in thread
From: Sami Kerola @ 2014-09-06 11:24 UTC (permalink / raw)
  To: util-linux; +Cc: kerolasa

Hello,

The renice(1) code looked a bit dated, so I decided to change it a bit
which resulted few more changes than I thought.  The --priority <arg>
change makes the command line interface a bit less awkward, and the
numeric uid argument parsing is a bug fix.  Rest of the changes are clean
ups.


The following changes since commit 3c2e64b0ca73b90b80365842e95abb2858217e60:

  lsblk: add notes about udev to the man page (2014-09-05 10:02:45 +0200)

are available in the git repository at:

  git://github.com/kerolasa/lelux-utiliteetit.git renice

for you to fetch changes up to 13f618783fb0b8e19d2edef31074454fc4ad5a68:

  rename: add getpriority() message lookup table (2014-09-06 12:07:10 +0100)

----------------------------------------------------------------

Sami Kerola (6):
  renice: reorder functions to avoid need of function prototype
  rename: use usage and version print out macros
  renice: disallow --priority <arg> without pid argument
  renice: avoid having same lines of code twice
  renice: fix numeric uid argument parsing
  rename: add getpriority() message lookup table

 sys-utils/renice.c | 107 +++++++++++++++++++++++++++--------------------------
 1 file changed, 54 insertions(+), 53 deletions(-)

-- 
2.1.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/6] renice: reorder functions to avoid need of function prototype
  2014-09-06 11:24 [PATCH 0/6] pull: renice changes Sami Kerola
@ 2014-09-06 11:24 ` Sami Kerola
  2014-09-06 11:24 ` [PATCH 2/6] rename: use usage and version print out macros Sami Kerola
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 15+ messages in thread
From: Sami Kerola @ 2014-09-06 11:24 UTC (permalink / raw)
  To: util-linux; +Cc: kerolasa

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 sys-utils/renice.c | 65 +++++++++++++++++++++++++++---------------------------
 1 file changed, 32 insertions(+), 33 deletions(-)

diff --git a/sys-utils/renice.c b/sys-utils/renice.c
index c0378e1..5d643fb 100644
--- a/sys-utils/renice.c
+++ b/sys-utils/renice.c
@@ -48,8 +48,6 @@
 #include "c.h"
 #include "closestream.h"
 
-static int donice(int,int,int);
-
 static void __attribute__((__noreturn__)) usage(FILE *out)
 {
 	fputs(_("\nUsage:\n"), out);
@@ -72,6 +70,38 @@ static void __attribute__((__noreturn__)) usage(FILE *out)
 	exit(out == stderr ? EXIT_FAILURE : EXIT_SUCCESS);
 }
 
+static int
+donice(int which, int who, int prio) {
+	int oldprio, newprio;
+	const char *idtype = _("process ID");
+
+	if (which == PRIO_USER)
+		idtype = _("user ID");
+	else if (which == PRIO_PGRP)
+		idtype = _("process group ID");
+
+	errno = 0;
+	oldprio = getpriority(which, who);
+	if (oldprio == -1 && errno) {
+		warn(_("failed to get priority for %d (%s)"), who, idtype);
+		return 1;
+	}
+	if (setpriority(which, who, prio) < 0) {
+		warn(_("failed to set priority for %d (%s)"), who, idtype);
+		return 1;
+	}
+	errno = 0;
+	newprio = getpriority(which, who);
+	if (newprio == -1 && errno) {
+		warn(_("failed to get priority for %d (%s)"), who, idtype);
+		return 1;
+	}
+
+	printf(_("%d (%s) old priority %d, new priority %d\n"),
+	       who, idtype, oldprio, newprio);
+	return 0;
+}
+
 /*
  * Change the priority (the nice value) of processes
  * or groups of processes which are already running.
@@ -155,34 +185,3 @@ main(int argc, char **argv)
 	return errs != 0 ? EXIT_FAILURE : EXIT_SUCCESS;
 }
 
-static int
-donice(int which, int who, int prio) {
-	int oldprio, newprio;
-	const char *idtype = _("process ID");
-
-	if (which == PRIO_USER)
-		idtype = _("user ID");
-	else if (which == PRIO_PGRP)
-		idtype = _("process group ID");
-
-	errno = 0;
-	oldprio = getpriority(which, who);
-	if (oldprio == -1 && errno) {
-		warn(_("failed to get priority for %d (%s)"), who, idtype);
-		return 1;
-	}
-	if (setpriority(which, who, prio) < 0) {
-		warn(_("failed to set priority for %d (%s)"), who, idtype);
-		return 1;
-	}
-	errno = 0;
-	newprio = getpriority(which, who);
-	if (newprio == -1 && errno) {
-		warn(_("failed to get priority for %d (%s)"), who, idtype);
-		return 1;
-	}
-
-	printf(_("%d (%s) old priority %d, new priority %d\n"),
-	       who, idtype, oldprio, newprio);
-	return 0;
-}
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/6] rename: use usage and version print out macros
  2014-09-06 11:24 [PATCH 0/6] pull: renice changes Sami Kerola
  2014-09-06 11:24 ` [PATCH 1/6] renice: reorder functions to avoid need of function prototype Sami Kerola
@ 2014-09-06 11:24 ` Sami Kerola
  2014-09-06 11:24 ` [PATCH 3/6] renice: disallow --priority <arg> without pid argument Sami Kerola
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 15+ messages in thread
From: Sami Kerola @ 2014-09-06 11:24 UTC (permalink / raw)
  To: util-linux; +Cc: kerolasa

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 sys-utils/renice.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/sys-utils/renice.c b/sys-utils/renice.c
index 5d643fb..0605680 100644
--- a/sys-utils/renice.c
+++ b/sys-utils/renice.c
@@ -50,23 +50,20 @@
 
 static void __attribute__((__noreturn__)) usage(FILE *out)
 {
-	fputs(_("\nUsage:\n"), out);
+	fputs(USAGE_HEADER, out);
 	fprintf(out,
 	      _(" %1$s [-n] <priority> [-p|--pid] <pid>...\n"
 		" %1$s [-n] <priority>  -g|--pgrp <pgid>...\n"
 		" %1$s [-n] <priority>  -u|--user <user>...\n"),
 		program_invocation_short_name);
-
-	fputs(_("\nOptions:\n"), out);
+	fputs(USAGE_OPTIONS, out);
 	fputs(_(" -g, --pgrp <id>        interpret argument as process group ID\n"
 		" -n, --priority <num>   specify the nice increment value\n"
 		" -p, --pid <id>         interpret argument as process ID (default)\n"
 		" -u, --user <name|id>   interpret argument as username or user ID\n"
 		" -h, --help             display help text and exit\n"
 		" -V, --version          display version information and exit\n"), out);
-
-	fputs(_("\nFor more information see renice(1).\n"), out);
-
+	fprintf(out, USAGE_MAN_TAIL("renice(1)"));
 	exit(out == stderr ? EXIT_FAILURE : EXIT_SUCCESS);
 }
 
@@ -129,9 +126,8 @@ main(int argc, char **argv)
 		if (strcmp(*argv, "-v") == 0 ||
 		    strcmp(*argv, "-V") == 0 ||
 		    strcmp(*argv, "--version") == 0) {
-			printf(_("%s from %s\n"),
-			       program_invocation_short_name, PACKAGE_STRING);
-			exit(EXIT_SUCCESS);
+			printf(UTIL_LINUX_VERSION);
+			return EXIT_SUCCESS;
 		}
 	}
 
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/6] renice: disallow --priority <arg> without pid argument
  2014-09-06 11:24 [PATCH 0/6] pull: renice changes Sami Kerola
  2014-09-06 11:24 ` [PATCH 1/6] renice: reorder functions to avoid need of function prototype Sami Kerola
  2014-09-06 11:24 ` [PATCH 2/6] rename: use usage and version print out macros Sami Kerola
@ 2014-09-06 11:24 ` Sami Kerola
  2014-09-06 11:24 ` [PATCH 4/6] renice: avoid having same lines of code twice Sami Kerola
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 15+ messages in thread
From: Sami Kerola @ 2014-09-06 11:24 UTC (permalink / raw)
  To: util-linux; +Cc: kerolasa

Earlier a lonely priority with an argument but without pid resulted to no
action and success, when the invocation should have failed.

$ renice --priority 42 ; echo $?
0

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 sys-utils/renice.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sys-utils/renice.c b/sys-utils/renice.c
index 0605680..2075d40 100644
--- a/sys-utils/renice.c
+++ b/sys-utils/renice.c
@@ -131,14 +131,14 @@ main(int argc, char **argv)
 		}
 	}
 
-	if (argc < 2)
-		usage(stderr);
-
-	if (strcmp(*argv, "-n") == 0 || strcmp(*argv, "--priority") == 0) {
+	if (*argv && (strcmp(*argv, "-n") == 0 || strcmp(*argv, "--priority") == 0)) {
 		argc--;
 		argv++;
 	}
 
+	if (argc < 2)
+		usage(stderr);
+
 	prio = strtol(*argv, &endptr, 10);
 	if (*endptr)
 		usage(stderr);
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 4/6] renice: avoid having same lines of code twice
  2014-09-06 11:24 [PATCH 0/6] pull: renice changes Sami Kerola
                   ` (2 preceding siblings ...)
  2014-09-06 11:24 ` [PATCH 3/6] renice: disallow --priority <arg> without pid argument Sami Kerola
@ 2014-09-06 11:24 ` Sami Kerola
  2014-09-12  7:35   ` Karel Zak
  2014-09-06 11:24 ` [PATCH 5/6] renice: fix numeric uid argument parsing Sami Kerola
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 15+ messages in thread
From: Sami Kerola @ 2014-09-06 11:24 UTC (permalink / raw)
  To: util-linux; +Cc: kerolasa

Add getprio() function to avoid duplication of a simple task.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 sys-utils/renice.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/sys-utils/renice.c b/sys-utils/renice.c
index 2075d40..1c4b840 100644
--- a/sys-utils/renice.c
+++ b/sys-utils/renice.c
@@ -67,8 +67,21 @@ static void __attribute__((__noreturn__)) usage(FILE *out)
 	exit(out == stderr ? EXIT_FAILURE : EXIT_SUCCESS);
 }
 
-static int
-donice(int which, int who, int prio) {
+static int getprio(const int which, const int who, const char *idtype)
+{
+	int ret;
+
+	errno = 0;
+	ret = getpriority(which, who);
+	if (ret == -1 && errno) {
+		warn(_("failed to get priority for %d (%s)"), who, idtype);
+		return PRIO_MAX + 1;
+	}
+	return ret;
+}
+
+static int donice(int which, int who, int prio)
+{
 	int oldprio, newprio;
 	const char *idtype = _("process ID");
 
@@ -76,24 +89,14 @@ donice(int which, int who, int prio) {
 		idtype = _("user ID");
 	else if (which == PRIO_PGRP)
 		idtype = _("process group ID");
-
-	errno = 0;
-	oldprio = getpriority(which, who);
-	if (oldprio == -1 && errno) {
-		warn(_("failed to get priority for %d (%s)"), who, idtype);
+	if ((oldprio = getprio(which, who, idtype)) == PRIO_MAX + 1)
 		return 1;
-	}
 	if (setpriority(which, who, prio) < 0) {
 		warn(_("failed to set priority for %d (%s)"), who, idtype);
 		return 1;
 	}
-	errno = 0;
-	newprio = getpriority(which, who);
-	if (newprio == -1 && errno) {
-		warn(_("failed to get priority for %d (%s)"), who, idtype);
+	if ((newprio = getprio(which, who, idtype)) == PRIO_MAX + 1)
 		return 1;
-	}
-
 	printf(_("%d (%s) old priority %d, new priority %d\n"),
 	       who, idtype, oldprio, newprio);
 	return 0;
@@ -103,8 +106,7 @@ donice(int which, int who, int prio) {
  * Change the priority (the nice value) of processes
  * or groups of processes which are already running.
  */
-int
-main(int argc, char **argv)
+int main(int argc, char **argv)
 {
 	int which = PRIO_PROCESS;
 	int who = 0, prio, errs = 0;
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 5/6] renice: fix numeric uid argument parsing
  2014-09-06 11:24 [PATCH 0/6] pull: renice changes Sami Kerola
                   ` (3 preceding siblings ...)
  2014-09-06 11:24 ` [PATCH 4/6] renice: avoid having same lines of code twice Sami Kerola
@ 2014-09-06 11:24 ` Sami Kerola
  2014-09-12  8:03   ` Karel Zak
  2014-09-06 11:24 ` [PATCH 6/6] rename: add getpriority() message lookup table Sami Kerola
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 15+ messages in thread
From: Sami Kerola @ 2014-09-06 11:24 UTC (permalink / raw)
  To: util-linux; +Cc: kerolasa

The following was inconflict with what usage() tells are valid option
arguments.

$ renice 1 -u 1000
renice: unknown user 1000
$ id
uid=1000(kerolasa) ...

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 sys-utils/renice.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/sys-utils/renice.c b/sys-utils/renice.c
index 1c4b840..1fef526 100644
--- a/sys-utils/renice.c
+++ b/sys-utils/renice.c
@@ -111,6 +111,7 @@ int main(int argc, char **argv)
 	int which = PRIO_PROCESS;
 	int who = 0, prio, errs = 0;
 	char *endptr = NULL;
+	const char *error_msg;
 
 	setlocale(LC_ALL, "");
 	bindtextdomain(PACKAGE, LOCALEDIR);
@@ -162,18 +163,20 @@ int main(int argc, char **argv)
 			continue;
 		}
 		if (which == PRIO_USER) {
-			register struct passwd *pwd = getpwnam(*argv);
+			struct passwd *pwd = getpwnam(*argv);
 
-			if (pwd == NULL) {
-				warnx(_("unknown user %s"), *argv);
-				errs = 1;
-				continue;
+			if (pwd != NULL)
+				who = pwd->pw_uid;
+			else {
+				error_msg = _("unknown user %s");
+				goto numeric_pid;
 			}
-			who = pwd->pw_uid;
 		} else {
+			error_msg = _("bad value %s");
+ numeric_pid:
 			who = strtol(*argv, &endptr, 10);
 			if (who < 0 || *endptr) {
-				warnx(_("bad value %s"), *argv);
+				warnx(error_msg, *argv);
 				errs = 1;
 				continue;
 			}
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 6/6] rename: add getpriority() message lookup table
  2014-09-06 11:24 [PATCH 0/6] pull: renice changes Sami Kerola
                   ` (4 preceding siblings ...)
  2014-09-06 11:24 ` [PATCH 5/6] renice: fix numeric uid argument parsing Sami Kerola
@ 2014-09-06 11:24 ` Sami Kerola
  2014-09-16 20:21 ` [PATCH 0/6] pull: renice changes Sami Kerola
  2014-09-22 12:48 ` Karel Zak
  7 siblings, 0 replies; 15+ messages in thread
From: Sami Kerola @ 2014-09-06 11:24 UTC (permalink / raw)
  To: util-linux; +Cc: kerolasa

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 sys-utils/renice.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/sys-utils/renice.c b/sys-utils/renice.c
index 1fef526..a5bf50b 100644
--- a/sys-utils/renice.c
+++ b/sys-utils/renice.c
@@ -48,6 +48,12 @@
 #include "c.h"
 #include "closestream.h"
 
+const char *idtype[] = {
+	[PRIO_PROCESS]	= N_("process ID"),
+	[PRIO_PGRP]	= N_("process group ID"),
+	[PRIO_USER]	= N_("user ID"),
+};
+
 static void __attribute__((__noreturn__)) usage(FILE *out)
 {
 	fputs(USAGE_HEADER, out);
@@ -67,38 +73,33 @@ static void __attribute__((__noreturn__)) usage(FILE *out)
 	exit(out == stderr ? EXIT_FAILURE : EXIT_SUCCESS);
 }
 
-static int getprio(const int which, const int who, const char *idtype)
+static int getprio(const int which, const int who)
 {
 	int ret;
 
 	errno = 0;
 	ret = getpriority(which, who);
 	if (ret == -1 && errno) {
-		warn(_("failed to get priority for %d (%s)"), who, idtype);
+		warn(_("failed to get priority for %d (%s)"), who, idtype[which]);
 		return PRIO_MAX + 1;
 	}
 	return ret;
 }
 
-static int donice(int which, int who, int prio)
+static int donice(const int which, const int who, const int prio)
 {
 	int oldprio, newprio;
-	const char *idtype = _("process ID");
 
-	if (which == PRIO_USER)
-		idtype = _("user ID");
-	else if (which == PRIO_PGRP)
-		idtype = _("process group ID");
-	if ((oldprio = getprio(which, who, idtype)) == PRIO_MAX + 1)
+	if ((oldprio = getprio(which, who)) == PRIO_MAX + 1)
 		return 1;
 	if (setpriority(which, who, prio) < 0) {
-		warn(_("failed to set priority for %d (%s)"), who, idtype);
+		warn(_("failed to set priority for %d (%s)"), who, idtype[which]);
 		return 1;
 	}
-	if ((newprio = getprio(which, who, idtype)) == PRIO_MAX + 1)
+	if ((newprio = getprio(which, who)) == PRIO_MAX + 1)
 		return 1;
 	printf(_("%d (%s) old priority %d, new priority %d\n"),
-	       who, idtype, oldprio, newprio);
+	       who, idtype[which], oldprio, newprio);
 	return 0;
 }
 
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/6] renice: avoid having same lines of code twice
  2014-09-06 11:24 ` [PATCH 4/6] renice: avoid having same lines of code twice Sami Kerola
@ 2014-09-12  7:35   ` Karel Zak
  2014-09-16 20:13     ` Sami Kerola
  0 siblings, 1 reply; 15+ messages in thread
From: Karel Zak @ 2014-09-12  7:35 UTC (permalink / raw)
  To: Sami Kerola; +Cc: util-linux

On Sat, Sep 06, 2014 at 12:24:51PM +0100, Sami Kerola wrote:
> +static int getprio(const int which, const int who, const char *idtype)
> +{
> +	int ret;
> +
> +	errno = 0;
> +	ret = getpriority(which, who);
> +	if (ret == -1 && errno) {
> +		warn(_("failed to get priority for %d (%s)"), who, idtype);
> +		return PRIO_MAX + 1;
> +	}
> +	return ret;
> +}

This is exactly the situation when you want to use "return" only for
function status rather than for any data.

static int getprio(const int which,
                   const int who,
                   const char *idtype,
                   int *prio)                       <--------!
{
    errno = 0;
    *prio = getpriority(which, who);
    if (*prio == -1 && errno) {
        warn(_("failed to get priority for %d (%s)"), who, idtype);
        return -errno;
    }
    return 0;
}


> -	errno = 0;
> -	newprio = getpriority(which, who);
> -	if (newprio == -1 && errno) {
> -		warn(_("failed to get priority for %d (%s)"), who, idtype);
> +	if ((newprio = getprio(which, who, idtype)) == PRIO_MAX + 1)
>  		return 1;
> -	}

   if (getprio(which, who, idtype, &newprio) != 0)
        return 1;

 .. so you don't need complicated things like PRIO_MAX + 1.

    Karel

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/6] renice: fix numeric uid argument parsing
  2014-09-06 11:24 ` [PATCH 5/6] renice: fix numeric uid argument parsing Sami Kerola
@ 2014-09-12  8:03   ` Karel Zak
  2014-09-16 20:17     ` Sami Kerola
  0 siblings, 1 reply; 15+ messages in thread
From: Karel Zak @ 2014-09-12  8:03 UTC (permalink / raw)
  To: Sami Kerola; +Cc: util-linux

On Sat, Sep 06, 2014 at 12:24:52PM +0100, Sami Kerola wrote:
>  		if (which == PRIO_USER) {
> -			register struct passwd *pwd = getpwnam(*argv);
> +			struct passwd *pwd = getpwnam(*argv);
>  
> -			if (pwd == NULL) {
> -				warnx(_("unknown user %s"), *argv);
> -				errs = 1;
> -				continue;
> +			if (pwd != NULL)
> +				who = pwd->pw_uid;
> +			else {
> +				error_msg = _("unknown user %s");
> +				goto numeric_pid;
>  			}
> -			who = pwd->pw_uid;
>  		} else {
> +			error_msg = _("bad value %s");
> + numeric_pid:
>  			who = strtol(*argv, &endptr, 10);
>  			if (who < 0 || *endptr) {
> -				warnx(_("bad value %s"), *argv);
> +				warnx(error_msg, *argv);
>  				errs = 1;
>  				continue;
>  			}

 The goto is unnecessary. Just set who = -1 by default.

  who = -1;
  if (which == PRIO_USER) {
        ...
        who = pwd->pw_uid;
  }

  if (who < 0) {
        who = strtol(*argv, &endptr, 10);
        ...
        if (who < 0 || *endptr)
            warnx(_("failed to parse %s argument"), idtype[which]);
  }


    Karel

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/6] renice: avoid having same lines of code twice
  2014-09-12  7:35   ` Karel Zak
@ 2014-09-16 20:13     ` Sami Kerola
  0 siblings, 0 replies; 15+ messages in thread
From: Sami Kerola @ 2014-09-16 20:13 UTC (permalink / raw)
  To: Karel Zak; +Cc: util-linux

On Fri, 12 Sep 2014, Karel Zak wrote:

> On Sat, Sep 06, 2014 at 12:24:51PM +0100, Sami Kerola wrote:
>> +static int getprio(const int which, const int who, const char *idtype)
>> +{
>> +	int ret;
>> +
>> +	errno = 0;
>> +	ret = getpriority(which, who);
>> +	if (ret == -1 && errno) {
>> +		warn(_("failed to get priority for %d (%s)"), who, idtype);
>> +		return PRIO_MAX + 1;
>> +	}
>> +	return ret;
>> +}
>
> This is exactly the situation when you want to use "return" only for 
> function status rather than for any data.
>
> static int getprio(const int which,
>                   const int who,
>                   const char *idtype,
>                   int *prio)                       <--------!
> {
>    errno = 0;
>    *prio = getpriority(which, who);
>    if (*prio == -1 && errno) {
>        warn(_("failed to get priority for %d (%s)"), who, idtype);
>        return -errno;
>    }
>    return 0;
> }
>
>
>> -	errno = 0;
>> -	newprio = getpriority(which, who);
>> -	if (newprio == -1 && errno) {
>> -		warn(_("failed to get priority for %d (%s)"), who, idtype);
>> +	if ((newprio = getprio(which, who, idtype)) == PRIO_MAX + 1)
>>  		return 1;
>> -	}
>
>   if (getprio(which, who, idtype, &newprio) != 0)
>        return 1;
>
> .. so you don't need complicated things like PRIO_MAX + 1.

Hi Karel,

That is a great improvement. Here is updated version of the change.

-->8---
From: Sami Kerola <kerolasa@iki.fi>
Date: Fri, 5 Sep 2014 23:33:16 +0100
Subject: [PATCH 4/7] renice: avoid having same lines of code twice

Add getprio() function to avoid duplication of a simple task.

Reviewed-by: Karel Zak <kzak@redhat.com>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
  sys-utils/renice.c | 32 ++++++++++++++++----------------
  1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/sys-utils/renice.c b/sys-utils/renice.c
index 2075d40..d83fc4a 100644
--- a/sys-utils/renice.c
+++ b/sys-utils/renice.c
@@ -67,8 +67,19 @@ static void __attribute__((__noreturn__)) usage(FILE *out)
  	exit(out == stderr ? EXIT_FAILURE : EXIT_SUCCESS);
  }

-static int
-donice(int which, int who, int prio) {
+static int getprio(const int which, const int who, const char *idtype, int *prio)
+{
+	errno = 0;
+	*prio = getpriority(which, who);
+	if (*prio == -1 && errno) {
+		warn(_("failed to get priority for %d (%s)"), who, idtype);
+		return -errno;
+	}
+	return 0;
+}
+
+static int donice(int which, int who, int prio)
+{
  	int oldprio, newprio;
  	const char *idtype = _("process ID");

@@ -76,24 +87,14 @@ donice(int which, int who, int prio) {
  		idtype = _("user ID");
  	else if (which == PRIO_PGRP)
  		idtype = _("process group ID");
-
-	errno = 0;
-	oldprio = getpriority(which, who);
-	if (oldprio == -1 && errno) {
-		warn(_("failed to get priority for %d (%s)"), who, idtype);
+	if (getprio(which, who, idtype, &oldprio) != 0)
  		return 1;
-	}
  	if (setpriority(which, who, prio) < 0) {
  		warn(_("failed to set priority for %d (%s)"), who, idtype);
  		return 1;
  	}
-	errno = 0;
-	newprio = getpriority(which, who);
-	if (newprio == -1 && errno) {
-		warn(_("failed to get priority for %d (%s)"), who, idtype);
+	if (getprio(which, who, idtype, &newprio) != 0)
  		return 1;
-	}
-
  	printf(_("%d (%s) old priority %d, new priority %d\n"),
  	       who, idtype, oldprio, newprio);
  	return 0;
@@ -103,8 +104,7 @@ donice(int which, int who, int prio) {
   * Change the priority (the nice value) of processes
   * or groups of processes which are already running.
   */
-int
-main(int argc, char **argv)
+int main(int argc, char **argv)
  {
  	int which = PRIO_PROCESS;
  	int who = 0, prio, errs = 0;
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/6] renice: fix numeric uid argument parsing
  2014-09-12  8:03   ` Karel Zak
@ 2014-09-16 20:17     ` Sami Kerola
  0 siblings, 0 replies; 15+ messages in thread
From: Sami Kerola @ 2014-09-16 20:17 UTC (permalink / raw)
  To: Karel Zak; +Cc: util-linux

On Fri, 12 Sep 2014, Karel Zak wrote:

> On Sat, Sep 06, 2014 at 12:24:52PM +0100, Sami Kerola wrote:
>>  		if (which == PRIO_USER) {
>> -			register struct passwd *pwd = getpwnam(*argv);
>> +			struct passwd *pwd = getpwnam(*argv);
>>
>> -			if (pwd == NULL) {
>> -				warnx(_("unknown user %s"), *argv);
>> -				errs = 1;
>> -				continue;
>> +			if (pwd != NULL)
>> +				who = pwd->pw_uid;
>> +			else {
>> +				error_msg = _("unknown user %s");
>> +				goto numeric_pid;
>>  			}
>> -			who = pwd->pw_uid;
>>  		} else {
>> +			error_msg = _("bad value %s");
>> + numeric_pid:
>>  			who = strtol(*argv, &endptr, 10);
>>  			if (who < 0 || *endptr) {
>> -				warnx(_("bad value %s"), *argv);
>> +				warnx(error_msg, *argv);
>>  				errs = 1;
>>  				continue;
>>  			}
>
> The goto is unnecessary. Just set who = -1 by default.
>
>  who = -1;
>  if (which == PRIO_USER) {
>        ...
>        who = pwd->pw_uid;
>  }
>
>  if (who < 0) {
>        who = strtol(*argv, &endptr, 10);
>        ...
>        if (who < 0 || *endptr)
>            warnx(_("failed to parse %s argument"), idtype[which]);
>  }

Hi Karel,

Oh yes, I clearly over complicated the previous version. This ought to be 
a bit closer what one could consider acceptable.

--->8----
From: Sami Kerola <kerolasa@iki.fi>
Date: Fri, 5 Sep 2014 23:54:17 +0100
Subject: [PATCH 5/7] renice: fix numeric uid argument parsing

The following was inconflict with what usage() tells are valid option
arguments.

$ renice 1 -u 1000
renice: unknown user 1000
$ id
uid=1000(kerolasa) ...

Reviewed-by: Karel Zak <kzak@redhat.com>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
  sys-utils/renice.c | 10 ++++++----
  1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/sys-utils/renice.c b/sys-utils/renice.c
index d83fc4a..a123ed1 100644
--- a/sys-utils/renice.c
+++ b/sys-utils/renice.c
@@ -160,14 +160,17 @@ int main(int argc, char **argv)
  			continue;
  		}
  		if (which == PRIO_USER) {
-			register struct passwd *pwd = getpwnam(*argv);
+			struct passwd *pwd = getpwnam(*argv);

-			if (pwd == NULL) {
+			if (pwd != NULL)
+				who = pwd->pw_uid;
+			else
+				who = strtol(*argv, &endptr, 10);
+			if (who < 0 || *endptr) {
  				warnx(_("unknown user %s"), *argv);
  				errs = 1;
  				continue;
  			}
-			who = pwd->pw_uid;
  		} else {
  			who = strtol(*argv, &endptr, 10);
  			if (who < 0 || *endptr) {
@@ -180,4 +183,3 @@ int main(int argc, char **argv)
  	}
  	return errs != 0 ? EXIT_FAILURE : EXIT_SUCCESS;
  }
-
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/6] pull: renice changes
  2014-09-06 11:24 [PATCH 0/6] pull: renice changes Sami Kerola
                   ` (5 preceding siblings ...)
  2014-09-06 11:24 ` [PATCH 6/6] rename: add getpriority() message lookup table Sami Kerola
@ 2014-09-16 20:21 ` Sami Kerola
  2014-09-17  7:57   ` Benno Schulenberg
  2014-09-22 12:48 ` Karel Zak
  7 siblings, 1 reply; 15+ messages in thread
From: Sami Kerola @ 2014-09-16 20:21 UTC (permalink / raw)
  To: util-linux

On Sat, 6 Sep 2014, Sami Kerola wrote:

> The renice(1) code looked a bit dated, so I decided to change it a bit
> which resulted few more changes than I thought.  The --priority <arg>
> change makes the command line interface a bit less awkward, and the
> numeric uid argument parsing is a bug fix.  Rest of the changes are clean
> ups.

Pull request I sent earlier is no longer completely up to date.  The 
renice branch is rebased on top of upstream master, and there is one new 
change 0007: reorder usage() option descriptions.


The following changes since commit a3b92242ad76a7468cf508e1d878d0815c7e031f:

   libmount: hide details about failed search in fstab/mtab (2014-09-16 15:30:03 +0200)

are available in the git repository at:

   git://github.com/kerolasa/lelux-utiliteetit.git renice

for you to fetch changes up to a32b3f68da162eadbb17ff65a585d0249805c32f:

   renice: reorder usage() option descriptions (2014-09-16 21:09:11 +0100)

----------------------------------------------------------------
Sami Kerola (7):
       renice: reorder functions to avoid need of function prototype
       rename: use usage and version print out macros
       renice: disallow --priority <arg> without pid argument
       renice: avoid having same lines of code twice
       renice: fix numeric uid argument parsing
       rename: add getpriority() message lookup table
       renice: reorder usage() option descriptions

  sys-utils/renice.c | 111 
++++++++++++++++++++++++++---------------------------
  1 file changed, 55 insertions(+), 56 deletions(-)

-->8---
From: Sami Kerola <kerolasa@iki.fi>
Date: Sun, 14 Sep 2014 11:12:58 +0100
Subject: [PATCH 7/7] renice: reorder usage() option descriptions

Make the Usage: and Options: sections to be in same order, which I found
to be quicker to use than alphabetical order.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
  sys-utils/renice.c | 13 +++++++------
  1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/sys-utils/renice.c b/sys-utils/renice.c
index 994c9de..100f6a5 100644
--- a/sys-utils/renice.c
+++ b/sys-utils/renice.c
@@ -63,12 +63,13 @@ static void __attribute__((__noreturn__)) usage(FILE *out)
  		" %1$s [-n] <priority>  -u|--user <user>...\n"),
  		program_invocation_short_name);
  	fputs(USAGE_OPTIONS, out);
-	fputs(_(" -g, --pgrp <id>        interpret argument as process group ID\n"
-		" -n, --priority <num>   specify the nice increment value\n"
-		" -p, --pid <id>         interpret argument as process ID (default)\n"
-		" -u, --user <name|id>   interpret argument as username or user ID\n"
-		" -h, --help             display help text and exit\n"
-		" -V, --version          display version information and exit\n"), out);
+	fputs(_(" -n, --priority <num>   specify the nice increment value\n"), out);
+	fputs(_(" -p, --pid <id>         interpret argument as process ID (default)\n"), out);
+	fputs(_(" -g, --pgrp <id>        interpret argument as process group ID\n"), out);
+	fputs(_(" -u, --user <name|id>   interpret argument as username or user ID\n"), out);
+	fputs(USAGE_SEPARATOR, out);
+	fputs(USAGE_HELP, out);
+	fputs(USAGE_VERSION, out);
  	fprintf(out, USAGE_MAN_TAIL("renice(1)"));
  	exit(out == stderr ? EXIT_FAILURE : EXIT_SUCCESS);
  }
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/6] pull: renice changes
  2014-09-16 20:21 ` [PATCH 0/6] pull: renice changes Sami Kerola
@ 2014-09-17  7:57   ` Benno Schulenberg
  2014-09-17 10:06     ` Sami Kerola
  0 siblings, 1 reply; 15+ messages in thread
From: Benno Schulenberg @ 2014-09-17  7:57 UTC (permalink / raw)
  To: Sami Kerola; +Cc: Util-Linux


On Tue, Sep 16, 2014, at 22:21, Sami Kerola wrote:
> +	fputs(_(" -u, --user <name|id>   interpret argument as username or user ID\n"), out);

The argument here should be <name>|<id>.
(Think of <> as the // markers in email.)

Benno

-- 
http://www.fastmail.fm - The way an email service should be


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/6] pull: renice changes
  2014-09-17  7:57   ` Benno Schulenberg
@ 2014-09-17 10:06     ` Sami Kerola
  0 siblings, 0 replies; 15+ messages in thread
From: Sami Kerola @ 2014-09-17 10:06 UTC (permalink / raw)
  To: Benno Schulenberg; +Cc: Util-Linux

On 17 September 2014 08:57, Benno Schulenberg <bensberg@justemail.net> wrote:
> On Tue, Sep 16, 2014, at 22:21, Sami Kerola wrote:
>> +     fputs(_(" -u, --user <name|id>   interpret argument as username or user ID\n"), out);
>
> The argument here should be <name>|<id>.
> (Think of <> as the // markers in email.)

Thank you for review Benno,

Update is in my git repo.
https://github.com/kerolasa/lelux-utiliteetit/commit/94a6c9b3cde3741ec1f7fe10cc3cead1a42990ae

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/6] pull: renice changes
  2014-09-06 11:24 [PATCH 0/6] pull: renice changes Sami Kerola
                   ` (6 preceding siblings ...)
  2014-09-16 20:21 ` [PATCH 0/6] pull: renice changes Sami Kerola
@ 2014-09-22 12:48 ` Karel Zak
  7 siblings, 0 replies; 15+ messages in thread
From: Karel Zak @ 2014-09-22 12:48 UTC (permalink / raw)
  To: Sami Kerola; +Cc: util-linux

On Sat, Sep 06, 2014 at 12:24:47PM +0100, Sami Kerola wrote:
> Sami Kerola (6):
>   renice: reorder functions to avoid need of function prototype
>   rename: use usage and version print out macros
>   renice: disallow --priority <arg> without pid argument
>   renice: avoid having same lines of code twice
>   renice: fix numeric uid argument parsing
>   rename: add getpriority() message lookup table

 Applied, thanks.

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2014-09-22 12:48 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-06 11:24 [PATCH 0/6] pull: renice changes Sami Kerola
2014-09-06 11:24 ` [PATCH 1/6] renice: reorder functions to avoid need of function prototype Sami Kerola
2014-09-06 11:24 ` [PATCH 2/6] rename: use usage and version print out macros Sami Kerola
2014-09-06 11:24 ` [PATCH 3/6] renice: disallow --priority <arg> without pid argument Sami Kerola
2014-09-06 11:24 ` [PATCH 4/6] renice: avoid having same lines of code twice Sami Kerola
2014-09-12  7:35   ` Karel Zak
2014-09-16 20:13     ` Sami Kerola
2014-09-06 11:24 ` [PATCH 5/6] renice: fix numeric uid argument parsing Sami Kerola
2014-09-12  8:03   ` Karel Zak
2014-09-16 20:17     ` Sami Kerola
2014-09-06 11:24 ` [PATCH 6/6] rename: add getpriority() message lookup table Sami Kerola
2014-09-16 20:21 ` [PATCH 0/6] pull: renice changes Sami Kerola
2014-09-17  7:57   ` Benno Schulenberg
2014-09-17 10:06     ` Sami Kerola
2014-09-22 12:48 ` Karel Zak

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).