You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@directory.apache.org by Emmanuel Lécharny <el...@gmail.com> on 2015/12/23 13:54:04 UTC
[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 :
...
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.*;
- make the enum private (there is no reason we wuld like to expose it
anyway)
- name it something more meaningful, like TicketField inseatd if MyEnum
- use it with the enum name, like in new
ExplicitField(TicketField.TKT_VNO, 0, Asn1Integer.class)
<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>
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...)
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...
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.
wdyt ?
RE: [kerby] ASN1 Quick review
Posted by "Zheng, Kai" <ka...@intel.com>.
Resend after formatted.
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 <de...@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.
- 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).
- 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.
- 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.
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.
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.
wdyt ?
RE: [kerby] ASN1 Quick review
Posted by "Zheng, Kai" <ka...@intel.com>.
Thanks Yan for the hard taking! Note it means you would need to manually modify 200+ classes one by one since we'll need a friendly enum name.
Regards,
Kai
-----Original Message-----
From: Yan, Yan A [mailto:yan.a.yan@intel.com]
Sent: Thursday, December 24, 2015 9:13 AM
To: kerby@directory.apache.org; Apache Directory Developers List <de...@directory.apache.org>
Subject: RE: [kerby] ASN1 Quick review
Thanks for Emmanuel and Kai's discussion.
I'm wondering if any contributor would help with such.
I'd like to help with the issues, and by the way be familiar with the core part codes.
Best regards,
Yan
-----Original Message-----
From: Zheng, Kai [mailto:kai.zheng@intel.com]
Sent: Thursday, December 24, 2015 8:31 AM
To: Apache Directory Developers List <de...@directory.apache.org>; kerby@directory.apache.org
Subject: RE: [kerby] ASN1 Quick review
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 <de...@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.
- 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).
- 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.
- 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.
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.
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.
wdyt ?
RE: [kerby] ASN1 Quick review
Posted by "Zheng, Kai" <ka...@intel.com>.
Thanks Yan for the hard taking! Note it means you would need to manually modify 200+ classes one by one since we'll need a friendly enum name.
Regards,
Kai
-----Original Message-----
From: Yan, Yan A [mailto:yan.a.yan@intel.com]
Sent: Thursday, December 24, 2015 9:13 AM
To: kerby@directory.apache.org; Apache Directory Developers List <de...@directory.apache.org>
Subject: RE: [kerby] ASN1 Quick review
Thanks for Emmanuel and Kai's discussion.
I'm wondering if any contributor would help with such.
I'd like to help with the issues, and by the way be familiar with the core part codes.
Best regards,
Yan
-----Original Message-----
From: Zheng, Kai [mailto:kai.zheng@intel.com]
Sent: Thursday, December 24, 2015 8:31 AM
To: Apache Directory Developers List <de...@directory.apache.org>; kerby@directory.apache.org
Subject: RE: [kerby] ASN1 Quick review
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 <de...@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.
- 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).
- 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.
- 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.
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.
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.
wdyt ?
RE: [kerby] ASN1 Quick review
Posted by "Yan, Yan A" <ya...@intel.com>.
Thanks for Emmanuel and Kai's discussion.
I'm wondering if any contributor would help with such.
I'd like to help with the issues, and by the way be familiar with the core part codes.
Best regards,
Yan
-----Original Message-----
From: Zheng, Kai [mailto:kai.zheng@intel.com]
Sent: Thursday, December 24, 2015 8:31 AM
To: Apache Directory Developers List <de...@directory.apache.org>; kerby@directory.apache.org
Subject: RE: [kerby] ASN1 Quick review
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 <de...@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.
- 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).
- 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.
- 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.
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.
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.
wdyt ?
RE: [kerby] ASN1 Quick review
Posted by "Zheng, Kai" <ka...@intel.com>.
Yeah, I can see that it's a good try. Sometime when I work on the compiler or more accurately, code generator, I can learn about the ASN1 parser and borrow some idea. Thanks!
-----Original Message-----
From: Emmanuel Lécharny [mailto:elecharny@gmail.com]
Sent: Thursday, December 24, 2015 9:37 PM
To: kerby@directory.apache.org
Subject: Re: [kerby] ASN1 Quick review
Le 24/12/15 14:14, Zheng, Kai a écrit :
> Ah it happens so fast and thank you so much. I wish we could have a compiler that accepts the ASN1 module definition files and then help generate/update these classes. It's one thing I want to develop but had better focus on our Kerberos things first. Thanks again.
I have started that years ago. Never had time to go very far :
http://svn.apache.org/repos/asf/labs/dungeon/
>
> I noted our new contributor Yan said she would help with the boring task. Maybe the left can be for her and you take a good rest? She needs to be familiar with such codes anyway.
Your call !
i'll be out anyway for the day, so feel free to task her ;-)
Re: [kerby] ASN1 Quick review
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 24/12/15 14:14, Zheng, Kai a écrit :
> Ah it happens so fast and thank you so much. I wish we could have a compiler that accepts the ASN1 module definition files and then help generate/update these classes. It's one thing I want to develop but had better focus on our Kerberos things first. Thanks again.
I have started that years ago. Never had time to go very far :
http://svn.apache.org/repos/asf/labs/dungeon/
>
> I noted our new contributor Yan said she would help with the boring task. Maybe the left can be for her and you take a good rest? She needs to be familiar with such codes anyway.
Your call !
i'll be out anyway for the day, so feel free to task her ;-)
RE: [kerby] ASN1 Quick review
Posted by "Zheng, Kai" <ka...@intel.com>.
Ah it happens so fast and thank you so much. I wish we could have a compiler that accepts the ASN1 module definition files and then help generate/update these classes. It's one thing I want to develop but had better focus on our Kerberos things first. Thanks again.
I noted our new contributor Yan said she would help with the boring task. Maybe the left can be for her and you take a good rest? She needs to be familiar with such codes anyway.
Regards,
Kai
-----Original Message-----
From: Emmanuel Lécharny [mailto:elecharny@gmail.com]
Sent: Thursday, December 24, 2015 8:52 PM
To: kerby@directory.apache.org
Subject: Re: [kerby] ASN1 Quick review
Le 24/12/15 10:10, Emmanuel Lécharny a écrit :
> Le 24/12/15 01:31, Zheng, Kai a écrit :
> [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).
Just committed a change for almost half of the classes. Piece of cake, took me less then an hour...
The other half is all in the pkix project.
Re: [kerby] ASN1 Quick review
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 24/12/15 10:10, Emmanuel Lécharny a écrit :
> Le 24/12/15 01:31, Zheng, Kai a écrit :
> [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).
Just committed a change for almost half of the classes. Piece of cake,
took me less then an hour...
The other half is all in the pkix project.
Re: [kerby] ASN1 Quick review
Posted by Emmanuel Lécharny <el...@gmail.com>.
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 <de...@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)
RE: [kerby] ASN1 Quick review
Posted by "Yan, Yan A" <ya...@intel.com>.
Thanks for Emmanuel and Kai's discussion.
I'm wondering if any contributor would help with such.
I'd like to help with the issues, and by the way be familiar with the core part codes.
Best regards,
Yan
-----Original Message-----
From: Zheng, Kai [mailto:kai.zheng@intel.com]
Sent: Thursday, December 24, 2015 8:31 AM
To: Apache Directory Developers List <de...@directory.apache.org>; kerby@directory.apache.org
Subject: RE: [kerby] ASN1 Quick review
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 <de...@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.
- 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).
- 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.
- 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.
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.
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.
wdyt ?
RE: [kerby] ASN1 Quick review
Posted by "Zheng, Kai" <ka...@intel.com>.
Resend after formatted.
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 <de...@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.
- 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).
- 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.
- 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.
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.
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.
wdyt ?
RE: [kerby] ASN1 Quick review
Posted by "Zheng, Kai" <ka...@intel.com>.
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 <de...@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.
- 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).
- 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.
- 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.
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.
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.
wdyt ?
RE: [kerby] ASN1 Quick review
Posted by "Zheng, Kai" <ka...@intel.com>.
The third approach looks good to me, we might probably prototype it in a branch and see the effect. The priority to me would be low, if the result looks nice then we can target it for 1.0.0 or later?
Regards,
Kai
-----Original Message-----
From: Emmanuel Lécharny [mailto:elecharny@gmail.com]
Sent: Friday, December 25, 2015 12:06 AM
To: kerby@directory.apache.org
Subject: Re: [kerby] ASN1 Quick review
Ah, crap I should have posted this mail to the Kerby mailing list...
Will do that starting from now.
Le 24/12/15 14:07, Zheng, Kai a écrit :
> Nice comments! Let me focus on something to explain.
>
> Regarding merge AbstractAsn1Type and Asn1Encodeable. As I said before, the former wraps a Java value, the latter does the encoding/decoding dirty work. The separation may does no good so far, but would also do no hurt for the left. Something may start with the latter and doesn't like the generic thing. How about if we make the parser results encodeable/decodeable? On other Java platform that doesn't support the generic thing very well? How about if we bridge this library to other ASN1 stuffs? Yes as you said these are just maybe, uncertain things. But if the merging doesn't generate some good for now, I thought it may be not bad to go the way.
>
> I guess you're mentioning two approaches: either let the ASN1 objects do encode/decode themselves, or have some help class to delegate the dirty work out. Yeah this is an architecture decision that's done when coming up the first piece of the codes. I remembered I tried many times, but never got a way that works perfect in every aspect. We used the former approach that goes not bad so far if we don't attempt to support many other coding rules and do BER/DER as good as possible.
I see. It would be a real pain to have a class having a encodeBer(), encodeDer(), encodeWTFer()... methods into it.
May I suggest a third approach ? The Visitor pattern might be a good candidate here : https://en.wikipedia.org/wiki/Visitor_pattern
The idea is to delegate the encoding to a visitor, that will use the right encoding type. We decouple the encoding/decoding from the value classes. It adds a bit of complexity, but in our case, this might be the correct approach.
Re: [kerby] ASN1 Quick review
Posted by Emmanuel Lécharny <el...@gmail.com>.
Ah, crap I should have posted this mail to the Kerby mailing list...
Will do that starting from now.
Le 24/12/15 14:07, Zheng, Kai a écrit :
> Nice comments! Let me focus on something to explain.
>
> Regarding merge AbstractAsn1Type and Asn1Encodeable. As I said before, the former wraps a Java value, the latter does the encoding/decoding dirty work. The separation may does no good so far, but would also do no hurt for the left. Something may start with the latter and doesn't like the generic thing. How about if we make the parser results encodeable/decodeable? On other Java platform that doesn't support the generic thing very well? How about if we bridge this library to other ASN1 stuffs? Yes as you said these are just maybe, uncertain things. But if the merging doesn't generate some good for now, I thought it may be not bad to go the way.
>
> I guess you're mentioning two approaches: either let the ASN1 objects do encode/decode themselves, or have some help class to delegate the dirty work out. Yeah this is an architecture decision that's done when coming up the first piece of the codes. I remembered I tried many times, but never got a way that works perfect in every aspect. We used the former approach that goes not bad so far if we don't attempt to support many other coding rules and do BER/DER as good as possible.
I see. It would be a real pain to have a class having a encodeBer(),
encodeDer(), encodeWTFer()... methods into it.
May I suggest a third approach ? The Visitor pattern might be a good
candidate here : https://en.wikipedia.org/wiki/Visitor_pattern
The idea is to delegate the encoding to a visitor, that will use the
right encoding type. We decouple the encoding/decoding from the value
classes. It adds a bit of complexity, but in our case, this might be the
correct approach.
RE: [kerby] ASN1 Quick review
Posted by "Zheng, Kai" <ka...@intel.com>.
Nice comments! Let me focus on something to explain.
Regarding merge AbstractAsn1Type and Asn1Encodeable. As I said before, the former wraps a Java value, the latter does the encoding/decoding dirty work. The separation may does no good so far, but would also do no hurt for the left. Something may start with the latter and doesn't like the generic thing. How about if we make the parser results encodeable/decodeable? On other Java platform that doesn't support the generic thing very well? How about if we bridge this library to other ASN1 stuffs? Yes as you said these are just maybe, uncertain things. But if the merging doesn't generate some good for now, I thought it may be not bad to go the way.
I guess you're mentioning two approaches: either let the ASN1 objects do encode/decode themselves, or have some help class to delegate the dirty work out. Yeah this is an architecture decision that's done when coming up the first piece of the codes. I remembered I tried many times, but never got a way that works perfect in every aspect. We used the former approach that goes not bad so far if we don't attempt to support many other coding rules and do BER/DER as good as possible. But if we would do, then we may want to simplify the ASN1 objects and need to delegate the encoding/decoding things out to helper/facet classes. I don't investigate the latter direction very much, to be frankly. One thing in mind would be, let the library be driven by our applications. If it's required and needs to solve some new cases, then evolve the library to get the new cases done elegantly. I would admit that developing the library itself is much a burden for us because our limited effort should be focused on Kerberos specific, but as you see, we've spent very much time on this, though I believe it's worth because it can make Kerberos side easier with it.
Yeah Asn1Specifix should be Asn1Specific, good catch. Currently it's very simple and doesn't do much work. Will see if any further cases that require it to do more work. A reason for this is, the systems so far using the library are majorly type/model driven encoding/decoding from top to bottom, which makes the library behave very differently from others. The major work for application/context specific types are already handled elsewhere. We need it for Asn1.decodeAndDump() call.
Regards,
Kai
-----Original Message-----
From: Emmanuel Lécharny [mailto:elecharny@gmail.com]
Sent: Thursday, December 24, 2015 5:03 PM
To: kerby@directory.apache.org
Subject: Re: [kerby] ASN1 Quick review
Le 24/12/15 01:56, Zheng, Kai a écrit :
> Nice picture!! My comments are embedded marked as [Kai].
>
> -----Original Message-----
> From: Emmanuel Lécharny [mailto:elecharny@gmail.com]
> Sent: Thursday, December 24, 2015 1:29 AM
> To: Apache Directory Developers List <de...@directory.apache.org>
> Subject: Re: [kerby] ASN1 Quick review
>
> Some more questions, now that I have drawn the full Asn1 hierarchy
> (http://pasteboard.co/fGVRQFm.png)
> [Kai] Thanks a lot. Very cool! Could we integrate it in the library doc?
Of course !
Find the attached graphml file (the source from which I generated the picture, using yEd - https://www.yworks.com/products/yed)
>
> - can't we merge the Asn1Encodable and AbstractAsn1Type classes ?
> [Kai] I guess we can, actually before AbstractAsn1Type is very large. I would prefer to separate them becaue the roles of them are clearly different.
Separation of concern is actually better done with Interfaces.
> Asn1Encodable is responsible for all the encoding/decoding dirty works; and AbstractAsn1Type would wrap a Java type value using Java generic.
The real question here is more or less an architctural question. Let's consider that an object might have to support multiple 'facets' (as in
http://i.cs.hku.hk/~clwang/projects/facet.html)
- should we let a Class implement a 'facet' ?
- or should we delegate this facet to an helper class ?
For encodable, I would rather think that the help class approach would be way too complex, as each ASN1 class has its own implementation of some of teh Asn1Encodable classes. This make me think that the first approach is better.
Now, as all the Asn1 classes that extend AbstractAsn1Type already have their own implementations of methods like encodingLength(), I'd rather see the AbstractAsn1Type abstract class implement what is currently in the Asn1Encodable class, and make Asn1Encodable an Interface (becaise the other branch of Asn1Object aren't implementing it).
> For now all Asn1Type object are AbstractAsn1Type, but may be not in the future, because there are possible situations where some types don't want to start with AbstractAsn1Type using the generic things.
DO you have some types in mind ? Or is this just a door you want to maintain open, just in case ?
> I wish the library could evolve further so be able to standalone as a complete solution some day.
Sure, but I think it's preferable to have a clear vision and design at each step, which can evolve in the future.
>
> - same question for the Asn1Cnstructed and Asn1Collection classes
> [Kai] Again they were together before but separated out recently. Asn1Collection is purely for set/setoff/sequence/sequenceof. Asn1Constructed can be another case where primitive type but using constructed encoding. You could check the places where it's used. An example in Asn1Converter.
Here, that makes more sense.
>
> - I'm not sure the Asn1Dumpable interface makes a lot of sense. Why don't we simply implement a toString() method that takes an identation as a parameter ?
> [Kai] Right initially when implementing the dump support we went the way, we used a separate method toStr() but found it doesn't work nicely. toString() with a indent parameter looks anti-intuitive I guess. A dumpable can be passed a dumper as well and using the dumper it can share the same underlying String builder along with all kinds of utility functions. Note only complex and bridge types like Asn1CollecionType, Asn1Any and Asn1Container need to go the way, simple types use Java toString() naturally.
Forget what I suggested. It's clear that we *need* this interface, it forces the classes to implement the methods, otehrwise we might forget doing so, ending with an incomplete 'dump' feature.
Asn1Specifix (which name should probably be Asn1Specifics) should implement this interface, btw.
Thanks !
RE: [kerby] ASN1 Quick review
Posted by "Zheng, Kai" <ka...@intel.com>.
Nice comments! Let me focus on something to explain.
Regarding merge AbstractAsn1Type and Asn1Encodeable. As I said before, the former wraps a Java value, the latter does the encoding/decoding dirty work. The separation may does no good so far, but would also do no hurt for the left. Something may start with the latter and doesn't like the generic thing. How about if we make the parser results encodeable/decodeable? On other Java platform that doesn't support the generic thing very well? How about if we bridge this library to other ASN1 stuffs? Yes as you said these are just maybe, uncertain things. But if the merging doesn't generate some good for now, I thought it may be not bad to go the way.
I guess you're mentioning two approaches: either let the ASN1 objects do encode/decode themselves, or have some help class to delegate the dirty work out. Yeah this is an architecture decision that's done when coming up the first piece of the codes. I remembered I tried many times, but never got a way that works perfect in every aspect. We used the former approach that goes not bad so far if we don't attempt to support many other coding rules and do BER/DER as good as possible. But if we would do, then we may want to simplify the ASN1 objects and need to delegate the encoding/decoding things out to helper/facet classes. I don't investigate the latter direction very much, to be frankly. One thing in mind would be, let the library be driven by our applications. If it's required and needs to solve some new cases, then evolve the library to get the new cases done elegantly. I would admit that developing the library itself is much a burden for us because our limited effort should be focused on Kerberos specific, but as you see, we've spent very much time on this, though I believe it's worth because it can make Kerberos side easier with it.
Yeah Asn1Specifix should be Asn1Specific, good catch. Currently it's very simple and doesn't do much work. Will see if any further cases that require it to do more work. A reason for this is, the systems so far using the library are majorly type/model driven encoding/decoding from top to bottom, which makes the library behave very differently from others. The major work for application/context specific types are already handled elsewhere. We need it for Asn1.decodeAndDump() call.
Regards,
Kai
-----Original Message-----
From: Emmanuel Lécharny [mailto:elecharny@gmail.com]
Sent: Thursday, December 24, 2015 5:03 PM
To: kerby@directory.apache.org
Subject: Re: [kerby] ASN1 Quick review
Le 24/12/15 01:56, Zheng, Kai a écrit :
> Nice picture!! My comments are embedded marked as [Kai].
>
> -----Original Message-----
> From: Emmanuel Lécharny [mailto:elecharny@gmail.com]
> Sent: Thursday, December 24, 2015 1:29 AM
> To: Apache Directory Developers List <de...@directory.apache.org>
> Subject: Re: [kerby] ASN1 Quick review
>
> Some more questions, now that I have drawn the full Asn1 hierarchy
> (http://pasteboard.co/fGVRQFm.png)
> [Kai] Thanks a lot. Very cool! Could we integrate it in the library doc?
Of course !
Find the attached graphml file (the source from which I generated the picture, using yEd - https://www.yworks.com/products/yed)
>
> - can't we merge the Asn1Encodable and AbstractAsn1Type classes ?
> [Kai] I guess we can, actually before AbstractAsn1Type is very large. I would prefer to separate them becaue the roles of them are clearly different.
Separation of concern is actually better done with Interfaces.
> Asn1Encodable is responsible for all the encoding/decoding dirty works; and AbstractAsn1Type would wrap a Java type value using Java generic.
The real question here is more or less an architctural question. Let's consider that an object might have to support multiple 'facets' (as in
http://i.cs.hku.hk/~clwang/projects/facet.html)
- should we let a Class implement a 'facet' ?
- or should we delegate this facet to an helper class ?
For encodable, I would rather think that the help class approach would be way too complex, as each ASN1 class has its own implementation of some of teh Asn1Encodable classes. This make me think that the first approach is better.
Now, as all the Asn1 classes that extend AbstractAsn1Type already have their own implementations of methods like encodingLength(), I'd rather see the AbstractAsn1Type abstract class implement what is currently in the Asn1Encodable class, and make Asn1Encodable an Interface (becaise the other branch of Asn1Object aren't implementing it).
> For now all Asn1Type object are AbstractAsn1Type, but may be not in the future, because there are possible situations where some types don't want to start with AbstractAsn1Type using the generic things.
DO you have some types in mind ? Or is this just a door you want to maintain open, just in case ?
> I wish the library could evolve further so be able to standalone as a complete solution some day.
Sure, but I think it's preferable to have a clear vision and design at each step, which can evolve in the future.
>
> - same question for the Asn1Cnstructed and Asn1Collection classes
> [Kai] Again they were together before but separated out recently. Asn1Collection is purely for set/setoff/sequence/sequenceof. Asn1Constructed can be another case where primitive type but using constructed encoding. You could check the places where it's used. An example in Asn1Converter.
Here, that makes more sense.
>
> - I'm not sure the Asn1Dumpable interface makes a lot of sense. Why don't we simply implement a toString() method that takes an identation as a parameter ?
> [Kai] Right initially when implementing the dump support we went the way, we used a separate method toStr() but found it doesn't work nicely. toString() with a indent parameter looks anti-intuitive I guess. A dumpable can be passed a dumper as well and using the dumper it can share the same underlying String builder along with all kinds of utility functions. Note only complex and bridge types like Asn1CollecionType, Asn1Any and Asn1Container need to go the way, simple types use Java toString() naturally.
Forget what I suggested. It's clear that we *need* this interface, it forces the classes to implement the methods, otehrwise we might forget doing so, ending with an incomplete 'dump' feature.
Asn1Specifix (which name should probably be Asn1Specifics) should implement this interface, btw.
Thanks !
Re: [kerby] ASN1 Quick review
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 24/12/15 01:56, Zheng, Kai a écrit :
> Nice picture!! My comments are embedded marked as [Kai].
>
> -----Original Message-----
> From: Emmanuel Lécharny [mailto:elecharny@gmail.com]
> Sent: Thursday, December 24, 2015 1:29 AM
> To: Apache Directory Developers List <de...@directory.apache.org>
> Subject: Re: [kerby] ASN1 Quick review
>
> Some more questions, now that I have drawn the full Asn1 hierarchy
> (http://pasteboard.co/fGVRQFm.png)
> [Kai] Thanks a lot. Very cool! Could we integrate it in the library doc?
Of course !
Find the attached graphml file (the source from which I generated the
picture, using yEd - https://www.yworks.com/products/yed)
>
> - can't we merge the Asn1Encodable and AbstractAsn1Type classes ?
> [Kai] I guess we can, actually before AbstractAsn1Type is very large. I would prefer to separate them becaue the roles of them are clearly different.
Separation of concern is actually better done with Interfaces.
> Asn1Encodable is responsible for all the encoding/decoding dirty works; and AbstractAsn1Type would wrap a Java type value using Java generic.
The real question here is more or less an architctural question. Let's
consider that an object might have to support multiple 'facets' (as in
http://i.cs.hku.hk/~clwang/projects/facet.html)
- should we let a Class implement a 'facet' ?
- or should we delegate this facet to an helper class ?
For encodable, I would rather think that the help class approach would
be way too complex, as each ASN1 class has its own implementation of
some of teh Asn1Encodable classes. This make me think that the first
approach is better.
Now, as all the Asn1 classes that extend AbstractAsn1Type already have
their own implementations of methods like encodingLength(), I'd rather
see the AbstractAsn1Type abstract class implement what is currently in
the Asn1Encodable class, and make Asn1Encodable an Interface (becaise
the other branch of Asn1Object aren't implementing it).
> For now all Asn1Type object are AbstractAsn1Type, but may be not in the future, because there are possible situations where some types don't want to start with AbstractAsn1Type using the generic things.
DO you have some types in mind ? Or is this just a door you want to
maintain open, just in case ?
> I wish the library could evolve further so be able to standalone as a complete solution some day.
Sure, but I think it's preferable to have a clear vision and design at
each step, which can evolve in the future.
>
> - same question for the Asn1Cnstructed and Asn1Collection classes
> [Kai] Again they were together before but separated out recently. Asn1Collection is purely for set/setoff/sequence/sequenceof. Asn1Constructed can be another case where primitive type but using constructed encoding. You could check the places where it's used. An example in Asn1Converter.
Here, that makes more sense.
>
> - I'm not sure the Asn1Dumpable interface makes a lot of sense. Why don't we simply implement a toString() method that takes an identation as a parameter ?
> [Kai] Right initially when implementing the dump support we went the way, we used a separate method toStr() but found it doesn't work nicely. toString() with a indent parameter looks anti-intuitive I guess. A dumpable can be passed a dumper as well and using the dumper it can share the same underlying String builder along with all kinds of utility functions. Note only complex and bridge types like Asn1CollecionType, Asn1Any and Asn1Container need to go the way, simple types use Java toString() naturally.
Forget what I suggested. It's clear that we *need* this interface, it
forces the classes to implement the methods, otehrwise we might forget
doing so, ending with an incomplete 'dump' feature.
Asn1Specifix (which name should probably be Asn1Specifics) should
implement this interface, btw.
Thanks !
RE: [kerby] ASN1 Quick review
Posted by "Zheng, Kai" <ka...@intel.com>.
Nice picture!! My comments are embedded marked as [Kai].
-----Original Message-----
From: Emmanuel Lécharny [mailto:elecharny@gmail.com]
Sent: Thursday, December 24, 2015 1:29 AM
To: Apache Directory Developers List <de...@directory.apache.org>
Subject: Re: [kerby] ASN1 Quick review
Some more questions, now that I have drawn the full Asn1 hierarchy
(http://pasteboard.co/fGVRQFm.png)
[Kai] Thanks a lot. Very cool! Could we integrate it in the library doc?
- can't we merge the Asn1Encodable and AbstractAsn1Type classes ?
[Kai] I guess we can, actually before AbstractAsn1Type is very large. I would prefer to separate them becaue the roles of them are clearly different. Asn1Encodable is responsible for all the encoding/decoding dirty works; and AbstractAsn1Type would wrap a Java type value using Java generic. For now all Asn1Type object are AbstractAsn1Type, but may be not in the future, because there are possible situations where some types don't want to start with AbstractAsn1Type using the generic things. I wish the library could evolve further so be able to standalone as a complete solution some day.
- same question for the Asn1Cnstructed and Asn1Collection classes
[Kai] Again they were together before but separated out recently. Asn1Collection is purely for set/setoff/sequence/sequenceof. Asn1Constructed can be another case where primitive type but using constructed encoding. You could check the places where it's used. An example in Asn1Converter.
- I'm not sure the Asn1Dumpable interface makes a lot of sense. Why don't we simply implement a toString() method that takes an identation as a parameter ?
[Kai] Right initially when implementing the dump support we went the way, we used a separate method toStr() but found it doesn't work nicely. toString() with a indent parameter looks anti-intuitive I guess. A dumpable can be passed a dumper as well and using the dumper it can share the same underlying String builder along with all kinds of utility functions. Note only complex and bridge types like Asn1CollecionType, Asn1Any and Asn1Container need to go the way, simple types use Java toString() naturally.
I thought the dumping support is very important because without it, many time it's very hard, time-consuming, and frustrating when digging into a large hierarchy structure or more than 1k bytes for a bug. It's surely a place we can further improve.
Regards,
Kai
RE: [kerby] ASN1 Quick review
Posted by "Zheng, Kai" <ka...@intel.com>.
Nice picture!! My comments are embedded marked as [Kai].
-----Original Message-----
From: Emmanuel Lécharny [mailto:elecharny@gmail.com]
Sent: Thursday, December 24, 2015 1:29 AM
To: Apache Directory Developers List <de...@directory.apache.org>
Subject: Re: [kerby] ASN1 Quick review
Some more questions, now that I have drawn the full Asn1 hierarchy
(http://pasteboard.co/fGVRQFm.png)
[Kai] Thanks a lot. Very cool! Could we integrate it in the library doc?
- can't we merge the Asn1Encodable and AbstractAsn1Type classes ?
[Kai] I guess we can, actually before AbstractAsn1Type is very large. I would prefer to separate them becaue the roles of them are clearly different. Asn1Encodable is responsible for all the encoding/decoding dirty works; and AbstractAsn1Type would wrap a Java type value using Java generic. For now all Asn1Type object are AbstractAsn1Type, but may be not in the future, because there are possible situations where some types don't want to start with AbstractAsn1Type using the generic things. I wish the library could evolve further so be able to standalone as a complete solution some day.
- same question for the Asn1Cnstructed and Asn1Collection classes
[Kai] Again they were together before but separated out recently. Asn1Collection is purely for set/setoff/sequence/sequenceof. Asn1Constructed can be another case where primitive type but using constructed encoding. You could check the places where it's used. An example in Asn1Converter.
- I'm not sure the Asn1Dumpable interface makes a lot of sense. Why don't we simply implement a toString() method that takes an identation as a parameter ?
[Kai] Right initially when implementing the dump support we went the way, we used a separate method toStr() but found it doesn't work nicely. toString() with a indent parameter looks anti-intuitive I guess. A dumpable can be passed a dumper as well and using the dumper it can share the same underlying String builder along with all kinds of utility functions. Note only complex and bridge types like Asn1CollecionType, Asn1Any and Asn1Container need to go the way, simple types use Java toString() naturally.
I thought the dumping support is very important because without it, many time it's very hard, time-consuming, and frustrating when digging into a large hierarchy structure or more than 1k bytes for a bug. It's surely a place we can further improve.
Regards,
Kai
RE: [kerby] ASN1 Quick review
Posted by "Zheng, Kai" <ka...@intel.com>.
While before someone may find and complain (I like it actually) about it, I'd like to mention that Javadoc is another thing we need to complement. I will add comments and Javadoc in the API and critical places once the code base is stabilized after some left issues are cleared.
Regards,
Kai
-----Original Message-----
From: Emmanuel Lécharny [mailto:elecharny@gmail.com]
Sent: Thursday, December 24, 2015 1:29 AM
To: Apache Directory Developers List <de...@directory.apache.org>
Subject: Re: [kerby] ASN1 Quick review
Some more questions, now that I have drawn the full Asn1 hierarchy
(http://pasteboard.co/fGVRQFm.png)
- can't we merge the Asn1Encodable and AbstractAsn1Type classes ?
- same question for the Asn1Cnstructed and Asn1Collection classes
- I'm not sure the Asn1Dumpable interface makes a lot of sense. Why don't we simply implement a toString() method that takes an identation as a parameter ?
RE: [kerby] ASN1 Quick review
Posted by "Zheng, Kai" <ka...@intel.com>.
While before someone may find and complain (I like it actually) about it, I'd like to mention that Javadoc is another thing we need to complement. I will add comments and Javadoc in the API and critical places once the code base is stabilized after some left issues are cleared.
Regards,
Kai
-----Original Message-----
From: Emmanuel Lécharny [mailto:elecharny@gmail.com]
Sent: Thursday, December 24, 2015 1:29 AM
To: Apache Directory Developers List <de...@directory.apache.org>
Subject: Re: [kerby] ASN1 Quick review
Some more questions, now that I have drawn the full Asn1 hierarchy
(http://pasteboard.co/fGVRQFm.png)
- can't we merge the Asn1Encodable and AbstractAsn1Type classes ?
- same question for the Asn1Cnstructed and Asn1Collection classes
- I'm not sure the Asn1Dumpable interface makes a lot of sense. Why don't we simply implement a toString() method that takes an identation as a parameter ?
Re: [kerby] ASN1 Quick review
Posted by Emmanuel Lécharny <el...@gmail.com>.
Some more questions, now that I have drawn the full Asn1 hierarchy
(http://pasteboard.co/fGVRQFm.png)
- can't we merge the Asn1Encodable and AbstractAsn1Type classes ?
- same question for the Asn1Cnstructed and Asn1Collection classes
- I'm not sure the Asn1Dumpable interface makes a lot of sense. Why
don't we simply implement a toString() method that takes an identation
as a parameter ?
RE: [kerby] ASN1 Quick review
Posted by "Zheng, Kai" <ka...@intel.com>.
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 <de...@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.
- 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).
- 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.
- 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.
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.
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.
wdyt ?