Wednesday, June 6, 2018 1:17 AM
Firstly, in general I think it looks really good. I have some small comments
> +static int attach_request(struct ptp_message *msg, int log_period,
> + uint8_t message_type, int duration)
Why not put message_type before the period and duration.
static int attach_request(struct ptp_message *msg, uint8_t message_type,
int log_period , int duration)
> +static struct unicast_master_address *unicast_client_ok(struct port *p,
> + struct ptp_message *m)
Why use *m some times and *msg sometime?
Isn´t it better to use only *m or *msg as an inparameter and the other one if
declared
inside a function?
It is cleaner to use same name always if possible.
Also, is it the best naming for the function, it returns ucma, not just
checking if client is ok.
> +static int unicast_client_peer(struct port *p)
The naming does not say anything abour renew (like below function)
Sugguest unicast_client_peer_renew or unicast_client_renew_peer
> + struct unicast_master_address *ucma
> + struct unicast_master_address *peer
> + struct unicast_master_address *dst
> + struct unicast_master_address *master
General: Same goes for this, why not use ucma always, find it more easier to
read the code
when the naming has less variations.
> + if (dst->state == UC_HAVE_SYDY) {
> + err = attach_request(msg, p->logSyncInterval, SYNC,
> + p->unicast_req_duration);
> + if (err) {
> + goto out;
> + }
> + if (p->delayMechanism != DM_P2P) {
> + err = attach_request(msg, p->logMinDelayReqInterval,
> + DELAY_RESP,
> + p->unicast_req_duration);
> + if (err) {
> + goto out;
> + }
> + }
> + }
> +
> + err = port_prepare_and_send(p, msg, TRANS_GENERAL);
Could add a goto send and get rid of the if inside if inside if 😊
if (dst->state != UC_HAVE_SYDY) {
goto send;
}
err = attach_request(msg, p->logSyncInterval, SYNC,
p->unicast_req_duration);
if (err) {
goto out;
}
if (p->delayMechanism == DM_P2P) {
goto send;
}
err = attach_request(msg, p->logMinDelayReqInterval, DELAY_RESP,
p->unicast_req_duration);
if (err) {
goto out;
}
send:
err = port_prepare_and_send(p, msg, TRANS_GENERAL);
> +static void unicast_client_set_renewal(struct port *p,
> + struct unicast_master_address *master,
> + long duration)
> +{
> + struct timespec now;
> + long tmo;
> +
> + if (clock_gettime(CLOCK_MONOTONIC, &now)) {
> + pr_err("clock_gettime failed: %m");
> + return;
> + }
> + duration = (3 * duration) / 4;
> + tmo = now.tv_sec + duration;
> + if (!master->renewal_tmo || tmo < master->renewal_tmo) {
> + master->renewal_tmo = tmo;
> + pr_debug("port %d: renewal timeout at %ld", portnum(p), tmo);
> + }
> +}
Remove tmo and put duration calculation inside if
if (!master->renewal_tmo || tmo < master->renewal_tmo) {
duration = (3 * duration) / 4;
master->renewal_tmo = now.tv_sec + duration;
pr_debug("port %d: renewal timeout at %ld",
portnum(p), master->renewal_tmo);
}
> +int unicast_client_cancel(struct port *p, struct ptp_message *m,
> + struct tlv_extra *extra)
Since more and more functions becomes global for transmit and receive messages,
isn´t it a good idea to use a good naming like:
unicast_client_tx_announce
unicast_client_tx_sydy
unicast_client_rx_cancel
unicast_client_rx_grant
...
Also maybe implement them grouped in a logical order.
> + struct grant_unicast_xmit_tlv *g;
Why not *grant; ? You have used ack, req and cancel in other functions, not
with single letters.
/Anders
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linuxptp-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
