[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Bug-sysutils] argp changes for chage.c
From: |
Marco Gerards |
Subject: |
Re: [Bug-sysutils] argp changes for chage.c |
Date: |
Fri, 21 May 2004 13:07:46 +0200 |
User-agent: |
Gnus/5.1006 (Gnus v5.10.6) Emacs/21.3 (gnu/linux) |
Barry deFreese <address@hidden> writes:
> OK, I mutilated the hell out of chage.c to add argp functionality. I
> still have a few minor problems but I wanted to run it by you guys to
> see what you think. I'm still struggling a little with the uses of
> min, max, etc when they should exist in the expiry struct??
I had a quick look at your patch. Hopefully you will find my comments
useful.
Thanks,
Marco
> +const char *argp_program_version = "chage (sysutils) 0.1.2";
> +const char *argp_program_bug_address = "<address@hidden>";
The best thing you could do is using a shared macro for the version
and bug report address. You can do this by using autoconf.
> + {"min", 'm', "VALUE", 0, "the minimum number of days before the
> password "
> + "can be changed"},
> + {"max", 'M', "VALUE", 0, "the maximum number of days before the
> password "
> + "must be changed"},
> + {"warn", 'W', "VALUE", 0, "the number of days to warn before the
> password expires"},
Using the word VALUE is quite generic. I would use DAYS instead.
What do you think?
> + case ARGP_KEY_INIT:
> + memset(&arguments, -2, sizeof(arguments));
Why do you want to set it to -2? Using 0 does not seem logical to me,
is there a reason? If you set it to 0 you won't need the next line:
> + arguments->users = (char**) malloc(sizeof(char **));
> - * @return 0 on success, errno on failure
> + * @return 0 on success, errno on failure
Please be careful about this. I assume this is accidental.
> - /* Initialise support for locales, and set the program-name */
> + /* Initialize support for locales, and set the program-name */
Same here.
> + /* FIXME?? There is probably a better way to handle since the userlist
> + * is now in an argp struct but I didn't want to hack the code
> + * up too bad. bdd
> + */
[...]
This code looks like it splits the arguments by ','. I thought you
wanted to replace that? :)
> - if (optind == 1) {
> + if (argc == 1 && arguments.usercount >1) {
I think it is better to do this in your argp parser. That would be
cleaner IMHO.