Last Comment Bug 449334 - pk12util has duplicate options letters
: pk12util has duplicate options letters
Product: NSS
Classification: Components
Component: Tools (show other bugs)
: 3.12
: All All
: -- normal (vote)
: 3.12.2
Assigned To: Elio Maldonado
Depends on:
Blocks: 157942
  Show dependency treegraph
Reported: 2008-08-05 20:50 PDT by Nelson Bolyard (seldom reads bugmail)
Modified: 2008-10-14 16:21 PDT (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

patch fixes duplicate options (1.34 KB, patch)
2008-09-10 07:58 PDT, Elio Maldonado
nelson: review-
Details | Diff | Splinter Review
solve duplicate options with long and short options (1.32 KB, patch)
2008-09-26 09:16 PDT, Elio Maldonado
nelson: review+
Details | Diff | Splinter Review

Description Nelson Bolyard (seldom reads bugmail) 2008-08-05 20:50:18 PDT
While reviewing a patch for bug 157942 today, I discovered that a nasty bug
got into pk12util for NSS 3.12 as part of the checkins for bug 401928.

The checkins for that bug added two new command line options using letters
that were already in use.  There are now *TWO* -k and *TWO* -K options 
defined for pk12util.  See 

This needs to be fixed, ASAP, with the introduction of new option letters.
Also, the behavior of those option letters needs to be TESTED.  Those 
options were obviously untested before, because they could not have worked.

The work on bug 157942 is blocked by this. See also bug 449332.
Comment 1 Elio Maldonado 2008-08-06 08:41:46 PDT
I propose using long options for the duplicate -k and -K arguments
    { /* opt_KeyLength	       */ '0', PR_TRUE,  0, PR_FALSE, "key_len" },
    { /* opt_CertKeyLength     */ '0', PR_TRUE,  0, PR_FALSE, "cert_key_len" }
Since I'm blocked bug 157942 I may as well work on this one as well. 

Comment 2 Nelson Bolyard (seldom reads bugmail) 2008-08-06 12:42:44 PDT
Please check one more thing about pk12util. 
Put the softoken in FIPS mode, and then test to see if the -l (list) option
Comment 3 Elio Maldonado 2008-09-10 07:58:59 PDT
Created attachment 337877 [details] [diff] [review]
patch fixes duplicate options

Using long options as to resolve the options letters collision.
Comment 4 Nelson Bolyard (seldom reads bugmail) 2008-09-15 17:56:40 PDT
Comment on attachment 337877 [details] [diff] [review]
patch fixes duplicate options

r-   I'm sorry this review took so long.

>     { /* opt_CertCipher	       */ 'C', PR_TRUE,  0, PR_FALSE },
>-    { /* opt_KeyLength	       */ 'k', PR_TRUE,  0, PR_FALSE },
>-    { /* opt_CertKeyLength     */ 'K', PR_TRUE,  0, PR_FALSE }
>+    { /* opt_KeyLength         */ '0', PR_TRUE,  0, PR_FALSE, "key_len" },
>+    { /* opt_CertKeyLength     */ '0', PR_TRUE,  0, PR_FALSE, "cert_key_len" }
> };

This has the effect of defining both of these new options has being represented by
the same single character option character, namely the ASCII zero '0' character.
So, this patch does ensure that these two options don't duplicate the other existing
-k and -K options, but it has the effect of creating a new duplicate -0 option.

Note that it is possible for an option to have both a single character form 
and a multiple-character form.  This patch does just that for these two options.
If you want an option to have only the multiple character form, the single character
"flag" value must be binary zero (not ASCII zero digit).  It would be correct to 
specify simply  0,  or '\0', but '0' is the ASCII zero digit.

Having said all that, I see that this is pk12util, which (unlike certutil) is 
nowhere near running out of single-character options yet.  So, please give these
new options both unique single-character options and also multi-character options.
Comment 5 Elio Maldonado 2008-09-26 09:16:00 PDT
Created attachment 340590 [details] [diff] [review]
solve duplicate options with long and short options

Using -m and -n the as the one-letter versions of -key_len and cert_key_len long options, respectively.
Comment 6 Nelson Bolyard (seldom reads bugmail) 2008-09-26 09:58:28 PDT
Comment on attachment 340590 [details] [diff] [review]
solve duplicate options with long and short options

>+    FPS "\t\t [-c key_cipher] [-C cert_cipher] [m|--key_len keyLen] [n|--cert_key_len certKeyLen]\n");

Put a dash before the new m and n, and spaces around the | characters, e.g.
[-m | --key_len ...

Do that for both the new m and new n options.
Then r=nelson
Comment 7 Elio Maldonado 2008-09-26 10:47:48 PDT
Changes checked in. Changed: mozilla/security/nss/cmd/pk12util/pk12util.c
new revision: 1.40; previous revision: 1.39

Note You need to log in before you can comment on or make changes to this bug.