Discussion:
[Linuxptp-devel] [PATCH 0/4] Flexible-sized TLVs and CLOCK/USER_DESCRIPTION
Geoff Salmon
2013-02-09 15:08:28 UTC
Permalink
This is my second attempt at adding support for more of the management
TLVs. It adds a last_tlv struct to the ptp_message that holds the data
for a flexibly-sized TLV. The layout of these TLVs' structs don't have
to match their on-the-wire representation.

This is a more conservative approach than the previous one, and only
supports messages where the last TLV is a flexible one, but this is
enough to implement the management messages.

I have an alternate version of the first patch that I'll send as a
reply for comparison. Instead of requiring the tlv->length to be
calculated before msg_pre_send is called, the patch uses the size
returned by the pack_* functions to adjust the tlv->length inside
mgt_pre_send. This requires msg_pre_send return the number of bytes in
the final message which made for a larger patch, but the result was
cleaner, I think. Debatable though...

Geoff Salmon (4):
support TLVs with flexible size
adds CLOCK_DESCRIPTION struct
support GET CLOCK_DESCRIPTION and USER_DESCRIPTION mgmt messages
pmc: get CLOCK_DESCRIPTION and USER_DESCRIPTION

clock.c | 14 ++++++
clock.h | 7 +++
ddt.h | 5 ++
msg.c | 12 ++---
msg.h | 7 +++
pmc.c | 114 ++++++++++++++++++++++++++++++++++++++++++++-
port.c | 60 ++++++++++++++++++++++++
tlv.c | 160 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
tlv.h | 36 ++++++++++++--
9 files changed, 398 insertions(+), 17 deletions(-)
--
1.7.9.5
Geoff Salmon
2013-02-09 15:08:30 UTC
Permalink
Signed-off-by: Geoff Salmon <***@se-instruments.com>
---
ddt.h | 5 +++++
tlv.h | 24 ++++++++++++++++++++++--
2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/ddt.h b/ddt.h
index 760b443..843ddce 100644
--- a/ddt.h
+++ b/ddt.h
@@ -54,6 +54,11 @@ struct PortAddress {
Octet *address;
};

