sysrepo / sysrepo-cpp

C++ bindings for the sysrepo library
BSD 3-Clause "New" or "Revised" License
7 stars 6 forks source link

sr_data_s got removed from sysrepo #15

Closed gotthardp closed 1 year ago

gotthardp commented 1 year ago

You are probably aware of that, but anyway:

The sr_data_s datatype used in: https://github.com/sysrepo/sysrepo-cpp/blob/e66b2f0c53a428eeb743d355cf86fb30e8e491f1/src/Session.cpp#L173

Was removed in the commit https://github.com/sysrepo/sysrepo/commit/cb13582363e1353c09d1f58785113a3e5068d9f2

This causes a build error. The fix seems quite simple: replace sr_data_s by sr_data_t. (Only on this one line.)

jktjkt commented 1 year ago

Thanks. The correct fix is to remove these forward declarations, but after that the test suite hits some internal behavior changes related to the way how operational values are deleted:

6/9 Test #1: test_session .....................***Failed    0.15 sec
[INF] Connection 2 created.
[INF] Session 1 (user "jkt", CID 2) created.
[INF] No datastore changes to apply.
[INF] No datastore changes to apply.
[INF] Connection 3 created.
[INF] Session 2 (user "jkt", CID 3) created.
[INF] No datastore changes to apply.
[INF] There are no subscribers for changes of the module "test_module" in running DS.
[INF] Connection 4 created.
[INF] Session 3 (user "jkt", CID 4) created.
[INF] No datastore changes to apply.
[INF] No datastore changes to apply.
[INF] Connection 5 created.
[INF] Session 4 (user "jkt", CID 5) created.
[INF] There are no subscribers for changes of the module "test_module" in running DS.
[INF] There are no subscribers for changes of the module "test_module" in running DS.
[INF] There are no subscribers for changes of the module "test_module" in running DS.
[INF] There are no subscribers for changes of the module "test_module" in running DS.
[ERR] Not found node "non-existent" in path.
[ERR] Invalid datastore edit.
[INF] No datastore changes to apply.
[INF] Connection 6 created.
[INF] Session 5 (user "jkt", CID 6) created.
[INF] No datastore changes to apply.
[INF] There are no subscribers for changes of the module "test_module" in running DS.
[ERR] Invalid type int32 empty value. (Schema location "/test_module:leafInt32".)
[ERR] Invalid datastore edit.
/home/jkt/work/cesnet/gerrit/CzechLight/libyang-cpp/src/DataNode.cpp:322:20: runtime error: member access within null pointer of type 'lyd_node'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/jkt/work/cesnet/gerrit/CzechLight/libyang-cpp/src/DataNode.cpp:322:20 in 
[doctest] doctest version is "2.4.6"
[doctest] run with "--help" for options
===============================================================================
/home/jkt/work/cesnet/gerrit/CzechLight/sysrepo-cpp/tests/session.cpp:15:
TEST CASE:  session
  Session::deleteOperItem

/home/jkt/work/cesnet/gerrit/CzechLight/sysrepo-cpp/tests/session.cpp:15: FATAL ERROR: test case CRASHED: SIGSEGV - Segmentation violation signal

===============================================================================
/home/jkt/work/cesnet/gerrit/CzechLight/sysrepo-cpp/tests/session.cpp:15:
TEST CASE:  session

DEEPEST SUBCASE STACK REACHED (DIFFERENT FROM THE CURRENT ONE):
  Session::deleteOperItem

===============================================================================
[doctest] test cases:  1 |  0 passed | 1 failed | 0 skipped
[doctest] assertions: 12 | 12 passed | 0 failed |
[doctest] Status: FAILURE!
AddressSanitizer:DEADLYSIGNAL
=================================================================
==111062==ERROR: AddressSanitizer: SEGV on unknown address 0x03e80001b1d6 (pc 0x7f812c71a4ff bp 0x7ffe3c058f50 sp 0x7ffe3c058e90 T0)
==111062==The signal is caused by a READ memory access.
    #0 0x7f812c71a4ff in libyang::DataNode::isTerm() const /home/jkt/work/cesnet/gerrit/CzechLight/libyang-cpp/src/DataNode.cpp:322:20
    #1 0x7f812c71a642 in libyang::DataNode::asTerm() const /home/jkt/work/cesnet/gerrit/CzechLight/libyang-cpp/src/DataNode.cpp:331:10
    #2 0x5233cf in DOCTEST_ANON_FUNC_2() /home/jkt/work/cesnet/gerrit/CzechLight/sysrepo-cpp/tests/session.cpp:91:9
    #3 0x547d89 in doctest::Context::run() /home/jkt/work/prog/_build/czechlight-clang14-asan-ubsan-ly2/target/include/doctest/doctest.h:6510:21
    #4 0x54d036 in main /home/jkt/work/prog/_build/czechlight-clang14-asan-ubsan-ly2/target/include/doctest/doctest.h:6595:71
    #5 0x7f812c42b236 in __libc_start_call_main (/nix/store/scd5n7xsn0hh0lvhhnycr9gx0h8xfzsl-glibc-2.34-210/lib/libc.so.6+0x29236) (BuildId: cd4b41522c8f722f5054a0316d293e6d85eef07d)
    #6 0x7f812c42b2f4 in __libc_start_main@GLIBC_2.2.5 (/nix/store/scd5n7xsn0hh0lvhhnycr9gx0h8xfzsl-glibc-2.34-210/lib/libc.so.6+0x292f4) (BuildId: cd4b41522c8f722f5054a0316d293e6d85eef07d)
    #7 0x42a330 in _start /build/glibc-2.34/csu/../sysdeps/x86_64/start.S:116

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/jkt/work/cesnet/gerrit/CzechLight/libyang-cpp/src/DataNode.cpp:322:20 in libyang::DataNode::isTerm() const

I'll keep you posted.

jktjkt commented 1 year ago

@michalvasko It looks like there's been a behavior change in sysrpeo 2.2.x. Previously, sr_oper_delete_item_str would only perform the deletion if the string value to be deleted matched the current value, now it appears to always do the delete even when the value doesn't match. Is that intentional?

michalvasko commented 1 year ago

Yes, it is intentional and I have updated the docs to reflect this change. The reason is that it became mostly an artificial restriction and by removing it the code got significantly simpler and the functionality is more akin to a standard edit.

jktjkt commented 1 year ago

Any chance of starting to allow a NULL, then? It feels weird that one has to come up with a value which is valid for the target data type.

gotthardp commented 1 year ago

The build error is fixed now. Thanks!

michalvasko commented 1 year ago

Any chance of starting to allow a NULL, then? It feels weird that one has to come up with a value which is valid for the target data type.

So I have ultimately decided that this generally makes sense but quickly found that supporting it requires a surprisingly large update to the code and I have been working on it since. You can create a separate issue if you want to be notified once it is finished.