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:[email protected]]
> Sent: Wednesday, December 23, 2015 8:54 PM
> To: Apache Directory Developers List <[email protected]>
> 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)
