Re: [PATCH 10/23] avrcp: handle GetCurrentPlayerAplicationSettingValue pdu

[Johan Hedberg <johan.hedberg@gmail.com>]

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