directory-kerby mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Emmanuel Lécharny <elecha...@gmail.com>
Subject Re: [kerby] ASN1 Quick review
Date Thu, 24 Dec 2015 09:10:20 GMT
Le 24/12/15 01:31, Zheng, Kai a écrit :
> Thanks Emmanuel for the review and great comments! The questions are hard but fortunately
I'm still kept in the loop so my pleasure to address them. My comments are embedded and marked
by [Kai].
>
> -----Original Message-----
> From: Emmanuel Lécharny [mailto:elecharny@gmail.com] 
> Sent: Wednesday, December 23, 2015 8:54 PM
> To: Apache Directory Developers List <dev@directory.apache.org>
> Subject: [kerby] ASN1 Quick review
>
> Hi band,
>
> I'm having a quick look at the kerby-asn1 code, as I wanted to paly around the idea of
porting the LDAP codec to use this piece of nice code. AFAIU, when you want to declare an
object that can be encoded or decoded, you have to extend the correct Asn1Object child. Looking
at the Ticket object, here is what I see :
>
> [Kai] Glad you want a try. This is also something I wish to help with as discussed before,
but am not able to do it immediately because of bandwidth. Sorry for that. I certainly would
and will also be able to try to provide any help if needed.
>
> ...
> import static org.apache.kerby.kerberos.kerb.type.ticket.Ticket.MyEnum.*;
> ...
> public class Ticket extends KrbAppSequenceType {
>     public static final int TKT_KVNO = KrbConstant.KRB_V5;
>     public static final int TAG = 1;
>
>     protected enum MyEnum implements EnumType {
>         TKT_VNO,
>         REALM,
>         SNAME,
>         ENC_PART;
>
>         @Override
>         public int getValue() {
>             return ordinal();
>         }
>
>         @Override
>         public String getName() {
>             return name();
>         }
>     }
>
>     static Asn1FieldInfo[] fieldInfos = new Asn1FieldInfo[] {
>             new ExplicitField(TKT_VNO, 0, Asn1Integer.class),
>             new ExplicitField(REALM, 1, KerberosString.class),
>             new ExplicitField(SNAME, 2, PrincipalName.class),
>             new ExplicitField(ENC_PART, 3, EncryptedData.class)
>     };
>
> I like the idea of defining the fields this way, except that I would suggest some slight
modifications :
>
> - get read of the import ...Ticket.MyEnum.*;
> [Kai] Hmmm, I guess you mean "get rid of", good point. It's like this because, initially
we didn't use enum, but int constants. And some weeks ago when we want to dump user defined
type objects like this with meaningful field info, we switched to use enum because it can
give a friendly name. We were in a hurry at that time and wanted to do it as less effort as
possible, thus it's like this now: the enum constant is rather like int constant and used
as before.

Okie, so I guess you don't have any provision against the suggested
modification.
>
> - make the enum private (there is no reason we wuld like to expose it anyway)
> [Kai] Well, actually there are some reasons to make it protected and disciplined exposed.
Such user defined types can be extended and these fields may be accessed there. Ref. child
classes of ContentInfo (also some other examples I remembered when defining protected int
constants).

Okie for 'protected'.

>
> - name it something more meaningful, like TicketField inseatd if MyEnum
> [Kai] Right agree. Again it was like this because it's out as a simple pattern in a whole
project scope replacement.

Okie. A rename is then possible.

>
> - use it with the enum name, like in new ExplicitField(TicketField.TKT_VNO, 0, Asn1Integer.class)
> [Kai] Agree.
>
> <side note>
> I find the "import static xxx.*;" atrocious, most of the time. It makes it barely readable
: you have to go to the top of your file to actually
> *know* where the constant comes from... That may seems of for small files, but otherwise...
I accept it for Asserts, because it's really clear, in a test context - pretty much like annotations.
> </side note>
> [Kai] I thought you may be right, but ...
>
> Here is what I come with :
>
> public class Ticket extends KrbAppSequenceType {
>     public static final int TKT_KVNO = KrbConstant.KRB_V5;
>     public static final int TAG = 1;
>
>     private enum TicketField implements EnumType {
>         TKT_VNO,
>         REALM,
>         SNAME,
>         ENC_PART;
>
>         @Override
>         public int getValue() {
>             return ordinal();
>         }
>
>         @Override
>         public String getName() {
>             return name();
>         }
>     }
>
>     static Asn1FieldInfo[] fieldInfos = new Asn1FieldInfo[] {
>             new ExplicitField(TicketField.TKT_VNO, 0, Asn1Integer.class),
>             new ExplicitField(TicketField.REALM, 1, KerberosString.class),
>             new ExplicitField(TicketField.SNAME, 2, PrincipalName.class),
>             new ExplicitField(TicketField.ENC_PART, 3, EncryptedData.class)
>     };
>
> (note that it's just a suggestion at this point...)
> [Kai] They're very good suggestions and should become true, though a tiresome work because
there are so many user defined types now. I'm wondering if any contributor would help with
such. 

I can do that. This is typically the kind of (boring) task that I like
to do when I have little brain cycles (like when it's late, or when I
don't have energy to focus on serious code : doing boring things make me
feel I'm still working and contributing to the community).
>
> Now, let's foduc on the fields declarations. Here, we create ExplicitFields, passing
a Xxx.class as the third parameter, and a number as the second parameter.
>
> First of all, as the enum being used has an implicit ordering, can't we simply avoid
passing those numbers ? There is already an ExplicitField constructor that takes only 2 parameters,
deducing the number from teh enum...
> [Kai] Yeah sure we can just use the 2 parameters constructor for this type. You're good
at finding this as the example. We relied on the enum order value mostly but this one and
possible many slipped out.

Okie. Another boring task ;-)

>
> Second, why don't we write things like :
>
>
>             new ExplicitField(TicketField.TKT_VNO, 0, new Asn1Integer()),
>
> instead of passing a class ? Bottom line, the instance will be created the same way.
I don't think it will make any difference in term of performances, and as all the object *must*
extends the Asn1Object (or implement the Asn1Type), this should be good enough.
> [Kai] Well, let the framework determine when to create the field instances would be most
flexible. We may want to be lazy on creating them and create them on demand; we may need to
avoid creating them in decoding because they're nulls; in the framework codes there're some
places that uses (value == null) to check some conditions. For Any, the actual value type
can only be set by applications in specific contexts.

Makes perfect sense.

Side note : I wonder if calling newInstance(class) is really slower that
doing new Class at this point... It's not anymore the case, I think (it
was before Java 6)



Mime
View raw message