From 3c93eea02d4a536770ab271e0c65d956e7d1e27e Mon Sep 17 00:00:00 2001 From: Frode Nordahl Date: Sat, 2 Dec 2023 07:57:21 +0100 Subject: [PATCH 1/2] tests/ctests: Build ctests separately At present the ctests are not properly linked with the built code, this causes issues when there are interdependencies between the modules. The meson build system does not appear to have support for internal build order or dependncies, so the only way I have found to make this work is to break out the build of the ctests as a separate project. This is required to avoid undefined symbols in the test code for the next patch in this series. Signed-off-by: Frode Nordahl --- meson.build | 7 ++++++- tests/ctests/build.sh | 9 +++++++++ tests/ctests/meson.build | 23 ++++++++++++++++++++--- tests/ctests/meson_options.txt | 2 ++ tests/ctests/run.sh | 1 + tools/run_asan.sh | 13 +++++++++++-- 6 files changed, 49 insertions(+), 6 deletions(-) create mode 100755 tests/ctests/build.sh create mode 100644 tests/ctests/meson_options.txt create mode 120000 tests/ctests/run.sh diff --git a/meson.build b/meson.build index 66cda8b5a..53d17623a 100644 --- a/meson.build +++ b/meson.build @@ -71,7 +71,12 @@ test_env = [ ] if get_option('unit_testing') - subdir('tests/ctests') + test('ctests', + find_program('tests/ctests/run.sh'), + workdir: join_paths(meson.current_source_dir(), 'tests/ctests'), + args: [join_paths(meson.current_build_dir(), '_ctestsbuild'), + '-Dparent_current_source_dir=' + meson.current_source_dir(), + '-Dparent_current_build_dir=' + meson.current_build_dir()]) endif #FIXME: exclude doc/env/ diff --git a/tests/ctests/build.sh b/tests/ctests/build.sh new file mode 100755 index 000000000..093e5bcf7 --- /dev/null +++ b/tests/ctests/build.sh @@ -0,0 +1,9 @@ +#!/bin/sh + +BUILDDIR="$1"; shift +meson setup ${BUILDDIR} $* +meson compile -C ${BUILDDIR} --verbose + +if [ "$(basename $0)" = "run.sh" ]; then + meson test -C ${BUILDDIR} +fi diff --git a/tests/ctests/meson.build b/tests/ctests/meson.build index 8a41e9a30..ab503153f 100644 --- a/tests/ctests/meson.build +++ b/tests/ctests/meson.build @@ -1,3 +1,10 @@ +project('ctests', 'c') + +add_project_arguments( + '-DSBINDIR="' + join_paths(get_option('prefix'), get_option('sbindir')) + '"', + '-D_GNU_SOURCE', + language: 'c') + tests = { 'test_netplan_parser': false, 'test_netplan_state': false, @@ -10,15 +17,25 @@ tests = { 'test_netplan_openvswitch': false, } +glib = dependency('glib-2.0') +gio = dependency('gio-2.0') +yaml = dependency('yaml-0.1') +uuid = dependency('uuid') cmocka = dependency('cmocka', required: true) +netplan = meson.get_compiler('c').find_library( + 'netplan', + dirs: [join_paths(get_option('parent_current_build_dir'), 'src')]) foreach name, should_fail: tests exe = executable(name, '@0@.c'.format(name), - include_directories: [inc, inc_internal], - dependencies: [cmocka, glib, gio, yaml, uuid], + include_directories: [ + join_paths(get_option('parent_current_source_dir'), 'include'), + join_paths(get_option('parent_current_source_dir'), 'src'), + ], + dependencies: [cmocka, glib, gio, yaml, uuid, netplan], c_args: [ - '-DFIXTURESDIR="' + meson.project_source_root() + '/tests/ctests/fixtures"', + '-DFIXTURESDIR="' + meson.project_source_root() + '/fixtures"', '-Wno-deprecated-declarations', '-D_GNU_SOURCE', ], diff --git a/tests/ctests/meson_options.txt b/tests/ctests/meson_options.txt new file mode 100644 index 000000000..da1127f6a --- /dev/null +++ b/tests/ctests/meson_options.txt @@ -0,0 +1,2 @@ +option('parent_current_source_dir', type: 'string', value: '') +option('parent_current_build_dir', type: 'string', value: '') diff --git a/tests/ctests/run.sh b/tests/ctests/run.sh new file mode 120000 index 000000000..c07a74de4 --- /dev/null +++ b/tests/ctests/run.sh @@ -0,0 +1 @@ +build.sh \ No newline at end of file diff --git a/tools/run_asan.sh b/tools/run_asan.sh index 73ed35e38..7541ca3ad 100755 --- a/tools/run_asan.sh +++ b/tools/run_asan.sh @@ -5,11 +5,19 @@ set -x BUILDDIR="_leakcheckbuild" CLEANBUILDDIR="_cleanbuild" +CTESTSBUILDDIR="_ctestsbuild" CC=gcc meson setup ${BUILDDIR} -Db_sanitize=address,undefined meson compile -C ${BUILDDIR} --verbose +tests/ctests/build.sh \ + ${CTESTSBUILDDIR} \ + tests/ctests \ + -Dparent_current_source_dir=$(realpath .) \ + -Dparent_current_build_dir=$(realpath ${BUILDDIR}) \ + -Db_sanitize=address,undefined + meson setup ${CLEANBUILDDIR} meson compile -C ${CLEANBUILDDIR} --verbose @@ -18,10 +26,11 @@ ${CC} tools/keyfile_to_yaml.c -o tools/keyfile_to_yaml \ -Iinclude -L${BUILDDIR}/src \ -fsanitize=address,undefined -g -TESTS=$(find ${BUILDDIR}/tests/ctests/ -executable -type f) +TESTS=$(find ${CTESTSBUILDDIR} -executable -type f) for test in ${TESTS} do - ./${test} + # https://github.com/google/sanitizers/issues/1017 + ASAN_OPTIONS=detect_odr_violation=0 ./${test} done mkdir -p ${BUILDDIR}/fakeroot/{etc/netplan,run} From 880db4e3a79bdf8f866b3661df6014b229eb6f5d Mon Sep 17 00:00:00 2001 From: Frode Nordahl Date: Sat, 2 Dec 2023 08:04:16 +0100 Subject: [PATCH 2/2] ovs: Support consuming OVS from snap At present netplan hard codes the location of the `ovs-vsctl` binary. This does not work well when trying to integrate with snaps. Determine path of `ovs-vsctl` at runtime by checking for both '/snap/bin/ovs-vsctl' and '/usr/bin/ovs-vsctl' with a preference of the former. This allows making use of the Netplan Open vSwitch integration with for example the MicroOVN snap (and its consumers such as MicroCloud). TODO: Figure out how to hanlde the ovsdb-server.service unit check TODO: Add test cases that checks the snap workflow Signed-off-by: Frode Nordahl --- netplan_cli/cli/ovs.py | 23 ++++++--- src/openvswitch.c | 111 +++++++++++++++++++++++++++++++++-------- src/openvswitch.h | 3 ++ src/util-internal.h | 6 ++- src/validation.c | 5 +- 5 files changed, 118 insertions(+), 30 deletions(-) diff --git a/netplan_cli/cli/ovs.py b/netplan_cli/cli/ovs.py index 0ba0482b6..04c836c6b 100644 --- a/netplan_cli/cli/ovs.py +++ b/netplan_cli/cli/ovs.py @@ -20,10 +20,9 @@ import subprocess import re -from .utils import systemctl_is_active - -OPENVSWITCH_OVS_VSCTL = '/usr/bin/ovs-vsctl' -OPENVSWITCH_OVSDB_SERVER_UNIT = 'ovsdb-server.service' +OPENVSWITCH_OVS_VSCTL = ( + '/snap/bin/ovs-vsctl' if os.path.exists('/snap/bin/ovs-vsctl') else + '/usr/bin/ovs-vsctl') # Defaults for non-optional settings, as defined here: # http://www.openvswitch.org/ovs-vswitchd.conf.db.5.pdf DEFAULTS = { @@ -43,6 +42,19 @@ class OvsDbServerNotRunning(Exception): pass +def _assert_ovsdb_server_connection(): + """Invoke OPENVSWITCH_OVS_VSCTL, raise OvsDbServerNotRunning on error.""" + try: + # The `get-manager` command is a light weight database operation which + # we use as indication of the client being able to connect. + subprocess.check_call( + [OPENVSWITCH_OVS_VSCTL, '--timeout', '5', 'get-manager']) + except (subprocess.CalledProcessError, OSError): + raise OvsDbServerNotRunning('{} is unable to connect to database, ' + 'ensure ovsdb-server is running' + .format(OPENVSWITCH_OVS_VSCTL)) + + def _del_col(type, iface, column, value): """Cleanup values from a column (i.e. "column=value")""" default = DEFAULTS.get(column) @@ -125,8 +137,7 @@ def apply_ovs_cleanup(config_manager, ovs_old, ovs_current): # pragma: nocover Also filter for individual settings tagged netplan/[/. */ +#include #include #include @@ -29,6 +30,48 @@ #include "util.h" #include "util-internal.h" +#ifndef PREFIX +#define PREFIX "/usr/" +#endif + +#ifndef BINDIR +#define BINDIR PREFIX"bin" +#endif + +#ifndef SNAPBINDIR +#define SNAPBINDIR "/snap/bin" +#endif + +#define OPENVSWITCH_OVS_VSCTL "ovs-vsctl" + +static char netplan_openvswitch_ovs_vsctl_path[ + MAX(sizeof BINDIR + 1 + sizeof OPENVSWITCH_OVS_VSCTL, + sizeof SNAPBINDIR + 1 + sizeof OPENVSWITCH_OVS_VSCTL)]; + +void +netplan_openvswitch_vsctl__(void) +{ + if (g_file_test(SNAPBINDIR"/"OPENVSWITCH_OVS_VSCTL, G_FILE_TEST_EXISTS)) { + snprintf(netplan_openvswitch_ovs_vsctl_path, + sizeof netplan_openvswitch_ovs_vsctl_path, + SNAPBINDIR"/"OPENVSWITCH_OVS_VSCTL); + } else { + snprintf(netplan_openvswitch_ovs_vsctl_path, + sizeof netplan_openvswitch_ovs_vsctl_path, + BINDIR"/"OPENVSWITCH_OVS_VSCTL); + } + +} + +const char * +netplan_openvswitch_ovs_vsctl(void) +{ + if (!*netplan_openvswitch_ovs_vsctl_path) { + netplan_openvswitch_vsctl__(); + } + return (const char *) &netplan_openvswitch_ovs_vsctl_path; +} + static gboolean write_ovs_systemd_unit(const char* id, const GString* cmds, const char* rootdir, gboolean physical, gboolean cleanup, const char* dependency, GError** error) { @@ -51,7 +94,8 @@ write_ovs_systemd_unit(const char* id, const GString* cmds, const char* rootdir, g_string_append_printf(s, "After=netplan-ovs-cleanup.service\n"); } else { /* The netplan-ovs-cleanup unit shall not run on systems where Open vSwitch is not installed. */ - g_string_append(s, "ConditionFileIsExecutable=" OPENVSWITCH_OVS_VSCTL "\n"); + g_string_append_printf(s, "ConditionFileIsExecutable=%s\n", + netplan_openvswitch_ovs_vsctl()); } g_string_append(s, "Before=network.target\nWants=network.target\n"); if (dependency) { @@ -127,7 +171,8 @@ write_ovs_tag_setting(const gchar* id, const char* type, const char* col, const if (key) g_string_append_printf(s, "/%s", key); g_string_append_printf(s, "=%s", clean_value); - append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " set %s %s %s", type, id, s->str); + append_systemd_cmd(cmds, "%s set %s %s %s", + netplan_openvswitch_ovs_vsctl(), type, id, s->str); g_string_free(s, TRUE); } @@ -142,8 +187,9 @@ write_ovs_additional_data(GHashTable *data, const char* type, const gchar* id, G while (g_hash_table_iter_next(&iter, (gpointer) &key, (gpointer) &value)) { /* XXX: we need to check what happens when an invalid key=value pair gets supplied here. We might want to handle this somehow. */ - append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " set %s %s %s:%s=%s", - type, id, setting, key, value); + append_systemd_cmd(cmds, "%s set %s %s %s:%s=%s", + netplan_openvswitch_ovs_vsctl(), type, id, setting, + key, value); write_ovs_tag_setting(id, type, setting, key, value, cmds); } } @@ -175,8 +221,9 @@ write_ovs_bond_interfaces(const NetplanState* np_state, const NetplanNetDefiniti return NULL; } - s = g_string_new(OPENVSWITCH_OVS_VSCTL " --may-exist add-bond"); - g_string_append_printf(s, " %s %s", def->bridge, def->id); + s = g_string_new(NULL); + g_string_printf(s, "%s --may-exist add-bond %s %s", + netplan_openvswitch_ovs_vsctl(), def->bridge, def->id); g_hash_table_iter_init(&iter, np_state->netdefs); while (g_hash_table_iter_next(&iter, (gpointer) &key, (gpointer) &tmp_nd)) { @@ -202,8 +249,8 @@ static void write_ovs_tag_netplan(const gchar* id, const char* type, GString* cmds) { /* Mark this bridge/port/interface as created by netplan */ - append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " set %s %s external-ids:netplan=true", - type, id); + append_systemd_cmd(cmds, "%s set %s %s external-ids:netplan=true", + netplan_openvswitch_ovs_vsctl(), type, id); } static gboolean @@ -216,7 +263,8 @@ write_ovs_bond_mode(const NetplanNetDefinition* def, GString* cmds, GError** err !strcmp(def->bond_params.mode, "balance-tcp") || !strcmp(def->bond_params.mode, "balance-slb")) { value = def->bond_params.mode; - append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " set Port %s bond_mode=%s", def->id, value); + append_systemd_cmd(cmds, "%s set Port %s bond_mode=%s", + netplan_openvswitch_ovs_vsctl(), def->id, value); write_ovs_tag_setting(def->id, "Port", "bond_mode", NULL, value, cmds); } else { g_set_error(error, NETPLAN_BACKEND_ERROR, NETPLAN_ERROR_VALIDATION, "%s: bond mode '%s' not supported by Open vSwitch\n", @@ -233,7 +281,8 @@ write_ovs_bridge_interfaces(const NetplanState* np_state, const NetplanNetDefini GHashTableIter iter; gchar* key; - append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " --may-exist add-br %s", def->id); + append_systemd_cmd(cmds, "%s --may-exist add-br %s", + netplan_openvswitch_ovs_vsctl(), def->id); g_hash_table_iter_init(&iter, np_state->netdefs); while (g_hash_table_iter_next(&iter, (gpointer) &key, (gpointer) &tmp_nd)) { @@ -243,8 +292,9 @@ write_ovs_bridge_interfaces(const NetplanState* np_state, const NetplanNetDefini GString * patch_ports = g_string_new(""); if (tmp_nd->type == NETPLAN_DEF_TYPE_PORT) setup_patch_port(patch_ports, tmp_nd); - append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " --may-exist add-port %s %s%s", - def->id, tmp_nd->id, patch_ports->str); + append_systemd_cmd(cmds, "%s --may-exist add-port %s %s%s", + netplan_openvswitch_ovs_vsctl(), def->id, + tmp_nd->id, patch_ports->str); g_string_free(patch_ports, TRUE); } } @@ -259,7 +309,8 @@ write_ovs_protocols(const NetplanOVSSettings* ovs_settings, const gchar* bridge, for (unsigned i = 1; i < ovs_settings->protocols->len; ++i) g_string_append_printf(s, ",%s", g_array_index(ovs_settings->protocols, char*, i)); - append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " set Bridge %s protocols=%s", bridge, s->str); + append_systemd_cmd(cmds, "%s set Bridge %s protocols=%s", + netplan_openvswitch_ovs_vsctl(), bridge, s->str); write_ovs_tag_setting(bridge, "Bridge", "protocols", NULL, s->str, cmds); g_string_free(s, TRUE); } @@ -301,7 +352,8 @@ write_ovs_bridge_controller_targets(const NetplanOVSSettings* settings, const Ne } g_string_erase(s, s->len-1, 1); - append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " set-controller %s %s", bridge, s->str); + append_systemd_cmd(cmds, "%s set-controller %s %s", + netplan_openvswitch_ovs_vsctl(), bridge, s->str); write_ovs_tag_setting(bridge, "Bridge", "global", "set-controller", s->str, cmds); cleanup: @@ -340,7 +392,9 @@ netplan_netdef_write_ovs(const NetplanState* np_state, const NetplanNetDefinitio write_ovs_tag_netplan(def->id, type, cmds); /* Set LACP mode, default to "off" */ value = def->ovs_settings.lacp? def->ovs_settings.lacp : "off"; - append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " set Port %s lacp=%s", def->id, value); + append_systemd_cmd(cmds, "%s set Port %s lacp=%s", + netplan_openvswitch_ovs_vsctl(), + def->id, value); write_ovs_tag_setting(def->id, type, "lacp", NULL, value, cmds); if (def->bond_params.mode && !write_ovs_bond_mode(def, cmds, error)) return FALSE; @@ -351,15 +405,23 @@ netplan_netdef_write_ovs(const NetplanState* np_state, const NetplanNetDefinitio write_ovs_tag_netplan(def->id, type, cmds); /* Set fail-mode, default to "standalone" */ value = def->ovs_settings.fail_mode? def->ovs_settings.fail_mode : "standalone"; - append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " set-fail-mode %s %s", def->id, value); + append_systemd_cmd(cmds, "%s set-fail-mode %s %s", + netplan_openvswitch_ovs_vsctl(), + def->id, value); write_ovs_tag_setting(def->id, type, "global", "set-fail-mode", value, cmds); /* Enable/disable mcast-snooping */ value = def->ovs_settings.mcast_snooping? "true" : "false"; - append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " set Bridge %s mcast_snooping_enable=%s", def->id, value); + append_systemd_cmd(cmds, + "%s set Bridge %s mcast_snooping_enable=%s", + netplan_openvswitch_ovs_vsctl(), + def->id, value); write_ovs_tag_setting(def->id, type, "mcast_snooping_enable", NULL, value, cmds); /* Enable/disable rstp */ value = def->ovs_settings.rstp? "true" : "false"; - append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " set Bridge %s rstp_enable=%s", def->id, value); + append_systemd_cmd(cmds, + "%s set Bridge %s rstp_enable=%s", + netplan_openvswitch_ovs_vsctl(), + def->id, value); write_ovs_tag_setting(def->id, type, "rstp_enable", NULL, value, cmds); /* Set protocols */ if (def->ovs_settings.protocols && def->ovs_settings.protocols->len > 0) @@ -374,7 +436,11 @@ netplan_netdef_write_ovs(const NetplanState* np_state, const NetplanNetDefinitio /* Set controller connection mode, only applicable if at least one controller target address was set */ if (def->ovs_settings.controller.connection_mode) { value = def->ovs_settings.controller.connection_mode; - append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " set Controller %s connection-mode=%s", def->id, value); + append_systemd_cmd(cmds, + "%s set Controller %s " + "connection-mode=%s", + netplan_openvswitch_ovs_vsctl(), + def->id, value); write_ovs_tag_setting(def->id, "Controller", "connection-mode", NULL, value, cmds); } } @@ -400,7 +466,9 @@ netplan_netdef_write_ovs(const NetplanState* np_state, const NetplanNetDefinitio g_assert(def->vlan_link); dependency = def->vlan_link->id; /* Create a fake VLAN bridge */ - append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " --may-exist add-br %s %s %i", def->id, def->vlan_link->id, def->vlan_id) + append_systemd_cmd(cmds, "%s --may-exist add-br %s %s %i", + netplan_openvswitch_ovs_vsctl(), + def->id, def->vlan_link->id, def->vlan_id) write_ovs_tag_netplan(def->id, type, cmds); break; @@ -477,7 +545,8 @@ netplan_state_finish_ovs_write(const NetplanState* np_state, const char* rootdir settings->ssl.client_key, settings->ssl.client_certificate, settings->ssl.ca_certificate); - append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " set-ssl %s", value->str); + append_systemd_cmd(cmds, "%s set-ssl %s", + netplan_openvswitch_ovs_vsctl(), value->str); write_ovs_tag_setting(".", "open_vswitch", "global", "set-ssl", value->str, cmds); g_string_free(value, TRUE); } diff --git a/src/openvswitch.h b/src/openvswitch.h index 67aff9add..5982b11f8 100644 --- a/src/openvswitch.h +++ b/src/openvswitch.h @@ -29,3 +29,6 @@ netplan_netdef_write_ovs( NETPLAN_INTERNAL gboolean netplan_ovs_cleanup(const char* rootdir); + +NETPLAN_INTERNAL const char * +netplan_openvswitch_ovs_vsctl(void); diff --git a/src/util-internal.h b/src/util-internal.h index fe41c776e..1d590dd1f 100644 --- a/src/util-internal.h +++ b/src/util-internal.h @@ -61,8 +61,6 @@ wifi_get_freq5(int channel); NETPLAN_ABI gchar* systemd_escape(char* string); -#define OPENVSWITCH_OVS_VSCTL "/usr/bin/ovs-vsctl" - void mark_data_as_dirty(NetplanParser* npp, const void* data_ptr); @@ -179,3 +177,7 @@ _netplan_netdef_pertype_iter_next(struct netdef_pertype_iter* it); NETPLAN_INTERNAL void _netplan_netdef_pertype_iter_free(struct netdef_pertype_iter* it); + +#ifndef MAX +#define MAX(X, Y) ((X) > (Y) ? (X) : (Y)) +#endif diff --git a/src/validation.c b/src/validation.c index 4e372135f..baaa381c7 100644 --- a/src/validation.c +++ b/src/validation.c @@ -31,6 +31,7 @@ #include "error.h" #include "util-internal.h" #include "validation.h" +#include "openvswitch.h" /* Check coherence for address types */ @@ -401,7 +402,9 @@ validate_netdef_grammar(const NetplanParser* npp, NetplanNetDefinition* nd, GErr if (nd->backend == NETPLAN_BACKEND_OVS) { // LCOV_EXCL_START - if (!g_file_test(OPENVSWITCH_OVS_VSCTL, G_FILE_TEST_EXISTS)) { + if (!g_file_test(netplan_openvswitch_ovs_vsctl(), + G_FILE_TEST_EXISTS)) + { /* Tested via integration test */ return yaml_error(npp, NULL, error, "%s: The 'ovs-vsctl' tool is required to setup OpenVSwitch interfaces.", nd->id); }