I find it easier to just apply the patches to the tree to have a better context.  Some of the things I've found may be from the past.

* TPSEngine -
  - you have defined a number TKS_RESPONSE_xxx constants which I have defined in IRemoteRequest.java.  I think IRemoteRequest.java is a better place since I have TKS server side using those constants as well.  If agreed, how about removing those 6 constants from TPSEngine?  I checked and i see in your code you are not using them yet anyway, so it should not be too much trouble.

* TPSFormatProcessor
 - In our old tps, I have always questioned the existance of the RA_Format_Processor as well as the other xxx_Processor's other than the RA_Processor.cpp and  RA_Enroll_Processor.cpp where the real code were in.
  My question is, is there going to be an attempt of putting real format code into the TPSFormatProcessor, etc, or we are merely going to do a direct conversion of C++ to Java?  The reason why I ask is because the TPSFormatProcessor and all probably serve not much purpose unless we put something useful in them.  We can probably leave them as is for now and think about it later.  I just want to bring it up so we can keep it in mind to ponder on.

* Would it make sense for APDUResponse to carry a boolean status so that checkTokenPDUResponse() only needs to be processed once and status set at the time?  Anyway, if there is concern of veering off to far from the old tps course then we can keep the return rc style.

* I don' t see in the format() where you pass in the skipAuth value.

* I may have missed it, but I don't see you checking if cplc_data < 47 (a magic number in old tps).

* why do you change the variable name token_status to status?  I think token_status, or if you wish, tokenStatus, is more descriptive.

* why do you check if (session == null) in the middle of assigning major and minor versions?  Could you not have checked this earlier, like near the beginning of format()?

* the old tps might be wrong, but I just want to know why is it that it would assign 0 to all those versions if token_status == NULL, while you bumped it out?

* perhaps we want to consider keeping the tokenType somewhere we can retrieve (not re-calculated and re-retrieved) easily during a session, instead of passing it around as a parameter.  Where is a good place to do that?

that's it for now.
Christina

On 03/13/2014 04:29 PM, John Magne wrote:
Ticket #895 https://fedorahosted.org/pki/ticket/895

 Further work on TPS Processor, format operation.

This patch gets a bit farther on the TPS format operation, just before applet upgrade, which will also need secure channel functionality.

Also, patch provides some misc clean up and functionality.

1. Method to calculate the token type.
2. Some added convenience methods to get various config params for the Format operation.
3. More progress for the format operation up until we attempt to upgrade the applet.
4. Added TPSException that holds a message and end op return code. Can be used to throw from anywhere and the return code makes it back to the client.


---



_______________________________________________
Pki-devel mailing list
Pki-devel@redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel