Closed Bug 863871 Opened 7 years ago Closed 7 years ago

Remove CVS keywords from NSS source files

Categories

(NSS :: Libraries, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: briansmith, Assigned: briansmith)

Details

Attachments

(4 files, 5 obsolete files)

1.63 KB, patch
elio.maldonado.batiz
: review+
wtc
: review+
Details | Diff | Splinter Review
250.36 KB, patch
wtc
: review+
Details | Diff | Splinter Review
7.54 KB, patch
wtc
: review+
briansmith
: checked-in+
Details | Diff | Splinter Review
2.64 KB, patch
wtc
: review+
Details | Diff | Splinter Review
Since we switched to Mercurial, it doesn't make sense to have CVS keywords in the source code any more. Wan-Teh mentioned that they should be removed on the nss-dev list.

I removed the "-v" option to ssltap as part of doing this, since it output the CVS ID.

I found one issue with this work that I did not address: certdata.perl will embed the CVS ID information into the generated PKCS#11 module, and that data is exposed at runtime by the PKCS#11 module:


  if( /(^CVS_ID\s+)(.*)/ ) {
    $cvsid = $2 . "\"; $cvs_id\"";
    my $scratch = $cvsid;
    $size = 1 + $scratch =~ s/[^"\n]//g;
    @{$objects[0][0]} = ( "CKA_CLASS", "&cko_data", "sizeof(CK_OBJECT_CLASS)" );
    @{$objects[0][1]} = ( "CKA_TOKEN", "&ck_true", "sizeof(CK_BBOOL)" );
    @{$objects[0][2]} = ( "CKA_PRIVATE", "&ck_false", "sizeof(CK_BBOOL)" );
    @{$objects[0][3]} = ( "CKA_MODIFIABLE", "&ck_false", "sizeof(CK_BBOOL)" );
    @{$objects[0][4]} = ( "CKA_LABEL", "\"CVS ID\"", "7" );
    @{$objects[0][5]} = ( "CKA_APPLICATION", "\"NSS\"", "4" );
    @{$objects[0][6]} = ( "CKA_VALUE", $cvsid, "$size" );
    $objsize[0] = 7;
    next;
  }
Elio, I think you are the best reviewer because packagers like you are most likely to be affected by these changes, though I don't know of any negative effects.
Attachment #739802 - Flags: review?(emaldona)
Comment on attachment 739803 [details] [diff] [review]
Part 2: Remove ssltap's -v (version) option since it is based on CVS keywords

r=wtc.

If no other NSS tool has the -v (version) option, it's fine to remove
it.
Attachment #739803 - Flags: review+
Brian:

I reviewed your Part 1 patch. Some files in the following directories
need to be manually adjusted to avoid leaving behind empty comment
lines or even empty comment blocks:
lib/certdb
lib/certhigh
lib/freebl
lib/freebl/mpi
lib/pkcs7
lib/smime
lib/softoken/legacydb
lib/util

I reverted the changes to third-party code in lib/sqlite and lib/zlib.
I believe the CVS keywords in those files come from upstream.

The CVS keyword in lib/jar/jzconf.h may have also come from zlib
upstream, but since we have forked that file, I think it's fine to
remove the CVS keyword from that file.

https://hg.mozilla.org/projects/nss/rev/9483ef9455fe
Attachment #739802 - Attachment is obsolete: true
Attachment #739802 - Flags: review?(emaldona)
Attachment #742068 - Flags: review+
Attachment #742068 - Flags: checked-in+
Comment on attachment 739803 [details] [diff] [review]
Part 2: Remove ssltap's -v (version) option since it is based on CVS keywords

https://hg.mozilla.org/projects/nss/rev/cd2ba3fec0ce

Brian: if you don't have any other patches, please mark this bug fixed.
Thanks.
Attachment #739803 - Flags: checked-in+
Comment on attachment 739803 [details] [diff] [review]
Part 2: Remove ssltap's -v (version) option since it is based on CVS keywords

r+, i didn't find any dependencies that the tools would have on this.
Attachment #739803 - Flags: review?(emaldona) → review+
This removes the generation of the CVS_ID component of the NSS builtin token.

Unfortunately, because the CVS_ID component was at the start of the array, the diff between the old generated certdata.c and the new generated certdata.c is very noisy, as every variable has its index in its name, and so all variables end up getting renamed.

Note I think there was a bug in the original code, where if CVS_ID wasn't present in the input, then things would fail to work.
Attachment #745465 - Flags: review?(wtc)
Comment on attachment 745465 [details] [diff] [review]
Part 3: Remove CVS ID from generated code

Review of attachment 745465 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for writing the patch.

There is an off-by-one error in certdata.perl. You can see
that if you check the last array element in the generated
certdata.c file.

::: lib/ckfw/builtins/certdata.perl
@@ +6,4 @@
>  use strict;
>  
>  my %constants;
> +my $count = -1;

Why do you initialize $count to -1?

I guess this change is required for $count to start with 0
instead of 1?

@@ +92,5 @@
>  doprint();
>  
>  sub dudump {
>  my $i;
> +for( $i = 0; $i < $count; $i++ ) {

Why do you change $i <= $count to $i < $count?

I think this caused the off-by-one error.

In the generated certdata.c, the last array element is now
nss_builtins_types_373.  nss_builtins_types_373 is the certificate
data of "CA Disig Root R2".  This means certdata.c is missing the
trust settings of "CA Disig Root R2".

@@ +176,5 @@
>  
>  print "};\n";
>  
>  print "const PRUint32\n";
> +print "nss_builtins_nObjects = $count;\n";

This should be $count+1, if the for loops test the condition
$i <= $count.
Attachment #745465 - Flags: review?(wtc) → review-
Attachment #746145 - Attachment description: Path that minimizes the diff in the generated output by keeping the same (but misleading) variable names. → Patch that minimizes the diff in the generated output by keeping the same (but misleading) variable names.
Thanks for catching my error.

The use of $count in this program is confusing. At the beginning of the program, it holds the number of objects (0). But, most of the time, it holds the index of the current item, which is one less than the number of objects. That is, most of the time, $count is not really the count of the number of objects. This is what caused me to create the off-by-one error.

I have rewritten the code to store the current index is stored in one variable while the count of items is stored in a separate variable. This way, $count now always refers to the number of items in the arrays. This is much more clear.

attachment 746145 [details] [diff] [review] is a variant of the patch where the generated variable names are kept the same as they were before my patch. This allows you to generate the diff in attachment 746147 [details] [diff] [review], showing that the changes I made are correctly. The bad aspect of this patch is that nss_builtins_data[i] refers to nss_builtins_types_(i+1) and nss_builtins_items_(i+1).

The actual patch to review is attachment 746147 [details] [diff] [review]. It is the same as attachment 746145 [details] [diff] [review] except that it does not do extra work to maintain the same variable names that were generated in previous versions. This is clearer because now nss_builtins_data[i] refers to nss_builtins_types_i and nss_builtins_items_i+1 like before. But, the diff of this version of the patch is basically unreadable.
Attachment #746155 - Flags: review?(wtc)
Attachment #745465 - Attachment is obsolete: true
Comment on attachment 746155 [details] [diff] [review]
Part 3 [v3]: Remove CVS ID from generated code

Review of attachment 746155 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ckfw/builtins/certdata.perl
@@ +91,3 @@
>  
>   # print "$fields[0] | $fields[1] | $size | $fields[2]\n";
>  }

I have no problem with for loops that check $i <= $count.

If you prefer to check $i < $count, we can accomplish that easily
by incrementing $count immediately after this while loop.

Right now we increment $count whenever we see the CKA_CLASS line.
So my proposed change simulates a fictitious CKA_CLASS line beyond
the end of the certdata.txt file.

Using the same variable as both an index and a count is quite
common. With a zero-based index, for the count to be correct,
the variable needs to be incremented *after* processing each item.
certdata.txt only gives us a simple way to detect the beginning
of each item, so we have to use this workaround.
Target Milestone: --- → 3.15
Wan-teh, I do not think we should spend too much more time on this. I updated the patch to keep it simple: it now clearly just removes everything related to the 0th object, which was the CVS ID. $count always contains the correct count and no extra counters are used.
Attachment #746145 - Attachment is obsolete: true
Attachment #746155 - Attachment is obsolete: true
Attachment #746155 - Flags: review?(wtc)
Attachment #750617 - Flags: review?(wtc)
Attachment #750621 - Flags: review+
Comment on attachment 750617 [details] [diff] [review]
Part 3 [v4]: Remove CVS ID from generated code

Review of attachment 750617 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc.
Attachment #750617 - Flags: review?(wtc) → review+
Comment on attachment 750617 [details] [diff] [review]
Part 3 [v4]: Remove CVS ID from generated code

https://hg.mozilla.org/projects/nss/rev/c63c14c95855

Thanks for the review, Wan-Teh (and sorry about the typo in my previous comment.)
Attachment #750617 - Flags: checked-in+
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Priority: -- → P2
You need to log in before you can comment on or make changes to this bug.