DPDK patches and discussions
 help / color / mirror / Atom feed
From: Bruce Richardson <bruce.richardson@intel.com>
To: Thomas Monjalon <thomas@monjalon.net>
Cc: dev@dpdk.org, "Hunt, David" <david.hunt@intel.com>,
	Hajkowski <marcinx.hajkowski@intel.com>,
	john.mcnamara@intel.com, marko.kovacevic@intel.com
Subject: Re: [dpdk-dev] [PATCH 1/1] doc: announce change in power API
Date: Fri, 10 May 2019 10:28:14 +0100	[thread overview]
Message-ID: <20190510092814.GA76@bricha3-MOBL.ger.corp.intel.com> (raw)
Message-ID: <20190510092814.cJO7v-t1fFQKp62bV5C65b0odxL3HDXHQhWlGfArOjE@z> (raw)
In-Reply-To: <2474197.K3lgESSsOM@xps>

On Fri, May 10, 2019 at 01:28:09AM +0200, Thomas Monjalon wrote:
> > > From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> > >
> > > Function rte_power_set_env will no longer return
> > > success on attempt to set env in initialized state.
> > >
> > > Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> > 
> > Acked-by: David Hunt <david.hunt@intel.com>
> 
> Any other comment about this deprecation notice?
> 
Seems ok to me, though the actual text is maybe a little unclear. It
implies that the function will always return -1 unless the variable is
unset when the function terminates (which seems to imply a failure case).
What I presume is meant is that we have three possibilities:

* The variable is set by the function -> return 0
* The varaible is already set, so no action needed -> return -1 (and set
  rte_errno to EEXIST or EALREADY??)
* Setting the variable failed -> return -1 (and set rte_errno to ??)

Is my understanding correct? Can the deprecation notice be improved to make
it clear that only the middle case is the one being changed, e.g. by adding
"in this case" to the second sentence. It might also be worthwhile calling
out what the errno value will be to identify this failure vs regular
failures.

/Bruce

PS: For this case, is there a reason to make it an error? Would a +1 value
not also do, so anything non-zero implies no work done, and anything >=0
means that the value is set? Call set on something already set doesn't
really seem like an error case to me.

  parent reply	other threads:[~2019-05-10  9:28 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-13 19:21 Hajkowski
2019-03-13 19:21 ` Hajkowski
2019-03-14 18:50 ` Rami Rosen
2019-03-14 18:50   ` Rami Rosen
2019-04-18 14:33 ` Hunt, David
2019-04-18 14:33   ` Hunt, David
2019-05-09 23:28   ` Thomas Monjalon
2019-05-09 23:28     ` Thomas Monjalon
2019-05-10  9:28     ` Bruce Richardson [this message]
2019-05-10  9:28       ` Bruce Richardson
2019-05-10  9:33       ` Bruce Richardson
2019-05-10  9:33         ` Bruce Richardson
2019-05-10  9:26 ` Burakov, Anatoly
2019-05-10  9:26   ` Burakov, Anatoly
2019-05-13 13:52 ` [dpdk-dev] [PATCH v2] " Hajkowski
2019-05-13 13:52   ` Hajkowski
2019-05-13 21:35   ` Thomas Monjalon
2019-05-13 21:35     ` 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=20190510092814.GA76@bricha3-MOBL.ger.corp.intel.com \
    --to=bruce.richardson@intel.com \
    --cc=david.hunt@intel.com \
    --cc=dev@dpdk.org \
    --cc=john.mcnamara@intel.com \
    --cc=marcinx.hajkowski@intel.com \
    --cc=marko.kovacevic@intel.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
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).