[Linuxptp-devel] [PATCH RFC 0/5] Management client improvements

[Richard Cochran]

This series add two improvements to the pmc program.

1. Format GET requests as in interpretation #29.
2. Interactively change the target of the requests.

Thanks in advance for your review.

Richard Cochran (5):
pmc: make the TLV length a functional result.
pmc: send GET management messages according to interpretation #29.
pmc: optional legacy zero length TLV for GET actions.
pmc_common: introduce a variable for the target port identity.
pmc: add a command to select the target port.

phc2sys.c | 2 +-
pmc.8 | 11 +++++-
pmc.c | 30 +++++++++++++--
pmc_common.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
pmc_common.h | 4 +-
util.c | 16 ++++++++
util.h | 9 +++++
7 files changed, 179 insertions(+), 13 deletions(-)

[Richard Cochran]

This patch lets the TLV length field of GET messages come from a
function. For now, the function still results in a length of two,
but the intent is to allow different values later.

Signed-off-by: Richard Cochran <***@gmail.com>
---
pmc_common.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/pmc_common.c b/pmc_common.c
index 26666f8..7591484 100644
--- a/pmc_common.c
+++ b/pmc_common.c
@@ -134,6 +134,11 @@ static int pmc_send(struct pmc *pmc, struct ptp_message *msg, int pdulen)
return 0;
}

