DPDK patches and discussions
 help / color / mirror / Atom feed
From: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
To: Neil Horman <nhorman@tuxdriver.com>
Cc: dev@dpdk.org, Bruce Richardson <bruce.richardson@intel.com>,
	Thomas Monjalon <thomas@monjalon.net>,
	robin.jarry@6wind.com
Subject: Re: [dpdk-dev] [RFC PATCH 0/2] pmdinfogen: rewrite in Python
Date: Mon, 22 Jun 2020 22:39:46 +0300
Message-ID: <20200622223946.25977c13@sovereign> (raw)
In-Reply-To: <20200622124117.GA216823@hmswarspite.think-freely.org>

On Mon, 22 Jun 2020 08:41:17 -0400, Neil Horman wrote:
> On Mon, Jun 22, 2020 at 03:45:01AM +0300, Dmitry Kozlyuk wrote:
[snip]
> > 1. No standard ELF or COFF module for Python
> >     (amount of Python code without libelf on par with C code using it).  
> Thats not really true, pyelftools is pretty widely used, and packaged for
> most(all) major distributions.  Requiring this kicks the can for (2) above down
> the road a bit, but I would prefer to see that used rather than a custom
> implementation, just to reduce the maint. burden

I must have underestimated pyelftools spread. I'll look into using it. The
less new code the better, I agree here.  Windows users can get it via PyPI.


