> - Make outport optional in 'nf-add'.
> - Accept 'vtap' mode in 'nfg-add'.
>
> Signed-off-by: Naveen Yerramneni <naveen.yerramneni at nutanix.com
> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>>
> Acked-by: Sragdhara Datta Chaudhuri <sragdha.chaudhu at nutanix.com
> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>>
> Acked-by: Aditya Mehakare <aditya.mehakare at nutanix.com
> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>>
> ---
> tests/ovn-nbctl.at | 14 ++++++++++++++
> utilities/ovn-nbctl.c | 20 ++++++++++++--------
> 2 files changed, 26 insertions(+), 8 deletions(-)
Hi Naveen, Sragdhara and Aditya! This isn’t a full review of the entire series
— just this patch — but
I’m also interested in going through the rest of it. Aside from the small
comments below,
it looks like utilities/ovn-nbctl documentation wasn’t updated - it still
states that only inline mode is supported.
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index 890d24dbf..95de94b5b 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -3358,6 +3358,20 @@ AT_CHECK([ovn-nbctl nf-list | uuidfilt], [0], [dnl
> <0> (nf3) id:10 in:svc-port6 out:svc-port7
> ])
> AT_CHECK([ovn-nbctl nf-del nf3])
> +
> +# Create network-function with only inport (vtap mode).
> +AT_CHECK([ovn-nbctl nf-add nf0 1 svc-port6])
> +AT_CHECK([ovn-nbctl nf-list | uuidfilt], [0], [dnl
> +<0> (nf0) id:1 in:svc-port6 out:<not_set>
> +])
> +
> +# Create a network-function-group in vtap mode.
> +AT_CHECK([ovn-nbctl nfg-add nfg0 1 vtap nf0])
> +AT_CHECK([ovn-nbctl nfg-list | uuidfilt], [0], [dnl
> +<0> (nfg0)
> +])
> +AT_CHECK([ovn-nbctl nfg-del nfg0])
> +AT_CHECK([ovn-nbctl nf-del nf0])
> ])
>
> AT_SETUP([ovn-nbctl - TLS server name indication (SNI) with
> --ssl-server-name])
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 0ef207272..7388bc4b7 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -405,7 +405,7 @@ Network function group commands:\n\
> network-function-group\n\
> \n\
> Network function commands:\n\
> - nf-add NETWORK-FUNCTION ID PORT-IN PORT-OUT\n\
> + nf-add NETWORK-FUNCTION ID PORT-IN [PORT-OUT]\n\
> create a network-function\n\
> nf-del NETWORK-FUNCTION delete a network-function\n\
> nf-list print all network-functions\n\
> @@ -2332,9 +2332,9 @@ nbctl_nf_group_add(struct ctl_context *ctx)
>
> /* Validate and set mode */
> const char *nfg_mode = ctx->argv[3];
> - if (strcmp(nfg_mode, "inline")) {
> + if (strcmp(nfg_mode, "inline") && strcmp(nfg_mode, "vtap")) {
> ctl_error(ctx, "Unsupported mode provided for "
> - "network-function-group:%s, supported values: inline",
> + "network-function-group:%s, supported values: inline,
> vtap",
> nfg_name);
Nit: This might just be my personal preference, but I think it could be
valuable to include tests covering any exceptions in the configuration.
The case of specifying an invalid type is not currently covered in the tests.
> return;
> }
> @@ -2533,10 +2533,14 @@ nbctl_nf_add(struct ctl_context *ctx)
> ctx->error = error;
> return;
> }
> - error = lsp_by_name_or_uuid(ctx, ctx->argv[4], true, &lsp_out);
> - if (error) {
> - ctx->error = error;
> - return;
> + if (ctx->argc == 5) {
> + error = lsp_by_name_or_uuid(ctx, ctx->argv[4], true, &lsp_out);
> + if (error) {
> + ctx->error = error;
> + return;
> + }
> + } else {
This else looks overweighted, lsp_out can be assigned NULL directly in
declaration.
> + lsp_out = NULL;
> }
>
> /* Validate and parse ID */
> @@ -9362,7 +9366,7 @@ static const struct ctl_command_syntax nbctl_commands[]
> = {
> nbctl_nf_group_del_network_function, NULL, "--if-exists", RW },
>
> /* network-function commands. */
> - { "nf-add", 4, 4, "NETWORK-FUNCTION ID PORT-IN PORT-OUT",
> + { "nf-add", 3, 4, "NETWORK-FUNCTION ID PORT-IN [PORT-OUT]",
> nbctl_pre_nf_add,
> nbctl_nf_add,
> NULL, "--may-exist", RW },
> --
> 2.43.5
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
