From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thomas Monjalon <thomas.monjalon@6wind.com>
Subject: Re: [PATCH v1 02/10] app/test: support resources
	externally linked
Date: Fri, 06 May 2016 16:32:53 +0200
Message-ID: <2406220.vlQ29PEzMg@xps13>
References: <1462531720-26217-1-git-send-email-viktorin@rehivetech.com>
 <1462531720-26217-3-git-send-email-viktorin@rehivetech.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7Bit
Cc: dev@dpdk.org, Bruce Richardson <bruce.richardson@intel.com>,
 David Marchand <david.marchand@6wind.com>
To: Jan Viktorin <viktorin@rehivetech.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mail-wm0-f41.google.com (mail-wm0-f41.google.com [74.125.82.41])
 by dpdk.org (Postfix) with ESMTP id 1B30F2986
 for <dev@dpdk.org>; Fri,  6 May 2016 16:32:56 +0200 (CEST)
Received: by mail-wm0-f41.google.com with SMTP id a17so81170290wme.0
 for <dev@dpdk.org>; Fri, 06 May 2016 07:32:56 -0700 (PDT)
In-Reply-To: <1462531720-26217-3-git-send-email-viktorin@rehivetech.com>
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

It looks a lot too much tricky to be integrated without code comments ;)
Please make a documentation effort.

2016-05-06 12:48, Jan Viktorin:
> --- a/app/test/Makefile
> +++ b/app/test/Makefile
> @@ -33,6 +33,37 @@ include $(RTE_SDK)/mk/rte.vars.mk
>  
>  ifeq ($(CONFIG_RTE_APP_TEST),y)

A comment is needed here to explain the intent of the following code.

> +ifeq ($(RTE_ARCH),arm)
> +RTE_OBJCOPY_O = elf32-littlearm
> +RTE_OBJCOPY_B = arm
> +else ifeq ($(RTE_ARCH),arm64)
> +RTE_OBJCOPY_O = elf64-littleaarch64
> +RTE_OBJCOPY_B = aarch64
> +else ifeq ($(RTE_ARCH),i686)
> +RTE_OBJCOPY_O = elf32-i386
> +RTE_OBJCOPY_B = i386
> +else ifeq ($(RTE_ARCH),x86_64)
> +RTE_OBJCOPY_O = elf64-x86-64
> +RTE_OBJCOPY_B = i386:x86-64
> +else ifeq ($(RTE_ARCH),x86_x32)
> +RTE_OBJCOPY_O = elf32-x86-64
> +RTE_OBJCOPY_B = i386:x86-64
> +else
> +$(error Unrecognized RTE_ARCH: $(RTE_ARCH))
> +endif

RTE_OBJCOPY_O could be renamed RTE_OBJCOPY_TARGET.
RTE_OBJCOPY_B could be renamed RTE_OBJCOPY_ARCH.

These definitions could be done in mk/arch/

> +
> +define resource

When defining a makefile macro, the arguments must be documented on
the previous line. Otherwise, nobody (including you) will be able
to read it without parsing the code below.

It is not a simple resource, so the name must avoid the confusion.
Why not linked_resource?

> +SRCS-y += $(1).res.o
> +$(1).res.o: $(2)
> +       $(OBJCOPY) -I binary -B $(RTE_OBJCOPY_B) -O $(RTE_OBJCOPY_O)     \
> +               --rename-section                                         \
> +                       .data=.rodata,alloc,load,data,contents,readonly  \
> +               --redefine-sym _binary__dev_stdin_start=beg_$(1)         \
> +               --redefine-sym _binary__dev_stdin_end=end_$(1)           \
> +               --redefine-sym _binary__dev_stdin_size=siz_$(1)          \
> +               /dev/stdin $$@ < $$<
> +endef

[...]

> +#define REGISTER_LINKED_RESOURCE(_n) \
> +extern const char beg_ ##_n;         \
> +extern const char end_ ##_n;         \
> +REGISTER_RESOURCE(_n, &beg_ ##_n, &end_ ##_n); \

Please explain the begin/end trick.
Thanks