On Thu, Nov 04, 2010 at 07:53:12AM +0100, Oliver Hartkopp wrote:
> On 04.11.2010 01:01, Marc Kleine-Budde wrote:
> > On 11/03/2010 07:41 PM, Oliver Hartkopp wrote:
> >
> > I have some nitpicking...see comments inline:
> >
>
> >> @@ -266,7 +265,12 @@ static void bcm_can_tx(struct bcm_op *op)
> >> /* send with loopback */
> >> skb->dev = dev;
> >> skb->sk = op->sk;
> >> - can_send(skb, 1);
> >> +
> >> + if (can_send(skb, 1)) {
> >
> > That's unusual coding style. In the kernel often an intermediate
> > variable "ret" or "err" is used.
> >
> >> + struct bcm_sock *bo = bcm_sk(op->sk);
> >> +
> >> + bo->dropped_tx_msgs++;
> >> + }
>
> Hi Marc,
>
> i think assignments inside if-statements are really bad.
>
> As can_send() returns zero on success, i can't see why
>
> ret = can_send(skb, 1);
> if (ret) {
> ...
> }
>
Just my impression:
I _think_ the usual style is:
ret = can_send(skb, 1);
if (ret)
goto failure_handler;
...
failure_handler:
return ret;
The advantage of using 'ret' here is that the precise (error) return code
is saved for returning later. If this is not necessary (returning the
saved return code), I see no improvement neither.
Kurt
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
