I discovered a few issues with this patch related to linking and
interacting with external code while bootstrapping the ovn-vif
repository.

Will send a v3 shortly.

On Wed, Aug 11, 2021 at 2:10 PM Frode Nordahl
<[email protected]> wrote:

[ snip ]

> diff --git a/Makefile.am b/Makefile.am
> index 0169c96ef..b1d9aba35 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -15,6 +15,8 @@ OVS_LIBDIR=@ovs_libdir@
>  OVSDB_LIBDIR=@ovsdb_libdir@
>  OVS_MANDIR=@ovs_mandir@
>
> +PLUG_PROVIDER_OBJECT=@plug_provider_OBJECT@
> +
>  AM_CPPFLAGS = $(SSL_CFLAGS)
>  AM_LDFLAGS = $(SSL_LDFLAGS)
>  AM_LDFLAGS += $(OVS_LDFLAGS)

Need to also add a -DHAVE_PLUG_PROVIDER to activate the code protected
by preprocessor ifdef's.

> diff --git a/acinclude.m4 b/acinclude.m4
> index e7f829520..0fb8bc97f 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -441,3 +441,28 @@ AC_DEFUN([OVN_CHECK_OVS], [
>    AC_MSG_CHECKING([OVS version])
>    AC_MSG_RESULT([$OVSVERSION])
>  ])
> +
> +dnl OVN_CHECK_PLUG_PROVIDER
> +dnl
> +dnl Check for external plug provider
> +AC_DEFUN([OVN_CHECK_PLUG_PROVIDER], [
> +  AC_ARG_VAR([PLUG_PROVIDER])
> +  AC_ARG_WITH(
> +    [plug-provider],
> +    [AC_HELP_STRING([--with-plug-provider=/path/to/provider.o],
> +                    [Specify path to a built plug provider object])],
> +    [if test "$withval" = yes; then
> +       if test -z "$PLUG_PROVIDER"; then
> +         AC_MSG_ERROR([To build with plug provider, specify the path to a 
> built plug provider library object on --with-plug-provider or in 
> \$PLUG_PROVIDER]),
> +       fi
> +     elif test ! -f "$withval"; then
> +       AC_MSG_ERROR([$withval is not a built plug provider library object])
> +     else
> +       PLUG_PROVIDER="$(realpath $withval)"
> +     fi],
> +    [PLUG_PROVIDER=no])
> +  AC_MSG_CHECKING([for plug provider])
> +  AC_MSG_RESULT([$PLUG_PROVIDER])
> +  AC_SUBST([PLUG_PROVIDER])
> +  AM_CONDITIONAL([HAVE_PLUG_PROVIDER], [test "$PLUG_PROVIDER" != no])
> +])

When linking we need to do `-Lpath/to/repo/lib/.libs
-lname-of-library`, so we need to make ^ a bit more elaborate, and
perhaps make it specific to the ovn-vif project.

[ snip ]
> diff --git a/controller/automake.mk b/controller/automake.mk
> index ad2d68af2..925f3f861 100644
> --- a/controller/automake.mk
> +++ b/controller/automake.mk
> @@ -40,6 +40,9 @@ controller_ovn_controller_SOURCES = \
>         controller/ovsport.c
>
>  controller_ovn_controller_LDADD = lib/libovn.la 
> $(OVS_LIBDIR)/libopenvswitch.la
> +if HAVE_PLUG_PROVIDER
> +controller_ovn_controller_LDADD += $(PLUG_PROVIDER)
> +endif
>  man_MANS += controller/ovn-controller.8
>  EXTRA_DIST += controller/ovn-controller.8.xml
>  CLEANFILES += controller/ovn-controller.8

Linking only for the controller does not make sense as the plugging
interface lives in lib/, move to lib/automake.mk

[ snip ]

> diff --git a/lib/plug.c b/lib/plug.c
> new file mode 100644
> index 000000000..09c290ac7
> --- /dev/null
> +++ b/lib/plug.c

[ snip ]

> +/* Initialize the the plug infrastructure by registering known plug classes 
> */
> +static void
> +plug_initialize(void)
> +{
> +    static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> +
> +    if (ovsthread_once_start(&once)) {
> +#ifdef HAVE_PLUG_PROVIDER
> +        for (int i = 0; i < ARRAY_SIZE(plug_provider_classes); i++) {
> +            plug_register_provider(plug_provider_classes[i]);
> +        }
> +#endif
> +        ovsthread_once_done(&once);
> +    }
> +}

The ARRAY_SIZE macro will not work here as the array is defined in
code external to the project. Need to update this to use a pointer and
NULL-terminated array instead.

-- 
Frode Nordahl
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to