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

[Zbigniew Jędrzejewski-Szmek]

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".
> 
> > 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) {
		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