On 11/28/2020 9:08 AM, Richard Cochran wrote:
> Prefix the function with the module name and correct the return code
> semantics.
>
> The active word in the function's name is "query" rather that "get" in
> order to distinguish methods that send and receive over the network
> from those that merely return a cached value.
>
> Signed-off-by: Richard Cochran <richardcoch...@gmail.com>
> ---
> phc2sys.c | 41 +++++++++++++++--------------
> pmc_agent.c | 74 ++++++++++++++++++++++++++---------------------------
> pmc_agent.h | 22 +++++++++++++---
> 3 files changed, 78 insertions(+), 59 deletions(-)
>
> diff --git a/phc2sys.c b/phc2sys.c
> index 725a25c..610b8bb 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -292,24 +292,26 @@ static struct port *port_add(struct phc2sys_private
> *priv, unsigned int number,
> static void clock_reinit(struct phc2sys_private *priv, struct clock *clock,
> int new_state)
> {
> - int phc_index = -1, phc_switched = 0;
> - int state, timestamping, ret = -1;
> + int err = -1, phc_index = -1, phc_switched = 0, state, timestamping;
> struct port *p;
> struct sk_ts_info ts_info;
> char iface[IFNAMSIZ];
> clockid_t clkid = CLOCK_INVALID;
>
> LIST_FOREACH(p, &priv->ports, list) {
> - if (p->clock == clock) {
> - ret = run_pmc_port_properties(priv->node, 1000,
> p->number,
> - &state, ×tamping,
> - iface);
> - if (ret > 0)
> - p->state = normalize_state(state);
> + if (p->clock != clock) {
> + continue;
> + }
We do a continue now to skip over the clock until we found it. Ok.. Bit odd.
Seems like it might be worth introducing some helper function that loops
over the port list and locates the clock.
> + err = pmc_agent_query_port_properties(priv->node, 1000,
> + p->number, &state,
> + ×tamping, iface);
> + if (!err) {
> + p->state = normalize_state(state);
> }
> + break;
> }
>
To me, this entire block might have read better as something like:
LIST_FOREACH(p, &priv->ports, list) {
if (p->clock == clock) {
break
}
}
/* exit if we failed to find a clock */
/* do the run_pmc_port_properties */
But that's a bit awkward because you do need to check that the loop
didn't hit the end without finding a clock. In the previous code that
was snuck in by having "ret" be -1 by initialization. This could be even
further reduced if we had a helper function that returned the first port
with the matching clock.
None of this is really the fault of this patch, and could easily be left
for a future cleanup/refactor. I believe the patch as written has the
same semantics as the original before the return code cleanup.
> - if (ret > 0 && timestamping != TS_SOFTWARE) {
> + if (!err && timestamping != TS_SOFTWARE) {
> /* Check if device changed */
> if (strcmp(clock->device, iface)) {
> free(clock->device);
> @@ -841,12 +843,12 @@ static int phc2sys_recv_subscribed(void *context,
> struct ptp_message *msg,
>
> static int auto_init_ports(struct phc2sys_private *priv, int add_rt)
> {
> - struct port *port;
> - struct clock *clock;
> - int number_ports, res;
> - unsigned int i;
> + int err, number_ports, res;
> int state, timestamping;
> char iface[IFNAMSIZ];
> + struct clock *clock;
> + struct port *port;
> + unsigned int i;
>
> while (1) {
> if (!is_running())
> @@ -866,20 +868,21 @@ static int auto_init_ports(struct phc2sys_private
> *priv, int add_rt)
> return -1;
> }
>
> - res = pmc_agent_subscribe(priv->node, 1000);
> - if (res) {
> + err = pmc_agent_subscribe(priv->node, 1000);
> + if (err) {
> pr_err("failed to subscribe");
> return -1;
> }
>
> for (i = 1; i <= number_ports; i++) {
> - res = run_pmc_port_properties(priv->node, 1000, i, &state,
> - ×tamping, iface);
> - if (res == -1) {
> + err = pmc_agent_query_port_properties(priv->node, 1000, i,
> + &state, ×tamping,
> + iface);
> + if (err == -ENODEV) {
> /* port does not exist, ignore the port */
> continue;
> }
> - if (res <= 0) {
> + if (err) {
> pr_err("failed to get port properties");
> return -1;
> }
> diff --git a/pmc_agent.c b/pmc_agent.c
> index 5e066ac..f5ce59d 100644
> --- a/pmc_agent.c
> +++ b/pmc_agent.c
> @@ -251,43 +251,6 @@ void run_pmc_events(struct pmc_agent *node)
> run_pmc(node, 0, -1, &msg);
> }
>
> -int run_pmc_port_properties(struct pmc_agent *node, int timeout,
> - unsigned int port, int *state,
> - int *tstamping, char *iface)
> -{
> - struct ptp_message *msg;
> - int res, len;
> - struct port_properties_np *ppn;
> -
> - pmc_target_port(node->pmc, port);
> - while (1) {
> - res = run_pmc(node, timeout, TLV_PORT_PROPERTIES_NP, &msg);
> - if (res <= 0)
> - goto out;
> -
> - ppn = management_tlv_data(msg);
> - if (ppn->portIdentity.portNumber != port) {
> - msg_put(msg);
> - continue;
> - }
> -
> - *state = ppn->port_state;
> - *tstamping = ppn->timestamping;
> - len = ppn->interface.length;
> - if (len > IFNAMSIZ - 1)
> - len = IFNAMSIZ - 1;
> - memcpy(iface, ppn->interface.text, len);
> - iface[len] = '\0';
> -
> - msg_put(msg);
> - res = 1;
> - break;
> - }
> -out:
> - pmc_target_all(node->pmc);
> - return res;
> -}
> -
> int run_pmc_clock_identity(struct pmc_agent *node, int timeout)
> {
> struct ptp_message *msg;
> @@ -346,6 +309,43 @@ int pmc_agent_get_sync_offset(struct pmc_agent *agent)
> return agent->sync_offset;
> }
>
> +int pmc_agent_query_port_properties(struct pmc_agent *node, int timeout,
> + unsigned int port, int *state,
> + int *tstamping, char *iface)
> +{
> + struct port_properties_np *ppn;
> + struct ptp_message *msg;
> + int res, len;
> +
> + pmc_target_port(node->pmc, port);
> + while (1) {
> + res = run_pmc(node, timeout, TLV_PORT_PROPERTIES_NP, &msg);
> + if (is_run_pmc_error(res)) {
> + goto out;
> + }
> + ppn = management_tlv_data(msg);
> + if (ppn->portIdentity.portNumber != port) {
> + msg_put(msg);
> + continue;
> + }
> + *state = ppn->port_state;
> + *tstamping = ppn->timestamping;
> + len = ppn->interface.length;
> + if (len > IFNAMSIZ - 1) {
> + len = IFNAMSIZ - 1;
> + }
> + memcpy(iface, ppn->interface.text, len);
> + iface[len] = '\0';
> +
> + msg_put(msg);
> + res = 0;
> + break;
> + }
> +out:
> + pmc_target_all(node->pmc);
> + return run_pmc_err2errno(res);
> +}
> +
> int pmc_agent_query_utc_offset(struct pmc_agent *node, int timeout)
> {
> struct timePropertiesDS *tds;
> diff --git a/pmc_agent.h b/pmc_agent.h
> index 050d52d..7fd37eb 100644
> --- a/pmc_agent.h
> +++ b/pmc_agent.h
> @@ -37,9 +37,6 @@ int run_pmc_clock_identity(struct pmc_agent *agent, int
> timeout);
> int run_pmc_wait_sync(struct pmc_agent *agent, int timeout);
> int run_pmc_get_number_ports(struct pmc_agent *agent, int timeout);
> void run_pmc_events(struct pmc_agent *agent);
> -int run_pmc_port_properties(struct pmc_agent *agent, int timeout,
> - unsigned int port, int *state,
> - int *tstamping, char *iface);
>
> /**
> * Creates an instance of a PMC agent.
> @@ -67,6 +64,25 @@ int pmc_agent_get_leap(struct pmc_agent *agent);
> */
> int pmc_agent_get_sync_offset(struct pmc_agent *agent);
>
> +/**
> + * Queries the port properties of a given port from the ptp4l service.
> + *
> + * In addition:
> + *
> + * - The port state notification callback might be invoked.
> + *
> + * @param agent Pointer to a PMC instance obtained via @ref
> pmc_agent_create().
> + * @param timeout Transmit and receive timeout in milliseconds.
> + * @param port The port index of interest.
> + * @param state Buffer to hold the returned port state.
> + * @param tstamping Buffer to hold the returned time stamping flavor.
> + * @param iface Buffer to hold the returned interface name.
> + * @return Zero on success, negative error code otherwise.
> + */
> +int pmc_agent_query_port_properties(struct pmc_agent *agent, int timeout,
> + unsigned int port, int *state,
> + int *tstamping, char *iface);
> +
> /**
> * Queries the TAI-UTC offset and the current leap adjustment from the
> * ptp4l service.
>
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
