On Sat, 3 May 2014 20:19:10 +0200, Richard Cochran wrote: > On Fri, May 02, 2014 at 12:37:48PM +0200, Jiri Benc wrote: > > +static void clock_flush_subscriptions(struct clock *c) > > +{ > > + struct clock_subscriber *s, *tmp; > > + > > + LIST_FOREACH_SAFE(s, &c->subscribers, list, tmp) { > > + remove_subscriber(s); > > + } > > You are removing every item in the list, and so can just do this... > > while ((s = LIST_FIRST(&c->subscribers)) != NULL) { > LIST_REMOVE(s, list); > /* or */ > remove_subscriber(s); > } > > can't you?
As you said, we'll need LIST_FOREACH_SAFE anyway (and it's actually already included in some sys/queue.h implementations). I think the way I used makes clearer what's happening here. The additional variable will likely be stored in a register by the compiler and this is not a performance critical path anyway. > > +} > > + > > +void clock_send_notification(struct clock *c, struct ptp_message *msg, > > + int msglen, enum notification event) > > +{ > > + unsigned int event_pos = event / 8; > > + uint8_t mask = 1 << (event % 8); > > + struct port *uds = c->port[c->nports]; > > + struct clock_subscriber *s; > > + > > + LIST_FOREACH(s, &c->subscribers, list) { > > + if (!(s->events[event_pos] & mask)) > > + continue; > > + /* send event */ > > + msg->header.sequenceId = htons(s->sequenceId); > > + s->sequenceId++; > > + msg->management.targetPortIdentity.clockIdentity = > > s->targetPortIdentity.clockIdentity; > > + msg->management.targetPortIdentity.portNumber = > > htons(s->targetPortIdentity.portNumber); > > Lines too long. I'll wrap them but the result will look uglier than this. > > @@ -842,6 +975,8 @@ int clock_manage(struct clock *c, struct port *p, > > struct ptp_message *msg) > > return changed; > > break; > > case COMMAND: > > + if (clock_management_cmd_response(c, p, mgt->id, msg)) > > + return changed; > > So does this really have to be a COMMAND action? It feels to me more > like a SET action, configuring a per-client table of events. The border between COMMAND and SET is fuzzy here. I don't have too strong preference, except that I'd need to rewrite the set for the 4th time and retest it again :-/ > None of the existing COMMAND actions send any data (except for that > useless initialization key) or configure anything. Instead, they are > some sort of imperative like "restart!" or "reset!" Except that the INITIALIZE command does have data. I thought about this more like a "send me notifications!" command. As a counter-argument, none of the existing SET actions act per-client, they are used to set the (global) clock or port state. In fact, neither of those fits. The closest thing in the standard is unicast negotiation which uses a different TLV than management which is not applicable here. I won't argue about this but I'm not thrilled about rewriting and retesting this again for something that's just a matter of taste. > > +#define EVENT_BITMASK_CNT 32 > > Let's make this 64, for 512 event types. Okay, whatever. Jiri -- Jiri Benc ------------------------------------------------------------------------------ Is your legacy SCM system holding you back? Join Perforce May 7 to find out: • 3 signs your SCM is hindering your productivity • Requirements for releasing software faster • Expert tips and advice for migrating your SCM now http://p.sf.net/sfu/perforce _______________________________________________ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel