[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: Tuesday, October 29, 2013 9:38 PM
> To: YangZhiyong
> 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 Tue, Oct 29, 2013 at 01:34:33PM +0800, YangZhiyong wrote:
> >
> >
> > > -----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.
> 
> Yes! So your code is fine, but please don't say "unknown option", but rather
> "Extraneous argument: %s" or something like that.
> 
> > > > >                  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() ?
> No, atou, because the timeout must be non-negative.
> 
> > Because the type of "seconds" is "int".
> >

If we use atou,GCC will print "WARNING  ./src/shared/util.h:136:5: note: expected ‘unsigned int *’ but argument is of type ‘int *’ "
because the function prototype of safe_atoi  is " int safe_atou(const char *s, unsigned *ret_u); ".

> > > Shall I write like this:
> > case 't':
> >	if (safe_atoi(optarg,&seconds) < 0){
> >		fprintf(stderr, "invalid timeout value\n");
> > 		exit(EXIT_FAILURE);
> >	}
> >	break;
> The error message can be improved:
> 
>         int r;
> 
> 	r = safe_atou(optarg, &seconds);
> 	if (r < 0) {
I think I should modify 2 lines of code above  like this:
                r = safe_atoi(optarg,&seconds);
                if (r < 0 || seconds < 0) {
And I have test it successfully.

Yang Zhiyong
> 		fprintf(stderr, "Invalid timeout value '%s': %s\n",
> 		        optarg, strerror(-r));
>  		exit(EXIT_FAILURE);
> 	}
> 	break;
> 
> (Notice the extra whitespace at various points. And please ident with spaces)
> 
> Zbyszek