Re: [B.A.T.M.A.N.] [PATCH v8 05/14] batman-adv: netlink: add translation table query

Antonio Quartulli
2016-05-26T08:42:32Z

[-- Attachment #1: Type: text/plain, Size: 1453 bytes --]

On Thu, May 26, 2016 at 10:30:15AM +0200, Sven Eckelmann wrote:
> Hi,
>
> thanks a lot for your feedback.
>
>
> On Thursday 26 May 2016 06:39:14 Linus Lüssing wrote:
> > On Mon, May 23, 2016 at 05:59:36PM +0200, Simon Wunderlich wrote:
> > > +static int
> > > +batadv_tt_local_dump_entry(struct sk_buff *msg, u32 portid, u32 seq,
> > > + struct batadv_priv *bat_priv,
> > > + struct batadv_tt_common_entry *common)
> > > +{
> > [...]
> > > + if (!(common->flags & BATADV_TT_CLIENT_NOPURGE)) {
> > > + if (nla_put_u32(msg, BATADV_ATTR_LAST_SEEN_MSECS,
> > > + last_seen_msecs))
> > > + goto nla_put_failure;
> > > + }
> >
> > Unnecessary brackets here, maybe use '&&' to skip a layer of
> > indention?
>
> I would disagree here. The { } braces make sense because it is not a simple
> (single) statement inside the outer if body. But I am partially on your side
> regarding the extra indent. But having both in one statement is also weird
> because one is a simple check and the other one is a function call with side
> effects. So I am not 100% sure at the moment and will leave this untouched for
> now. Maybe Simon wants to add it or I will think about it a little bit more
> about it.

Although having both statements in the same check wouldn't change the semantics,
I do agree that having a clear separation makes it easier to understand.

my 2 cents.

Cheers,

--
Antonio Quartulli

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]