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

[YangZhiyong]

> -----Original Message-----
> From: Zbigniew Jędrzejewski-Szmek [mailto:zbyszek at in.waw.pl]
> Sent: Monday, November 04, 2013 12:56 AM
> To: Yang Zhiyong
> Cc: systemd-devel at lists.freedesktop.org; kay at vrfy.org
> Subject: Re: [systemd-devel][PATCH 1/2] add parameters checking for
> 'udevadm settle"
> 
> 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.
OK, I understand .safe_atou  is more efficient.

Yang Zhiyong
> 
> 
> > +                        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