Hi Dumitru,
Thanks for the comments. Have addressed all except one. Please see below.
Thanks,
Sragdhara
From: Dumitru Ceara <dce...@redhat.com>
Date: Thursday, August 21, 2025 at 3:23 AM
To: Sragdhara Datta Chaudhuri <sragdha.chau...@nutanix.com>,
ovs-dev@openvswitch.org <ovs-dev@openvswitch.org>
Cc: Numan Siddique <num...@ovn.org>
Subject: Re: [ovs-dev] [PATCH OVN v6 2/5] ovn-nbctl: Network Function insertion
commands.
!-------------------------------------------------------------------|
CAUTION: External Email
|-------------------------------------------------------------------!
On 8/20/25 3:25 AM, Sragdhara Datta Chaudhuri wrote:
> Network function:
> network-function-add <nf-name> <inport> <outport> - Add a new NF
> network-function-del <nf-name-or-uuid> - Delete a NF
> network-function-list - List all NFs
>
> Network function group:
> network-function-group-add <nfg-name> [<nf1> <nf2> ...] - Add a new NFG
> network-function-group-del <nfg-name-or-uuid> - Delete a NFG
> network-function-group-list [<nfg-name-or-uuid>] - List all NFGs
> network-function-group-add-network-function <nfg-name-or-uuid>
> <nf-name-or-uuid>
> - Add a NF to a NFG
> network-function-group-del-network-function <nfg-name-or-uuid>
> <nf-name-or-uuid>
> - Remove a NF from a NFG
>
> ACL (new optional parameter added for NFG):
> acl-add <ls>|<pg> <direction> <priority> <match> <action> [<nfg-name-or-uuid>]
>
> Signed-off-by: Sragdhara Datta Chaudhuri <sragdha.chau...@nutanix.com>
> Acked-by: Naveen Yerramneni <naveen.yerramn...@nutanix.com>
> Acked-by:Numan Siddique <num...@ovn.org>
> ---
Hi Sragdhara,
> utilities/ovn-nbctl.8.xml | 106 +++++++-
> utilities/ovn-nbctl.c | 533 +++++++++++++++++++++++++++++++++++++-
> 2 files changed, 635 insertions(+), 4 deletions(-)
>
> diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml
> index 60936a2b5..ce8f6f3a4 100644
> --- a/utilities/ovn-nbctl.8.xml
> +++ b/utilities/ovn-nbctl.8.xml
> @@ -399,7 +399,7 @@
> must be either <code>switch</code> or <code>port-group</code>.
> </p>
> <dl>
> - <dt>[<code>--type=</code>{<code>switch</code> |
> <code>port-group</code>}] [<code>--log</code>]
> [<code>--meter=</code><var>meter</var>]
> [<code>--severity=</code><var>severity</var>]
> [<code>--name=</code><var>name</var>] [<code>--label=</code><var>label</var>]
> [<code>--sample-new=</code><var>sample</var>]
> [<code>--sample-est=</code><var>sample</var>] [<code>--may-exist</code>]
> [<code>--apply-after-lb</code>] [<code>--tier</code>] <code>acl-add</code>
> <var>entity</var> <var>direction</var> <var>priority</var> <var>match</var>
> <var>verdict</var></dt>
> + <dt>[<code>--type=</code>{<code>switch</code> |
> <code>port-group</code>}] [<code>--log</code>]
> [<code>--meter=</code><var>meter</var>]
> [<code>--severity=</code><var>severity</var>]
> [<code>--name=</code><var>name</var>] [<code>--label=</code><var>label</var>]
> [<code>--sample-new=</code><var>sample</var>]
> [<code>--sample-est=</code><var>sample</var>] [<code>--may-exist</code>]
> [<code>--apply-after-lb</code>] [<code>--tier</code>] <code>acl-add</code>
> <var>entity</var> <var>direction</var> <var>priority</var> <var>match</var>
> <var>verdict</var> [<var>network-function-group</var>]</dt>
> <dd>
> <p>
> Adds the specified ACL to <var>entity</var>. <var>direction</var>
> @@ -472,6 +472,110 @@
> </dd>
> </dl>
>
> + <h2>Network Function Commands</h2>
> +
> + <dl>
> + <dt>[<code>--may-exist</code>] | <code>--add-duplicate</code>]
> <code>network-function-add</code> <var>nf</var> <var>inport</var>
> <var>outport</var></dt>
> + <dd>
> + <p>
> + Creates a new network function named <var>nf</var> with logical
> + switch ports <var>inport</var> and <var>outport</var>. Both the
> + ports must be on the same logical switch and must be already
> + created. When used in an ACL action, traffic matching the ACL
> + are redirected to the <var>inport</var> if it is from-lport ACL
> + and to the <var>outport</var> if it is to-lport ACL. The response
> + packets are sent through the same ports in reverse order.
> + </p>
> +
> + <p>
> + Without any options, this command regards it as an error if
> + <var>nf</var> is a duplicate name. With <code>--may-exist</code>,
> + adding a duplicate name succeeds but does not create a new network
> + function. With <code>--add-duplicate</code>, the command really
> + creates a new network function with a duplicate name. It is an
> + error to specify both options. If there are multiple network
> + functions with the same name, configure the network functions
> + using the UUID instead of the <var>nf</var> name.
Why would we allow network functions with duplicate names? In any case,
if we do want to allow that, please ignore my comment as adding the
network function "name" as an unique index in the schema. We still need
the "id" to be a unique index.
In general, all new tables in the NB/SB schema should have at least one
unique index. That's a good practice and avoids the (limited) risk of
spurious duplicates in the database when running with raft, see the
following examples for situations where we had to handle spurious
records that were created due to lack of uniqueness constraints:
https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ovn-2Dorg_ovn_commit_9deb000&d=DwICaQ&c=s883GpUCOChKOHiocYtGcg&r=uXnTjPDrt8WYa8nbZqANTqL0TyzFTTKpPHphGFPgvBw&m=d2L3VNIWqglvWWyaeRvu_gD66zSbKUIIFck4XCsYyZJGC3bR6FcXKK3Z6vyz34pw&s=gT__RfP5hrGjMon1lcpiKymnuhFPIVVdj0tviEgAE5g&e=
https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ovn-2Dorg_ovn_commit_b5387b3&d=DwICaQ&c=s883GpUCOChKOHiocYtGcg&r=uXnTjPDrt8WYa8nbZqANTqL0TyzFTTKpPHphGFPgvBw&m=d2L3VNIWqglvWWyaeRvu_gD66zSbKUIIFck4XCsYyZJGC3bR6FcXKK3Z6vyz34pw&s=QmPqb3CVN-FSQfy0_3Kh9aOoHIp3ziMRvxS3_kSEOb4&e=
[Sragdhara] Not allowing duplicate any more after adding index in the schema.
> + </p>
> + </dd>
> +
> + <dt>[<code>--if-exists</code>] <code>network-function-del</code>
> <var>nf</var></dt>
> + <dd>
> + <p>
> + Deletes <var>nf</var> specified as name or uuid. It is an error if
> + <var>nf</var> does not exist, unless <code>--if-exists</code> is
> + specified.
> + </p>
> + </dd>
> +
> + <dt><code>network-function-list</code></dt>
> + <dd>
> + <p>
> + Lists all network functions.
> + </p>
> + </dd>
> +
> + <dt>[<code>--may-exist</code>] | <code>--add-duplicate</code>]
> <code>network-function-group-add</code> <var>nfg</var> [<var>nf</var>]...</dt>
> + <dd>
> + <p>
> + Creates a new network function group named <var>nfg</var> with
> + optionally one of more <code>nf</code> added to the group. The nfs
> + must be already created. Traffic redirection would be done towards
> + one of the active network functions, based on health monitoring.
> + If all are down, any one would be chosen for redirection.
> + </p>
> +
> + <p>
> + Without any options, this command regards it as an error if
> + <var>nfg</var> is a duplicate name. With <code>--may-exist</code>,
> + adding a duplicate name succeeds but does not create a new network
> + function group. With <code>--add-duplicate</code>, the command
> + really creates a new network function group with a duplicate name.
I have here the same comment about name uniqueness as above.
> + It is an error to specify both options. If there are multiple
> + network function groups with the same name, configure the network
> + function groups using the UUID instead of the <var>nfg</var> name.
> + </p>
> + </dd>
> +
> + <dt>[<code>--if-exists</code>] <code>network-function-group-del</code>
> <var>nfg</var></dt>
> + <dd>
> + <p>
> + Deletes <var>nfg</var> specified as name or uuid. It is an error if
> + <var>nfg</var> does not exist, unless <code>--if-exists</code> is
> + specified.
> + </p>
> + </dd>
> +
> + <dt><code>network-function-group-list</code></dt>
> + <dd>
> + <p>
> + Lists all network function groups.
> + </p>
> + </dd>
> +
> + <dt>[<code>--may-exist</code>]
> <code>network-function-group-add-network-function</code> <var>nfg</var>
> <var>nf</var></dt>
> + <dd>
> + <p>
> + Add a network function named <var>nf</var> to a network function
> + group named <var>nfg</var>. It is an error if <var>nfg</var> or
> + <var>nf</var> does not exist. Unless <code>--may-exist</code> is
> + specified, it is an error if the <var>nf</var> being added is
> + already a part of the <var>nfg</var>.
> + </p>
> + </dd>
> +
> + <dt><code>network-function-group-del-network-function</code>
> <var>nfg</var> <var>nf</var></dt>
> + <dd>
> + <p>
> + Delete a network function named <var>nf</var> from a network
> function
> + group named <var>nfg</var>. It is an error if <var>nfg</var> or
> + <var>nf</var> does not exist. It is an error if <var>nf</var> is
> not
> + a part of the <var>nfg</var>, unless <code>--if-exists</code> is
> + specified.
> + </p>
> + </dd>
> + </dl>
> +
> <h2>Logical Switch QoS Rule Commands</h2>
> <dl>
> <dt>[<code>--may-exist</code>] <code>qos-add</code>
> <var>switch</var> <var>direction</var> <var>priority</var> <var>match</var>
> [<code>mark=</code><var>mark</var>] [<code>dscp=</code><var>dscp</var>]
> [<code>rate=</code><var>rate</var> [<code>burst=</code><var>burst</var>]]</dt>
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 58517f966..5c0874106 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -283,7 +283,7 @@ Logical switch commands:\n\
> \n\
> ACL commands:\n\
> [--type={switch | port-group}] [--log] [--severity=SEVERITY] [--name=NAME]
> [--may-exist]\n\
> - acl-add {SWITCH | PORTGROUP} DIRECTION PRIORITY MATCH ACTION\n\
> + acl-add {SWITCH | PORTGROUP} DIRECTION PRIORITY MATCH ACTION
> [NETWORK-FUNCTION-GROUP]\n\
> add an ACL to SWITCH/PORTGROUP\n\
> [--type={switch | port-group}]\n\
> acl-del {SWITCH | PORTGROUP} [DIRECTION [PRIORITY MATCH]]\n\
> @@ -373,6 +373,26 @@ Forwarding group commands:\n\
> fwd-group-del GROUP delete a forwarding group\n\
> fwd-group-list [SWITCH] print forwarding groups\n\
> \n\
> +Network function group commands:\n\
> + network-function-group-add NETWORK-FUNCTION-GROUP [NETWORK-FUNCTION]...\n\
> + create a network-functionr-group\n\
Typo: network-functionr-group
> + network-function-group-del NETWORK-FUNCTION-GROUP\n\
> + delete a network-function-group\n\
> + network-function-group-list print all network-function-groups\n\
> + network-function-group-add-network-function NETWORK-FUNCTION-GROUP
> NETWORK-FUNCTION\n\
> + add a network-function to a\n\
> + network-function-group\n\
> + network-function-group-del-network-function NETWORK-FUNCTION-GROUP
> NETWORK-FUNCTION\n\
> + delete a network-function from a\n\
> + network-function-group\n\
> +\n\
> +Network function commands:\n\
> + network-function-add NETWORK-FUNCTION PORT-IN PORT-OUT\n\
> + create a network-function\n\
> + network-function-del NETWORK-FUNCTION delete a network-function\n\
> + network-function-list print all network-functions\n\
> +\n\n",program_name, program_name);
> + printf("\
Why do we need a new printf() here? Why not just update the string?
[Sragdhara] Needed to break it here, otherwise “line too long” error was coming
from test.
> Logical router commands:\n\
> lr-add [ROUTER] create a logical router named ROUTER\n\
> lr-del ROUTER delete ROUTER and all its ports\n\
> @@ -429,7 +449,7 @@ Policy commands:\n\
> lr-policy-del ROUTER [{PRIORITY | UUID} [MATCH]]\n\
> remove policies from ROUTER\n\
> lr-policy-list ROUTER print policies for ROUTER\n\
> -\n\n",program_name, program_name);
> +\n\n");
> printf("\
> NAT commands:\n\
> [--stateless]\n\
> @@ -2135,6 +2155,462 @@ nbctl_lsp_get_ls(struct ctl_context *ctx)
> }
> }
>
> +
> +static char * OVS_WARN_UNUSED_RESULT
> +nf_group_by_name_or_uuid(
> + struct ctl_context *ctx, const char *id,
> + const bool must_exist,
> + const struct nbrec_network_function_group **nf_group_p)
> +{
> + const struct nbrec_network_function_group *nf_group = NULL;
> + struct uuid nf_group_uuid;
> + bool is_uuid = uuid_from_string(&nf_group_uuid, id);
> +
> + *nf_group_p = NULL;
> + if (is_uuid) {
> + nf_group = nbrec_network_function_group_get_for_uuid(ctx->idl,
> + &nf_group_uuid);
> + }
> +
> + if (!nf_group) {
> + const struct nbrec_network_function_group *iter;
> + NBREC_NETWORK_FUNCTION_GROUP_FOR_EACH (iter, ctx->idl) {
> + if (strcmp(iter->name, id)) {
> + continue;
> + }
> + if (nf_group) {
> + return xasprintf("Multiple network function group named
> '%s'. "
> + "Use a UUID.", id);
> + }
> + nf_group = iter;
We don't really need the local 'nf_group' variable, we could just use
'*nf_group_p' instead.
> + }
> + }
> + if (!nf_group && must_exist) {
> + return xasprintf("%s: network function group %s not found", id,
> + is_uuid ? "UUID" : "name");
> + }
> +
> + *nf_group_p = nf_group;
> + return NULL;
> +}
> +
> +static char * OVS_WARN_UNUSED_RESULT
> +nf_by_name_or_uuid(
> + struct ctl_context *ctx, const char *id,
> + bool must_exist, const struct nbrec_network_function **nf_p)
> +{
> + const struct nbrec_network_function *nf = NULL;
> + struct uuid nf_uuid;
> + bool is_uuid = uuid_from_string(&nf_uuid, id);
> +
> + *nf_p = NULL;
> + if (is_uuid) {
> + nf = nbrec_network_function_get_for_uuid(ctx->idl, &nf_uuid);
> + }
> +
> + if (!nf) {
> + const struct nbrec_network_function *iter;
> + NBREC_NETWORK_FUNCTION_FOR_EACH (iter, ctx->idl) {
> + if (strcmp(iter->name, id)) {
> + continue;
> + }
> + if (nf) {
> + return xasprintf("Multiple network functions named '%s'. "
> + "Use a UUID.", id);
> + }
> + nf = iter;
We don't really need the local 'nf' variable, we could just use '*nf_p'
instead.
> + }
> + }
> + if (!nf && must_exist) {
> + return xasprintf("%s: network function %s not found", id,
> + is_uuid ? "UUID" : "name");
> + }
> +
> + *nf_p = nf;
> + return NULL;
> +}
> +
> +/*
> + * Network Function Groups CLI Functions
> + */
> +static void
> +nbctl_pre_nf_group_add(struct ctl_context *ctx)
> +{
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_col_name);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_group_col_name);
> + ovsdb_idl_add_column(
> + ctx->idl, &nbrec_network_function_group_col_network_function);
> +}
> +
> +static char *
> +set_network_function_in_network_function_group(
> + struct ctl_context *ctx,
> + const struct nbrec_network_function_group *nf_group,
> + char **new_network_function, size_t num_new_network_function)
> +{
> + struct nbrec_network_function **nfs;
> + nfs = xmalloc(sizeof *nfs * num_new_network_function);
Nit:
struct nbrec_network_function **nfs =
xmalloc(...)
> +
> + size_t i;
> + char *error = NULL;
> + for (i = 0; i < num_new_network_function; i++) {
Please add the size_t here, to restrict the scope of the variable:
for (size_t i = 0; ...
> + const struct nbrec_network_function *nf;
> + error = nf_by_name_or_uuid(ctx, new_network_function[i], true, &nf);
> + if (error) {
> + free(nfs);
> + return error;
> + }
> + nfs[i] = (struct nbrec_network_function *) nf;
For better type safety please use:
nfs[i] = CONST_CAST(struct nbrec_network_function *, nf);
> + }
> + nbrec_network_function_group_set_network_function(
> + nf_group, nfs, num_new_network_function);
> + free(nfs);
> + return error;
> +}
> +
> +static void
> +nbctl_nf_group_add(struct ctl_context *ctx)
> +{
> + const struct nbrec_network_function_group *nf_group;
> + const char *nfg_name = ctx->argv[1];
> +
> + const bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> + const bool add_duplicate = shash_find(&ctx->options,
No need for "const" for both of the variables above. This comment
applies to all "const bool" variable definitions in this patch.
> + "--add-duplicate") != NULL;
Nit: indentation. I think I'd write this as:
bool add_duplicate =
shash_find(&ctx->options, "--add-duplicate") != NULL;
> +
> + if (!add_duplicate) {
> + NBREC_NETWORK_FUNCTION_GROUP_FOR_EACH (nf_group, ctx->idl) {
> + if (!strcmp(nf_group->name, nfg_name)) {
> + if (may_exist) {
> + return;
> + }
> + ctl_error(ctx, "%s: a network-function-group with this name "
> + "already exists", nfg_name);
> + return;
> + }
> + }
> + }
> +
> + nf_group = nbrec_network_function_group_insert(ctx->txn);
> + nbrec_network_function_group_set_name(nf_group, nfg_name);
> + nbrec_network_function_group_set_mode(nf_group, "inline");
> +
> + if (ctx->argc > 2) {
> + ctx->error = set_network_function_in_network_function_group(
> + ctx, nf_group, ctx->argv + 2, ctx->argc - 2);
> + }
> +}
> +
> +static void
> +nbctl_pre_nf_group_del(struct ctl_context *ctx)
> +{
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_group_col_name);
> +}
> +
> +static void
> +nbctl_nf_group_del(struct ctl_context *ctx)
> +{
> + const struct nbrec_network_function_group *nf_group;
> + const bool must_exist = !shash_find(&ctx->options, "--if-exists");
> +
> + char *error = nf_group_by_name_or_uuid(
> + ctx, ctx->argv[1], must_exist, &nf_group);
> + if (error) {
> + ctx->error = error;
> + return;
> + }
> + if (!nf_group) {
> + return;
> + }
> +
> + nbrec_network_function_group_delete(nf_group);
> +}
> +
> +static void
> +nbctl_pre_nf_group_list(struct ctl_context *ctx)
> +{
> + ovsdb_idl_add_column(ctx->idl,
> + &nbrec_network_function_group_col_network_function);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_group_col_name);
> +}
> +
> +static void
> +nbctl_nf_group_list(struct ctl_context *ctx)
> +{
> + const struct nbrec_network_function_group *nf_group;
> + struct smap nf_groups;
> + size_t i;
> +
> + smap_init(&nf_groups);
Slightly better:
struct smap nf_groups = SMAP_INITIALIZER(&nf_groups);
> +
> + NBREC_NETWORK_FUNCTION_GROUP_FOR_EACH (nf_group, ctx->idl) {
> + smap_add_format(&nf_groups, nf_group->name,
> + UUID_FMT " (%s)",
> + UUID_ARGS(&nf_group->header_.uuid),
> + nf_group->name);
> + }
> + const struct smap_node **nodes = smap_sort(&nf_groups);
> + for (i = 0; i < smap_count(&nf_groups); i++) {
for (size_t i = 0; ...)
> + const struct smap_node *node = nodes[i];
> + ds_put_format(&ctx->output, "%s\n", node->value);
> + }
> + smap_destroy(&nf_groups);
> + free(nodes);
> +}
> +
> +static void
> +nbctl_pre_nf_group_add_network_function(struct ctl_context *ctx)
> +{
> + ovsdb_idl_add_column(ctx->idl,
> + &nbrec_network_function_group_col_network_function);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_group_col_name);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_col_name);
> +}
> +
> +static void
> +nbctl_nf_group_add_network_function(struct ctl_context *ctx)
> +{
> + const struct nbrec_network_function_group *nf_group;
> + const struct nbrec_network_function *nf;
> + const bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> +
> + char * error = nf_group_by_name_or_uuid(ctx, ctx->argv[1], true,
> + &nf_group);
> + if (error) {
> + ctx->error = error;
> + return;
> + }
> + if (!nf_group) {
> + return;
> + }
> +
> + /* Check that network-function exists */
> + error = nf_by_name_or_uuid(ctx, ctx->argv[2], true, &nf);
> + if (error) {
> + ctx->error = error;
> + return;
> + }
> +
> + /* Do not add network_function more than once in a given
> + * network-function-group */
> + for (size_t i = 0; i < nf_group->n_network_function; i++) {
> + if (nf_group->network_function[i] == nf) {
> + if (!may_exist) {
> + ctl_error(ctx, "network-function %s is already added to "
> + "network-function-group %s",
> + ctx->argv[2], ctx->argv[1]);
> + return;
> + }
> + return;
> + }
> + }
> +
> + /* Insert the logical network-function into the logical
> + * network-function-group. */
> + nbrec_network_function_group_verify_network_function(nf_group);
> + struct nbrec_network_function **new_network_function =
> + xmalloc(sizeof *new_network_function
> + * (nf_group->n_network_function + 1));
> + memcpy(new_network_function, nf_group->network_function,
> + sizeof *new_network_function * nf_group->n_network_function);
> + new_network_function[nf_group->n_network_function] =
> + CONST_CAST(struct nbrec_network_function *, nf);
> + nbrec_network_function_group_set_network_function(nf_group,
> + new_network_function, nf_group->n_network_function + 1);
> + free(new_network_function);
> +}
> +
> +static void
> +remove_nf_from_network_function_group(
> + const struct nbrec_network_function_group *nf_group, size_t idx)
> +{
> +
Please remove this empty line.
> + struct nbrec_network_function **new_network_function
> + = xmemdup(nf_group->network_function, sizeof *new_network_function *
> + nf_group->n_network_function);
> + new_network_function[idx]
> + = new_network_function[nf_group->n_network_function - 1];
> + nbrec_network_function_group_verify_network_function(nf_group);
> + nbrec_network_function_group_set_network_function(
> + nf_group, new_network_function,
> + nf_group->n_network_function - 1);
> + free(new_network_function);
I think this whole function can be rewritten as (not tested):
nf_group->network_function[idx] =
nf_group->network_function[nf_group->n_network_function - 1];
nbrec_network_function_group_set_network_function(
nf_group, nf_group->n_network_function,
nf_group->n_network_function - 1);
> +}
> +
> +static void
> +nbctl_pre_nf_group_del_network_function(struct ctl_context *ctx)
> +{
> + ovsdb_idl_add_column(ctx->idl,
> + &nbrec_network_function_group_col_network_function);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_group_col_name);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_col_name);
> +}
> +
> +static void
> +nbctl_nf_group_del_network_function(struct ctl_context *ctx)
> +{
> + const struct nbrec_network_function *nf;
> + const struct nbrec_network_function_group *nf_group;
> + const bool must_exist = !shash_find(&ctx->options, "--if-exists");
> +
> +
Please remove the extra new line, one is likely enough.
> + char * error = nf_group_by_name_or_uuid(ctx, ctx->argv[1], must_exist,
> + &nf_group);
> + if (error) {
> + ctx->error = error;
> + return;
> + }
> + if (!nf_group) {
> + return;
> + }
> +
> + error = nf_by_name_or_uuid(ctx, ctx->argv[2], must_exist, &nf);
> + if (error) {
> + ctx->error = error;
> + return;
> + }
Please add an empty line here.
> + /* Find the network-function_group that contains 'network-function',
> + * then delete it. */
> +
Please remove this empty line.
> + for (size_t i = 0; i < nf_group->n_network_function; i++) {
> + if (nf_group->network_function[i] == nf) {
> + remove_nf_from_network_function_group(nf_group,i);
Please add a space before "i":
remove_nf_from_network_function_group(nf_group, i);
> + return;
> + }
> + }
> + if (must_exist) {
> + ctl_error(ctx, "network-function %s is not part of any logical
> switch",
> + ctx->argv[1]);
> + return;
> + }
> +}
> +/* End of network-function-group operations */
> +
> +/*
> + * network-function operations
> + */
> +static void
> +nbctl_pre_nf_add(struct ctl_context *ctx)
> +{
> + nbctl_pre_context(ctx);
> +
> + ovsdb_idl_add_column(ctx->idl, &nbrec_logical_switch_port_col_name);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_col_name);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_col_inport);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_col_outport);
> +}
> +
> +static void
> +nbctl_nf_add(struct ctl_context *ctx)
> +{
> + const struct nbrec_logical_switch_port *lsp_in, *lsp_out;
> + const struct nbrec_network_function *nf;
> +
> + const bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> + const bool add_duplicate = shash_find(&ctx->options,
> + "--add-duplicate") != NULL;
> + if (may_exist && add_duplicate) {
> + ctl_error(ctx,
> + "--may-exist and --add-duplicate may not be used
> together");
> + return;
> + }
> +
> + char * error = lsp_by_name_or_uuid(ctx, ctx->argv[2], true, &lsp_in);
> + if (error) {
> + ctx->error = error;
> + return;
> + }
> + error = lsp_by_name_or_uuid(ctx, ctx->argv[3], true, &lsp_out);
> + if (error) {
> + ctx->error = error;
> + return;
> + }
> +
> + const char *nf_name = ctx->argv[1];
> +
> + if (!add_duplicate) {
> + NBREC_NETWORK_FUNCTION_FOR_EACH (nf, ctx->idl) {
> + if (!strcmp(nf->name, nf_name)) {
> + if (may_exist) {
> + return;
> + }
> + ctl_error(ctx, "%s: same name network-function already
> exists",
> + nf_name);
> + return;
> + }
> + }
> + }
> +
> + /* create the logical network-function. */
> + nf = nbrec_network_function_insert(ctx->txn);
> + nbrec_network_function_set_inport(nf, lsp_in);
> + nbrec_network_function_set_outport(nf, lsp_out);
> + nbrec_network_function_set_name(nf, nf_name);
> +}
> +
> +static void
> +nbctl_pre_nf_del(struct ctl_context *ctx)
> +{
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_col_name);
> +}
> +
> +static void
> +nbctl_nf_del(struct ctl_context *ctx)
> +{
> + const struct nbrec_network_function *nf;
> + const bool must_exist = !shash_find(&ctx->options, "--if-exists");
> +
> + char *error = nf_by_name_or_uuid(ctx, ctx->argv[1], must_exist, &nf);
> + if (error) {
> + ctx->error = error;
> + return;
> + }
> + if (!nf) {
> + return;
> + }
> + nbrec_network_function_delete(nf);
> +}
> +
> +static void
> +nbctl_pre_nf_list(struct ctl_context *ctx)
> +{
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_col_name);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_col_inport);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_col_outport);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_logical_switch_port_col_name);
> +}
> +
> +static void
> +nbctl_nf_list(struct ctl_context *ctx)
> +{
> + const struct nbrec_network_function *nf;
> + struct smap nfs;
> + size_t i;
> +
> + smap_init(&nfs);
> +
A slightly better way to do this is:
struct smap nfs = SMAP_INITIALIZER(&nfs);
> + NBREC_NETWORK_FUNCTION_FOR_EACH (nf, ctx->idl) {
> + const struct nbrec_logical_switch_port *linport = nf->inport;
> + const struct nbrec_logical_switch_port *loutport = nf->outport;
> + const char *linport_name = linport ? linport->name : "<not_set>";
> + const char *loutport_name = loutport ? loutport->name : "<not_set>";
> + smap_add_format(&nfs, nf->name,
> + UUID_FMT " (%s) in:%s out:%s",
> + UUID_ARGS(&nf->header_.uuid),
> + nf->name, linport_name, loutport_name);
> + }
> + const struct smap_node **nodes = smap_sort(&nfs);
> + for (i = 0; i < smap_count(&nfs); i++) {
for (size_t i = 0; ....
> + const struct smap_node *node = nodes[i];
> + ds_put_format(&ctx->output, "%s\n", node->value);
> + }
> + smap_destroy(&nfs);
> + free(nodes);
> +}
> +
> +/* End of network-function operations */
> +
> +
> enum {
> DIR_FROM_LPORT,
> DIR_TO_LPORT
> @@ -2232,6 +2708,10 @@ nbctl_acl_print(struct ctl_context *ctx, const struct
> nbrec_acl **acls,
> ds_put_format(&ctx->output, "%10s %5"PRId64" (%s) %s",
> acl->direction, acl->priority, acl->match,
> acl->action);
> + if (acl->network_function_group) {
> + ds_put_format(&ctx->output, " network-function-group=%s",
> + acl->network_function_group->name);
> + }
> if (acl->log) {
> ds_put_cstr(&ctx->output, " log(");
> if (acl->name) {
> @@ -2405,6 +2885,8 @@ nbctl_pre_acl(struct ctl_context *ctx)
>
> ovsdb_idl_add_table(ctx->idl, &nbrec_table_sample_collector);
> ovsdb_idl_add_table(ctx->idl, &nbrec_table_sample);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_acl_col_network_function_group);
> + ovsdb_idl_add_column(ctx->idl, &nbrec_network_function_group_col_name);
> }
>
> static void
> @@ -2461,10 +2943,21 @@ nbctl_acl_add(struct ctl_context *ctx)
>
> /* Create the acl. */
> struct nbrec_acl *acl = nbrec_acl_insert(ctx->txn);
> + const struct nbrec_network_function_group *network_function_group = NULL;
> nbrec_acl_set_priority(acl, priority);
> nbrec_acl_set_direction(acl, direction);
> nbrec_acl_set_match(acl, ctx->argv[4]);
> nbrec_acl_set_action(acl, action);
> + if ((ctx->argv[6]) && strcmp(ctx->argv[6], "--")) {
This seems a bit odd. Why are you comparing against "--"?
I think this should just be:
if (ctx->argc > 6) {
> + error = nf_group_by_name_or_uuid(ctx, ctx->argv[6], true,
> + &network_function_group);
> + if (error) {
> + ctx->error = error;
> + return;
> + }
> +
> + nbrec_acl_set_network_function_group(acl, network_function_group);
> + }
>
> /* Logging options. */
> bool log = shash_find(&ctx->options, "--log") != NULL;
> @@ -8266,7 +8759,8 @@ static const struct ctl_command_syntax nbctl_commands[]
> = {
> { "ls-list", 0, 0, "", nbctl_pre_ls_list, nbctl_ls_list, NULL, "", RO },
>
> /* acl commands. */
> - { "acl-add", 5, 6, "{SWITCH | PORTGROUP} DIRECTION PRIORITY MATCH
> ACTION",
> + { "acl-add", 5, 7,
> + "{SWITCH | PORTGROUP} DIRECTION PRIORITY MATCH ACTION
> [NETWORK-FUNCTION-GROUP]",
> nbctl_pre_acl, nbctl_acl_add, NULL,
> "--log,--may-exist,--type=,--name=,--severity=,--meter=,--label=,"
> "--apply-after-lb,--tier=,--sample-new=,--sample-est=", RW },
> @@ -8358,6 +8852,39 @@ static const struct ctl_command_syntax
> nbctl_commands[] = {
> { "lsp-detach-mirror", 2, 2, "PORT MIRROR", nbctl_pre_lsp_mirror,
> nbctl_lsp_detach_mirror, NULL, "", RW },
>
> + /* network-function-group commands. */
> + { "network-function-group-add", 1, INT_MAX,
> + "NETWORK-FUNCTION-GROUP [NETWORK-FUNCTION]",
> + nbctl_pre_nf_group_add,
> + nbctl_nf_group_add, NULL, "--may-exist,--add-duplicate", RW },
> + { "network-function-group-del", 1, 1, "NETWORK-FUNCTION-GROUP",
> + nbctl_pre_nf_group_del,
> + nbctl_nf_group_del,
> + NULL, "--if-exists", RW },
> + { "network-function-group-list", 0, 1, "[NETWORK-FUNCTION-GROUP]",
> + nbctl_pre_nf_group_list,
> + nbctl_nf_group_list,
> + NULL, "", RO },
> + { "network-function-group-add-network-function", 2, 2,
> + "NETWORK-FUNCTION-GROUP NETWORK-FUNCTION",
> + nbctl_pre_nf_group_add_network_function,
> + nbctl_nf_group_add_network_function, NULL, "--may-exist", RW },
> + { "network-function-group-del-network-function", 2, 2,
> + "NETWORK-FUNCTION-GROUP NETWORK-FUNCTION",
> + nbctl_pre_nf_group_del_network_function,
> + nbctl_nf_group_del_network_function, NULL, "--if-exists", RW },
> +
> + /* network-function commands. */
> + { "network-function-add", 3, 3, "NETWORK-FUNCTION, PORT-IN, PORT-OUT",
We don't need the commas in the command arguments, should be:
"NETWORK-FUNCTION PORT-IN PORT-OUT"
> + nbctl_pre_nf_add,
> + nbctl_nf_add,
> + NULL, "--may-exist,--add-duplicate", RW },
> + { "network-function-del", 1, 1, "NETWORK-FUNCTION", nbctl_pre_nf_del,
> + nbctl_nf_del,
> + NULL, "--if-exists", RW },
> + { "network-function-list", 0, 0, "", nbctl_pre_nf_list,
> + nbctl_nf_list, NULL, "", RO },
> +
> /* forwarding group commands. */
> { "fwd-group-add", 4, INT_MAX, "SWITCH GROUP VIP VMAC PORT...",
> nbctl_pre_fwd_group_add, nbctl_fwd_group_add, NULL, "--liveness", RW },
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
