directory-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ayyagarikiran <ayyagariki...@gmail.com>
Subject Re: About the Cert generation Extended request
Date Mon, 05 Jan 2009 13:41:56 GMT

hi Emmanuel,

Emmanuel Lecharny wrote:
> Hi Kiran,
> 
> you will be the first person able to add a new Extended request using 
> the ASN.1 codec !
> 
> Ok, it's really good. Just a few things
> - Where do you got the ASN.1 grammar from ? Is there a reference 
> somwhere on the web, or is this just soemthing you defined from scratch ?
yep, created on my own referring the existing code and the wiki doc
> 
> - There are some minor issues in the grammar class : when all the fields 
> are mandatory (ie, the OPTIONAL keyword does not appears), then you 
> should insure that the TLVs are not empty. Typically, if the sequence is 
> empty, you will get a PDU containing 0x30 0x00. Using your current 
> implementation, this will be accepted.
> The problem is that your first transition contains this line :
> 
>                    CertGenContainer.grammarEndAllowed( true );
ah ok, I didn't get this part but assumed this as a way to tell the decoder to continue to
the next transition, will fix it
> which allow the PDU to e terminated immediately.
> 
> In this case, you should just ommit this line, unless you want the 
> grammar to allow empty sequences. Here, I think that the only transition 
> where this line should appear is the last one : keyAlgorithm.
> 
> You should also check that if all the fields are not present, then it 
> generates an error (ie, adding a test for each bad field).
+1
> 
> - You are storing DN as String, but maybe it would be a better iead to 
> check that those DNs are valid. You can use the LdapDN.isValid( dn ) to 
> do so. Or you can store LdapDN instead. It's up to you.
> 
+1, think this makes more sense and I can throw error as early as possible

> - In tests, when expecting an exception, don't add a e.printStackTrace() 
> : it's a burden when running integration tests. Also don't use '*' in 
> imports.
> 

ah, my bad, will take them out ( was lazy to add each method of Assert to the static imports
;) )

> I just commited some fixes.

ah great, thanks
> 
> Otherwise, it's pretty clean. I didn't thought someone could understand 
> this portion of the code :) Thanks !
> 

  Thanks a lot for your time and comments, I appreciate them :)

Kiran Ayyagari

Mime
View raw message