[Pki-devel] [PATCH] 0001-First-cut-at-Java-TPS-Buffer-class-and-APDU-class.patch

[Endi Sukma Dewata]

Some comments for patch #2:

1. Build failed due to uncompiled files. See javac command in 
base/common/src/CMakeLists.txt. The new namespace should be added into 
the javac command (sorry, I forgot to tell you about this).

   javac(pki-certsrv-classes
       SOURCES
           com/netscape/certsrv/*.java
           org/dogtagpki/*.java

or it probably can be simplified as this:

   javac(pki-certsrv-classes
       SOURCES
           *.java

And same thing with the packaging:

   jar(pki-certsrv-jar
       FILES
           *.class

2. Once issue #1 is fixed, there seem to be some dependency issues. The 
packages are built in this order:
  - base/common (client/common classes)
  - base/server/cms (server classes)
  - base/server/cmscore (legacy private server classes)
  - base/<subsystem: ca,kra,ocsp,tks,tps-tomcat>
A class cannot depend on another class in a lower package. So some 
classes may need to be refactored or shuffled around.

3. If TPSSession can only be used by the server, it should remain in 
base/tps-tomcat. If this is something a client might use, the dependency 
should be resolved and the package should be changed into 
org.dogtagpki.tps instead of org.dogtagpki.server.tps.

4. The TPSServlet probably should remain in base/tps-tomcat because it's 
a server class.

5. The org.dogtagpki.server.tps.engine.TPS class defines many constants. 
If the constants are used by clients the class probably should be moved 
to org.dogtagpki.tps.TPS. If the constants are used by server only, it 
probably should be moved to org.dogtagpki.server.tps.TPSServer, or maybe 
merged with TPSSubsystem. But if you'd like to keep it where it is now, 
it's probably better to call it TPSEngine. The "TPS" name should be 
reserved for namespace (e.g. defining constants, static methods).

6. There are some methods that start with uppercase:
  - APDU.Set*()
  - TPSFormatProcessor.Process()
Generally Java methods start with lowercase.

7. An enumeration is used like a class, so it should follow class naming 
convention. So the TPSProcessor.TPS_Status probably shouldn't have an 
underscore in its name.

8. There are some classes with attributes defined at the bottom of the 
class. Usually a class is organized as follows:

   public class Something {
      // constants
      // attributes
      // constructors
      // methods
   }

-- 
Endi S. Dewata


On 2/28/2014 10:00 PM, John Magne wrote:
> 0002-TPS-Rewrite-Requested-Review-Changes.patch
>
>
> TPS Rewrite Requested Review Changes:
>
>      1. Change the location of some of the classes.
>      2. Change the file names to reflect naming convention.
>      3. Change some of the method names to reflect convention.
>      4. Variable naming changes to reflect convention.
>
>
>
>
>
> ----- Original Message -----
> From: "John Magne" <jmagne at redhat.com>
> To: "pki-devel" <pki-devel at redhat.com>
> Sent: Wednesday, February 26, 2014 7:22:05 PM
> Subject: [Pki-devel] [PATCH] 0001-First-cut-at-Java-TPS-Buffer-class-and-APDU-class.patch
>
> First cut at Java TPS Buffer class and APDU classes.
>
> 1. Also simple framework for working with APDU commands.
> 2. Implemented a few APDU commands in TPS_Processor class.
> 3. Can now attempt a format operation with TPS client.
>     The code can perform a few apdu's talking to the client
>     and return a success "EndOp" apdu to terminate the conversation.
> 4. APDU are being encoded/decoded properly to appease tpsclient.
>
> More info.
>
> 1. Patch is large but most of it consists of many similar apdu and msg classes.
> 2. APDU and msg classes are now bare bones and may need more work. Will address when class is needed.
> 3. A test tpsclient script call it (format.tst) to test this out is as follows:
>
> op=var_set name=ra_host value=localhost
> op=var_set name=ra_port value=8080
> op=var_set name=ra_uri value=/tps/tps
> op=token_set cuid=40906145C76224192D2B msn=0120304 app_ver=6FBBC105 key_info=0101 major_ver=1 minor_ver=1
> op=token_set auth_key=404142434445464748494a4b4c4d4e4f
> op=token_set mac_key=404142434445464748494a4b4c4d4e4f
> op=token_set kek_key=404142434445464748494a4b4c4d4e4f
> op=ra_format uid=jmagne pwd=redhat new_pin=rehat num_threads=1
> op=exit
>
> 4: Execute as follows:
>
> tpsclient < format.tst