On Wed, Jul 26, 2017 at 01:55:14PM -0700, Justin Pettit wrote:
> Signed-off-by: Justin Pettit <jpet...@ovn.org>
checkpatch says:
WARNING: Line has non-spaces leading whitespace
#856 FILE: ovn/utilities/ovn-nbctl.c:1385:
if (log_severity_from_string(severity) == UINT8_MAX) {
parse_log_arg() ignores the later "names" if there is more than one. Is
that reasonable? Should it give an error?
parse_log_arg() issues an error "expecting integer" when there's a bad
severity id. Seems like a deceptive message.
In pinctrl_handle_log(), I think it would be wise to escape the ACL name
with json_string_escape(). It also seems a little odd to put the packet
in quotes there; it's going to be rather long and I'd usually lean
toward something like just a colon separator.
It might be worth calling VLOG_IS_INFO_ENABLED() early on in
pinctrl_handle_log(), to allow bailing out cheaply if nothing is going
to be logged.
I find myself wondering whether the name should just be the tail of the
log_pin_header without bothering with a length. It's actually easier to
deal with: once you've pulled the header, you can get the name easily:
size_t name_len = userdata->size;
char *name = name_len ? xmemdup0(userdata->data, name_len) : NULL;
in pinctrl_handle_log().
I think that the documentation in ovn-nb.xml and ovn-sb.xml can be
improved. I'm appending suggestions.
I'm appending a suggested incremental that acts on some of my comments
above.
--8<--------------------------cut here-------------------------->8--
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index 6a8f54523f2c..d09529b52fdd 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -1763,7 +1763,6 @@ ovnact_dns_lookup_free(struct ovnact_dns_lookup *dl
OVS_UNUSED)
static void
parse_log_arg(struct action_context *ctx, struct ovnact_log *log)
{
-
if (lexer_match_id(ctx->lexer, "verdict")) {
if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
return;
@@ -1808,15 +1807,11 @@ parse_log_arg(struct action_context *ctx, struct
ovnact_log *log)
uint8_t severity = log_severity_from_string(ctx->lexer->token.s);
if (severity != UINT8_MAX) {
log->severity = severity;
- } else {
- lexer_syntax_error(ctx->lexer, "expecting integer");
lexer_get(ctx->lexer);
return;
}
- } else {
- lexer_syntax_error(ctx->lexer, "expecting token");
}
- lexer_get(ctx->lexer);
+ lexer_syntax_error(ctx->lexer, "expecting severity");
} else {
lexer_syntax_error(ctx->lexer, NULL);
}
@@ -1852,11 +1847,7 @@ format_LOG(const struct ovnact_log *log, struct ds *s)
}
ds_put_format(s, "verdict=%s, ", log_verdict_to_string(log->verdict));
- ds_put_format(s, "severity=%s, ", log_severity_to_string(log->severity));
-
- ds_chomp(s, ' ');
- ds_chomp(s, ',');
- ds_put_cstr(s, ");");
+ ds_put_format(s, "severity=%s);", log_severity_to_string(log->severity));
}
static void
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index 123b43525843..31303a846a51 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -958,13 +958,6 @@
<ref column="match"/>, and <code>deny</code> as <ref column="action"/>.)
</p>
- <column name="name">
- <p>
- A name for the ACL. The name has no special meaning, but may be
- used by the logging system to identify the source rule.
- </p>
- </column>
-
<column name="priority">
<p>
The ACL rule's priority. Rules with numerically higher priority
@@ -1042,24 +1035,43 @@
</ul>
</column>
- <column name="log">
+ <group title="Logging">
<p>
- If set to <code>true</code>, packets that match the ACL will trigger a
- log message on the transport node or nodes that perform ACL processing.
- Logging may be combined with any <ref column="action"/>.
+ These columns control whether and how OVN logs packets that match an
+ ACL.
</p>
- </column>
- <column name="severity">
- <p>
- When <ref column="log"/> is <code>true</code> indicates the
- severity of the ACL. The severity levels match those of syslog
- with the following values (from more to less serious):
- <code>alert</code>, <code>warning</code>, <code>notice</code>,
- <code>info</code>, or <code>debug</code>. If a severity is not
- specified, the default is <code>info</code>.
- </p>
- </column>
+ <column name="log">
+ <p>
+ If set to <code>true</code>, packets that match the ACL will trigger
+ a log message on the transport node or nodes that perform ACL
+ processing. Logging may be combined with any <ref column="action"/>.
+ </p>
+
+ <p>
+ If set to <code>false</code>, the remaining columns in this group
+ have no significance.
+ </p>
+ </column>
+
+ <column name="name">
+ <p>
+ This name, if it is provided, is included in log records. It
+ provides the administrator and the cloud management system a way to
+ associate a log record with a particular ACL.
+ </p>
+ </column>
+
+ <column name="severity">
+ <p>
+ The severity of the ACL. The severity levels match those of syslog,
+ in decreasing level of severity: <code>alert</code>,
+ <code>warning</code>, <code>notice</code>, <code>info</code>, or
+ <code>debug</code>. When the column is empty, the default is
+ <code>info</code>.
+ </p>
+ </column>
+ </group>
<group title="Common Columns">
<column name="external_ids">
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index 3ffc2c417d9e..b254931412bb 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -1517,33 +1517,42 @@
<dl>
<dt>
- <code>log(<var>argument</var>, </code>...<code>);</code>
+ <code>log(<var>key</var>=<var>value</var>, </code>...<code>);</code>
</dt>
<dd>
<p>
- Send the packet to <code>ovn-controller</code> for potential
- logging. The local <code>ovn-controller</code> will make a
- decision whether or where to log the packet.
- </p>
- <p>
- The <code>log</code> action takes zero or more of the
- following <var>arguments</var>:
- </p>
- <ul>
- <li><code>name</code> An optional name for the ACL.</li>
- <li><code>severity</code> An indication of the severity
- of the event. The value is one of following (from more to
- less serious): <code>alert</code>, <code>warning</code>,
- <code>notice</code>, <code>info</code>, or
- <code>debug</code>. If a severity is not provided, the
- default is <code>info</code>.
- </li>
- <li><code>verdict</code> The verdict for packets matching
- the flow. One of <code>allow</code>, <code>deny</code>,
- or <code>reject</code>.
- </li>
- </ul>
+ Causes <code>ovn-controller</code> to log the packet on the chassis
+ that processes it. Packet logging currently uses the same logging
+ mechanism as other Open vSwitch and OVN messages, which means that
+ whether and where log messages appear depends on the local logging
+ configuration that can be configured with <code>ovs-appctl</code>,
+ etc.
+ </p>
+ <p>
+ The <code>log</code> action takes zero or more of the following
+ key-value pair arguments that control what is logged:
+ </p>
+ <dl>
+ <dt><code>name=</code><var>string</var></dt>
+ <dd>
+ An optional name for the ACL. The <var>string</var> is
+ currently limited to 64 bytes.
+ </dd>
+ <dt><code>severity=</code><var>level</var></dt>
+ <dd>
+ Indicates the severity of the event. The <var>level</var> is one
+ of following (from more to less serious): <code>alert</code>,
+ <code>warning</code>, <code>notice</code>, <code>info</code>, or
+ <code>debug</code>. If a severity is not provided, the default
+ is <code>info</code>.
+ </dd>
+ <dt><code>verdict=</code><var>value</var></dt>
+ <dd>
+ The verdict for packets matching the flow. The value must be one
+ of <code>allow</code>, <code>deny</code>, or <code>reject</code>.
+ </dd>
+ </dl>
</dd>
</dl>
diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
index 122fb03d12fe..d26677db9ec1 100644
--- a/ovn/utilities/ovn-nbctl.8.xml
+++ b/ovn/utilities/ovn-nbctl.8.xml
@@ -76,7 +76,7 @@
<h1>Logical Switch ACL Commands</h1>
<dl>
- <dt>[<code>--log</code>] [<code>--may-exist</code>] <code>acl-add</code>
<var>switch</var> <var>direction</var> <var>priority</var> <var>match</var>
<var>verdict</var> [<code>severity=</code><var>severity</var>]
[<code>name=</code><var>name</var>]</dt>
+ <dt>[<code>--log</code>] [<code>--severity=</code><var>severity</var>]
[<code>--name=</code><var>name</var>] [<code>--may-exist</code>]
<code>acl-add</code> <var>switch</var> <var>direction</var> <var>priority</var>
<var>match</var> <var>verdict</var></dt>
<dd>
<p>
Adds the specified ACL to <var>switch</var>.
@@ -91,13 +91,12 @@
</p>
<p>
- If <code>--log</code> is specified, packet logging is enabled
- for the ACL. When logging is enabled, the optional arguments
- <code>severity</code> and <code>name</code> specify a severity
- and name for the log entries, respectively. The severity must
- be one of <code>alert</code>, <code>warning</code>,
- <code>notice</code>, <code>info</code>, or <code>debug</code>.
- If a severity is not specified, the default is
+ The <code>--log</code> option enables packet logging for the ACL.
+ The options <code>--severity</code> and <code>--name</code> specify a
+ severity and name, respectively, for log entries (and also enable
+ logging). The severity must be one of <code>alert</code>,
+ <code>warning</code>, <code>notice</code>, <code>info</code>, or
+ <code>debug</code>. If a severity is not specified, the default is
<code>info</code>.
</p>
</dd>
diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index c95926768014..0a928cccec81 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -333,7 +333,8 @@ Logical switch commands:\n\
ls-list print the names of all logical switches\n\
\n\
ACL commands:\n\
- acl-add [--log] SWITCH DIRECTION PRIORITY MATCH ACTION\n\
+ [--log] [--severity=SEVERITY] [--name=NAME] [--may-exist]\n\
+ acl-add SWITCH DIRECTION PRIORITY MATCH ACTION\n\
add an ACL to SWITCH\n\
acl-del SWITCH [DIRECTION [PRIORITY MATCH]]\n\
remove ACLs from SWITCH\n\
@@ -1371,24 +1372,21 @@ nbctl_acl_add(struct ctl_context *ctx)
nbrec_acl_set_match(acl, ctx->argv[4]);
nbrec_acl_set_action(acl, action);
- if (shash_find(&ctx->options, "--log") != NULL) {
+ /* Logging options. */
+ bool log = shash_find(&ctx->options, "--log") != NULL;
+ const char *severity = shash_find_data(&ctx->options, "--severity");
+ const char *name = shash_find_data(&ctx->options, "--name");
+ if (log || severity || name) {
nbrec_acl_set_log(acl, true);
}
-
- /* Optional fields for logging. */
- char **settings = (char **) &ctx->argv[6];
- int n_settings = ctx->argc - 6;
-
- for (int i = 0; i < n_settings; i++) {
- if (!strncmp(settings[i], "severity=", 9)) {
- const char *severity = settings[i] + 9;
- if (log_severity_from_string(severity) == UINT8_MAX) {
- ctl_fatal("bad severity: %s", severity);
- }
- nbrec_acl_set_severity(acl, severity);
- } else if (!strncmp(settings[i], "name=", 5)) {
- nbrec_acl_set_name(acl, settings[i] + 5);
+ if (severity) {
+ if (log_severity_from_string(severity) == UINT8_MAX) {
+ ctl_fatal("bad severity: %s", severity);
}
+ nbrec_acl_set_severity(acl, severity);
+ }
+ if (name) {
+ nbrec_acl_set_name(acl, name);
}
/* Check if same acl already exists for the ls */
@@ -3561,7 +3559,7 @@ static const struct ctl_command_syntax nbctl_commands[] =
{
{ "ls-list", 0, 0, "", NULL, nbctl_ls_list, NULL, "", RO },
/* acl commands. */
- { "acl-add", 5, 8, "SWITCH DIRECTION PRIORITY MATCH ACTION", NULL,
+ { "acl-add", 5, 6, "SWITCH DIRECTION PRIORITY MATCH ACTION", NULL,
nbctl_acl_add, NULL, "--log,--may-exist,--name=,--severity=", RW },
{ "acl-del", 1, 4, "SWITCH [DIRECTION [PRIORITY MATCH]]", NULL,
nbctl_acl_del, NULL, "", RW },
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
