[systemd-devel] [PATCH 1/2] add parameters checking for 'udevadm settle"

[Zbigniew Jędrzejewski-Szmek]

On Sun, Nov 03, 2013 at 07:49:02PM +0800, Yang Zhiyong wrote:
> This patch adds parameters checking for 'udevadm settle'.
> ---
>  src/udev/udevadm-settle.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/src/udev/udevadm-settle.c b/src/udev/udevadm-settle.c
> index c4fc4ee..06f4004 100644
> --- a/src/udev/udevadm-settle.c
> +++ b/src/udev/udevadm-settle.c
> @@ -35,6 +35,7 @@
>  #include <sys/types.h>
>  
>  #include "udev.h"
> +#include "util.h"
>  
>  static int adm_settle(struct udev *udev, int argc, char *argv[])
>  {
> @@ -56,14 +57,19 @@ static int adm_settle(struct udev *udev, int argc, char *argv[])
>          struct pollfd pfd[1] = { {.fd = -1}, };
>          struct udev_queue *udev_queue = NULL;
>          int rc = EXIT_FAILURE;
> -
> +        int r = -1;
>          for (;;) {
>                  int option;
>                  int seconds;
>  
>                  option = getopt_long(argc, argv, "s:e:t:E:qh", options, NULL);
> -                if (option == -1)
> +                if (option == -1) {
> +                        if (optind < argc) {
> +                                fprintf(stderr, "Extraneous argument: '%s'\n",argv[optind]);
> +                                exit(EXIT_FAILURE);
> +                        }
>                          break;
> +                }
>  
>                  switch (option) {
>                  case 's':
> @@ -73,11 +79,13 @@ static int adm_settle(struct udev *udev, int argc, char *argv[])
>                          end = strtoull(optarg, NULL, 0);
>                          break;
>                  case 't':
> -                        seconds = atoi(optarg);
> -                        if (seconds >= 0)
> +                        r = safe_atoi(optarg,&seconds);
So, why not make seconds unsigned? Actually, seconds is only used to set
timout, which is already unsigned. And safe_atou will not touch the destination
variable unless sucessfull, so you can simply do away with seconds,
making the code simpler.


> +                        if (r < 0 || seconds < 0) {
> +                                fprintf(stderr, "Invalid timeout value '%s': %s\n", optarg, strerror(-r));
> +                                exit(EXIT_FAILURE);
> +                        } else {
>                                  timeout = seconds;
> -                        else
> -                                fprintf(stderr, "invalid timeout value\n");
> +                        }
>                          break;
>                  case 'q':
>                          quiet = 1;

Zbyszek