On 9/9/25 11:11 AM, Eelco Chaudron wrote: > > > On 9 Sep 2025, at 10:45, Eelco Chaudron wrote: > >> On 5 Sep 2025, at 11:07, Ales Musil via dev wrote: >> >>> Add a way to set IDL txn as read only, that is useful in cases when >>> we want to ensure that the program doesn't write anything into the >>> txn. This is done via assert because this is considered a bug which >>> should be fixed in the IDL caller. >>> >>> Signed-off-by: Ales Musil <[email protected]> >>> --- >>> More context on why, this would be used by OVN to ensure >>> that certain I-P nodes do not write into the database when >>> they shouldn't >>
>> Hi Ales, >> >> I haven’t done a full code review yet, but I do want to raise an >> objection regarding the assert() approach used here. >> >> To me, this doesn’t make much sense, why should the whole dbserver >> crash just because a client is misbehaving? It feels like this >> should be handled more gracefully, maybe with an error return or >> logging instead of taking down the entire process. >> >> What do you think? > > Someone pointed out to me that this is only for the client side, > which I guess might be fine to crash in some cases (is it?). > > That said, it still feels odd that ovsdb-client would crash rather > than return a proper error. From a user perspective, I’d expect it > to fail gracefully with an error message instead of aborting. > Hi Eelco, There are quite a few situations in which the ovsdb-idl library crashes the client (assert()) when the IDL API is used incorrectly. Some examples: - ovsdb_idl_index_destroy_row() called on a non-index row - ovsdb_idl_txn_write__() called by all <dbrec>_set_<column>() helpers if the column we try to write to is synthetic or we're trying to write to columns that are not monitored e.g. - ovsdb_idl_txn_insert_persist_uuid() called without explicitly providing a UUID. In my opinion that's fine. Regards, Dumitru > //Eelco _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
