1:42 p.m.
Please review the patch for trac ticket 217.
Added an additional check over the actual result of a revoke/unrevoke
operation.
--Abhishek
2:41 p.m.
On 4/19/2013 1:42 PM, Abhishek Koneru wrote:
Please review the patch for trac ticket 217.
Added an additional check over the actual result of a revoke/unrevoke
operation.
Some comments:
1. In CertRequestInfo the synchronized keywords don't seem to be
necessary. CertRequestInfo objects are created to pass REST operation
results to the client. They don't seem to be used by multiple threads.
2. In CertRequestInfoFactory the operationResult uses 'pass/fail' values
which are duplicated in several places. It might be better to use
'success/error' to match RES_SUCCESS and RES_ERROR constants, and these
values can be put in constant variables in CertRequestInfo:
public static final String REQ_SUCCESS = "success";
public static final String REQ_ERROR = "error";
This way we can be sure the operationResult will be used consistently.
3. In CertRequestInfoFactory the code for operationResult assignment
seems to be incorrect because it will assign 'fail' to completed
requests without error. Try cert-request-find, it will show all requests
for system certs as failed.
4. In CertCLI if the request status is COMPLETE but the operation result
is 'fail' the status will appear as REJECTED. This could be confusing
because the request is not actually rejected. I think we should display
the operation result separately from the status.
--
Endi S. Dewata
1:19 a.m.
New subject: [PATCH] 53-2 Fixes for comments for patch 53
Please review the patch with fixes for review comments on patch 53.
On Mon, 2013-04-22 at 14:41 -0500, Endi Sukma Dewata wrote:
On 4/19/2013 1:42 PM, Abhishek Koneru wrote:
> Please review the patch for trac ticket 217.
>
> Added an additional check over the actual result of a revoke/unrevoke
> operation.
Some comments:
1. In CertRequestInfo the synchronized keywords don't seem to be
necessary. CertRequestInfo objects are created to pass REST operation
results to the client. They don't seem to be used by multiple threads.
--Removed the synchronized keywords
2. In CertRequestInfoFactory the operationResult uses
'pass/fail' values
which are duplicated in several places. It might be better to use
'success/error' to match RES_SUCCESS and RES_ERROR constants, and these
values can be put in constant variables in CertRequestInfo:
public static final String REQ_SUCCESS = "success";
public static final String REQ_ERROR = "error";
This way we can be sure the operationResult will be used consistently.
-- Added the constant variables in CertRequestInfo
3. In CertRequestInfoFactory the code for operationResult assignment
seems to be incorrect because it will assign 'fail' to completed
requests without error. Try cert-request-find, it will show all requests
for system certs as failed.
-- Corrected. Null value for result is given as a SUCCESS
4. In CertCLI if the request status is COMPLETE but the operation
result
is 'fail' the status will appear as REJECTED. This could be confusing
because the request is not actually rejected. I think we should display
the operation result separately from the status.
-- Printing the operation result seperately.
--Abhishek
2:53 p.m.
New subject: [PATCH] 53-2 Fixes for comments for patch 53
Some additional comments below.
On 4/23/2013 1:19 AM, Abhishek Koneru wrote:
> 2. In CertRequestInfoFactory the operationResult uses
'pass/fail' values
> which are duplicated in several places. It might be better to use
> 'success/error' to match RES_SUCCESS and RES_ERROR constants, and these
> values can be put in constant variables in CertRequestInfo:
>
> public static final String REQ_SUCCESS = "success";
> public static final String REQ_ERROR = "error";
>
> This way we can be sure the operationResult will be used consistently.
>
-- Added the constant variables in CertRequestInfo
Actually they should use RES_ prefix instead of REQ_ to match
IRequest.RES_SUCCESS/ERROR since it's for 'result', or don't use prefix
at all.
Also, the new constants use uppercase values "SUCCESS/ERROR", but the
the request status is in lower case. It would be more consistent to use
lowercase values too for the result.
Request ID: 5
Type: enrollment
Request Status: complete
Operation Result: SUCCESS <-- use lower case
Certificate ID: 0x5
> 3. In CertRequestInfoFactory the code for operationResult
assignment
> seems to be incorrect because it will assign 'fail' to completed
> requests without error. Try cert-request-find, it will show all requests
> for system certs as failed.
>
-- Corrected. Null value for result is given as a SUCCESS
The code can be simplified as follows:
if (result == null || result.equals(IRequest.RES_SUCCESS)) {
info.setOperationResult(CertRequestInfo.RES_SUCCESS);
} else {
info.setOperationResult(CertRequestInfo.RES_ERROR);
String error = request.getExtDataInString(IRequest.ERROR);
info.setErrorMessage(error); <-- no need to check for null
}
Everything else is good. ACK.
--
Endi S. Dewata
3:15 p.m.
New subject: [PATCH] 53-2 Fixes for comments for patch 53
Added the latest suggestion. Pushed to master.
--Abhishek
On Tue, 2013-04-23 at 14:53 -0500, Endi Sukma Dewata wrote:
Some additional comments below.
On 4/23/2013 1:19 AM, Abhishek Koneru wrote:
>> 2. In CertRequestInfoFactory the operationResult uses 'pass/fail'
values
>> which are duplicated in several places. It might be better to use
>> 'success/error' to match RES_SUCCESS and RES_ERROR constants, and these
>> values can be put in constant variables in CertRequestInfo:
>>
>> public static final String REQ_SUCCESS = "success";
>> public static final String REQ_ERROR = "error";
>>
>> This way we can be sure the operationResult will be used consistently.
>>
> -- Added the constant variables in CertRequestInfo
Actually they should use RES_ prefix instead of REQ_ to match
IRequest.RES_SUCCESS/ERROR since it's for 'result', or don't use prefix
at all.
Also, the new constants use uppercase values
"SUCCESS/ERROR", but the
the request status is in lower case. It would be more consistent to use
lowercase values too for the result.
Request ID: 5
Type: enrollment
Request Status: complete
Operation Result: SUCCESS <-- use lower case
Certificate ID: 0x5
-- Changed as mentioned.
>> 3. In CertRequestInfoFactory the code for operationResult
assignment
>> seems to be incorrect because it will assign 'fail' to completed
>> requests without error. Try cert-request-find, it will show all requests
>> for system certs as failed.
>>
> -- Corrected. Null value for result is given as a SUCCESS
The code can be simplified as follows:
if (result == null || result.equals(IRequest.RES_SUCCESS)) {
info.setOperationResult(CertRequestInfo.RES_SUCCESS);
} else {
info.setOperationResult(CertRequestInfo.RES_ERROR);
String error = request.getExtDataInString(IRequest.ERROR);
info.setErrorMessage(error); <-- no need to check for null
}
-- Changed as suggested.
Everything else is good. ACK.
4265
days inactive
4269
days old
4 comments
2 participants
participants (2)
-
Abhishek Koneru -
Endi Sukma Dewata