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

[tao]

Quoting Barry deFreese <address@hidden>:

> Marco Gerards wrote:

[snip]

> >>+const char *argp_program_version = "chage (sysutils) 0.1.2";

Use PACKAGE and VERSION here to get "sysutils" and the version# respectively.

> >>+const char *argp_program_bug_address = "<address@hidden>";

Bug-address should be PACKAGE_BUGREPORT.

[snip]

> >>+   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:
> >
> >  
> >
> I had it set to 0 originally but in Davids code later on he checks
> against -2 and I didn't want to hack up the code more than I already
> have.. :-)

And lo!, there is indeed a reason. string_to_date (does what is says)
that is used for the conversion needs to be able to tell the difference
between -2 (argument not changed), -1 (something failed), and 0
(never).

chage is not really the primary user here, it could probably handle
this in an easier manner, but the interface to the function is meant to
work with the attribute-parser used by chuser and chgroup.

[snip]

> >>-   /* Initialise support for locales, and set the program-name */
> >>+   /* Initialize support for locales, and set the program-name */
> >>    
> >>
> >
> >Same here.
> >
> >  
> >
> Same here.. :-)

The spelling shall be Initialise, not Initialize.

[snip]

See separate e-mail about comma-separated user-lists.


> >>-   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.
> >
> Probably but I wasn't quite sure what he was getting at here?  What do
> you suggest?

What I'm getting at?  I think the comment in the code explains that pretty well.
 
The code probably doesn't, though.  Looking at the code, I see some obvious
errors that I need to fix.

Basically, the restraints are:

No username specified -- Invalid

Username == ALL or more than one user -- Only valid in batch-mode (that is,
at least one of -l, -m, -M, -W, -I, -E, or -d need to be specified)

A single username -- enter interactive mode

The code needs another parameter;

`--force', which should be required to be able to use ALL, since changing
ALL users will include administrative users (including root) as well...


Regards: David