+struct PhysicalAddress {
+ UInteger16 length;
+ Octet *address;
+};
+
struct ClockQuality {
UInteger8 clockClass;
Enumeration8 clockAccuracy;
diff --git a/tlv.h b/tlv.h
index cfcab68..327d6c7 100644
--- a/tlv.h
+++ b/tlv.h
@@ -21,6 +21,7 @@
#define HAVE_TLV_H

#include "ddt.h"
+#include "ds.h"

/* TLV types */
#define TLV_MANAGEMENT 0x0001
@@ -176,10 +177,29 @@ struct time_status_np {
struct ClockIdentity gmIdentity;
} PACKED;

+enum clock_type {
+ CLOCK_TYPE_ORDINARY = 0x80,
+ CLOCK_TYPE_BOUNDARY = 0x40,
+ CLOCK_TYPE_P2P = 0x20,
+ CLOCK_TYPE_E2E = 0x10,
+ CLOCK_TYPE_MANAGEMENT = 0x08,
+};
+
+struct mgmt_clock_description {
+ UInteger16 clockType;
+ struct PTPText physicalLayerProtocol;
+ struct PhysicalAddress physicalAddress;
+ struct PortAddress protocolAddress;
+ Octet manufacturerIdentity[OUI_LEN];
+ struct PTPText productDescription;
+ struct PTPText revisionData;
+ struct PTPText userDescription;
+ Octet profileIdentity[6];
+};
+
struct tlv_extra {
union {
- /* Empty for now, but will contain structs for the
- * TLVs that use the tlv_extra support. */
+ struct mgmt_clock_description cd;
};
};
--
1.7.9.5
Geoff Salmon
2013-02-09 15:08:31 UTC
Permalink
Signed-off-by: Geoff Salmon <***@se-instruments.com>
---
clock.c | 14 +++++++
clock.h | 7 ++++
port.c | 60 ++++++++++++++++++++++++++++
tlv.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 218 insertions(+)

diff --git a/clock.c b/clock.c
index f5836fd..26f927b 100644
--- a/clock.c
+++ b/clock.c
@@ -169,6 +169,11 @@ static int clock_management_get_response(struct clock *c, struct port *p,
tlv->id = id;

switch (id) {
+ case USER_DESCRIPTION:
+ ptp_text_copy(&rsp->last_tlv.cd.userDescription, &c->desc.userDescription);
+ datalen = 1 + rsp->last_tlv.cd.userDescription.length;
+ respond = 1;
+ break;
case DEFAULT_DATA_SET:
memcpy(tlv->data, &c->dds, sizeof(c->dds));
datalen = sizeof(c->dds);
@@ -209,6 +214,10 @@ static int clock_management_get_response(struct clock *c, struct port *p,
break;
}
if (respond) {
+ if (datalen % 2) {
+ tlv->data[datalen] = 0;
+ datalen++;
+ }
tlv->length = sizeof(tlv->id) + datalen;
pdulen = rsp->header.messageLength + sizeof(*tlv) + datalen;
rsp->header.messageLength = pdulen;
@@ -1074,3 +1083,8 @@ struct clock_description *clock_description(struct clock *c)
{
return &c->desc;
}
+
+int clock_num_ports(struct clock *c)
+{
+ return c->nports;
+}
diff --git a/clock.h b/clock.h
index 581fa14..7b4bfb0 100644
--- a/clock.h
+++ b/clock.h
@@ -222,4 +222,11 @@ struct timePropertiesDS *clock_time_properties(struct clock *c);
*/
struct clock_description *clock_description(struct clock *c);

+/**
+ * Obtain the number of ports a clock has, excluding the UDS port.
+ * @param c The clock instance.
+ * @return The number of ports.
+ */
+int clock_num_ports(struct clock *c);
+
#endif
diff --git a/port.c b/port.c
index 1438808..ed8892e 100644
--- a/port.c
+++ b/port.c
@@ -438,6 +438,9 @@ static void port_management_send_error(struct port *p, struct port *ingress,
pr_err("port %hu: management error failed", portnum(p));
}

+static const Octet profile_id_drr[] = {0x00, 0x1B, 0x19, 0x00, 0x01, 0x00};
+static const Octet profile_id_p2p[] = {0x00, 0x1B, 0x19, 0x00, 0x02, 0x00};
+
static int port_management_get_response(struct port *target,
struct port *ingress, int id,
struct ptp_message *req)
@@ -447,6 +450,9 @@ static int port_management_get_response(struct port *target,
struct ptp_message *rsp;
struct portDS *pds;
struct PortIdentity pid = port_identity(target);
+ uint8_t protocol_addr[TRANSPORT_ADDR_LEN], physical_addr[TRANSPORT_ADDR_LEN];
+ struct clock_description *desc;
+ struct mgmt_clock_description *cd;

rsp = port_management_reply(pid, ingress, req);
if (!rsp) {
@@ -461,6 +467,56 @@ static int port_management_get_response(struct port *target,
datalen = 0;
respond = 1;
break;
+ case CLOCK_DESCRIPTION:
+ cd = &rsp->last_tlv.cd;
+ if (clock_num_ports(target->clock) > 1) {
+ cd->clockType = CLOCK_TYPE_BOUNDARY;
+ } else {
+ cd->clockType = CLOCK_TYPE_ORDINARY;
+ }
+
+ switch(transport_type(target->trp)) {
+ case TRANS_UDP_IPV4:
+ case TRANS_UDP_IPV6:
+ case TRANS_IEEE_802_3:
+ ptp_text_set(&cd->physicalLayerProtocol, "IEEE 802.3");
+ break;
+ default:
+ ptp_text_set(&cd->physicalLayerProtocol, NULL);
+ break;
+ }
+ cd->physicalAddress.length =
+ transport_physical_addr(target->trp, physical_addr);
+ cd->physicalAddress.address = physical_addr;
+
+ cd->protocolAddress.networkProtocol = transport_type(target->trp);
+ cd->protocolAddress.addressLength =
+ transport_protocol_addr(target->trp, protocol_addr);
+ cd->protocolAddress.address = protocol_addr;
+
+ desc = clock_description(target->clock);
+ memcpy(cd->manufacturerIdentity, desc->manufacturerIdentity, OUI_LEN);
+ ptp_text_copy(&cd->productDescription, &desc->productDescription);
+ ptp_text_copy(&cd->revisionData, &desc->revisionData);
+ ptp_text_copy(&cd->userDescription, &desc->userDescription);
+
+ if (target->delayMechanism == DM_P2P) {
+ memcpy(cd->profileIdentity, profile_id_p2p, sizeof(cd->profileIdentity));
+ } else {
+ memcpy(cd->profileIdentity, profile_id_drr, sizeof(cd->profileIdentity));
+ }
+
+ datalen = 2 + 1 + cd->physicalLayerProtocol.length
+ + 2 + cd->physicalAddress.length
+ + 4 + cd->protocolAddress.addressLength
+ + sizeof(cd->manufacturerIdentity)
+ + 1
+ + 1 + cd->productDescription.length
+ + 1 + cd->revisionData.length
+ + 1 + cd->userDescription.length
+ + sizeof(cd->profileIdentity);
+ respond = 1;
+ break;
case PORT_DATA_SET:
pds = (struct portDS *) tlv->data;
pds->portIdentity = target->portIdentity;
@@ -486,6 +542,10 @@ static int port_management_get_response(struct port *target,
break;
}
if (respond) {
+ if (datalen % 2) {
+ tlv->data[datalen] = 0;
+ datalen++;
+ }
tlv->length = sizeof(tlv->id) + datalen;
pdulen = rsp->header.messageLength + sizeof(*tlv) + datalen;
rsp->header.messageLength = pdulen;
diff --git a/tlv.c b/tlv.c
index 7d2c3a7..fafa50b 100644
--- a/tlv.c
+++ b/tlv.c
@@ -42,6 +42,91 @@ static void scaled_ns_h2n(ScaledNs *sns)
sns->fractional_nanoseconds = htons(sns->fractional_nanoseconds);
}

+static int pack_uint16(const UInteger16 *from, uint8_t *to)
+{
+ *(UInteger16 *)to = htons(*from);
+ return sizeof(UInteger16);
+}
+
+static int unpack_uint16(const uint8_t *from, UInteger16 *to)
+{
+ *to = htons(*(UInteger16 *) from);
+ return sizeof(UInteger16);
+}
+
+static int unpack_octet_ptr(const uint8_t *buf, Octet **bytes, int len)
+{
+ if (len)
+ *bytes = (Octet *)buf;
+ else
+ *bytes = 0;
+ return len;
+}
+
+static int pack_octet_ptr(Octet * const *bytes, uint8_t *buf, int len)
+{
+ /*
+ Use memmove instead of memcpy to support calling
+ msg_post_recv and then msg_pre_send on the same message,
+ which could cause parts of the message's TLVs (PTPTexts for
+ example) to be copied from and to the same message buffer.
+ The only place this happens currently, is when management
+ messages are forwarded.
+ */
+ memmove(buf, *bytes, len);
+ return len;
+}
+
+static int pack_port_address(const struct PortAddress *data, uint8_t *buf)
+{
+ int offset = 0;
+ offset += pack_uint16(&data->networkProtocol, buf + offset);
+ offset += pack_uint16(&data->addressLength, buf + offset);
+ offset += pack_octet_ptr(&data->address, buf + offset, data->addressLength);
+ return offset;
+}
+
+static int unpack_port_address(const uint8_t *buf, struct PortAddress *data)
+{
+ int offset = 0;
+ offset += unpack_uint16(buf + offset, &data->networkProtocol);
+ offset += unpack_uint16(buf + offset, &data->addressLength);
+ offset += unpack_octet_ptr(buf + offset, &data->address, data->addressLength);
+ return offset;
+}
+
+static int pack_ptp_text(const struct PTPText *data, uint8_t *buf)
+{
+ int offset = 0;
+ buf[offset++] = data->length;
+ offset += pack_octet_ptr(&data->text, buf + offset, data->length);
+ return offset;
+}
+
+static int unpack_ptp_text(const uint8_t *buf, struct PTPText *data)
+{
+ int offset = 0;
+ data->length = buf[offset++];
+ offset += unpack_octet_ptr(buf + offset, &data->text, data->length);
+ return offset;
+}
+
+static int pack_physical_address(const struct PhysicalAddress *data, uint8_t *buf)
+{
+ int offset = 0;
+ offset += pack_uint16(&data->length, buf + offset);
+ offset += pack_octet_ptr(&data->address, buf + offset, data->length);
+ return offset;
+}
+
+static int unpack_physical_address(const uint8_t *buf, struct PhysicalAddress *data)
+{
+ int offset = 0;
+ offset += unpack_uint16(buf + offset, &data->length);
+ offset += unpack_octet_ptr(buf + offset, &data->address, data->length);
+ return offset;
+}
+
static int mgt_post_recv(struct management_tlv *m, uint16_t data_len, struct tlv_extra *extra)
{
struct defaultDS *dds;
@@ -50,8 +135,36 @@ static int mgt_post_recv(struct management_tlv *m, uint16_t data_len, struct tlv
struct timePropertiesDS *tp;
struct portDS *p;
struct time_status_np *tsn;
+ struct mgmt_clock_description *cd;
int extra_len = 0;
+ uint8_t *buf;
switch (m->id) {
+ case CLOCK_DESCRIPTION:
+ cd = &extra->cd;
+ buf = m->data;
+ buf += unpack_uint16(buf, &cd->clockType);
+ buf += unpack_ptp_text(buf, &cd->physicalLayerProtocol);
+ unpack_uint16(buf, &cd->physicalAddress.length);
+ if (cd->physicalAddress.length > TRANSPORT_ADDR_LEN)
+ goto bad_length;
+ buf += unpack_physical_address(buf, &cd->physicalAddress);
+ unpack_uint16(buf + 2, &cd->protocolAddress.addressLength);
+ if (cd->protocolAddress.addressLength > TRANSPORT_ADDR_LEN)
+ goto bad_length;
+ buf += unpack_port_address(buf, &cd->protocolAddress);
+ memcpy(cd->manufacturerIdentity, buf, sizeof(cd->manufacturerIdentity));
+ buf += sizeof(cd->manufacturerIdentity);
+ buf += 1; /* reserved */
+ buf += unpack_ptp_text(buf, &cd->productDescription);
+ buf += unpack_ptp_text(buf, &cd->revisionData);
+ buf += unpack_ptp_text(buf, &cd->userDescription);
+ memcpy(cd->profileIdentity, buf, sizeof(cd->profileIdentity));
+ buf += sizeof(cd->profileIdentity);
+ extra_len = (buf - m->data);
+ break;
+ case USER_DESCRIPTION:
+ extra_len = unpack_ptp_text(m->data, &extra->cd.userDescription);
+ break;
case DEFAULT_DATA_SET:
if (data_len != sizeof(struct defaultDS))
goto bad_length;
@@ -126,7 +239,31 @@ static void mgt_pre_send(struct management_tlv *m, struct tlv_extra *extra)
struct timePropertiesDS *tp;
struct portDS *p;
struct time_status_np *tsn;
+ struct mgmt_clock_description *cd;
+ uint8_t *buf;
switch (m->id) {
+ case CLOCK_DESCRIPTION:
+ if (extra) {
+ cd = &extra->cd;
+ buf = m->data;
+ buf += pack_uint16(&cd->clockType, buf);
+ buf += pack_ptp_text(&cd->physicalLayerProtocol, buf);
+ buf += pack_physical_address(&cd->physicalAddress, buf);
+ buf += pack_port_address(&cd->protocolAddress, buf);
+ memcpy(buf, cd->manufacturerIdentity, sizeof(cd->manufacturerIdentity));
+ buf += sizeof(cd->manufacturerIdentity);
+ *(buf++) = 0;
+ buf += pack_ptp_text(&cd->productDescription, buf);
+ buf += pack_ptp_text(&cd->revisionData, buf);
+ buf += pack_ptp_text(&cd->userDescription, buf);
+ memcpy(buf, cd->profileIdentity, sizeof(cd->profileIdentity));
+ buf += sizeof(cd->profileIdentity);
+ }
+ break;
+ case USER_DESCRIPTION:
+ if (extra)
+ pack_ptp_text(&extra->cd.userDescription, m->data);
+ break;
case DEFAULT_DATA_SET:
dds = (struct defaultDS *) m->data;
dds->numberPorts = htons(dds->numberPorts);
--
1.7.9.5
Richard Cochran
2013-02-11 18:35:16 UTC
Permalink
Geoff,

I like the direction of this series, but I wonder if it could be made
yet simpler by making the tlv_extra union into tables of pointers.

Taking the mgmt_clock_description from the last patch (and simplifying
a bit for illustration), if all the fields were pointers, they can be
arranged like so.

| mgmt_clock_description | | payload suffix |
|------------------------+ +----------------|
| .clockType ----|----------|---> |
| .physicalLayerProtocol |----------|> |
| .physicalAddress ---|----------|------> |
| .protocolAddress --|----------|-> |
| .manufacturerIdentity -|----------|--------> |
| .productDescription | and | |
| .revisionData | so | |
| .userDescription | on | |
| .profileIdentity ---|----------|-------> |

Using such a scheme, the copying could be avoided altogether. I will
point out the places were I imagine this could happen, below. (This
patch is really quite close to what I am a talking about, I think.)
Post by Geoff Salmon
---
clock.c | 14 +++++++
clock.h | 7 ++++
port.c | 60 ++++++++++++++++++++++++++++
tlv.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 218 insertions(+)
diff --git a/clock.c b/clock.c
index f5836fd..26f927b 100644
--- a/clock.c
+++ b/clock.c
@@ -169,6 +169,11 @@ static int clock_management_get_response(struct clock *c, struct port *p,
tlv->id = id;
switch (id) {
+ ptp_text_copy(&rsp->last_tlv.cd.userDescription, &c->desc.userDescription);
+ datalen = 1 + rsp->last_tlv.cd.userDescription.length;
+ respond = 1;
+ break;
memcpy(tlv->data, &c->dds, sizeof(c->dds));
datalen = sizeof(c->dds);
@@ -209,6 +214,10 @@ static int clock_management_get_response(struct clock *c, struct port *p,
break;
}
if (respond) {
+ if (datalen % 2) {
+ tlv->data[datalen] = 0;
+ datalen++;
+ }
tlv->length = sizeof(tlv->id) + datalen;
pdulen = rsp->header.messageLength + sizeof(*tlv) + datalen;
rsp->header.messageLength = pdulen;
@@ -1074,3 +1083,8 @@ struct clock_description *clock_description(struct clock *c)
{
return &c->desc;
}
+
+int clock_num_ports(struct clock *c)
+{
+ return c->nports;
+}
diff --git a/clock.h b/clock.h
index 581fa14..7b4bfb0 100644
--- a/clock.h
+++ b/clock.h
@@ -222,4 +222,11 @@ struct timePropertiesDS *clock_time_properties(struct clock *c);
*/
struct clock_description *clock_description(struct clock *c);
+/**
+ * Obtain the number of ports a clock has, excluding the UDS port.
+ */
+int clock_num_ports(struct clock *c);
+
#endif
diff --git a/port.c b/port.c
index 1438808..ed8892e 100644
--- a/port.c
+++ b/port.c
@@ -438,6 +438,9 @@ static void port_management_send_error(struct port *p, struct port *ingress,
pr_err("port %hu: management error failed", portnum(p));
}
+static const Octet profile_id_drr[] = {0x00, 0x1B, 0x19, 0x00, 0x01, 0x00};
+static const Octet profile_id_p2p[] = {0x00, 0x1B, 0x19, 0x00, 0x02, 0x00};
+
static int port_management_get_response(struct port *target,
struct port *ingress, int id,
struct ptp_message *req)
@@ -447,6 +450,9 @@ static int port_management_get_response(struct port *target,
struct ptp_message *rsp;
struct portDS *pds;
struct PortIdentity pid = port_identity(target);
+ uint8_t protocol_addr[TRANSPORT_ADDR_LEN], physical_addr[TRANSPORT_ADDR_LEN];
+ struct clock_description *desc;
+ struct mgmt_clock_description *cd;
rsp = port_management_reply(pid, ingress, req);
if (!rsp) {
@@ -461,6 +467,56 @@ static int port_management_get_response(struct port *target,
datalen = 0;
respond = 1;
break;
+ cd = &rsp->last_tlv.cd;
+ if (clock_num_ports(target->clock) > 1) {
+ cd->clockType = CLOCK_TYPE_BOUNDARY;
+ } else {
+ cd->clockType = CLOCK_TYPE_ORDINARY;
+ }
+
+ switch(transport_type(target->trp)) {
+ ptp_text_set(&cd->physicalLayerProtocol, "IEEE 802.3");
+ break;
+ ptp_text_set(&cd->physicalLayerProtocol, NULL);
+ break;
+ }
+ cd->physicalAddress.length =
+ transport_physical_addr(target->trp, physical_addr);
+ cd->physicalAddress.address = physical_addr;
+
+ cd->protocolAddress.networkProtocol = transport_type(target->trp);
+ cd->protocolAddress.addressLength =
+ transport_protocol_addr(target->trp, protocol_addr);
+ cd->protocolAddress.address = protocol_addr;
+
+ desc = clock_description(target->clock);
+ memcpy(cd->manufacturerIdentity, desc->manufacturerIdentity, OUI_LEN);
+ ptp_text_copy(&cd->productDescription, &desc->productDescription);
+ ptp_text_copy(&cd->revisionData, &desc->revisionData);
+ ptp_text_copy(&cd->userDescription, &desc->userDescription);
+
+ if (target->delayMechanism == DM_P2P) {
+ memcpy(cd->profileIdentity, profile_id_p2p, sizeof(cd->profileIdentity));
+ } else {
+ memcpy(cd->profileIdentity, profile_id_drr, sizeof(cd->profileIdentity));
+ }
Here, the port code could instead copy the data directly into the
suffix buffer, setting the pointers of tlv_extra fields as it goes
along.
Post by Geoff Salmon
+
+ datalen = 2 + 1 + cd->physicalLayerProtocol.length
+ + 2 + cd->physicalAddress.length
+ + 4 + cd->protocolAddress.addressLength
+ + sizeof(cd->manufacturerIdentity)
+ + 1
+ + 1 + cd->productDescription.length
+ + 1 + cd->revisionData.length
+ + 1 + cd->userDescription.length
+ + sizeof(cd->profileIdentity);
+ respond = 1;
+ break;
pds = (struct portDS *) tlv->data;
pds->portIdentity = target->portIdentity;
@@ -486,6 +542,10 @@ static int port_management_get_response(struct port *target,
break;
}
if (respond) {
+ if (datalen % 2) {
+ tlv->data[datalen] = 0;
+ datalen++;
+ }
tlv->length = sizeof(tlv->id) + datalen;
pdulen = rsp->header.messageLength + sizeof(*tlv) + datalen;
rsp->header.messageLength = pdulen;
diff --git a/tlv.c b/tlv.c
index 7d2c3a7..fafa50b 100644
--- a/tlv.c
+++ b/tlv.c
@@ -42,6 +42,91 @@ static void scaled_ns_h2n(ScaledNs *sns)
sns->fractional_nanoseconds = htons(sns->fractional_nanoseconds);
}
+static int pack_uint16(const UInteger16 *from, uint8_t *to)
+{
+ *(UInteger16 *)to = htons(*from);
+ return sizeof(UInteger16);
+}
+
+static int unpack_uint16(const uint8_t *from, UInteger16 *to)
+{
+ *to = htons(*(UInteger16 *) from);
+ return sizeof(UInteger16);
+}
+
+static int unpack_octet_ptr(const uint8_t *buf, Octet **bytes, int len)
+{
+ if (len)
+ *bytes = (Octet *)buf;
+ else
+ *bytes = 0;
+ return len;
+}
+
+static int pack_octet_ptr(Octet * const *bytes, uint8_t *buf, int len)
+{
+ /*
+ Use memmove instead of memcpy to support calling
+ msg_post_recv and then msg_pre_send on the same message,
+ which could cause parts of the message's TLVs (PTPTexts for
+ example) to be copied from and to the same message buffer.
+ The only place this happens currently, is when management
+ messages are forwarded.
+ */
+ memmove(buf, *bytes, len);
+ return len;
+}
+
+static int pack_port_address(const struct PortAddress *data, uint8_t *buf)
+{
+ int offset = 0;
+ offset += pack_uint16(&data->networkProtocol, buf + offset);
+ offset += pack_uint16(&data->addressLength, buf + offset);
+ offset += pack_octet_ptr(&data->address, buf + offset, data->addressLength);
+ return offset;
+}
+
+static int unpack_port_address(const uint8_t *buf, struct PortAddress *data)
+{
+ int offset = 0;
+ offset += unpack_uint16(buf + offset, &data->networkProtocol);
+ offset += unpack_uint16(buf + offset, &data->addressLength);
+ offset += unpack_octet_ptr(buf + offset, &data->address, data->addressLength);
+ return offset;
+}
+
+static int pack_ptp_text(const struct PTPText *data, uint8_t *buf)
+{
+ int offset = 0;
+ buf[offset++] = data->length;
+ offset += pack_octet_ptr(&data->text, buf + offset, data->length);
+ return offset;
+}
+
+static int unpack_ptp_text(const uint8_t *buf, struct PTPText *data)
+{
+ int offset = 0;
+ data->length = buf[offset++];
+ offset += unpack_octet_ptr(buf + offset, &data->text, data->length);
+ return offset;
+}
+
+static int pack_physical_address(const struct PhysicalAddress *data, uint8_t *buf)
+{
+ int offset = 0;
+ offset += pack_uint16(&data->length, buf + offset);
+ offset += pack_octet_ptr(&data->address, buf + offset, data->length);
+ return offset;
+}
+
+static int unpack_physical_address(const uint8_t *buf, struct PhysicalAddress *data)
+{
+ int offset = 0;
+ offset += unpack_uint16(buf + offset, &data->length);
+ offset += unpack_octet_ptr(buf + offset, &data->address, data->length);
+ return offset;
+}
+
static int mgt_post_recv(struct management_tlv *m, uint16_t data_len, struct tlv_extra *extra)
{
struct defaultDS *dds;
@@ -50,8 +135,36 @@ static int mgt_post_recv(struct management_tlv *m, uint16_t data_len, struct tlv
struct timePropertiesDS *tp;
struct portDS *p;
struct time_status_np *tsn;
+ struct mgmt_clock_description *cd;
int extra_len = 0;
+ uint8_t *buf;
switch (m->id) {
+ cd = &extra->cd;
+ buf = m->data;
+ buf += unpack_uint16(buf, &cd->clockType);
+ buf += unpack_ptp_text(buf, &cd->physicalLayerProtocol);
+ unpack_uint16(buf, &cd->physicalAddress.length);
+ if (cd->physicalAddress.length > TRANSPORT_ADDR_LEN)
+ goto bad_length;
+ buf += unpack_physical_address(buf, &cd->physicalAddress);
+ unpack_uint16(buf + 2, &cd->protocolAddress.addressLength);
+ if (cd->protocolAddress.addressLength > TRANSPORT_ADDR_LEN)
+ goto bad_length;
+ buf += unpack_port_address(buf, &cd->protocolAddress);
+ memcpy(cd->manufacturerIdentity, buf, sizeof(cd->manufacturerIdentity));
+ buf += sizeof(cd->manufacturerIdentity);
+ buf += 1; /* reserved */
+ buf += unpack_ptp_text(buf, &cd->productDescription);
+ buf += unpack_ptp_text(buf, &cd->revisionData);
+ buf += unpack_ptp_text(buf, &cd->userDescription);
+ memcpy(cd->profileIdentity, buf, sizeof(cd->profileIdentity));
+ buf += sizeof(cd->profileIdentity);
+ extra_len = (buf - m->data);
+ break;
Here, the Rx code could set the pointers of the tlv_extra fields, and
then do ntohs on the four 2-byte fields in place.
Post by Geoff Salmon
+ extra_len = unpack_ptp_text(m->data, &extra->cd.userDescription);
+ break;
if (data_len != sizeof(struct defaultDS))
goto bad_length;
@@ -126,7 +239,31 @@ static void mgt_pre_send(struct management_tlv *m, struct tlv_extra *extra)
struct timePropertiesDS *tp;
struct portDS *p;
struct time_status_np *tsn;
+ struct mgmt_clock_description *cd;
+ uint8_t *buf;
switch (m->id) {
+ if (extra) {
+ cd = &extra->cd;
+ buf = m->data;
+ buf += pack_uint16(&cd->clockType, buf);
+ buf += pack_ptp_text(&cd->physicalLayerProtocol, buf);
+ buf += pack_physical_address(&cd->physicalAddress, buf);
+ buf += pack_port_address(&cd->protocolAddress, buf);
+ memcpy(buf, cd->manufacturerIdentity, sizeof(cd->manufacturerIdentity));
+ buf += sizeof(cd->manufacturerIdentity);
+ *(buf++) = 0;
+ buf += pack_ptp_text(&cd->productDescription, buf);
+ buf += pack_ptp_text(&cd->revisionData, buf);
+ buf += pack_ptp_text(&cd->userDescription, buf);
+ memcpy(buf, cd->profileIdentity, sizeof(cd->profileIdentity));
+ buf += sizeof(cd->profileIdentity);
Here the pointers would already have been set by the port code, so the
only work would be to call htons on the four 2-byte fields, in place.
Post by Geoff Salmon
+ }
+ break;
+ if (extra)
+ pack_ptp_text(&extra->cd.userDescription, m->data);
+ break;
dds = (struct defaultDS *) m->data;
dds->numberPorts = htons(dds->numberPorts);
--
1.7.9.5
Since most of the CLOCK_DESCRIPTION fields are strings (or byte
arrays), there is no need to copy the contents around.

BTW, if you do try this idea, please take care to do the ntohs/htons
on a temporary variable, using memcpy to and from the buffer, since
some non-x86 machines can give us alignment faults.

Thanks,
Richard
Geoff Salmon
2013-02-11 21:17:01 UTC
Permalink
Hi Richard
Post by Richard Cochran
Using such a scheme, the copying could be avoided altogether. I will
point out the places were I imagine this could happen, below. (This
patch is really quite close to what I am a talking about, I think.)
I think there is less copying going on than there appears to be. The
strings are not copied in the port code nor on receive in tlv.c. Both
ptp_text_set and ptp_text_copy only copy a pointer.

It does copy 6 bytes for profileIdentity and 3 bytes for
manufacturerIdentity. I don't think either is worth putting a 4 or 8
byte pointer in the mgmt_clock_description for. Ditto for all the 2 byte
int fields. Replacing them with pointers is copying more data, plus the
pointer arithmetic to create the pointers.

The only fields that are copied but may be worth avoiding the copy are
the physical and protocol addresses. Both are 1-16 bytes. I'll change
the patch to pass pointers into the payload suffix to
transport_physical_addr and transport_protocol_addr. These values aren't
copied when receiving.
Post by Richard Cochran
BTW, if you do try this idea, please take care to do the ntohs/htons>
on a temporary variable, using memcpy to and from the buffer, since
Post by Richard Cochran
some non-x86 machines can give us alignment faults.
Good point. I hadn't noticed that the physicalLayerProtocol and
physicalAddress fields could throw off the alignment.

- Geoff
Richard Cochran
2013-02-12 19:48:57 UTC
Permalink
Post by Geoff Salmon
Post by Richard Cochran
Using such a scheme, the copying could be avoided altogether. I will
point out the places were I imagine this could happen, below. (This
patch is really quite close to what I am a talking about, I think.)
I think there is less copying going on than there appears to be. The
strings are not copied in the port code nor on receive in tlv.c.
Both ptp_text_set and ptp_text_copy only copy a pointer.
It does copy 6 bytes for profileIdentity and 3 bytes for
manufacturerIdentity. I don't think either is worth putting a 4 or 8
byte pointer in the mgmt_clock_description for. Ditto for all the 2
byte int fields.
Sorry, can't see any problem here. We can afford the space in the msg
buffer. Comparing the size of a pointer with the size of data is like
comparing apples and oranges.
Post by Geoff Salmon
Replacing them with pointers is copying more data,
Huh?
Post by Geoff Salmon
plus the pointer arithmetic to create the pointers.
You are already doing the arithmetic. I am saying to do it in another
place, and to use the tlv_extra to remember where the offsets are.

Besides the (perhaps small) issue of extra copying from tlv_extra into
suffix, there is also a design issue. The code in tlv.c has the job of
converting the messages fields into host byte order, and nothing
more. It is the port layer's job to write the message fields.

This patch series would break this division of labor, and that seems
wrong to me. Also, the comment about the memmove call raises my
suspicions. Keeping the data in place, in the suffix buffer, would
avoid such uglies.

Thanks,
Richard
Geoff Salmon
2013-02-13 01:55:52 UTC
Permalink
Post by Richard Cochran
You are already doing the arithmetic. I am saying to do it in another
place, and to use the tlv_extra to remember where the offsets are.
Besides the (perhaps small) issue of extra copying from tlv_extra into
suffix, there is also a design issue. The code in tlv.c has the job of
converting the messages fields into host byte order, and nothing
more. It is the port layer's job to write the message fields.
I think this has been the source of my confusion. My approach was that
the port and clock layers' job is to put the data into or to read data
from structs, and then tlv.c does what is necessary to get the tlv into
the message buffer in the correct layout and in network byte order or
get the data into the struct in host byte order. In my mind all the
explicit pointer arithmetic for message layout belonged in tlv.c instead
of in port.c and clock.c for sending and in tlv.c for receiving.

Storing TLVs that can be represented as a packed struct directly in the
message buffer lets tlv.c handle only the byte order. For TLVs with
flexible sized fields, I copied the data or pointers to the data to a
struct outside the message buffer (tlv_extra) and then copied the data
to the message buffer in tlv.c, which fit the model I had in mind,
described above.
Post by Richard Cochran
Post by Geoff Salmon
It does copy 6 bytes for profileIdentity and 3 bytes for
manufacturerIdentity. I don't think either is worth putting a 4 or 8
byte pointer in the mgmt_clock_description for. Ditto for all the 2
byte int fields.
Sorry, can't see any problem here. We can afford the space in the msg
buffer. Comparing the size of a pointer with the size of data is like
comparing apples and oranges.
Post by Geoff Salmon
Replacing them with pointers is copying more data,
Huh?
Here's my point about the size of pointers. There's already little
redundant copying happening on send or receive: 9 bytes for
profileIdentity and manufacturerIdentity and max 32 bytes for the two
addresses. In v2 of the patch set, even that data is only copied once.
If the amount of copied data was a significant concern, then replacing
every field with a pointer, including even the uint16_t fields,
increases the size of tlv_extra and thus the amount of data copied.

I'm now realizing that the more important issue is tlv.c's intended
role. My feeling is that msg.c and tlv.c should handle reading and
writing the on-the-wire format of messages including both byte order and
layout when necessary. Consider that both clock/port and pmc could be
sending management TLVs with bodies. It seems cleaner to do the message
layout pointer arithmetic in tlv.c and not outside of msg_pre_send.
Granted, this argument falls apart for the existing flexible-sized TLVs
in the standard because they're not settable.

I'll try implementing it with tlv.c only doing byte-order conversion and
see how that goes.

- Geoff
Richard Cochran
2013-02-13 05:43:13 UTC
Permalink
Post by Geoff Salmon
Here's my point about the size of pointers. There's already little
redundant copying happening on send or receive: 9 bytes for
profileIdentity and manufacturerIdentity and max 32 bytes for the
two addresses. In v2 of the patch set, even that data is only copied
once. If the amount of copied data was a significant concern, then
replacing every field with a pointer, including even the uint16_t
fields, increases the size of tlv_extra and thus the amount of data
copied.
Well, if the fields in tlv_extra get alignment padding (think 32 bit
ARM), then the extra cost of using pointers is minimal.
Post by Geoff Salmon
I'm now realizing that the more important issue is tlv.c's intended
role. My feeling is that msg.c and tlv.c should handle reading and
writing the on-the-wire format of messages including both byte order
and layout when necessary. Consider that both clock/port and pmc
could be sending management TLVs with bodies. It seems cleaner to do
the message layout pointer arithmetic in tlv.c and not outside of
msg_pre_send. Granted, this argument falls apart for the existing
flexible-sized TLVs in the standard because they're not settable.
IIRC, there aren't any flexible-sized TLVs that both clock and pmc
would be sending. But if we encouter such a case, nothing prevents you
from putting the shared serialization code into its own file.
Post by Geoff Salmon
I'll try implementing it with tlv.c only doing byte-order conversion
and see how that goes.
Sounds good.

Thanks,
Richard

Geoff Salmon
2013-02-09 15:08:32 UTC
Permalink
Signed-off-by: Geoff Salmon <***@se-instruments.com>
---
pmc.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 112 insertions(+), 2 deletions(-)

diff --git a/pmc.c b/pmc.c
index 5e000bb..d0ccbde 100644
--- a/pmc.c
+++ b/pmc.c
@@ -24,6 +24,7 @@
#include <string.h>
#include <unistd.h>
#include <inttypes.h>
+#include <arpa/inet.h>

#include "ds.h"
#include "fsm.h"
@@ -53,7 +54,7 @@ struct management_id {

struct management_id idtab[] = {
/* Clock management ID values */
- { "USER_DESCRIPTION", USER_DESCRIPTION, not_supported },
+ { "USER_DESCRIPTION", USER_DESCRIPTION, do_get_action },
{ "SAVE_IN_NON_VOLATILE_STORAGE", SAVE_IN_NON_VOLATILE_STORAGE, not_supported },
{ "RESET_NON_VOLATILE_STORAGE", RESET_NON_VOLATILE_STORAGE, not_supported },
{ "INITIALIZE", INITIALIZE, not_supported },
@@ -86,7 +87,7 @@ struct management_id idtab[] = {
{ "TIME_STATUS_NP", TIME_STATUS_NP, do_get_action },
/* Port management ID values */
{ "NULL_MANAGEMENT", NULL_MANAGEMENT, null_management },
- { "CLOCK_DESCRIPTION", CLOCK_DESCRIPTION, not_supported },
+ { "CLOCK_DESCRIPTION", CLOCK_DESCRIPTION, do_get_action },
{ "PORT_DATA_SET", PORT_DATA_SET, do_get_action },
{ "LOG_ANNOUNCE_INTERVAL", LOG_ANNOUNCE_INTERVAL, not_supported },
{ "ANNOUNCE_RECEIPT_TIMEOUT", ANNOUNCE_RECEIPT_TIMEOUT, not_supported },
@@ -114,6 +115,88 @@ static char *action_string[] = {

#define IFMT "\n\t\t"

+static char *text2str_impl(struct PTPText *text, struct static_ptp_text *s) {
+ s->max_symbols = -1;
+ static_ptp_text_copy(s, text);
+ return (char*)(s->text);
+}
+
+static char *text2str(struct PTPText *text) {
+ static struct static_ptp_text s;
+ return text2str_impl(text, &s);
+}
+/* Extra copies of the text2str function so it can be used multiple
+ * times in the same fprintf call. Need this because of the static
+ * static_ptp_text inside the functions.
+ */
+static char *text2str1(struct PTPText *text) {
+ static struct static_ptp_text s;
+ return text2str_impl(text, &s);
+}
+static char *text2str2(struct PTPText *text) {
+ static struct static_ptp_text s;
+ return text2str_impl(text, &s);
+}
+static char *text2str3(struct PTPText *text) {
+ static struct static_ptp_text s;
+ return text2str_impl(text, &s);
+}
+
+#define MAX_PRINT_BYTES 16
+#define BIN_BUF_SIZE (MAX_PRINT_BYTES * 3 + 1)
+
+static char *bin2str_impl(Octet *data, int len, char *buf, int buf_len) {
+ int i, offset = 0;
+ if (len > MAX_PRINT_BYTES)
+ len = MAX_PRINT_BYTES;
+ buf[0] = '\0';
+ if (len)
+ offset += snprintf(buf, buf_len, "%02hhx", data[0]);
+ for (i = 1; i < len; i++) {
+ if (offset >= buf_len)
+ /* truncated output */
+ break;
+ offset += snprintf(buf + offset, buf_len - offset, ":%02hhx", data[i]);
+ }
+ return buf;
+}
+
+static char *bin2str(Octet *data, int len) {
+ static char buf[BIN_BUF_SIZE];
+ return bin2str_impl(data, len, buf, sizeof(buf));
+}
+
+/* Extra copies of the bin2str function so it can be used multiple
+ * times in the same fprintf call. Need this because of the static
+ * char array inside the functions.
+ */
+static char *bin2str1(Octet *data, int len) {
+ static char buf[BIN_BUF_SIZE];
+ return bin2str_impl(data, len, buf, sizeof(buf));
+}
+static char *bin2str2(Octet *data, int len) {
+ static char buf[BIN_BUF_SIZE];
+ return bin2str_impl(data, len, buf, sizeof(buf));
+}
+
+static char *portaddr2str(struct PortAddress *addr) {
+ static char buf[BIN_BUF_SIZE];
+ switch(addr->networkProtocol) {
+ case TRANS_UDP_IPV4:
+ if (addr->addressLength == 4
+ && inet_ntop(AF_INET, addr->address, buf, sizeof(buf)))
+ return buf;
+ break;
+ case TRANS_UDP_IPV6:
+ if (addr->addressLength == 16
+ && inet_ntop(AF_INET6, addr->address, buf, sizeof(buf)))
+ return buf;
+ break;
+ }
+ bin2str_impl(addr->address, addr->addressLength, buf, sizeof(buf));
+ return buf;
+}
+
static void pmc_show(struct ptp_message *msg, FILE *fp)
{
int action;
@@ -124,6 +207,7 @@ static void pmc_show(struct ptp_message *msg, FILE *fp)
struct parentDS *pds;
struct timePropertiesDS *tp;
struct time_status_np *tsn;
+ struct mgmt_clock_description *cd;
struct portDS *p;
if (msg_type(msg) != MANAGEMENT) {
return;
@@ -149,6 +233,32 @@ static void pmc_show(struct ptp_message *msg, FILE *fp)
}
mgt = (struct management_tlv *) msg->management.suffix;
switch (mgt->id) {
+ case CLOCK_DESCRIPTION:
+ fprintf(fp, "CLOCK_DESCRIPTION\n");
+ cd = &msg->last_tlv.cd;
+ fprintf(fp, "CLOCK_DESCRIPTION "
+ IFMT "clockType %hu"
+ IFMT "physicalLayerProtocol %s"
+ IFMT "physicalAddress %s"
+ IFMT "protocolAddress %hu %s"
+ IFMT "manufacturerId %s"
+ IFMT "productDescription %s"
+ IFMT "revisionData %s"
+ IFMT "userDescription %s"
+ IFMT "profileId %s",
+ cd->clockType, text2str(&cd->physicalLayerProtocol),
+ bin2str(cd->physicalAddress.address, cd->physicalAddress.length),
+ cd->protocolAddress.networkProtocol, portaddr2str(&cd->protocolAddress),
+ bin2str1(cd->manufacturerIdentity, sizeof(cd->manufacturerIdentity)),
+ text2str1(&cd->productDescription), text2str2(&cd->revisionData),
+ text2str3(&cd->userDescription),
+ bin2str2(cd->profileIdentity, sizeof(cd->profileIdentity)));
+ break;
+ case USER_DESCRIPTION:
+ fprintf(fp, "USER_DESCRIPTION "
+ IFMT "userDescription %s",
+ text2str(&msg->last_tlv.cd.userDescription));
+ break;
case DEFAULT_DATA_SET:
dds = (struct defaultDS *) mgt->data;
fprintf(fp, "DEFAULT_DATA_SET "
--
1.7.9.5
Richard Cochran
2013-02-11 18:39:02 UTC
Permalink
Post by Geoff Salmon
@@ -114,6 +115,88 @@ static char *action_string[] = {
#define IFMT "\n\t\t"
+static char *text2str_impl(struct PTPText *text, struct static_ptp_text *s) {
+ s->max_symbols = -1;
+ static_ptp_text_copy(s, text);
+ return (char*)(s->text);
+}
+
+static char *text2str(struct PTPText *text) {
+ static struct static_ptp_text s;
+ return text2str_impl(text, &s);
+}
+/* Extra copies of the text2str function so it can be used multiple
+ * times in the same fprintf call. Need this because of the static
+ * static_ptp_text inside the functions.
+ */
I find this a little excessive. Just break the single fprintf into
mutliple calls.
Post by Geoff Salmon
+static char *text2str1(struct PTPText *text) {
+ static struct static_ptp_text s;
+ return text2str_impl(text, &s);
+}
+static char *text2str2(struct PTPText *text) {
+ static struct static_ptp_text s;
+ return text2str_impl(text, &s);
+}
+static char *text2str3(struct PTPText *text) {
+ static struct static_ptp_text s;
+ return text2str_impl(text, &s);
+}
Thanks,
Richard
Geoff Salmon
2013-02-09 15:08:29 UTC
Permalink
These flexible TLVs don't need to be represented as a struct directly
in the message buffer. Instead the TLV data is carried as a separate
struct in the message struct. The flexible TLV can only be the final
TLV in a message and the TLV type and length must still be written to
the message buffer (and also the management ID for management TLVs).

Signed-off-by: Geoff Salmon <***@se-instruments.com>
---
msg.c | 12 ++++++------
msg.h | 7 +++++++
tlv.c | 23 +++++++++++++++++------
tlv.h | 16 +++++++++++++---
4 files changed, 43 insertions(+), 15 deletions(-)

diff --git a/msg.c b/msg.c
index 79fee6d..8fd2008 100644
--- a/msg.c
+++ b/msg.c
@@ -144,7 +144,7 @@ static void port_id_pre_send(struct PortIdentity *pid)
pid->portNumber = htons(pid->portNumber);
}

-static int suffix_post_recv(uint8_t *ptr, int len)
+static int suffix_post_recv(uint8_t *ptr, int len, struct tlv_extra *last)
{
int cnt;
struct TLV *tlv;
@@ -166,14 +166,14 @@ static int suffix_post_recv(uint8_t *ptr, int len)
}
len -= tlv->length;
ptr += tlv->length;
- if (tlv_post_recv(tlv)) {
+ if (tlv_post_recv(tlv, len ? NULL : last)) {
return -1;
}
}
return cnt;
}

-static void suffix_pre_send(uint8_t *ptr, int cnt)
+static void suffix_pre_send(uint8_t *ptr, int cnt, struct tlv_extra *last)
{
int i;
struct TLV *tlv;
@@ -183,7 +183,7 @@ static void suffix_pre_send(uint8_t *ptr, int cnt)

for (i = 0; i < cnt; i++) {
tlv = (struct TLV *) ptr;
- tlv_pre_send(tlv);
+ tlv_pre_send(tlv, i == cnt - 1 ? last : NULL);
ptr += sizeof(struct TLV) + tlv->length;
tlv->type = htons(tlv->type);
tlv->length = htons(tlv->length);
@@ -344,7 +344,7 @@ int msg_post_recv(struct ptp_message *m, int cnt)
return -1;
}

- m->tlv_count = suffix_post_recv(suffix, cnt - pdulen);
+ m->tlv_count = suffix_post_recv(suffix, cnt - pdulen, &m->last_tlv);
if (m->tlv_count == -1) {
return -1;
}
@@ -401,7 +401,7 @@ int msg_pre_send(struct ptp_message *m)
default:
return -1;
}
- suffix_pre_send(suffix, m->tlv_count);
+ suffix_pre_send(suffix, m->tlv_count, &m->last_tlv);
return 0;
}

diff --git a/msg.h b/msg.h
index 2feb804..279f3e6 100644
--- a/msg.h
+++ b/msg.h
@@ -26,6 +26,7 @@

#include "ddt.h"
#include "transport.h"
+#include "tlv.h"

#define PTP_VERSION 2

@@ -204,6 +205,12 @@ struct ptp_message {
* Contains the number of TLVs in the suffix.
*/
int tlv_count;
+ /**
+ * Used to hold the data of the last TLV in the message when
+ * the layout of the TLV makes it difficult to access the data
+ * directly from the message's buffer.
+ */
+ struct tlv_extra last_tlv;
};

/**
diff --git a/tlv.c b/tlv.c
index 0bff332..7d2c3a7 100644
--- a/tlv.c
+++ b/tlv.c
@@ -21,6 +21,7 @@

#include "port.h"
#include "tlv.h"
+#include "msg.h"

#define TLV_LENGTH_INVALID(tlv, type) \
(tlv->length < sizeof(struct type) - sizeof(struct TLV))
@@ -41,7 +42,7 @@ static void scaled_ns_h2n(ScaledNs *sns)
sns->fractional_nanoseconds = htons(sns->fractional_nanoseconds);
}

-static int mgt_post_recv(struct management_tlv *m, uint16_t data_len)
+static int mgt_post_recv(struct management_tlv *m, uint16_t data_len, struct tlv_extra *extra)
{
struct defaultDS *dds;
struct currentDS *cds;
@@ -49,6 +50,7 @@ static int mgt_post_recv(struct management_tlv *m, uint16_t data_len)
struct timePropertiesDS *tp;
struct portDS *p;
struct time_status_np *tsn;
+ int extra_len = 0;
switch (m->id) {
case DEFAULT_DATA_SET:
if (data_len != sizeof(struct defaultDS))
@@ -105,12 +107,18 @@ static int mgt_post_recv(struct management_tlv *m, uint16_t data_len)
tsn->gmPresent = ntohl(tsn->gmPresent);
break;
}
+ if (extra_len) {
+ if (extra_len % 2)
+ extra_len++;
+ if (extra_len + sizeof(m->id) != m->length)
+ goto bad_length;
+ }
return 0;
bad_length:
return -1;
}

-static void mgt_pre_send(struct management_tlv *m)
+static void mgt_pre_send(struct management_tlv *m, struct tlv_extra *extra)
{
struct defaultDS *dds;
struct currentDS *cds;
@@ -209,12 +217,15 @@ static void org_pre_send(struct organization_tlv *org)
}
}

-int tlv_post_recv(struct TLV *tlv)
+int tlv_post_recv(struct TLV *tlv, struct tlv_extra *extra)
{
int result = 0;
struct management_tlv *mgt;
struct management_error_status *mes;
struct path_trace_tlv *ptt;
+ struct tlv_extra dummy_extra;
+ if (extra == 0)
+ extra = &dummy_extra;

switch (tlv->type) {
case TLV_MANAGEMENT:
@@ -223,7 +234,7 @@ int tlv_post_recv(struct TLV *tlv)
mgt = (struct management_tlv *) tlv;
mgt->id = ntohs(mgt->id);
if (tlv->length > sizeof(mgt->id))
- result = mgt_post_recv(mgt, tlv->length - sizeof(mgt->id));
+ result = mgt_post_recv(mgt, tlv->length - sizeof(mgt->id), extra);
break;
case TLV_MANAGEMENT_ERROR_STATUS:
if (TLV_LENGTH_INVALID(tlv, management_error_status))
@@ -261,7 +272,7 @@ bad_length:
return -1;
}

-void tlv_pre_send(struct TLV *tlv)
+void tlv_pre_send(struct TLV *tlv, struct tlv_extra *extra)
{
struct management_tlv *mgt;
struct management_error_status *mes;
@@ -269,7 +280,7 @@ void tlv_pre_send(struct TLV *tlv)
switch (tlv->type) {
case TLV_MANAGEMENT:
mgt = (struct management_tlv *) tlv;
- mgt_pre_send(mgt);
+ mgt_pre_send(mgt, extra);
mgt->id = htons(mgt->id);
break;
case TLV_MANAGEMENT_ERROR_STATUS:
diff --git a/tlv.h b/tlv.h
index 8cce615..cfcab68 100644
--- a/tlv.h
+++ b/tlv.h
@@ -21,7 +21,6 @@
#define HAVE_TLV_H

#include "ddt.h"
-#include "msg.h"

/* TLV types */
#define TLV_MANAGEMENT 0x0001
@@ -177,17 +176,28 @@ struct time_status_np {
struct ClockIdentity gmIdentity;
} PACKED;

+struct tlv_extra {
+ union {
+ /* Empty for now, but will contain structs for the
+ * TLVs that use the tlv_extra support. */
+ };
+};
+
/**
* Converts recognized value sub-fields into host byte order.
* @param tlv Pointer to a Type Length Value field.
+ * @param extra Additional struct where data from tlv will be saved,
+ * can be NULL.
* @return Zero if successful, otherwise non-zero
*/
-int tlv_post_recv(struct TLV *tlv);
+int tlv_post_recv(struct TLV *tlv, struct tlv_extra *extra);

/**
* Converts recognized value sub-fields into network byte order.
* @param tlv Pointer to a Type Length Value field.
+ * @param extra Additional struct containing tlv data to send, can be
+ * NULL.
*/
-void tlv_pre_send(struct TLV *tlv);
+void tlv_pre_send(struct TLV *tlv, struct tlv_extra *extra);

#endif
--
1.7.9.5
Geoff Salmon
2013-02-09 15:20:09 UTC
Permalink
Post by Geoff Salmon
I have an alternate version of the first patch that I'll send as a
reply for comparison.
Actually that patch is further out of date than I realized and needs
more work to be a useful comparison. The gist of it is it changes
msg_pre_send to return number of bytes instead of just an error
indication so all calls to msg_pre_send had to be changed to check for
-1 instead of non-zero and, if sending a flexible TLV is possible in
that case, pass the returned value to port_forward. This avoids the need
for calculating the length of the flexible TLV explicitly. For example,
in port_management_get_response the big "data = ... + ... + ..." line
wouldn't be necessary in the CLOCK_DESCRIPTION case.

If that sounds interesting, I can fix it up and send it. If it sounds
like not enough gain for the change, fair enough.

- Geoff
Loading...