+static int pmc_tlv_datalen(struct pmc *pmc, int id)
+{
+ return 0;
+}
+
int pmc_get_transport_fd(struct pmc *pmc)
{
return pmc->fdarray.fd[FD_GENERAL];
@@ -141,7 +146,7 @@ int pmc_get_transport_fd(struct pmc *pmc)

int pmc_send_get_action(struct pmc *pmc, int id)
{
- int pdulen;
+ int datalen, pdulen;
struct ptp_message *msg;
struct management_tlv *mgt;
msg = pmc_message(pmc, GET);
@@ -150,9 +155,10 @@ int pmc_send_get_action(struct pmc *pmc, int id)
}
mgt = (struct management_tlv *) msg->management.suffix;
mgt->type = TLV_MANAGEMENT;
- mgt->length = 2;
+ datalen = pmc_tlv_datalen(pmc, id);
+ mgt->length = 2 + datalen;
mgt->id = id;
- pdulen = msg->header.messageLength + sizeof(*mgt);
+ pdulen = msg->header.messageLength + sizeof(*mgt) + datalen;
msg->header.messageLength = pdulen;
msg->tlv_count = 1;
pmc_send(pmc, msg, pdulen);

[Richard Cochran]

This commit makes the GET messages have data bodies, just like the erratum
says to. It really doesn't make sense, but have to do it anyhow. We also
introduce a variable that will enable the legacy behavior of sending
empty bodies.

http://standards.ieee.org/findstds/interps/1588-2008.html

Signed-off-by: Richard Cochran <***@gmail.com>
---
pmc_common.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 93 insertions(+), 1 deletion(-)

diff --git a/pmc_common.c b/pmc_common.c
index 7591484..a9e7c78 100644
--- a/pmc_common.c
+++ b/pmc_common.c
@@ -27,6 +27,27 @@
#include "util.h"
#include "pmc_common.h"

+/*
+ Field Len Type
+ --------------------------------------------------------
+ clockType 2
+ physicalLayerProtocol 1 PTPText
+ physicalAddressLength 2 UInteger16
+ physicalAddress 0
+ protocolAddress 4 Enumeration16 + UInteger16
+ manufacturerIdentity 3
+ reserved 1
+ productDescription 1 PTPText
+ revisionData 1 PTPText
+ userDescription 1 PTPText
+ profileIdentity 6
+ --------------------------------------------------------
+ TOTAL 22
+*/
+#define EMPTY_CLOCK_DESCRIPTION 22
+/* Includes one extra byte to make length even. */
+#define EMPTY_PTP_TEXT 2
+
struct pmc {
UInteger16 sequence_id;
UInteger8 boundary_hops;
@@ -36,6 +57,7 @@ struct pmc {

struct transport *transport;
struct fdarray fdarray;
+ int zero_length_gets;
};

struct pmc *pmc_create(enum transport_type transport_type, char *iface_name,
@@ -136,7 +158,60 @@ static int pmc_send(struct pmc *pmc, struct ptp_message *msg, int pdulen)

static int pmc_tlv_datalen(struct pmc *pmc, int id)
{
- return 0;
+ int len = 0;
+
+ if (pmc->zero_length_gets)
+ return len;
+
+ switch (id) {
+ case USER_DESCRIPTION:
+ len += EMPTY_PTP_TEXT;
+ break;
+ case DEFAULT_DATA_SET:
+ len += sizeof(struct defaultDS);
+ break;
+ case CURRENT_DATA_SET:
+ len += sizeof(struct currentDS);
+ break;
+ case PARENT_DATA_SET:
+ len += sizeof(struct parentDS);
+ break;
+ case TIME_PROPERTIES_DATA_SET:
+ len += sizeof(struct timePropertiesDS);
+ break;
+ case PRIORITY1:
+ case PRIORITY2:
+ case DOMAIN:
+ case SLAVE_ONLY:
+ case CLOCK_ACCURACY:
+ case TRACEABILITY_PROPERTIES:
+ case TIMESCALE_PROPERTIES:
+ len += sizeof(struct management_tlv_datum);
+ break;
+ case TIME_STATUS_NP:
+ len += sizeof(struct time_status_np);
+ break;
+ case GRANDMASTER_SETTINGS_NP:
+ len += sizeof(struct grandmaster_settings_np);
+ break;
+ case NULL_MANAGEMENT:
+ break;
+ case CLOCK_DESCRIPTION:
+ len += EMPTY_CLOCK_DESCRIPTION;
+ break;
+ case PORT_DATA_SET:
+ len += sizeof(struct portDS);
+ break;
+ case LOG_ANNOUNCE_INTERVAL:
+ case ANNOUNCE_RECEIPT_TIMEOUT:
+ case LOG_SYNC_INTERVAL:
+ case VERSION_NUMBER:
+ case DELAY_MECHANISM:
+ case LOG_MIN_PDELAY_REQ_INTERVAL:
+ len += sizeof(struct management_tlv_datum);
+ break;
+ }
+ return len;
}

int pmc_get_transport_fd(struct pmc *pmc)
@@ -161,6 +236,23 @@ int pmc_send_get_action(struct pmc *pmc, int id)
pdulen = msg->header.messageLength + sizeof(*mgt) + datalen;
msg->header.messageLength = pdulen;
msg->tlv_count = 1;
+
+ if (id == CLOCK_DESCRIPTION && !pmc->zero_length_gets) {
+ /*
+ * Make sure the tlv_extra pointers dereferenced in
+ * mgt_pre_send() do point to something.
+ */
+ struct mgmt_clock_description *cd = &msg->last_tlv.cd;
+ uint8_t *buf = mgt->data;
+ cd->clockType = (UInteger16 *) buf;
+ buf += sizeof(*cd->clockType);
+ cd->physicalLayerProtocol = (struct PTPText *) buf;
+ buf += sizeof(struct PTPText) + cd->physicalLayerProtocol->length;
+ cd->physicalAddress = (struct PhysicalAddress *) buf;
+ buf += sizeof(struct PhysicalAddress) + 0;
+ cd->protocolAddress = (struct PortAddress *) buf;
+ }
+
pmc_send(pmc, msg, pdulen);
msg_put(msg);

[Richard Cochran]

This patch makes the original behavior of sending the
TLV values for GET actions with a length of zero.

Signed-off-by: Richard Cochran <***@gmail.com>
---
phc2sys.c | 2 +-
pmc.8 | 11 ++++++++++-
pmc.c | 11 ++++++++---
pmc_common.c | 3 ++-
pmc_common.h | 2 +-
5 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/phc2sys.c b/phc2sys.c
index 366587f..157d0e8 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -372,7 +372,7 @@ static void *get_mgt_data(struct ptp_message *msg)
static int init_pmc(struct clock *clock, int domain_number)
{
clock->pmc = pmc_create(TRANS_UDS, "/var/run/phc2sys", 0,
- domain_number, 0);
+ domain_number, 0, 1);
if (!clock->pmc) {
pr_err("failed to create pmc");
return -1;
diff --git a/pmc.8 b/pmc.8
index 1128bf7..d91c657 100644
--- a/pmc.8
+++ b/pmc.8
@@ -1,4 +1,4 @@
-.TH PMC 8 "November 2012" "linuxptp"
+.TH PMC 8 "July 2013" "linuxptp"
.SH NAME
pmc \- PTP management client

@@ -22,6 +22,8 @@ pmc \- PTP management client
.BI \-t " transport-specific-field"
] [
.B \-v
+] [
+.B \-z
] [ command ] ...

.SH DESCRIPTION
@@ -76,6 +78,13 @@ Display a help message.
.TP
.B \-v
Prints the software version and exits.
+.TP
+.B \-z
+The official interpretation of the 1588 standard mandates sending
+GET actions with valid (but meaningless) TLV values. Therefore the
+pmc program normally sends GET requests with properly formed TLV
+values. This option enables the legacy option of sending zero
+length TLV values instead.

.SH MANAGEMENT IDS

diff --git a/pmc.c b/pmc.c
index 05544a1..3097917 100644
--- a/pmc.c
+++ b/pmc.c
@@ -659,6 +659,7 @@ static void usage(char *progname)
" for network and '/var/run/pmc' for UDS.\n"
" -t [hex] transport specific field, default 0x0\n"
" -v prints the software version and exits\n"
+ " -z send zero length TLV values with the GET actions\n"
"\n",
progname);
}
@@ -666,7 +667,7 @@ static void usage(char *progname)
int main(int argc, char *argv[])
{
char *iface_name = NULL, *progname;
- int c, cnt, length, tmo = -1, batch_mode = 0;
+ int c, cnt, length, tmo = -1, batch_mode = 0, zero_datalen = 0;
char line[1024], *command = NULL;
enum transport_type transport_type = TRANS_UDP_IPV4;
UInteger8 boundary_hops = 1, domain_number = 0, transport_specific = 0;
@@ -677,7 +678,7 @@ int main(int argc, char *argv[])
/* Process the command line arguments. */
progname = strrchr(argv[0], '/');
progname = progname ? 1+progname : argv[0];
- while (EOF != (c = getopt(argc, argv, "246u""b:d:hi:t:v"))) {
+ while (EOF != (c = getopt(argc, argv, "246u""b:d:hi:t:vz"))) {
switch (c) {
case '2':
transport_type = TRANS_IEEE_802_3;
@@ -707,6 +708,9 @@ int main(int argc, char *argv[])
case 'v':
version_show(stdout);
return 0;
+ case 'z':
+ zero_datalen = 1;
+ break;
case 'h':
usage(progname);
return 0;
@@ -730,7 +734,8 @@ int main(int argc, char *argv[])
print_set_syslog(1);
print_set_verbose(1);

- pmc = pmc_create(transport_type, iface_name, boundary_hops, domain_number, transport_specific);
+ pmc = pmc_create(transport_type, iface_name, boundary_hops,
+ domain_number, transport_specific, zero_datalen);
if (!pmc) {
fprintf(stderr, "failed to create pmc\n");
return -1;
diff --git a/pmc_common.c b/pmc_common.c
index a9e7c78..44cd8b5 100644
--- a/pmc_common.c
+++ b/pmc_common.c
@@ -62,7 +62,7 @@ struct pmc {

struct pmc *pmc_create(enum transport_type transport_type, char *iface_name,
UInteger8 boundary_hops, UInteger8 domain_number,
- UInteger8 transport_specific)
+ UInteger8 transport_specific, int zero_datalen)
{
struct pmc *pmc;

@@ -92,6 +92,7 @@ struct pmc *pmc_create(enum transport_type transport_type, char *iface_name,
pr_err("failed to open transport");
goto failed;
}
+ pmc->zero_length_gets = zero_datalen ? 1 : 0;

return pmc;

diff --git a/pmc_common.h b/pmc_common.h
index 46e080e..3807a5f 100644
--- a/pmc_common.h
+++ b/pmc_common.h
@@ -27,7 +27,7 @@ struct pmc;

struct pmc *pmc_create(enum transport_type transport_type, char *iface_name,
UInteger8 boundary_hops, UInteger8 domain_number,
- UInteger8 transport_specific);
+ UInteger8 transport_specific, int zero_datalen);

void pmc_destroy(struct pmc *pmc);

[Richard Cochran]

This patch replaces the hard coded wild card target port identity with
a variable initially set to the wild card value. The intent is to allow
the caller to set specific targets.

Signed-off-by: Richard Cochran <***@gmail.com>
---
pmc_common.c | 13 ++++++++++---
pmc_common.h | 2 ++
2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/pmc_common.c b/pmc_common.c
index 44cd8b5..ed3c5da 100644
--- a/pmc_common.c
+++ b/pmc_common.c
@@ -54,6 +54,7 @@ struct pmc {
UInteger8 domain_number;
UInteger8 transport_specific;
struct PortIdentity port_identity;
+ struct PortIdentity target;

struct transport *transport;
struct fdarray fdarray;
@@ -76,8 +77,9 @@ struct pmc *pmc_create(enum transport_type transport_type, char *iface_name,
pr_err("failed to generate a clock identity");
goto failed;
}
-
pmc->port_identity.portNumber = 1;
+ memset(&pmc->target, 0xff, sizeof(pmc->target));
+
pmc->boundary_hops = boundary_hops;
pmc->domain_number = domain_number;
pmc->transport_specific = transport_specific;
@@ -131,8 +133,7 @@ static struct ptp_message *pmc_message(struct pmc *pmc, uint8_t action)
msg->header.control = CTL_MANAGEMENT;
msg->header.logMessageInterval = 0x7f;

- memset(&msg->management.targetPortIdentity, 0xff,
- sizeof(msg->management.targetPortIdentity));
+ msg->management.targetPortIdentity = pmc->target;
msg->management.startingBoundaryHops = pmc->boundary_hops;
msg->management.boundaryHops = pmc->boundary_hops;
msg->management.flags = action;
@@ -321,3 +322,9 @@ failed:
msg_put(msg);
return NULL;
}
+
+int pmc_target(struct pmc *pmc, struct PortIdentity *pid)
+{
+ pmc->target = *pid;
+ return 0;
+}
diff --git a/pmc_common.h b/pmc_common.h
index 3807a5f..a7b4ae7 100644
--- a/pmc_common.h
+++ b/pmc_common.h
@@ -39,4 +39,6 @@ int pmc_send_set_action(struct pmc *pmc, int id, void *data, int datasize);

struct ptp_message *pmc_recv(struct pmc *pmc);

+int pmc_target(struct pmc *pmc, struct PortIdentity *pid);
+
#endif

[Richard Cochran]

This patch adds a new pmc command called "target" that lets the user
address a particular clock and port. Previously all management requests
were sent to the wild card address.

Signed-off-by: Richard Cochran <***@gmail.com>
---
pmc.c | 19 +++++++++++++++++++
util.c | 16 ++++++++++++++++
util.h | 9 +++++++++
3 files changed, 44 insertions(+)

diff --git a/pmc.c b/pmc.c
index 3097917..59e087b 100644
--- a/pmc.c
+++ b/pmc.c
@@ -596,6 +596,19 @@ static int parse_id(char *s)
return index;
}

+static int parse_target(const char *str)
+{
+ struct PortIdentity pid;
+
+ if (str[0] == '*') {
+ memset(&pid, 0xff, sizeof(pid));
+ } else if (str2pid(str, &pid)) {
+ return -1;
+ }
+
+ return pmc_target(pmc, &pid);
+}
+
static void print_help(FILE *fp)
{
int i;
@@ -608,6 +621,9 @@ static void print_help(FILE *fp)
fprintf(fp, "\tThe [action] can be GET, SET, CMD, or COMMAND\n");
fprintf(fp, "\tCommands are case insensitive and may be abbreviated.\n");
fprintf(fp, "\n");
+ fprintf(fp, "\tTARGET [portIdentity]\n");
+ fprintf(fp, "\tTARGET *\n");
+ fprintf(fp, "\n");
}

static int do_command(char *str)
@@ -623,6 +639,9 @@ static int do_command(char *str)
if (2 != sscanf(str, " %10s %64s", action_str, id_str))
return -1;

+ if (0 == strncasecmp(action_str, "TARGET", strlen(action_str)))
+ return parse_target(id_str);
+
action = parse_action(action_str);
id = parse_id(id_str);

diff --git a/util.c b/util.c
index 30bd8a1..4b0ef6b 100644
--- a/util.c
+++ b/util.c
@@ -82,6 +82,22 @@ char *pid2str(struct PortIdentity *id)
return buf;
}

+int str2pid(const char *s, struct PortIdentity *result)
+{
+ struct PortIdentity pid;
+ unsigned char *ptr = pid.clockIdentity.id;
+ int c;
+ c = sscanf(s, " %02hhx%02hhx%02hhx.%02hhx%02hhx.%02hhx%02hhx%02hhx-%hu",
+ &ptr[0], &ptr[1], &ptr[2], &ptr[3],
+ &ptr[4], &ptr[5], &ptr[6], &ptr[7],
+ &pid.portNumber);
+ if (c == 9) {
+ *result = pid;
+ return 0;
+ }
+ return -1;
+}
+
int generate_clock_identity(struct ClockIdentity *ci, char *name)
{
unsigned char mac[6];
diff --git a/util.h b/util.h
index 52c4bcb..8cf3890 100644
--- a/util.h
+++ b/util.h
@@ -55,6 +55,15 @@ char *cid2str(struct ClockIdentity *id);
*/
char *pid2str(struct PortIdentity *id);

+/**
+ * Scan a string containing a port identity and convert it into binary form.
+ *
+ * @param s String in human readable form.
+ * @param result Pointer to a buffer to hold the result.
+ * @return Zero on success, or -1 if the string is incorrectly formatted.
+ */
+int str2pid(const char *s, struct PortIdentity *result);
+
int generate_clock_identity(struct ClockIdentity *ci, char *name);

/**

[Richard Cochran]

Post by Richard Cochran
This series add two improvements to the pmc program.

BTW, this series will work better together with the byte ordering bug
fix I just pushed out:

b4b5487 msg: Add missing network byte order conversion.

Thanks,
Richard