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

Hi Lucas,

On Mon, Aug 08, 2011, Lucas De Marchi wrote:

quoted

quoted

+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:

quoted

quoted

+               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