Hi Lucas,
On Mon, Aug 08, 2011, Lucas De Marchi wrote:
> >> +static int avrcp_handle_get_current_player_value(struct control *control,
> >> + struct avrcp_spec_avc_pdu *pdu)
> >> +{
> >> + uint16_t len = ntohs(pdu->params_len);
> >> + struct media_player *mp = control->mp;
> >> +
> >> + if (!mp)
> >> + goto done;
> >> +
> >> + if (len > 1 && pdu->params[0] == len - 1) {
> >
> > This if statement is quite long, perhaps you could do check the
> > opposite e.g. if (len <=1 || pdu->params[0] != len - 1) goto done;
>
> In all other parts of the code I do like you said because it's much
> cleaner. Here however I can't because of the following declaration:
>
> >> + uint8_t settings[pdu->params[0]];
>
> It depends on the value of pdu->params[0]
Actually Luiz should have objected to that even more than to the coding
style. I don't think we use anywhere else array size initializers which
are not determinable upon compile time (I suppose this is also a very
recent C feature?). Furthermore, since these values are coming from the
remote side you're effectively allowing them to to "explode" our stack
here. So, dynamically allocating memory from the heap would be a (much)
better idea in this case (and then you can easily use the coding style
fix suggested by Luiz).
Johan
--
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
