On Wed, Jun 20, 2018 at 12:27:24PM +0000, Anders Selhammer wrote:
> 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?
Just by accident.
> > +static void interval_increment(struct unicast_service_interval *i)
>
> *interval is used in other function, why not use it here also?
Because this is a super simpler helper function.
> > +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.
Right. It is not needed.
> > +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()?
Right. I'll test for (mask & ctmp->message_types) and then return.
> > +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.
I have them in alphabetical order.
> 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.
I like the names as is, but I will add doxygen comments to explain in
more detail what each function does.
Thanks,
Richard
------------------------------------------------------------------------------
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
