Re: [PATCH v2 1/2] android/bluetooth: Add support for get remote service record property cmd

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

Hi Grzegorz,

On Thu, Aug 14, 2014, Grzegorz Kolodziejczyk wrote:
> +		prop->channel = (uint16_t)channel;

This typecast seems unnecessary to me. At least I don't get any compiler
warnings/errors if I remove it.

> +static void find_remote_sdp_rec_cb(sdp_list_t *recs, int err,
> +							gpointer user_data)
> +{
> +	uint8_t name_len = 0;
> +	uint8_t status;
> +	char name_buf[256];
> +	int channel = 0;

Since both name_len and channel are tied to the specific record that
you're iterating it seems wrong to me to initialize these here.

> +	for ( ; recs; recs = recs->next) {
> +		sdp_rec = recs->data;
> +
> +		switch (sdp_rec->svclass.type) {
> +		case SDP_UUID16:
> +			sdp_uuid16_to_uuid128(&uuid128_sdp,
> +							&sdp_rec->svclass);
> +			break;
> +		case SDP_UUID32:
> +			sdp_uuid32_to_uuid128(&uuid128_sdp,
> +							&sdp_rec->svclass);
> +			break;
> +		case SDP_UUID128:
> +			break;
> +		default:
> +			error("wrong sdp uuid type");
> +			goto done;
> +		}
> +
> +		if (!sdp_get_access_protos(sdp_rec, &protos)) {
> +			channel = sdp_get_proto_port(protos, L2CAP_UUID);
> +			if (channel < 0)
> +				error("wrong channel");
> +		}

Probably this should be here:

		} else {
			channel = -1;
		}

> +		if (!sdp_get_service_name(sdp_rec, name_buf, sizeof(name_buf)))
> +			name_len = (uint8_t)strlen(name_buf);

And here:

		else
			name_len = 0;

Otherwise you'll end up including the name from previous records as the
name of subsequent ones which don't have a service name attribute. Also,
the (uint8_t) typecast seems unnecessary here (compiles fine for me
without it).

> +static uint8_t find_remote_sdp_rec(const bdaddr_t *addr,
> +						const uint8_t *find_uuid)
> +{
> +	uuid_t uuid;
> +	bdaddr_t *bdaddr;
> +
> +	/* from android we always get full 128bit length uuid */
> +	sdp_uuid128_create(&uuid, find_uuid);
> +
> +	bdaddr = malloc(sizeof(*bdaddr));

Could you tell me what's the rationale of choosing malloc vs g_malloc in
this code. In most places this c-file is using the GLib allocators and
having a mixed set is just begging for trouble (e.g. you might end up
calling g_free on something allocated with malloc, or free for something
allocated with g_malloc).

Johan