Hello, Johan.
-----Original Message-----
From: Johan Hedberg
Sent: Monday, August 22, 2011 5:27 AM
To: David Stockwell
Cc: linux-bluetooth@xxxxxxxxxxxxxxx
Subject: Re: [PATCH 2/3] AVRCP: Add Passthrough signal
Hi David,
On Sat, Aug 20, 2011, David Stockwell wrote:
Add Passthrough signal, passing key state.
If key is Vendor Unique (0x7E), also pass vendor's company_id and vendor-
unique message as string.
Signed-off-by: David Stockwell <dstockwell@xxxxxxxxxxxxxxxxx>
No signed-off-by in user-space patches please.
+++++ Got it. Will (not) do.
+/**
+ * get_company_id:
+ *
+ * Return three-byte Company_ID from AVRCP message
+ */
+static uint32_t get_company_id(uint8_t *cid) {
+ return *cid << 16 | *(cid + 1) << 8 | *(cid + 2);
+}
Please follow the coding style: new line before the opening {. Also,
wouldn't it be a bit clearer if the input parameter was defined as:
static uint32_t get_company_id(uint8_t cid[3])
Regarding new style...sorry, this is an old fix. Should have run this
through the check-patches tool. Will fix and resubmit.
Also agree regarding the input...it is more clear the way you recommend.
static void handle_panel_passthrough(struct control *control,
const unsigned char *operands,
int operand_count)
{
const char *status;
int pressed, i;
-
+ uint8_t key_pressed;
+ gboolean key_status;
+ uint32_t pass_company_id;
+ gchar *pass_string;
It seems to me you're adding some redundancies here with the existing
pressed and status variables. Try to consolidate these to the bare
minimum if possible.
+++++ OK, will take a look.
+ if (key_pressed==VENDOR_UNIQUE_OP) {
Coding style: space before and after ==
Right, forgot to run the patch check...got lazy.
+ if (operands[1] > 3) {
+ pass_company_id = get_company_id((uint8_t *) &operands[2]);
You're not checking that operand_count is large enough before accessing
operands[1] and operands[2], i.e. essentially exposing yourself to a
buffer overflow dependent on unchecked data received from the remote
device.
+++++ Yes... will fix.
+ pass_string = g_strndup((const char *) &operands[5],
+ (gsize) operands[1] - 3);
Same thing here with operands[5] and operands[1]
+ DBG("Passthrough Company_ID: %06X String: %s",
+ pass_company_id, pass_string);
Mixed tabs and spaces for indentation in the line above.
+ } else if (operands[1] == 3) {
+ pass_company_id = get_company_id((uint8_t *) &operands[2]);
Seems line an unnecessary typecast here, or then you need to change the
parameter type for get_company_id
+ pass_string = (gchar *) g_malloc0(1);
certainly an unnecessary typecast. gpointer and void* never need it.
+ DBG("Passthrough Company_ID: %06X String: <none>",
+ pass_company_id);
Mixed tabs and spaces in the line above.
+ } else {
+ pass_company_id = 0;
+ pass_string = (gchar *) g_malloc0(1);
Unnecessary typecast.
+ DBG("Passthrough: No Company_ID or String!");
+ };
+ } else {
+ pass_company_id = 0;
+ pass_string = (gchar *) g_malloc0(1);
Same here.
+ if (pass_company_id != IEEEID_BTSIG)
+ g_dbus_emit_signal(control->dev->conn, control->dev->path,
+ AUDIO_CONTROL_INTERFACE, "Passthrough",
+ DBUS_TYPE_BYTE, &key_pressed,
+ DBUS_TYPE_BOOLEAN, &key_status,
+ DBUS_TYPE_UINT32, &pass_company_id,
+ DBUS_TYPE_STRING, &pass_string,
+ DBUS_TYPE_INVALID);
+
Mixed tabs and spaces above. Additionally you've got a tab on the last
line which should be empty (i.e. only contain the newline character).
+++++ Actually, if I had just run check patches it would have caught all of
this.
Sorry to see you expend the effort to write all of this (unless you have a
"reject this patch" Perl script or something ;-).
Will resubmit, and it will be clean.
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
