On Tue, Sep 9, 2025 at 11:20 AM Dumitru Ceara <[email protected]> wrote:
> 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 > > Hi Eelco and Dumitru, yes there are cases when we crash the client. Also this is why it's an RFE, for our use case it is completely fine to crash. We only expect to detect the erronouns behavior once. If the API would return some error instead we would assert on that error anyway in OVN resulting in a crash. Which again for this use case is desired. Also there is a slight issue how to handle gracefully some places where the assertion is? There isn't any way to propagate that error without significant changes to the idl layer itself which probably isn't desirable. I'm open to suggestions, at the same time I'm trying to find a working solution that can be used in OVN. Thanks, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