> > 2. struct rte_pci_id must be synchronized with header file
> >     (it's a few lines that never change).
> >   
> I think you can just use ctypes and the native python C interface to just import
> the rte_pci_id structure from the native dpdk header to avoid this, no?

Not sure I follow. RFC script uses ctypes to describe struct rte_pci_id
in Python. If you're suggesting to create a Python module in C just to include
a header with a single small structure, I'd say it's not worth the trouble.


> > 1. Support for >65K section headers seems present in current pmdinfogen.
> >     However, the data it reads is not used after. Is it really needed?
> >   
> Its used.
> The support you're referring to is primarily the ability to extract the true
> number of section headers from the ELF file (in the event that its more than
> 64k).  Without that, on very large binaries, you may not be able to find the
> symbol table in the ELF file.

I was talking about these fields of struct elf_info:

	/* if Nth symbol table entry has .st_shndx = SHN_XINDEX,
	 * take shndx from symtab_shndx_start[N] instead
	 */
	Elf32_Word   *symtab_shndx_start;
	Elf32_Word   *symtab_shndx_stop;

They are not used after being filled in parse_elf() and their endianness
fixed in the end despite the comment.


> > 2. How much error-handling is required? This is a build-time tool,
> >     and Python gives nice stacktraces. However, segfaults are possible
> >     in Python version due to pointer handling. IMO, error checking
> >     must be just sufficient to prevent silent segfaults.
> >   
> I'm not really sure what the question is here.  You need to handle errors in the
> parsing process, we can't just have the tool crashing during the build.  How
> thats handled is an implementation detail I would think.

Consider a snippet from pmdinfogen.c:

	/* Check if file offset is correct */
	if (hdr->e_shoff > info->size) {
		fprintf(stderr, "section header offset=%lu in file '%s' "
		      "is bigger than filesize=%lu\n",
		      (unsigned long)hdr->e_shoff,
		      filename, info->size);
		return 0;
	}

It is required in C, because otherwise pmdinfogen would crash without
diagnostic. With Python, most errors like this result in a crash with a trace
to the error line and a message from ctypes (or ELF parsing library). I'm
arguing for leaving only the checks that prevent *silent* crashes (this can
happen with ctypes) which are hard to diagnose. Motivation is to keep the
code simple.


> > 3. On Unix, pmdinfogen is called for each object file extracted with ar
> >     from an .a library by a shell script. On Windows, other tools
> >     have to be used, shell script will not work. On the other hand, COFF
> >     library format is quite simple. Would it be appropriate for
> >     pmdinfogen to handle it to avoid intermediate script?
> >   
> I suppose you could, but extracting objects from archives seems outside the
> scope of what pmdinfogen normally does.  What is the problem with using a shell
> script on windows?  I thought WSL had support for executing bash syntax shell
> scripts there?  Why not key off an environment variable to make the relevant
> scripts do the right thing on your environment?

WSL2 is a Linux VM integrated in Windows, you can only cross-compile from
there. Native builds can use Python or PowerShell, which is as capable as Unix
shells, but incompatible with them. To call lib.exe (MSVC analog of ar) is
probably simpler then parsing COFF, yes. So meson could select one of the
following for a Windows target (objects are COFF):

Host     Toolchain  Archive  Script      Extractor
-------  ---------  -------  ---------   ---------
Linux    MinGW      .a       sh          ar
Windows  MinGW      .a       PowerShell  ar
Windows  Clang      .lib     PowerShell  lib

-- 
Dmitry Kozlyuk

  reply	other threads:[~2020-06-22 19:39 UTC|newest]

Thread overview: 88+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-22  0:45 Dmitry Kozlyuk
2020-06-22  0:45 ` [dpdk-dev] [RFC PATCH 1/2] pmdinfogen: prototype " Dmitry Kozlyuk
2020-06-22  0:45 ` [dpdk-dev] [RFC PATCH 2/2] build: use Python pmdinfogen Dmitry Kozlyuk
2020-06-22 12:41 ` [dpdk-dev] [RFC PATCH 0/2] pmdinfogen: rewrite in Python Neil Horman
2020-06-22 19:39   ` Dmitry Kozlyuk [this message]
2020-06-23 11:28     ` Neil Horman
2020-06-23 11:59       ` Bruce Richardson
2020-07-02  0:07       ` Dmitry Kozlyuk
2020-07-02  0:02 ` [dpdk-dev] [RFC PATCH v2 " Dmitry Kozlyuk
2020-07-02  0:02   ` [dpdk-dev] [RFC PATCH v2 1/3] pmdinfogen: prototype " Dmitry Kozlyuk
2020-07-02  0:02   ` [dpdk-dev] [RFC PATCH v2 2/3] build: use Python pmdinfogen Dmitry Kozlyuk
2020-07-02  0:02   ` [dpdk-dev] [RFC PATCH v2 3/3] doc/linux_gsg: require pyelftools for pmdinfogen Dmitry Kozlyuk
2020-07-06 12:52     ` Neil Horman
2020-07-06 13:24       ` Dmitry Kozlyuk
2020-07-06 16:46         ` Neil Horman
2020-07-08  0:53   ` [dpdk-dev] [PATCH v3 0/4] pmdinfogen: rewrite in Python Dmitry Kozlyuk
2020-07-08  0:53     ` [dpdk-dev] [PATCH v3 1/4] pmdinfogen: add Python implementation Dmitry Kozlyuk
2020-07-08  0:53     ` [dpdk-dev] [PATCH v3 2/4] build: use Python pmdinfogen Dmitry Kozlyuk
2020-07-08  0:53     ` [dpdk-dev] [PATCH v3 3/4] doc/linux_gsg: require pyelftools for pmdinfogen Dmitry Kozlyuk
2020-07-08  0:53     ` [dpdk-dev] [PATCH v3 4/4] pmdinfogen: remove C implementation Dmitry Kozlyuk
2020-07-08 21:23     ` [dpdk-dev] [PATCH v4 0/4] pmdinfogen: rewrite in Python Dmitry Kozlyuk
2020-07-08 21:23       ` [dpdk-dev] [PATCH v4 1/4] pmdinfogen: add Python implementation Dmitry Kozlyuk
2020-07-08 21:23       ` [dpdk-dev] [PATCH v4 2/4] build: use Python pmdinfogen Dmitry Kozlyuk
2020-07-21 14:04         ` Bruce Richardson
2020-07-21 14:59           ` Dmitry Kozlyuk
2020-07-08 21:23       ` [dpdk-dev] [PATCH v4 3/4] doc/linux_gsg: require pyelftools for pmdinfogen Dmitry Kozlyuk
2020-07-21 13:39         ` Bruce Richardson
2020-07-21 14:05           ` Bruce Richardson
2020-07-21 14:04         ` Bruce Richardson
2020-07-08 21:23       ` [dpdk-dev] [PATCH v4 4/4] pmdinfogen: remove C implementation Dmitry Kozlyuk
2020-07-09 10:42       ` [dpdk-dev] [PATCH v4 0/4] pmdinfogen: rewrite in Python Neil Horman
2020-07-21 13:51       ` Bruce Richardson
2020-09-27 21:47       ` [dpdk-dev] [PATCH v5 0/3] " Dmitry Kozlyuk
2020-09-27 21:47         ` [dpdk-dev] [PATCH v5 1/3] pmdinfogen: add Python implementation Dmitry Kozlyuk
2020-09-27 22:05           ` Stephen Hemminger
2020-09-27 21:47         ` [dpdk-dev] [PATCH v5 2/3] build: use Python pmdinfogen Dmitry Kozlyuk
2020-09-27 21:47         ` [dpdk-dev] [PATCH v5 3/3] pmdinfogen: remove C implementation Dmitry Kozlyuk
2020-09-27 23:15           ` Thomas Monjalon
2020-09-28  9:35         ` [dpdk-dev] [PATCH v5 0/3] pmdinfogen: rewrite in Python David Marchand
2020-10-04  1:59         ` [dpdk-dev] [PATCH v6 " Dmitry Kozlyuk
2020-10-04  1:59           ` [dpdk-dev] [PATCH v6 1/3] pmdinfogen: add Python implementation Dmitry Kozlyuk
2020-10-04  1:59           ` [dpdk-dev] [PATCH v6 2/3] build: use Python pmdinfogen Dmitry Kozlyuk
2020-10-04  1:59           ` [dpdk-dev] [PATCH v6 3/3] pmdinfogen: remove C implementation Dmitry Kozlyuk
2020-10-14 14:37           ` [dpdk-dev] [PATCH v6 0/3] pmdinfogen: rewrite in Python Maxime Coquelin
2020-10-14 15:40             ` Dmitry Kozlyuk
2020-10-14 18:31           ` [dpdk-dev] [PATCH v7 " Dmitry Kozlyuk
2020-10-14 18:31             ` [dpdk-dev] [PATCH v7 1/3] pmdinfogen: add Python implementation Dmitry Kozlyuk
2020-10-14 18:31             ` [dpdk-dev] [PATCH v7 2/3] build: use Python pmdinfogen Dmitry Kozlyuk
2020-10-14 18:31             ` [dpdk-dev] [PATCH v7 3/3] pmdinfogen: remove C implementation Dmitry Kozlyuk
2020-10-20 16:02             ` [dpdk-dev] [PATCH v7 0/3] pmdinfogen: rewrite in Python David Marchand
2020-10-20 17:45               ` Dmitry Kozlyuk
2020-10-20 22:09               ` Dmitry Kozlyuk
2020-10-20 17:44             ` [dpdk-dev] [PATCH v8 " Dmitry Kozlyuk
2020-10-20 17:44               ` [dpdk-dev] [PATCH v8 1/3] pmdinfogen: add Python implementation Dmitry Kozlyuk
2020-10-20 17:44               ` [dpdk-dev] [PATCH v8 2/3] build: use Python pmdinfogen Dmitry Kozlyuk
2020-10-21  9:00                 ` Bruce Richardson
2021-01-20  0:05                 ` Thomas Monjalon
2021-01-20  7:23                   ` Dmitry Kozlyuk
2021-01-20 10:24                     ` Thomas Monjalon
2021-01-22 20:31                       ` Dmitry Kozlyuk
2021-01-22 20:57                         ` Thomas Monjalon
2021-01-22 22:24                           ` Dmitry Kozlyuk
2021-01-23 11:38                             ` Thomas Monjalon
2021-01-24 20:52                               ` Dmitry Kozlyuk
2021-01-25  9:25                               ` Kinsella, Ray
2021-01-25 10:01                                 ` Kinsella, Ray
2021-01-25 10:29                                   ` David Marchand
2021-01-25 10:46                                     ` Kinsella, Ray
2021-01-25 11:03                                       ` Thomas Monjalon
2021-01-25 10:05                                 ` Dmitry Kozlyuk
2021-01-25 10:11                                   ` Kinsella, Ray
2021-01-25 10:31                                     ` Dmitry Kozlyuk
2020-10-20 17:44               ` [dpdk-dev] [PATCH v8 3/3] pmdinfogen: remove C implementation Dmitry Kozlyuk
2020-10-26 16:46                 ` Jie Zhou
2021-01-22 22:43               ` [dpdk-dev] [PATCH v9 0/3] pmdinfogen: rewrite in Python Dmitry Kozlyuk
2021-01-22 22:43                 ` [dpdk-dev] [PATCH v9 1/3] pmdinfogen: add Python implementation Dmitry Kozlyuk
2021-01-22 22:43                 ` [dpdk-dev] [PATCH v9 2/3] build: use Python pmdinfogen Dmitry Kozlyuk
2021-01-22 22:43                 ` [dpdk-dev] [PATCH v9 3/3] pmdinfogen: remove C implementation Dmitry Kozlyuk
2021-01-24 20:51                 ` [dpdk-dev] [PATCH v10 0/3] pmdinfogen: rewrite in Python Dmitry Kozlyuk
2021-01-24 20:51                   ` [dpdk-dev] [PATCH v10 1/3] pmdinfogen: add Python implementation Dmitry Kozlyuk
2021-01-24 20:51                   ` [dpdk-dev] [PATCH v10 2/3] build: use Python pmdinfogen Dmitry Kozlyuk
2021-01-25 10:12                     ` Thomas Monjalon
2021-01-24 20:51                   ` [dpdk-dev] [PATCH v10 3/3] pmdinfogen: remove C implementation Dmitry Kozlyuk
2021-01-25 13:13                   ` [dpdk-dev] [PATCH v10 0/3] pmdinfogen: rewrite in Python Thomas Monjalon
2021-01-25 16:08                     ` Brandon Lo
2021-02-02  8:48                       ` Tal Shnaiderman
2021-01-25 18:51                   ` Ali Alnubani
2021-01-25 22:15                     ` Dmitry Kozlyuk

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200622223946.25977c13@sovereign \
    --to=dmitry.kozliuk@gmail.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=nhorman@tuxdriver.com \
    --cc=robin.jarry@6wind.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git