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

[YangZhiyong]

> -----Original Message-----
> From: YangZhiyong [mailto:yangzy.fnst at cn.fujitsu.com]
> Sent: Tuesday, October 29, 2013 11:33 AM
> To: 'Zbigniew Jędrzejewski-Szmek'
> Cc: systemd-devel at lists.freedesktop.org; kay at vrfy.org; fnst-
> driver at cn.fujitsu.com
> Subject: 答复: [systemd-devel] [PATCH 1/2]add parameters checking for
> 'udevadm settle'
> 
> 
> 
> > ----- Original Message -----
> > From: Zbigniew Jędrzejewski-Szmek [mailto:zbyszek at in.waw.pl]
> > Sent: October 27, 2013 3:29 PM
> > 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 Wed, Oct 23, 2013 at 03:09:36PM +0800, Yang Zhiyong wrote:
> > > This patch adds parameters checking for 'udevadm settle'
> > > Signed-off-by: Yang Zhiyong <yangzy.fnst at cn.fujitsu.com>
> > > ---
> > >  src/udev/udevadm-settle.c |   15 ++++++++++++---
> > >  1 files changed, 12 insertions(+), 3 deletions(-)  mode change
> > > 100644 => 100755 src/udev/udevadm-settle.c
> > >
> > > diff --git a/src/udev/udevadm-settle.c b/src/udev/udevadm-settle.c
> > > old mode 100644 new mode 100755 index c4fc4ee..cacc7e3
> > > --- a/src/udev/udevadm-settle.c
> > > +++ b/src/udev/udevadm-settle.c
> > > @@ -62,8 +62,13 @@ static int adm_settle(struct udev *udev, int
> > > argc, char
> > *argv[])
> > >                  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, "unknown option\n");
> > > +                                exit(EXIT_FAILURE);
> > > +                        }
> > >                          break;
> > > +                }
> > I don't think that getopt_long will return -1 for an unknown option.
> > According to the manpage, -1 mean that all command-line options have
> been parsed.
> 
> I'm very sure that getopt_long() will return -1 when we use an unknown
> option such as " udevadm settle bjjkhgjk" by using "
> printf("option=%d\n",option);" in the code.
> 
If we use " udevadm  settle  --bjjkhgjk"(add "--")the  that getopt_long() will return "?" as the manpage said, and print error info.
But if lacking"--", getopt_long() will treat " bjjkhgjk "as parameter  instead of option .
Because of this , getopt_long() returns -1 and does not print any error info.
By the way,some other source code also uses  the same  method to  process the more rigorous parameter checking.
> 
> >
> > >                  switch (option) {
> > >                  case 's':
> > > @@ -74,10 +79,14 @@ static int adm_settle(struct udev *udev, int
> > > argc,
> > char *argv[])
> > >                          break;
> > >                  case 't':
> > >                          seconds = atoi(optarg);
> > > -                        if (seconds >= 0)
> > > +                        if (seconds > 0)
> > >                                  timeout = seconds;
> > > -                        else
> > > +                        else if (seconds == 0 && !strcmp(optarg,"0"))
> > > +                                timeout = seconds;
> > > +                        else {
> > >                                  fprintf(stderr, "invalid timeout
> > > value\n");
> > > +                                exit(EXIT_FAILURE);
> > > +                        }
> > >                          break;
> > >                  case 'q':
> > >                          quiet = 1;
> > This looks legitimate, but please use safe_atou().
> 
> You mean using safe_atoi() ?
Because the type of "seconds" is "int".

> Shall I write like this:
>                     case 't':
>                         if (safe_atoi(optarg,&seconds) < 0){
	            if (safe_atoi(optarg,&seconds) < 0 || seconds <0 ){

Yang Zhiyong
>                                 fprintf(stderr, "invalid timeout value\n");
>                                 exit(EXIT_FAILURE);
>                         }
>                         break;
> 
> >
> > Zbyszek
>