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

[John Magne]

Endi thanks for the comments, resolutions below:




----- Original Message -----
> From: "Endi Sukma Dewata" <edewata at redhat.com>
> To: "John Magne" <jmagne at redhat.com>, "pki-devel" <pki-devel at redhat.com>
> Sent: Tuesday, March 4, 2014 9:36:52 AM
> Subject: Re: [Pki-devel] [PATCH] 0001-First-cut-at-Java-TPS-Buffer-class-and-APDU-class.patch
> 
> Some comments for patch #2:
> 


Done. Sorry, totally spaced building by script.


> 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


Done, resolved by moving some things around.

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


Done.


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



Decided to take advice to leave and rename as TPSEngine, right now their
are only defines, but later will be functionality used by the server.

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


Done, sorry some of them simply escaped detection, they are elusive :)


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



Done.
> 
> 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.
> 


Done. Pretty sure I got all the examples of this.


> 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
> 
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Further-TPS-Rewrite-Requested-Review-Changes.patch
Type: text/x-patch
Size: 88204 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20140304/8a26c4f4/attachment.bin>