DPDK patches and discussions
 help / color / mirror / Atom feed

From: Dan Gora <dg@adax.com>
To: "Wiles, Keith" <keith.wiles@intel.com>
Cc: Andrew Rybchenko <arybchenko@solarflare.com>,
	"dev@dpdk.org" <dev@dpdk.org>,
	Olivier Matz <olivier.matz@6wind.com>
Subject: Re: [dpdk-dev] [PATCH 1/4] mbuf: add accessor function for private data area
Date: Fri, 8 Jun 2018 21:24:10 -0300	[thread overview]
Message-ID: <CAGyogRb5AWtkVenEu0nPjsVMTuRDm9P-Ta0LybsaKMiprnxYSw@mail.gmail.com> (raw)
In-Reply-To: <5F1328F1-6991-45EA-8DC2-300600702210@intel.com>

Hi All,

Thanks for the feedback.

>> Just a nit...
>> As I understand 'md' here follows previous function which is
>> rte_mbuf_to_baddr() and works with direct mbuf - that's why
>> parameter is named 'md' (mbuf direct). The most of functions
>> in the header use just 'm' for any mbuf.

Ok, I'll fix this for v2.

>>> +{
>>> +    if (md->priv_size == 0)
>>> +            return NULL;
>>> +
>>> +    return RTE_PTR_ADD(md, sizeof(struct rte_mbuf));
>>
>> Also a nit...
>> I'd use sizeof(*md) (or sizeof(*m) in fact as described above) here.
>> At least previous functions do it in such way.
>
> I believe the sizeof(struct rte_mbuf) is much more readable then sizeof(*m) it makes the reader have to look up what ‘m’ is defined to. I know this is a small function, but readability is still a good reason to not use sizeof(*m) IMO.

On one hand, using sizeof(*m) is useful in case the type of 'm' ever
changes, you don't have to remember to change the arguments to sizeof.
On the other hand, it does make it slightly harder to read, but not a
lot really.

For me, it's six one way, half a dozen the other.   I just cut-pasted
this from the ipsec-secgw code.  I'm kind of inclined to leave it
sizeof(struct rte_mbuf) just to leave it clear.

Any opinion on my question from the cover letter?

Specifically when should rte_mbuf_XXX be used vs rte_pktmbuf_XXX for
mbuf API functions?  Why is there this inconsistency there?  Are they
just historical names which couldn't get changed?

One more quick question:

When sending a v2 of a patch series, should I resend the whole bundle,
even if there are no changes in the other patches or just send a v2 of
the patch which actually contains changes from the v1 version?

thanks
dan

next prev parent reply	other threads:[~2018-06-09  0:24 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-07 23:54 Dan Gora
2018-06-08  8:30 ` Burakov, Anatoly
2018-06-08  9:06 ` Andrew Rybchenko
2018-06-08 17:19   ` Wiles, Keith
2018-06-09  0:24     ` Dan Gora [this message]
2018-06-09  9:23       ` Andrew Rybchenko
2018-06-12  2:24         ` Dan Gora

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=CAGyogRb5AWtkVenEu0nPjsVMTuRDm9P-Ta0LybsaKMiprnxYSw@mail.gmail.com \
    --to=dg@adax.com \
    --cc=arybchenko@solarflare.com \
    --cc=dev@dpdk.org \
    --cc=keith.wiles@intel.com \
    --cc=olivier.matz@6wind.com \
    /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

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).