DPDK patches and discussions
 help / color / mirror / Atom feed
From: Robin Jarry <robin.jarry@6wind.com>
To: Thomas Monjalon <thomas@monjalon.net>
Cc: Ciara Power <ciara.power@intel.com>,
	kevin.laatz@intel.com, stable@dpdk.org, dev@dpdk.org
Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH] usertools: add telemetry python3 compatibility
Date: Mon, 20 Jan 2020 09:47:07 +0100	[thread overview]
Message-ID: <20200120084707.kgo6346unadkpzzo@6wind.com> (raw)
In-Reply-To: <2200875.3c9HiEOlIg@xps>

16/01/2020 18:24, Ciara Power:
> The client script for use with the telemetry library did not support
> python3, as the data being sent over the socket was in string format.
> Python3 requires the data be explicitly converted to bytes before being
> sent.  Similarily, the received bytes need to be decoded into string
> format.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ciara Power <ciara.power@intel.com>

Overall, it looks good to me. One minor grudge:

Mind that when using this script with python2, the literal strings
actually are bytes. This means that .encode() does not make any sense on
them. As it turns out, the str objects of python2 do have an .encode()
method that does not do anything (it returns the byte string object
unchanged), so calling it does not cause any problem.

Long story short, for consistency you should consider adding a future
import at the top:

    from __future__ import unicode_literals

So that all literal strings are unicode with python2 as with python3.
See related commit 4da069194ef4 ("usertools: fix pmdinfo with python
3 and pyelftools>=0.24").

Maybe the patch title should contain the word "fix" and some Fixes:
lines as there already were some attemps to make this script python3
compatible.

Reviewed-by: Robin Jarry <robin.jarry@6wind.com>

-- 
Robin

  reply	other threads:[~2020-01-20  8:47 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-16 17:24 [dpdk-dev] " Ciara Power
2020-01-19 21:48 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
2020-01-20  8:47   ` Robin Jarry [this message]
2020-01-21 17:03 ` [dpdk-dev] [PATCH v2] usertools: fix " Ciara Power
2020-01-24 10:11   ` Laatz, Kevin
2020-02-16 21:25     ` Thomas Monjalon

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=20200120084707.kgo6346unadkpzzo@6wind.com \
    --to=robin.jarry@6wind.com \
    --cc=ciara.power@intel.com \
    --cc=dev@dpdk.org \
    --cc=kevin.laatz@intel.com \
    --cc=stable@dpdk.org \
    --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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).