From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id 8DEF55A1F for ; Wed, 14 Jan 2015 21:30:03 +0100 (CET) Received: from hmsreliant.think-freely.org ([2001:470:8:a08:7aac:c0ff:fec2:933b] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63) (envelope-from ) id 1YBUZg-00056k-DF; Wed, 14 Jan 2015 15:30:00 -0500 Date: Wed, 14 Jan 2015 15:29:50 -0500 From: Neil Horman To: Thomas Monjalon Message-ID: <20150114202950.GC28492@hmsreliant.think-freely.org> References: <1419109299-9603-1-git-send-email-nhorman@tuxdriver.com> <1419349913-21674-1-git-send-email-nhorman@tuxdriver.com> <2338166.O6glFPMYza@xps13> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2338166.O6glFPMYza@xps13> User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Score: -2.9 (--) X-Spam-Status: No Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v3 1/4] compat: Add infrastructure to support symbol versioning X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 14 Jan 2015 20:30:04 -0000 On Wed, Jan 14, 2015 at 04:25:19PM +0100, Thomas Monjalon wrote: > Hi Neil, > > 2014-12-23 10:51, Neil Horman: > > Add initial pass header files to support symbol versioning. > > [...] > > > +# Copyright(c) 2010-2014 Neil Horman > > Why these dates? > Because I copied the Makefile from librte_acl, and modified the name but not the dates. > > +# All rights reserved. > > I think this line is not required anymore: > http://en.wikipedia.org/wiki/All_rights_reserved > Hmm, apparently so. However, since it exists in every other copyright notice in the tree, I'd just as soon keep this language consistent, and make a tree wide change in a separate patch if the consensus is to do so. > [...] > > > +#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 > Its reserved for the implementation, and must not be used by a user using the header file. Its ok, and is common practice. See every other symlinked header file in the DPDK. > > +#define SA(x) #x > > It should be prefixed. But it's better to use RTE_STR. > very well > > +#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. > yup > > + * VERSION_SYMBOL macros are created. They, in conjunction with the > > + * _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_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 >