Re: [Bug-sysutils] argp changes for chage.c

[Marco Gerards]

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.