On 9 Sep 2025, at 12:04, Ales Musil wrote:
> 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. +Mike and Ilya, who have more experience in the OVSDB area. > > Thanks, > Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
