On Fri, May 7, 2021 at 3:23 PM Flavio Fernandes <[email protected]> wrote:
> On May 7, 2021, at 3:02 AM, Frode Nordahl <[email protected]> wrote:
> fre. 7. mai 2021, 03:22 skrev Flavio Fernandes <[email protected]>:
>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>> index 6106a9661..e4cbf3583 100644
>> --- a/controller/ovn-controller.c
>> +++ b/controller/ovn-controller.c
>> @@ -401,6 +401,12 @@ process_br_int(struct ovsdb_idl_txn *ovs_idl_txn,
>> ovs_table);
>> if (!br_int) {
>> br_int = create_br_int(ovs_idl_txn, ovs_table);
>> + } else if (ovs_idl_txn) {
>
> Would it make sense to put this code into the branch below instead? It
> appears to me it already executes on the condition you want.
>
> Yeah, we could do that. Right now, the creation of the bridge in
> "create_br_int" is already setting the fail-mode and
> by moving this below would make that potentially redundant. But the cost of
> an additional strcmp() for sake of simplicity
> may be a valid trade off? So the new code would look more like:
Ah I see, I like conservatism, but in reality it would only optimize
away the call for the first iteration of the main loop if the bridge
did not exist?
> if (br_int && ovs_idl_txn) {
> const struct ovsrec_open_vswitch *cfg;
> ...
> if (datapath_type && strcmp(br_int->datapath_type, datapath_type)) {
> ovsrec_bridge_set_datapath_type(br_int, datapath_type);
> }
> + const char *fail_mode = br_int->fail_mode;
> + if (!fail_mode || strcmp(fail_mode, "secure")) {
> + ovsrec_bridge_set_fail_mode(br_int, "secure");
> + VLOG_WARN("Integration bridge fail-mode needed to be set as
> secure.");}
> + }
> }
> return br_int;
>
> Sounds reasonable?
I think this looks much better, one minor nit: the fail_mode variable
is not really necessary, or perhaps we could use it to store the
desired fail_mode "secure" instead, to avoid repetition of that
constant in the code?
>
>> + const char *fail_mode = br_int->fail_mode;
>> + if (!fail_mode || strcmp(fail_mode, "secure")) {
>> + ovsrec_bridge_set_fail_mode(br_int, "secure");
>> + VLOG_WARN("Integration bridge fail-mode set to secure.");
>
>
> While I agree this action should be logged, I wonder if the text could be
> rephrased. As it stands it could read as having br-int in secure fail-mode is
> a undesirable situation, whereas the opposite is true.
>
>
> heh.... English troubles! ;) I can see what you mean now.
>
> Do you think this is more appropriate:
>
> Integration bridge fail-mode needed to be set as secure.
>
> ? Chime in with any tweaks, please.
Better, this could also work: "Integration bridge fail-mode changed to
'secure'."
Cheers!
--
Frode Nordahl
>
> -- flaviof
>
>
>> + }
>> }
>> if (br_int && ovs_idl_txn) {
>> const struct ovsrec_open_vswitch *cfg;
>> --
>> 2.25.1
>
>
> --
> Frode Nordahl
>
>>
>> _______________________________________________
>> dev mailing list
>> [email protected]
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
