Friday, June 8, 2018 7:53 AM
> +static struct timespec log_to_timespec(int log_seconds);
> +static void timespec_normalize(struct timespec *ts);
> +static int timespec_compare(struct timespec *a, struct timespec *b);
Why not in the same order as implemented?
> +static void interval_increment(struct unicast_service_interval *i)
*interval is used in other function, why not use it here also?
> +static struct timespec log_to_timespec(int log_seconds)
>...
> + if (log_seconds < 0) {
> + log_seconds *= -1;
> + for (i = 1, ns = 500000000ULL; i < log_seconds; i++) {
> + ns >>= 1;
> + }
> + ts.tv_nsec = ns;
> + timespec_normalize(&ts);
> + } else {
>...
I do not see the need of the timespec_normalize when log_seconds are below
zero.
> +static int unicast_service_clients(struct port *p,
> + struct unicast_service_interval *interval)
Better name of the function so it is clear that granted messages should be sent
for all
clients in interval.
> +static void unicast_service_extend(struct unicast_client_address *client,
> + struct request_unicast_xmit_tlv *req)
Same as above
> +int unicast_service_add(struct port *p, struct ptp_message *m,
> + struct tlv_extra *extra)
Took some minutes to get a grip on it but nicely done 😊
> +void unicast_service_remove(struct port *p, struct ptp_message *m,
> + struct tlv_extra *extra)
> ...
> + LIST_FOREACH(itmp, &p->unicast_service->intervals, list) {
> + LIST_FOREACH_SAFE(ctmp, &itmp->clients, list, next) {
> + if (!addreq(transport_type(p->trp),
> + &ctmp->addr, &m->address)) {
> + continue;
> + }
> + ctmp->message_types &= ~mask;
> + if (!ctmp->message_types) {
> + LIST_REMOVE(ctmp, list);
> + free(ctmp);
> + }
> + }
> + }
> +}
Since a client only can have a msg _type in one interval,
why not return after free()?
> +int unicast_service_add(struct port *p, struct ptp_message *m,
> + struct tlv_extra *extra);
> +void unicast_service_cleanup(struct port *p);
> +
> +int unicast_service_deny(struct port *p, struct ptp_message *m,
> + struct tlv_extra *extra);
> +int unicast_service_grant(struct port *p, struct ptp_message *m,
> + struct tlv_extra *extra);
> +int unicast_service_initialize(struct port *p);
> +void unicast_service_remove(struct port *p, struct ptp_message *m,
> + struct tlv_extra *extra);
> +int unicast_service_timer(struct port *p);
Functions could be ordered more logical.
Good solution overall. When you get a grip of all the list and the pqueue
it is easy to follow.
General comment is the naming of the functions. The one word after
unicast_service_ is not always crystal clear. You need to read the function
to understand what some functions should do.
/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
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
