On 4/28/23 15:27, Stefan Hoffmann wrote: > On Fri, 2023-04-28 at 15:12 +0200, Ilya Maximets wrote: >> On 4/28/23 10:16, Stefan Hoffmann wrote: >>> On Thu, 2023-04-27 at 18:27 +0200, Stefan Hoffmann wrote: >>>> This tests stream.c and stream.py with ssl connection at >>>> CHECK_STREAM_OPEN_BLOCK. >>>> For the tests, ovsdb needs to be build with libssl. >>>> >>>> Signed-off-by: Stefan Hoffmann <[email protected]> >>> >>> Some questions from myself: >>> >>> Are there other places, that should also get ssl or python checks and >>> should be included here? Maybe something (python) client related. >> >> Most of the tests are executed for both C and Python. Python IDL doesn't >> support some features that C implementation does, so some tests are not >> running for Python. There might be a few that are running for C only, but >> can technically be checked with Python as well, but it needs some careful >> read of the testsuite to find them. >> >> We also have some tests for json and jsonrpc that are implemented for both >> C and Python in the similar way, but they are mostly abstracted from the >> underlying stream implementation. >> >> We don't have a lot of negative tests, i.e. failure simulations, but >> these are hard to do. Mocking is a way, but it's only for Python, so >> should live in the pytest realm, I think. >> >>> >>> Besides that I add some comments to discuss my change below. >>> >>>> --- >>>> tests/ovsdb-idl.at | 45 +++++++++++++++++++++++++++++++++++++++----- >>>> tests/test-stream.c | 12 +++++++++++- >>>> tests/test-stream.py | 6 ++++++ >>>> 3 files changed, 57 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at >>>> index 5a7e76eaa..b40a167eb 100644 >>>> --- a/tests/ovsdb-idl.at >>>> +++ b/tests/ovsdb-idl.at >>>> @@ -8,7 +8,15 @@ m4_divert_text([PREPARE_TESTS], [ >>>> # specified). >>>> ovsdb_start_idltest () { >>>> ovsdb-tool create db ${2:-$abs_srcdir/idltest.ovsschema} || return $? >>>> - ovsdb-server -vconsole:warn --log-file --detach --no-chdir --pidfile >>>> --remote=punix:socket ${1:+--remote=$1} db || return $? >>>> + SSL_FLAGS="" >>>> + if [[ "${1::4}" == "pssl" ]]; then >>>> + SSL_FLAGS="--private-key=$PKIDIR/testpki-privkey2.pem \ >>>> + --certificate=$PKIDIR/testpki-cert2.pem \ >>>> + --ca-cert=$PKIDIR/testpki-cacert.pem" >> >> We need to define the PKIDIR in this function for completeness, >> I think. Even though it is already defined in the CHECK_STREAM_OPEN_BLOCK, >> it's not defined in other users of ovsdb_start_idltest. >> >>>> + fi >>>> + ovsdb-server -vconsole:warn --log-file --detach --no-chdir --pidfile \ >>>> + $SSL_FLAGS --remote=punix:socket ${1:+--remote=$1} \ >>>> + db || return $? >>>> on_exit 'kill `cat ovsdb-server.pid`' >>>> } >>> >>> I like this approach more than OVSDB_CHECK_IDL_SSL_PY, where the server >>> is started at the m4_define section, still I could move the server >>> start also to CHECK_STREAM_OPEN_BLOCK directly. >> >> It's actually better to avoid shell scripting if possible. m4 macros >> are playing much more nicely with the autotest. Shell commands will >> not be visible in the testsuite log and it's hard to figure out what >> exactly failed in the shell script for that reason. >> >> In this particular case, the code is split into a function to avoid >> huge code duplication. Ideally, though, this whole function should >> be replaced with a macro definition and all the calls inside should >> be wrapped into AT_CHECK, the 'if' should be an m4_if and the >> ${1:+--remote=$1} should be an m4_if as well. And all the '\' should >> be 'dnl' instead ('\' breaks command logging in the autotest). >> Some things are just easier to write with shell scripts though. >> Some things are just legacy code that doesn't worth being re-written >> just for the looks or minor debugability improvements. > > Thanks for explaining. > Should I introduce a m4 macro than, that we can use later to replace > ovsdb_start_idltest or keep it, as it is for now?
I'd say either keep the shell function as is or replace it entirely with a macro. See the OVSDB_CLUSTER_START_IDLTEST for example. Replacing will require changes in all the callers. And the macro should be defined outside of m4_divert_text. Both options are fine by me. In case of replacing, you may turn the change into a patch set with the first patch preforming some ground work on converting the function into a macro and the second patch adding SSL stream tests. Half-measure of introducing the macro but keeping the shell function doesn't sound great to me. > >> >>> >>>> >>>> @@ -2279,14 +2287,35 @@ m4_define([CHECK_STREAM_OPEN_BLOCK], >>>> [AT_SETUP([Check stream open block - $1 - $3]) >>>> AT_SKIP_IF([test "$3" = "tcp6" && test "$IS_WIN32" = "yes"]) >>>> AT_SKIP_IF([test "$3" = "tcp6" && test "$HAVE_IPV6" = "no"]) >>>> + AT_SKIP_IF([test "$3" = "ssl6" && test "$IS_WIN32" = "yes"]) >>>> + AT_SKIP_IF([test "$3" = "ssl6" && test "$HAVE_IPV6" = "no"]) >>>> + AT_SKIP_IF([test "$3" = "ssl" && test "$HAVE_OPENSSL" = "no"]) >>>> + $PYTHON3 -c "import ssl" >>>> + SSL_PRESENT=$? >>>> + AT_SKIP_IF([test "$3" = "ssl" && test $SSL_PRESENT != 0]) >>>> + AT_SKIP_IF([test "$3" = "ssl6" && test "$HAVE_OPENSSL" = "no"]) >>>> + AT_SKIP_IF([test "$3" = "ssl6" && test $SSL_PRESENT != 0]) >> >> We also need to check for a case where python is built without SSL >> support. OVSDB_CHECK_IDL_SSL_PY does that by trying to import ssl. > > This is done 5 lines above, I copied that part from > OVSDB_CHECK_IDL_SSL_PY. Oh, sorry. Missed that part. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
