Hi David,
On Tue, Apr 29, 2014 at 12:21:10PM -0400, David S wrote:
> On Wed, Apr 23, 2014 at 4:24 PM, Willy Tarreau <[email protected]> wrote:
>
> > On Wed, Apr 23, 2014 at 04:19:17PM -0400, David S wrote:
> > > On Wed, Apr 23, 2014 at 5:45 AM, Willy Tarreau <[email protected]> wrote:
> > >
> > > > (...)
> > > > Otherwise your patch looks fine. Do you want me to merge it ? If so,
> > > > please could you provide a commit message with it ?
> > > >
> > > > Thanks,
> > > > Willy
> > > >
> > >
> > > First, I'll update the documentation to keep it in sync with the code.
> > >
> > > Thinking ahead to adding TLVs to follow the header, do you have a
> > favorite
> > > generic TLV implementation that you can merge in, or would you rather us
> > > develop something specific from scratch?
> >
> > Take a look at src/peers.c, from what I remember, the sync protocol uses
> > TLV as well, but I don't remember if there was anything specific, as in
> > my opinion it's quite straightforward.
> >
> > Concerning the length, we could use a variable size encoding but that's
> > painful for the decoder in general. And sometimes even for the encoder.
> > So probably better stick to 16 bits for all lengths.
> >
> > Thanks,
> > Willy
> >
> >
> What do you think of this?
>
> #define PP2_TYPE_SSL 0x20
> #define PP2_TYPE_SSL_CN 0x21
> #define PP2_TYPE_SSL_DN 0x22
> #define PP2_TYPE_SSL_ALPN 0x23
> #define PP2_TYPE_SSL_SNI....0x24
>
> struct tlv {
> uint8_t type;
> uint8_t reserved;
> uint16_t length;
> uint8_t value[0]
> };
>
> struct tlv_ssl {
> uint8_t type;
> uint8_t reserved;
> uint16_t length;
> uint32_t version;
> uint32_t client;
> uint32_t verify;
> uint8_t sub_tlv[0]
> };
>
> struct tlv_cn {
> uint8_t type;
> uint8_t reserved;
> uint16_t length;
> unit8_t cn[0];
> };
>
> struct tlv_dn {
> uint8_t type;
> uint8_t reserved;
> uint16_t length;
> unit8_t dn[0];
> };
>
> struct tlv_alpn {
> uint8_t type;
> uint8_t reserved;
> uint16_t length;
> unit8_t alpn[0];
> };
>
> struct tlv_sni {
> uint8_t type;
> uint8_t reserved;
> uint16_t length;
> unit8_t sni[0];
> };
I suspect the "reserved" field above is to ensure 32-bit alignment, but
I really think we'd rather avoid this. Most important fields are already
aligned, and nobody will be able to guarantee that extensions will always
be aligned (and some implementations will randomly get them right or wrong).
So better cut that byte all the time.
In order to simplify declarations without having to explicitly mention
__attribute__((packed)) on most of these structures (though I think we'll
need to anyway), we can put the length before the type. Thus I think that
the following form would be better suited :
struct tlv {
uint16_t length;
uint8_t type;
uint8_t value[0];
};
Or maybe even this one which is more friendly to implementations with no
unaligned accesses, and which is not possible to get wrong :
struct tlv {
uint8_t len_h;
uint8_t len_l;
uint8_t type;
uint8_t value[0];
};
static inline uint16_t tlv_length(struct tlv *tlv)
{
return tlv->len_h * 256 + tlv->len_l;
}
Another point worth mentionning is that it can be painful to repeat that
header for all types. Also if for any reason we were to later add a new
TLV format (eg: think about datagram based communications), we'd rather
just change the header and not redefine all further blocks. So for this
reason, I'd rather go with something that plugs *after* struct tlv instead
of casting with it :
struct tlv_ssl {
uint32_t version;
uint32_t client;
uint32_t verify;
uint8_t sub_tlv[0];
};
Just a few thoughts...
Willy
