> On Jul 27, 2017, at 4:03 PM, Han Zhou <zhou...@gmail.com> wrote:
>
>
>
> On Wed, Jul 26, 2017 at 1:55 PM, Justin Pettit <jpet...@ovn.org> wrote:
> >
> > Signed-off-by: Justin Pettit <jpet...@ovn.org>
> > ---
> > NEWS | 1 +
> > include/ovn/actions.h | 67 ++++++++++++-------
> > ovn/controller/ovn-controller.8.xml | 9 +++
> > ovn/controller/pinctrl.c | 38 +++++++++++
> > ovn/lib/actions.c | 130
> > ++++++++++++++++++++++++++++++++++++
> > ovn/lib/automake.mk | 2 +
> > ovn/lib/ovn-log.c | 69 +++++++++++++++++++
> > ovn/lib/ovn-log.h | 52 +++++++++++++++
> > ovn/northd/ovn-northd.c | 77 ++++++++++++++++++---
> > ovn/ovn-nb.ovsschema | 15 ++++-
> > ovn/ovn-nb.xml | 16 ++++-
> > ovn/ovn-sb.xml | 32 +++++++++
> > ovn/utilities/ovn-nbctl.8.xml | 35 +++++++---
> > ovn/utilities/ovn-nbctl.c | 24 ++++++-
> > ovn/utilities/ovn-trace.c | 19 ++++++
> > tests/ovn.at | 105 +++++++++++++++++++++++++++++
> > 16 files changed, 640 insertions(+), 51 deletions(-)
> > create mode 100644 ovn/lib/ovn-log.c
> > create mode 100644 ovn/lib/ovn-log.h
> >
>
> Thanks for the patch!
Thanks for your patience on it.
> It seems "invalid" packets won't get logged in this patch. I think it would
> be useful: in addition to log packets per-ACL, enable logging for packets
> returned as "invalid" state from conntrack. It can be a global configuration
> to enable/disable this behavior.
That's a great point. I'd meant to do that when I started this series, but
forgot along the way. I think we need more than just logging for invalid
packets, though, so I'll add that as a follow-up patch.
> > static void
> > +pinctrl_handle_log(struct dp_packet *packet OVS_UNUSED,
>
> Why need this parameter if it is not useful?
Fixed.
> > + const struct flow *headers,
> > + struct ofpbuf *userdata)
> > +{
> > + struct log_pin_header *lph = ofpbuf_try_pull(userdata, sizeof *lph);
> > + if (!lph) {
> > + VLOG_WARN("log data missing");
> > + return;
> > + }
> > +
> > + int name_len = ntohs(lph->name_len);
> > + char *name = xmalloc(name_len+1);
> > + if (name_len) {
> > + char *tmp_name = ofpbuf_try_pull(userdata, name_len);
> > + if (!name) {
>
> It should be: if (!tmp_name)
Good catch. Fixed.
> > + VLOG_WARN("name missing");
> > + free(name);
> > + return;
> > + }
> > + memcpy(name, tmp_name, name_len);
> > + }
> > + name[name_len] = '\0';
> > +
> > + char *packet_str = flow_to_string(headers, NULL);
> > + VLOG_INFO("ACL name=%s, verdict=%s, severity=%s, packet=\"%s\"",
>
> The log action may be more generic than just for ACL. So would it better to
> avoid hardcode "ACL" here in the message? "ACL" could be indicated in verdict
> instead, since there could be more use cases that need packet logging in the
> future.
These are part of the ACL infrastructure, so I'd think ACL makes sense. I also
wanted to give some sort of handle that someone can easily grep through the
logs. Do you have a specific suggestion?
> > +enum log_verdict {
> > + LOG_VERDICT_ALLOW,
> > + LOG_VERDICT_DROP,
> > + LOG_VERDICT_REJECT,
>
> Better to be LOG_VERDICT_ACL_ALLOW, LOG_VERDICT_ACL_DROP,
> LOG_VERDICT_ACL_REJECT, so that in the future we can support other use cases
> for logging.
Do you have an example or two of the type of thing you think we might need in
the future? If it needs to be that flexible, we could just pass around
strings, but I do worry about passing too much data through the datapath upcall
mechanism.
> > static void
> > +build_acl_log(struct ds *actions, const struct nbrec_acl *acl)
> > +{
> > + if (!acl->log) {
> > + return;
> > + }
> > +
> > + ds_put_cstr(actions, "log(");
> > +
> > + if (acl->name) {
> > + ds_put_format(actions, "name=\"%s\", ", acl->name);
>
> Would it be good to use first byte of ACL uuid (which is also used as
> stage_hint) when name is empty?
That's an interesting idea, but I have mixed feelings about it. The first byte
would only allow 16 different identifiers, but I think generally people have
many more. We could use more bytes from the UUID, but these logs could last
quite a while, so it may cause confusion if they've changed, and people are
trying to correlate them later. Obviously, if they really want to correlate
them, they should use a name, but I wonder if it would be just useful enough to
be frustrating.
> >
> > + <column name="severity">
> > <p>
> > - Logging is not yet implemented.
> > + When <ref column="log"/> is <code>true</code> indicates the
> > + severity of the ACL. The severity levels match those of syslog
>
> s/severity of the ACL/severity of the log/ because ACL doesn't have severity.
I'm worried that that phrasing may indicate that it's the level it will logged
at, when it's really just an indication by the user about how concerned they
are with this ACL (or whatever you want to call it) entry.
> > @@ -332,7 +333,7 @@ Logical switch commands:\n\
> > ls-list print the names of all logical switches\n\
> > \n\
> > ACL commands:\n\
> > - acl-add SWITCH DIRECTION PRIORITY MATCH ACTION [log]\n\
> > + acl-add [--log] SWITCH DIRECTION PRIORITY MATCH ACTION\n\
>
> The optional "name" and "severity" are missing in the help message.
Yes, I was running out of space on an 80-column terminal. I think we have
other examples of dropping non-essential parameters. I'll poke around to see
what other project do.
> Acked-by: Han Zhou <zhou...@gmail.com>
Thanks!
--Justin
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
