On 11/04/2010 08:47 AM, Kurt Van Dijck wrote:
> 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;Yes. One remark about the goto error handling. It's mainly used in drivers when acquiring multiple resources, to avoid code duplication and errors during the multiple cleanups. > ... > 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. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions | Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
