Re: [Linuxptp-devel] [PATCH v3 05/11] Event subscribing
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:
#149; 3 signs your SCM is hindering your productivity
#149; Requirements for releasing software faster
#149; 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
Re: [Linuxptp-devel] [PATCH v3 05/11] Event subscribing
On Mon, May 05, 2014 at 02:39:58PM +0200, Jiri Benc wrote: I'll wrap them but the result will look uglier than this. You version looks like this: msg-management.targetPortIdentity.clockIdentity = s-targetPortIdentity.clockIdentity; msg-management.targetPortIdentity.portNumber = htons(s-targetPortIdentity.portNumber); Those are 103 and 104 columns. In contrast msg-management.targetPortIdentity.clockIdentity = s-targetPortIdentity.clockIdentity; msg-management.targetPortIdentity.portNumber = htons(s-targetPortIdentity.portNumber); doesn't look so ugly to me. (Try an 80x25 xterm.) 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 :-/ Sorry for being a pain, but I really don't want to have any COMMANDs. 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. It has a key with *one* allowed value. That is virtually the same as not having any data. The information content is zero. 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. You are right that the signal TLV is closest. I wouldn't object to having this TLV be a signal message instead. (I am planning on implementing unicast negotiation.) But if it is a management message, it really doesn't match the other commands. - SAVE_IN_NON_VOLATILE_STORAGE - RESET_NON_VOLATILE_STORAGE - INITIALIZE - FAULT_LOG_RESET - ENABLE_PORT - DISABLE_PORT If you can't find the time to reform this patch, then I'll do it, if you want. That won't spare you any testing, however. Thanks, Richard -- Is your legacy SCM system holding you back? Join Perforce May 7 to find out: #149; 3 signs your SCM is hindering your productivity #149; Requirements for releasing software faster #149; 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
[Linuxptp-devel] [PATCH v3 05/11] Event subscribing
This puts groundwork for event subscription and notification. The individual
events are added by subsequent patches.
Signed-off-by: Jiri Benc jb...@redhat.com
---
clock.c| 136
clock.h| 11 +
notification.h | 27 +++
tlv.c |4 ++
tlv.h |8 +++
5 files changed, 186 insertions(+), 0 deletions(-)
create mode 100644 notification.h
diff --git a/clock.c b/clock.c
index a85efdec2857..600fc058a391 100644
--- a/clock.c
+++ b/clock.c
@@ -22,6 +22,7 @@
#include string.h
#include time.h
+#include address.h
#include bmc.h
#include clock.h
#include clockadj.h
@@ -59,6 +60,14 @@ struct clock_stats {
unsigned int max_count;
};
+struct clock_subscriber {
+ LIST_ENTRY(clock_subscriber) list;
+ uint8_t events[EVENT_BITMASK_CNT];
+ struct PortIdentity targetPortIdentity;
+ struct address addr;
+ UInteger16 sequenceId;
+};
+
struct clock {
clockid_t clkid;
struct servo *servo;
@@ -99,6 +108,7 @@ struct clock {
int stats_interval;
struct clockcheck *sanity_check;
struct interface uds_interface;
+ LIST_HEAD(clock_subscribers_head, clock_subscriber) subscribers;
};
struct clock the_clock;
@@ -110,9 +120,96 @@ static int cid_eq(struct ClockIdentity *a, struct
ClockIdentity *b)
return 0 == memcmp(a, b, sizeof(*a));
}
+#ifndef LIST_FOREACH_SAFE
+#defineLIST_FOREACH_SAFE(var, head, field, tvar)
\
+ for ((var) = LIST_FIRST((head));\
+ (var) ((tvar) = LIST_NEXT((var), field), 1); \
+ (var) = (tvar))
+#endif
+
+static void remove_subscriber(struct clock_subscriber *s)
+{
+ LIST_REMOVE(s, list);
+ free(s);
+}
+
+static void clock_update_subscription(struct clock *c, struct ptp_message *req,
+ uint8_t *bitmask)
+{
+ struct clock_subscriber *s;
+ int i, remove = 1;
+
+ for (i = 0; i EVENT_BITMASK_CNT; i++) {
+ if (bitmask[i]) {
+ remove = 0;
+ break;
+ }
+ }
+
+ LIST_FOREACH(s, c-subscribers, list) {
+ if (!memcmp(s-targetPortIdentity,
req-header.sourcePortIdentity,
+ sizeof(struct PortIdentity))) {
+ /* Found, update the transport address and event
+* mask. */
+ if (!remove) {
+ s-addr = req-address;
+ memcpy(s-events, bitmask, EVENT_BITMASK_CNT);
+ } else {
+ remove_subscriber(s);
+ }
+ return;
+ }
+ }
+ if (remove)
+ return;
+ /* Not present yet, add the subscriber. */
+ s = malloc(sizeof(*s));
+ if (!s) {
+ pr_err(failed to allocate memory for a subscriber);
+ return;
+ }
+ s-targetPortIdentity = req-header.sourcePortIdentity;
+ s-addr = req-address;
+ memcpy(s-events, bitmask, EVENT_BITMASK_CNT);
+ s-sequenceId = 0;
+ LIST_INSERT_HEAD(c-subscribers, s, list);
+}
+
+static void clock_flush_subscriptions(struct clock *c)
+{
+ struct clock_subscriber *s, *tmp;
+
+ LIST_FOREACH_SAFE(s, c-subscribers, list, tmp) {
+ remove_subscriber(s);
+ }
+}
+
+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);
+ msg-address = s-addr;
+ port_forward_to(uds, msg);
+ }
+}
+
void clock_destroy(struct clock *c)
{
int i;
+
+ clock_flush_subscriptions(c);
for (i = 0; i c-nports; i++) {
port_close(c-port[i]);
close(c-fault_fd[i]);
@@ -328,6 +425,40 @@ static int clock_management_set(struct clock *c, struct
port *p,
return respond ? 1 : 0;
}
+static int clock_management_cmd_response(struct clock *c, struct port *p,
+int id, struct ptp_message *req)
+{
+ int respond = 0;
+ struct ptp_message *rsp = NULL;
+ struct
