Re: [dpdk-dev] [PATCH v3 1/4] compat: Add infrastructure to support symbol versioning

[Thomas Monjalon <thomas.monjalon@6wind.com>]

Hi Neil,

2014-12-23 10:51, Neil Horman:
> Add initial pass header files to support symbol versioning.

[...]

> +#   Copyright(c) 2010-2014 Neil Horman <nhorman@tuxdriver.com>

Why these dates?

> +#   All rights reserved.

I think this line is not required anymore:
	http://en.wikipedia.org/wiki/All_rights_reserved

[...]

> +#ifndef _RTE_COMPAT_H_
> +#define _RTE_COMPAT_H_

Why using underscores?
I think it's reserved:
	http://en.wikipedia.org/wiki/Include_guard#Use_of_.23include_guards

> +#define SA(x) #x

It should be prefixed. But it's better to use RTE_STR.

> +#ifdef RTE_BUILD_SHARED_LIB
> +
> +/*
> + * Provides backwards compatibility when updating exported functions.
> + * When a symol is exported from a library to provide an API, it also provides a
> + * calling convention (ABI) that is embodied in its name, return type,
> + * arguments, etc.  On occasion that function may need to change to accomodate
> + * new functionality, behavior, etc.  When that occurs, it is desireable to
> + * allow for backwards compatibility for a time with older binaries that are
> + * dynamically linked to the dpdk.  to support that the __vsym and

Should be "To support that," with uppercase and comma.

> + * VERSION_SYMBOL macros are created.  They, in conjunction with the
> + * <library>_version.map file for a given library allow for multiple versions of
> + * a symbol to exist in a shared library so that older binaries need not be
> + * immediately recompiled. Their use is outlined in the following example:
> + * Assumptions: DPDK 1.(X) contains a function int foo(char *string)
> + *              DPDK 1.(X+1) needs to change foo to be int foo(int index)
> + *
> + * To accomplish this:
> + * 1) Edit lib/<library>/library_version.map to add a DPDK_1.(X+1) node, in which
> + * foo is exported as a global symbol.
> + *
> + * 2) rename the existing function int foo(char *string) to 
> + * 	int __vsym foo_v18(char *string)
> + *
> + * 3) Add this macro immediately below the function
> + * 	VERSION_SYMBOL(foo, _v18, 1.8);
> + *
> + * 4) Implement a new version of foo.
> + * 	char foo(int value, int otherval) { ...}
> + *
> + * 5) Mark the newest version as the default version
> + * 	BIND_DEFAULT_SYMBOL(foo, 1.9);
> + *
> + */

Thanks for this good tutorial.

> +#define VERSION_SYMBOL(b, e, v) __asm__(".symver " SA(b) SA(e) ", "SA(b)"@DPDK_"SA(v))
> +#define BASE_SYMBOL(b, n) __asm__(".symver " SA(n) ", "SA(b)"@")
> +#define BIND_DEFAULT_SYMBOL(b, v) __asm__(".symver " SA(b) ", "SA(b)"@@DPDK_"SA(v))
> +#define __vsym __attribute__((used))

OK. It would be simpler to read if b, e, v and n were formally defined in a comment.

> +#else
[...]
> +/*
> + * RTE_BUILD_SHARED_LIB
> + */

This type of comment is strange. It makes me think that we are in the case
RTE_BUILD_SHARED_LIB=y

> +#endif

[...]

> +
> +CPU_LDFLAGS += --version-script=$(EXPORT_MAP)

Why this variable name? VERSION_SCRIPT or VERSION_MAP seems more appropriate.

> +
>  endif
>  
> +

Why this newline?

>  _BUILD = $(LIB)

-- 
Thomas