Really hope to hear any comment about my implementation, I'm not sure if this is a good way to do it (e.g using fd stream for file io).
There is a dilemma between implementing it as ofctl command or appctl command: Keith mentioned that he may want to be able to both output the snooped message to stdout and write it to a log file. But I couldn't find a way to make both happen. *If it is implemented as a appctl command,* pro: - I can specify the exact set of controllers to snoop, con: - but there is no way to write snoop output to stdout, - and it requires second patch "ofctl parse-ofpraw" to parse the raw messages *If it is implemented as a ofctl command,* pro: - it can output the snoop to stdout, - all printed messages are parsed (human readable). con: - I haven't found a way to pass arguments (e.g. file name/controller names) via ofctl command. And I don't think ofctl is designed for doing that. Kind Regards, Alex Wang, On Sun, Aug 4, 2013 at 8:14 PM, Alex Wang <al...@nicira.com> wrote: > This commit adds a new command "ovs-appctl log/controller" to ovs. > This command allows user to log OpenFlow messages between bridge and > its controllers into a specified file. > > Signed-off-by: Alex Wang <al...@nicira.com> > --- > lib/automake.mk | 2 ++ > lib/ovs-snoop.c | 71 > +++++++++++++++++++++++++++++++++++++++ > lib/ovs-snoop.h | 28 +++++++++++++++ > lib/rconn.c | 26 ++++++++++++++ > lib/rconn.h | 3 ++ > ofproto/automake.mk | 3 +- > ofproto/connmgr.c | 63 ++++++++++++++++++++++++++++++++++ > ofproto/connmgr.h | 4 +++ > ofproto/ofproto-dpif.c | 66 > ++++++++++++++++++++++++++++++++++++ > ofproto/ofproto-log-unixctl.man | 37 ++++++++++++++++++++ > tests/ofproto-dpif.at | 53 +++++++++++++++++++++++++++++ > 11 files changed, 355 insertions(+), 1 deletion(-) > create mode 100644 lib/ovs-snoop.c > create mode 100644 lib/ovs-snoop.h > create mode 100644 ofproto/ofproto-log-unixctl.man > > diff --git a/lib/automake.mk b/lib/automake.mk > index b46a868..f422422 100644 > --- a/lib/automake.mk > +++ b/lib/automake.mk > @@ -131,6 +131,8 @@ lib_libopenvswitch_a_SOURCES = \ > lib/ovs-atomic-pthreads.c \ > lib/ovs-atomic-pthreads.h \ > lib/ovs-atomic.h \ > + lib/ovs-snoop.c \ > + lib/ovs-snoop.h \ > lib/ovs-thread.c \ > lib/ovs-thread.h \ > lib/ovsdb-data.c \ > diff --git a/lib/ovs-snoop.c b/lib/ovs-snoop.c > new file mode 100644 > index 0000000..54e6bc1 > --- /dev/null > +++ b/lib/ovs-snoop.c > @@ -0,0 +1,71 @@ > +/* > + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc. > + * > + * Licensed under the Apache License, Version 2.0 (the "License"); > + * you may not use this file except in compliance with the License. > + * You may obtain a copy of the License at: > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * Unless required by applicable law or agreed to in writing, software > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or > implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > + > +#include <config.h> > +#include "ovs-snoop.h" > + > +#include <errno.h> > +#include <fcntl.h> > +#include <sys/stat.h> > +#include "dirs.h" > +#include "stream.h" > +#include "stream-fd.h" > +#include "vlog.h" > + > +VLOG_DEFINE_THIS_MODULE(ovs_snoop); > + > +/* Opens a snoop file of name 'file_name'. Uses default file name if > + * 'file_name' is null. Then creates a fd stream, stores the pointer of > + * the stream in 'streamp'. Returns 0 if successful, otherwise a > + * positive errno value. */ > +int > +snoop_open(struct stream **streamp, const char *file_name) > +{ > + char *snoop_file_name; > + int retval, new_fd; > + > + snoop_file_name = (file_name > + ? xstrdup(file_name) > + : xasprintf("%s/ovs-snoop.log", ovs_logdir())); > + > + /* Open new snoop file. */ > + new_fd = open(snoop_file_name, O_WRONLY | O_CREAT | O_APPEND, 0666); > + if (new_fd < 0) { > + VLOG_WARN("failed to open %s for snoop logging: %s", > + snoop_file_name, ovs_strerror(errno)); > + retval = errno; > + goto exit; > + } > + retval = new_fd_stream(snoop_file_name, new_fd, 0, streamp); > +exit: > + free(snoop_file_name); > + return retval; > +} > + > +/* Closes snoop file. */ > +void > +snoop_close(struct stream *stream) > +{ > + stream_close(stream); > +} > + > +/* Writes 'message' to the snoop log. */ > +void > +snoop_log(struct stream *stream, const void *msg, size_t size) > +{ > + stream_send(stream, msg, size); > + stream_send(stream, "\n", 1); > +} > diff --git a/lib/ovs-snoop.h b/lib/ovs-snoop.h > new file mode 100644 > index 0000000..bd5e37d > --- /dev/null > +++ b/lib/ovs-snoop.h > @@ -0,0 +1,28 @@ > +/* > + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc. > + * > + * Licensed under the Apache License, Version 2.0 (the "License"); > + * you may not use this file except in compliance with the License. > + * You may obtain a copy of the License at: > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * Unless required by applicable law or agreed to in writing, software > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or > implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > + > +#ifndef OVS_SNOOP_H > +#define OVS_SNOOP_H 1 > + > +#include <stddef.h> > + > +struct stream; > + > +int snoop_open(struct stream **, const char *file_name); > +void snoop_close(struct stream *); > +void snoop_log(struct stream *, const void *msg, size_t size); > + > +#endif /* ovs-snoop.h */ > diff --git a/lib/rconn.c b/lib/rconn.c > index 39a12c9..2d36d75 100644 > --- a/lib/rconn.c > +++ b/lib/rconn.c > @@ -25,6 +25,7 @@ > #include "ofp-util.h" > #include "ofpbuf.h" > #include "openflow/openflow.h" > +#include "ovs-snoop.h" > #include "poll-loop.h" > #include "sat-math.h" > #include "timeval.h" > @@ -131,6 +132,9 @@ struct rconn { > struct vconn *monitors[8]; > size_t n_monitors; > > + /* Controller logging. */ > + struct stream *snoop_stream; > + > uint32_t allowed_versions; > }; > > @@ -220,6 +224,8 @@ rconn_create(int probe_interval, int max_backoff, > uint8_t dscp, > rc->n_monitors = 0; > rc->allowed_versions = allowed_versions; > > + rc->snoop_stream = NULL; > + > return rc; > } > > @@ -580,6 +586,9 @@ rconn_recv(struct rconn *rc) > int error = vconn_recv(rc->vconn, &buffer); > if (!error) { > copy_to_monitor(rc, buffer); > + if (rc->snoop_stream) { > + snoop_log(rc->snoop_stream, buffer->data, buffer->size); > + } > if (rc->probably_admitted || is_admitted_msg(buffer) > || time_now() - rc->last_connected >= 30) { > rc->probably_admitted = true; > @@ -628,6 +637,9 @@ rconn_send(struct rconn *rc, struct ofpbuf *b, > if (rconn_is_connected(rc)) { > COVERAGE_INC(rconn_queued); > copy_to_monitor(rc, b); > + if (rc->snoop_stream) { > + snoop_log(rc->snoop_stream, b->data, b->size); > + } > b->private_p = counter; > if (counter) { > rconn_packet_counter_inc(counter, b->size); > @@ -697,6 +709,20 @@ rconn_add_monitor(struct rconn *rc, struct vconn > *vconn) > } > } > > +/* Enables writing to snoop log of 'rc'. */ > +void > +rconn_set_snoop(struct rconn *rc, struct stream *stream) > +{ > + rc->snoop_stream = stream; > +} > + > +/* Disables writing to snoop log of 'rc'. */ > +void > +rconn_clear_snoop(struct rconn *rc) > +{ > + rc->snoop_stream = NULL; > +} > + > /* Returns 'rc''s name. This is a name for human consumption, > appropriate for > * use in log messages. It is not necessarily a name that may be passed > * directly to, e.g., vconn_open(). */ > diff --git a/lib/rconn.h b/lib/rconn.h > index aa30238..4dd7a9c 100644 > --- a/lib/rconn.h > +++ b/lib/rconn.h > @@ -36,6 +36,7 @@ > > struct vconn; > struct rconn_packet_counter; > +struct stream; > > struct rconn *rconn_create(int inactivity_probe_interval, > int max_backoff, uint8_t dscp, > @@ -47,6 +48,8 @@ void rconn_set_max_backoff(struct rconn *, int > max_backoff); > int rconn_get_max_backoff(const struct rconn *); > void rconn_set_probe_interval(struct rconn *, int > inactivity_probe_interval); > int rconn_get_probe_interval(const struct rconn *); > +void rconn_set_snoop(struct rconn *, struct stream *); > +void rconn_clear_snoop(struct rconn *); > > void rconn_connect(struct rconn *, const char *target, const char *name); > void rconn_connect_unreliably(struct rconn *, > diff --git a/ofproto/automake.mk b/ofproto/automake.mk > index af9a12a..8b1066f 100644 > --- a/ofproto/automake.mk > +++ b/ofproto/automake.mk > @@ -48,7 +48,8 @@ BUILT_SOURCES += ofproto/ipfix-entities.def > > CLEANFILES += ofproto/ipfix-entities.def > > -MAN_FRAGMENTS += ofproto/ofproto-unixctl.man > ofproto/ofproto-dpif-unixctl.man > +MAN_FRAGMENTS += ofproto/ofproto-unixctl.man > ofproto/ofproto-dpif-unixctl.man \ > + ofproto/ofproto-log-unixctl.man > > # IPFIX entity definition macros generation from IANA's XML definition. > EXTRA_DIST += ofproto/ipfix.xml > diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c > index 4264934..3defbe3 100644 > --- a/ofproto/connmgr.c > +++ b/ofproto/connmgr.c > @@ -30,6 +30,7 @@ > #include "ofp-util.h" > #include "ofpbuf.h" > #include "ofproto-provider.h" > +#include "ovs-snoop.h" > #include "pinsched.h" > #include "poll-loop.h" > #include "pktbuf.h" > @@ -172,6 +173,9 @@ struct connmgr { > struct sockaddr_in *extra_in_band_remotes; > size_t n_extra_remotes; > int in_band_queue; > + > + /* Controller logging. */ > + struct stream *snoop_stream; > }; > > static void update_in_band_remotes(struct connmgr *); > @@ -211,6 +215,7 @@ connmgr_create(struct ofproto *ofproto, > mgr->n_extra_remotes = 0; > mgr->in_band_queue = -1; > > + mgr->snoop_stream = NULL; > return mgr; > } > > @@ -814,6 +819,64 @@ add_snooper(struct connmgr *mgr, struct vconn *vconn) > vconn_close(vconn); > } > } > + > +/* Sets all controller rconns of mgr to snoop OpenFlow packets. */ > +void > +connmgr_snoop_set_all(struct connmgr *mgr) > +{ > + struct ofconn *ofconn; > + > + ovs_assert(mgr->snoop_stream); > + /* Snoop all controller rconns. */ > + HMAP_FOR_EACH (ofconn, hmap_node, &mgr->controllers) { > + rconn_set_snoop(ofconn->rconn, mgr->snoop_stream); > + } > +} > + > +/* Checks the existence of controller matching the given 'target'. > + * If a controller is found, set its rconn to snoop OpenFlow packets. */ > +int > +connmgr_snoop_set_one(struct connmgr *mgr, const char *target) > +{ > + struct ofconn *ofconn; > + > + ovs_assert(mgr->snoop_stream); > + ofconn = find_controller_by_target(mgr, target); > + if (ofconn) { > + rconn_set_snoop(ofconn->rconn, mgr->snoop_stream); > + return 0; > + } > + return -1; > +} > + > +/* Clears the snooping of all controller rconns of mgr. */ > +static void > +connmgr_snoop_clear_all(struct connmgr *mgr) > +{ > + struct ofconn *ofconn; > + > + HMAP_FOR_EACH (ofconn, hmap_node, &mgr->controllers) { > + rconn_clear_snoop(ofconn->rconn); > + } > +} > + > +/* Opens the snoop file for this bridge. */ > +int > +connmgr_log_open(struct connmgr *mgr, const char *name) > +{ > + ovs_assert(!mgr->snoop_stream); > + return snoop_open(&mgr->snoop_stream, name); > +} > + > +/* Closes the snoop file for this bridge. */ > +void > +connmgr_log_close(struct connmgr *mgr) > +{ > + snoop_close(mgr->snoop_stream); > + connmgr_snoop_clear_all(mgr); > + mgr->snoop_stream = NULL; > +} > + > > /* Public ofconn functions. */ > > diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h > index f92a523..1a7a61a 100644 > --- a/ofproto/connmgr.h > +++ b/ofproto/connmgr.h > @@ -92,6 +92,10 @@ void connmgr_reconnect(const struct connmgr *); > int connmgr_set_snoops(struct connmgr *, const struct sset *snoops); > bool connmgr_has_snoops(const struct connmgr *); > void connmgr_get_snoops(const struct connmgr *, struct sset *snoops); > +void connmgr_snoop_set_all(struct connmgr *); > +int connmgr_snoop_set_one(struct connmgr *, const char *target); > +int connmgr_log_open(struct connmgr *, const char *name); > +void connmgr_log_close(struct connmgr *); > > /* Individual connections to OpenFlow controllers. */ > enum ofconn_type ofconn_get_type(const struct ofconn *); > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 7425855..f7a4778 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -501,6 +501,9 @@ struct ofproto_dpif { > struct ovs_mutex flow_mod_mutex; > struct list flow_mods OVS_GUARDED; > size_t n_flow_mods OVS_GUARDED; > + > + /* Controller logging. */ > + bool ctrl_logging; > }; > > /* Defer flow mod completion until "ovs-appctl ofproto/unclog"? (Useful > only > @@ -1318,6 +1321,7 @@ construct(struct ofproto *ofproto_) > ofproto->n_hit = 0; > ofproto->n_missed = 0; > > + ofproto->ctrl_logging = false; > return error; > } > > @@ -5599,6 +5603,64 @@ ofproto_unixctl_fdb_show(struct unixctl_conn *conn, > int argc OVS_UNUSED, > ds_destroy(&ds); > } > > +static void > +ofproto_unixctl_log_controller(struct unixctl_conn *conn, int argc, > + const char *argv[], void *aux OVS_UNUSED) > +{ > + struct ofproto_dpif *ofproto; > + > + ofproto = ofproto_dpif_lookup(argv[1]); > + if (!ofproto) { > + unixctl_command_reply_error(conn, "Unknown bridge name"); > + return; > + } > + > + /* Stops snooping when nothing is specified in the arguments. */ > + if (argc == 2) { > + connmgr_log_close(ofproto->up.connmgr); > + ofproto->ctrl_logging = false; > + } else { > + int error; > + > + /* If bridge is currently logging controller, return. */ > + if (ofproto->ctrl_logging) { > + unixctl_command_reply_error(conn, "Bridge already in logging " > + "controllers"); > + return; > + } > + > + /* Opens the log file. If name is 'default', use the default > path. > + * Otherwise, use the specified name. */ > + error = connmgr_log_open(ofproto->up.connmgr, > + !strcmp(argv[2], "default") > + ? NULL : argv[2]); > + if (error) { > + unixctl_command_reply_error(conn, "Open log file failed"); > + return; > + } > + > + /* If nothing is specified, log all controllers. */ > + if (argc == 3) { > + connmgr_snoop_set_all(ofproto->up.connmgr); > + } else { > + size_t i; > + > + /* Log only the specified controllers. If there is a > + * incorrect name, clear all configurations. */ > + for (i = 3; i < argc; i++) { > + if(connmgr_snoop_set_one(ofproto->up.connmgr, argv[i])) { > + connmgr_log_close(ofproto->up.connmgr); > + unixctl_command_reply_error(conn, "Incorrect > controller " > + "name, reconfigure > needed"); > + return; > + } > + } > + } > + ofproto->ctrl_logging = true; > + } > + unixctl_command_reply(conn, NULL); > +} > + > struct trace_ctx { > struct xlate_out xout; > struct xlate_in xin; > @@ -6341,6 +6403,10 @@ ofproto_dpif_unixctl_init(void) > ofproto_unixctl_dpif_disable_megaflows, > NULL); > unixctl_command_register("dpif/enable-megaflows", "", 0, 0, > ofproto_unixctl_dpif_enable_megaflows, NULL); > + unixctl_command_register("log/controller", > + "bridge file|default [controllers]", > + 1, INT_MAX, ofproto_unixctl_log_controller, NULL); > + > } > > /* Linux VLAN device support (e.g. "eth0.10" for VLAN 10.) > diff --git a/ofproto/ofproto-log-unixctl.man > b/ofproto/ofproto-log-unixctl.man > new file mode 100644 > index 0000000..4357982 > --- /dev/null > +++ b/ofproto/ofproto-log-unixctl.man > @@ -0,0 +1,37 @@ > +.SS "LOG COMMANDS" > +These commands manage the logging of OpenFlow messages. > +. > +.IP "\fBofproto/trace\fR \fIbridge\fR \fIfile|default\fR > [\fIcontrollers\fR]" > +Logs the OpenFlow messages exchanged between \fIbridge\fR and > +\fIcontroller(s)\fR into \fIfile\fR. After setting the log, > +you must use \fBofproto/trace\fR \fIbridge\fR before reset the > +bridge to another set of controllers. The arguments are: > +. > +.RS > +.IP "\fIbridge\fR" > +\fIbridge\fR is the name of the target bridge. > +. > +.IP "\fIfile|default\fR" > +\fIfile\fR can be an absolute or relative path of the log file > +to be created. If there is already a file with same name, the > +OpenFLow messages are appended to the log file. If \fIdefault\fR > +is specified, the log file will be named as "ovs-snoop.log" and > +be created at same directory as \fBvlog\fR. > +. > +.IP "Incomplete information." > +If nothing is specified after the \fIfile|default\fR, Open vSwitch > +will log all OpenFlow messages between \fIbridge\fR and all its > +controller(s). Alternatively, you can specify a set of controllers > +to log using the [\fIcontrollers\fR]. > +.RS > +.IP \fIcontrollers\fR > +\fIcontrollers\fR can be one or more controller name strings in the > +same form as the output of "\fBovs-vsctl get-controller\fR" command. > +If there is an incorrect controller name, the command will configure > +nothing and report error. > +.RE > +.RE > +.IP "\fBofproto/trace\fR \fIbridge\fR" > +Resets the logging of OpenFlow messages exchanged between \fIbridge\fR > +and controller(s). After this command, the \fIbridge\fR can be reset > +to log another set of controllers. > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index 2c14af6..3b9ecf9 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -2682,3 +2682,56 @@ AT_CHECK([tail -1 stdout], [0], [Datapath actions: 5 > ]) > OVS_VSWITCHD_STOP > AT_CLEANUP > + > +AT_SETUP([ofproto-dpif - log/controller]) > +OVS_VSWITCHD_START() > + > +# Test the creation of default (ovs-snoop.log) file. > +AT_CHECK([ovs-appctl log/controller br0 default], [0]) > +AT_CHECK([ls ovs-snoop.log], [0], [dnl > +ovs-snoop.log > +]) > + > +# Test the error message of set log/controller when is already in > +# logging. > +AT_CHECK([ovs-appctl log/controller br0 default], [2], [], [dnl > +Bridge already in logging controllers > +ovs-appctl: ovs-vswitchd: server returned an error > +]) > + > +# Test the reset of log/controller. > +AT_CHECK([ovs-appctl log/controller br0], [0]) > + > +# Test the creation of specified file (./log_controller.log) file. > +AT_CHECK([ovs-appctl log/controller br0 "./log_controller.log"], [0]) > +AT_CHECK([ls log_controller.log], [0], [dnl > +log_controller.log > +]) > + > +# Test the reset of log/controller. > +AT_CHECK([ovs-appctl log/controller br0], [0]) > + > +# Test the error report of a wrong controller name. > +AT_CHECK([ovs-appctl log/controller br0 default tcp:1.2.3.4:8000], [2], > [], [dnl > +Incorrect controller name, reconfigure needed > +ovs-appctl: ovs-vswitchd: server returned an error > +]) > +# Should still be able to log/controller br0. > +AT_CHECK([ovs-appctl log/controller br0 default], [0]) > +AT_CHECK([ovs-appctl log/controller br0], [0]) > + > +# Test the use of incorrect file name. > +AT_CHECK([ovs-appctl log/controller br0 > "./non_exist_dir/log_controller.log"], [2], [], [dnl > +Open log file failed > +ovs-appctl: ovs-vswitchd: server returned an error > +]) > + > +# Detect the warning log message > +AT_CHECK([sed -n "s/^.*\(|ovs_snoop|WARN|.*\)$/\1/p" ovs-vswitchd.log], > [0], [dnl > +|ovs_snoop|WARN|failed to open ./non_exist_dir/log_controller.log for > snoop logging: No such file or directory > +]) > +# Delete the warning log message > +AT_CHECK([sed "/|ovs_snoop|WARN|/d" ovs-vswitchd.log > ovs-vswitchd.log], > [0]) > + > +OVS_VSWITCHD_STOP > +AT_CLEANUP > -- > 1.7.9.5 > >
_______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
