On 8/27/25 5:38 AM, 屋顶上睡觉的猫 wrote: > Hi, Ilya Maximets. > > Thank you for taking the time to review my patch, and I truly admire your > dedication to the OVS community. > > If duplicate controller parameters are set, OVS should establish multiple > connections with the controller. If the controller does not perform relevant > verification, it may send flows to the same OVS. It may cause some > abnormalities. > Our operations colleagues configure OVS deployment through the command line. > The frequency is relatively low, but this situation has occurred several > times. > All caused by misoperation > > There are mainly the following reasons for implementing this function in the > ovs-vsctl module: > 1.fix this on the OVSDB side is indeed more comprehensive; however, fix this > on the ovs-vsctl side eliminates the need for database access, making it > faster and more lightweight in terms of overhead. > 2.For more manual operations, the command line is often chosen for > configuration, > which leads to a higher probability of misoperations. Direct access to the > OVSDB database is generally implemented through interface calls between > functional modules, so the probability of duplicate parameters should be > relatively low. > > I understand that this implementation has its advantages, and at the very > least, > it has no negative effects. Additionally, sufficient testing has been > conducted.
So I looked again and we can't really check that from the database side, because we want to be able to configure the same controller with different parameters for different bridges. Database schema will not allow us to create a restriction like this. So, instead ovs-vswitchd should be doing the check. This can be done by looking up the entry in the 'ocs' shash in bridge_configure_remotes() in vswitchd/bridge.c before adding the new one, and printing a warning in the log. > Is it possible to merge this patch first, and I will figure out how to fix > this > on the database side later? This isn't really a bug fix, just an enhancement, and the next OVS release is in February, so there is plenty of time to make the better change from the beginning. See also a couple of small comments for the patch, to avoid them in future submissions. Also, please, try to reply to the original thread instead of writing a new email. This one was not a reply to my previous email, i.e. it doesn't have In-Reply-To or References headers. This breaks tracking in patchwork and mail clients in general. Best regards, Ilya Maximets. > > Best regards, Lingfei Yao. > > > ------------------ 原始邮件 ------------------ > *发件人:* "Ilya Maximets" <[email protected]>; > *发送时间:* 2025年8月27日(星期三) 凌晨4:18 > *收件人:* "屋顶上睡觉的猫"<[email protected]>;"dev"<[email protected]>; > *抄送:* "i.maximets"<[email protected]>; > *主题:* Re: [ovs-dev] [PATCH v5] ovs-vsctl: Check controller parameters. > > On 8/19/25 9:41 AM, yaolingfei via dev wrote: >> Check the parameters when setting up the controller. The parameters will >> only take effect when none of the following three parameters are >> repeated; Otherwise, a prompt message will be returned >> 1.ip address >> 2.port no >> 3.protocol type >> >> v5: modified the subject summary >> >> Signed-off-by: yaolingfei <[email protected]> The signed-off should generally be in 'Firstname Lastname <email>' format if possible (and it should still match the commit author field). >> --- > > Hi, yaolingfei. Thanks for the patch. > > Could you, please, explain why this problem needs to be fixed? > Is it a common issue that users duplicate controllers on the command line? > Sounds a little unlikely. If it is, maybe we need to fix this on the > database side, e.g. by making the controller table indexed by the target. > If we just check on the command line level, users can still put duplicates > into the database. > > Best regards, Ilya Maximets. > > >> tests/ovs-vsctl.at | 4 ++++ >> utilities/ovs-vsctl.c | 26 +++++++++++++++++++++++++- >> 2 files changed, 29 insertions(+), 1 deletion(-) >> >> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at >> index 59245fff8..76fe74414 100644 >> --- a/tests/ovs-vsctl.at >> +++ b/tests/ovs-vsctl.at >> @@ -492,6 +492,8 @@ AT_CHECK([RUN_OVS_VSCTL_TOGETHER( >> [get-controller br0], >> >> [--inactivity-probe=30000 set-controller br0 tcp:1.2.3.4], >> + [get-controller br0], >> + [set-controller br0 tcp:8.9.10.11 tcp:8.9.10.11], >> [get-controller br0])], [0], [ >> >> >> @@ -501,6 +503,8 @@ tcp:4.5.6.7 >> >> tcp:5.4.3.2\ntcp:8.9.10.11 >> >> +tcp:1.2.3.4 >> +Controller parameter is duplicated >> tcp:1.2.3.4 >> ]) >> OVS_VSCTL_CLEANUP >> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c >> index d90db934b..f39952d72 100644 >> --- a/utilities/ovs-vsctl.c >> +++ b/utilities/ovs-vsctl.c >> @@ -2348,6 +2348,24 @@ insert_controllers(struct ctl_context *ctx, char >> *targets[], size_t n) >> return controllers; >> } >> >> +static int >> +check_dup_controllers(char *targets[], size_t n) >> +{ >> + size_t i; >> + size_t j; >> + >> + /* check if the input parameters are duplicated */ Comments should start with a capital letter and end with a period. See the coding style guide. >> + for (i = 0; i < n; i++) { >> + for (j = i + 1; j < n; j++) { >> + if (!strcmp(targets[i], targets[j])) { >> + return 0; >> + } >> + } >> + } >> + >> + return 1; >> +} >> + >> static void >> cmd_set_controller(struct ctl_context *ctx) >> { >> @@ -2361,9 +2379,15 @@ cmd_set_controller(struct ctl_context *ctx) >> br = find_real_bridge(vsctl_ctx, ctx->argv[1], true)->br_cfg; >> verify_controllers(br); >> >> + n = ctx->argc - 2; >> + if (!check_dup_controllers(&ctx->argv[2], n)) { >> + /* controller parameters are duplicated */ >> + ds_put_format(&ctx->output, "Controller parameter is duplicated\n"); >> + return; >> + } >> + >> delete_controllers(br->controller, br->n_controller); >> >> - n = ctx->argc - 2; >> controllers = insert_controllers(ctx, &ctx->argv[2], n); >> ovsrec_bridge_set_controller(br, controllers, n); >> free(controllers); _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
