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
