On Tue, Oct 29, 2013 at 01:34:33PM +0800, YangZhiyong wrote:
>
>
> > -----Original Message-----
> > From: YangZhiyong [mailto:[email protected]]
> > Sent: Tuesday, October 29, 2013 11:33 AM
> > To: 'Zbigniew Jędrzejewski-Szmek'
> > Cc: [email protected]; [email protected]; fnst-
> > [email protected]
> > Subject: 答复: [systemd-devel] [PATCH 1/2]add parameters checking for
> > 'udevadm settle'
> >
> >
> >
> > > ----- Original Message -----
> > > From: Zbigniew Jędrzejewski-Szmek [mailto:[email protected]]
> > > Sent: October 27, 2013 3:29 PM
> > > To: Yang Zhiyong
> > > Cc: [email protected]; [email protected]
> > > 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 <[email protected]>
> > > > ---
> > > > 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
_______________________________________________
systemd-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
