Hi Andrei,
On Wed, Feb 8, 2012 at 11:09 AM, Andrei Emeltchenko
<andrei.emeltchenko.news@xxxxxxxxx> wrote:
>
> I think this would be good to send as patch series so that people can
> comment. What comes to my mind is that the patch might be reduced if it
> does not change order of functions and defines like:
>
> <------8<-----------------------------------------------------------------
> | -#define L2CAP_EXT_CTRL_TXSEQ 0xFFFC0000
> | #define L2CAP_EXT_CTRL_SAR 0x00030000
> | -#define L2CAP_EXT_CTRL_SUPERVISE 0x00030000
> | #define L2CAP_EXT_CTRL_REQSEQ 0x0000FFFC
> | -
> | -#define L2CAP_EXT_CTRL_POLL 0x00040000
> | +#define L2CAP_EXT_CTRL_TXSEQ 0xFFFC0000
> | #define L2CAP_EXT_CTRL_FINAL 0x00000002
> | +#define L2CAP_EXT_CTRL_POLL 0x00040000
> | +#define L2CAP_EXT_CTRL_SUPERVISE 0x00030000
> | #define L2CAP_EXT_CTRL_FRAME_TYPE 0x00000001 /* I- or S-Frame */
> <------8<-----------------------------------------------------------------
>
> and I don't like this kind of change:
>
> <------8<--------------------------------------------------------------------
> | - if (__is_sar_start(chan, control) && !__is_sframe(chan, control))
> | - len -= L2CAP_SDULEN_SIZE;
> | + if ((control->frame_type == 'i') &&
> | + (control->sar == L2CAP_SAR_START))
> | + len -= 2;
> |
> | if (chan->fcs == L2CAP_FCS_CRC16)
> | - len -= L2CAP_FCS_SIZE;
> | + len -= 2;
> <------8<--------------------------------------------------------------------
>
> why not to use macros and defines for magic numbers?
>
> the same below:
>
> <------8<----------------------------------------------------------------
> | + if (test_bit(FLAG_EXT_CTRL, &chan->flags)) {
> | + __get_extended_control(get_unaligned_le32(skb->data),
> | + control);
> | + skb_pull(skb, 4);
> | + } else {
> | + __get_enhanced_control(get_unaligned_le16(skb->data),
> | + control);
> | + skb_pull(skb, 2);
> | + }
> |
> | - control = __get_control(chan, skb->data);
> | - skb_pull(skb, __ctrl_size(chan));
> <------8<----------------------------------------------------------------
>
> those magic number does not look nice IMO and the code is not looking any
> better.
In this aspect perhaps, but we don't need to take the code as it is,
but the point here is following the states defined by the spec and
that is IMO much better.
--
Luiz Augusto von Dentz
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
