Re: [dpdk-dev] [PATCH 1/4] mbuf: add accessor function for private data area

[Dan Gora <dg@adax.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