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

[Barry deFreese]

Marco Gerards wrote:

> 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
>
>
>  
Thanks for taking the time 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.
>
>  
I know, I need to fix this.

> > +    {"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?
>
>  
Yes, makes sense.

> > +       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.. :-)

> > +           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.
>
>
>  
This was code from David, I haven't changed this.

> > -       /* Initialise support for locales, and set the program-name */
> > +       /* Initialize support for locales, and set the program-name */
> >    
>
> Same here.
>
>  
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? :)
>
>  
I did change it.  ARGP_KEY_ARG now builds an array arguments.users of
the users rather than a string of users seperated by commas.

> > -       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?

Thanks for taking the time Marco!!

--
Barry deFreese
Debian 3.0r1 "Woody"
GNU/Hurd
Registered Linux "Newbie" #302256 - Hurd H4XX0r wannabe

"Programming today is a race between software engineers striving
to build bigger and better idiot-proof programs, and the Universe
trying to produce bigger and better idiots. So far, the Universe is
winning." Rich Cook.