diff mbox series

[BlueZ] audio/transport: Propagate errors from avrcp_set_volume to DBus

Message ID 20230311011202.486271-1-marijn.suijten@somainline.org (mailing list archive)
State Accepted
Commit 1de41786c1a22fe0d4f55805f3006ce66e5c65a6
Headers show
Series [BlueZ] audio/transport: Propagate errors from avrcp_set_volume to DBus | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch success CheckPatch PASS
tedd_an/GitLint success Gitlint PASS
tedd_an/BuildEll success Build ELL PASS
tedd_an/BluezMake success Bluez Make PASS
tedd_an/MakeCheck success Bluez Make Check PASS
tedd_an/MakeDistcheck success Make Distcheck PASS
tedd_an/CheckValgrind success Check Valgrind PASS
tedd_an/CheckSmatch success CheckSparse PASS
tedd_an/bluezmakeextell success Make External ELL PASS
tedd_an/IncrementalBuild success Incremental Build PASS
tedd_an/ScanBuild success Scan Build PASS

Commit Message

Marijn Suijten March 11, 2023, 1:12 a.m. UTC 
Any error while setting absolute volume on the peer, or notifying the
peer of changes was previously going completely unnoticed.  Propagate it
to the logs and back to the DBus "Volume" property setter so that they
aren't misled into thinking that the AVRCP command succeeded.

Note that an error is mostly harmless when the setter of the property is
an audio sink and the peer the audio source: in this case we're only
_notifying_ the peer of the change when it has already been applied on
the sink.

Also improve the other two error messages to more closely describe what
was "invalid" about the argument.
---
 profiles/audio/transport.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

Comments

bluez.test.bot@gmail.com March 11, 2023, 3:34 a.m. UTC | #1 
This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=728988

---Test result---

Test Summary:
CheckPatch                    PASS      0.56 seconds
GitLint                       PASS      0.36 seconds
BuildEll                      PASS      32.31 seconds
BluezMake                     PASS      1004.82 seconds
MakeCheck                     PASS      12.92 seconds
MakeDistcheck                 PASS      179.77 seconds
CheckValgrind                 PASS      295.28 seconds
CheckSmatch                   PASS      395.81 seconds
bluezmakeextell               PASS      120.26 seconds
IncrementalBuild              PASS      824.62 seconds
ScanBuild                     PASS      1216.73 seconds



---
Regards,
Linux Bluetooth
patchwork-bot+bluetooth@kernel.org March 21, 2023, 4 p.m. UTC | #2 
Hello:

This patch was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Sat, 11 Mar 2023 02:12:02 +0100 you wrote:
> Any error while setting absolute volume on the peer, or notifying the
> peer of changes was previously going completely unnoticed.  Propagate it
> to the logs and back to the DBus "Volume" property setter so that they
> aren't misled into thinking that the AVRCP command succeeded.
> 
> Note that an error is mostly harmless when the setter of the property is
> an audio sink and the peer the audio source: in this case we're only
> _notifying_ the peer of the change when it has already been applied on
> the sink.
> 
> [...]

Here is the summary with links:
  - [BlueZ] audio/transport: Propagate errors from avrcp_set_volume to DBus
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=1de41786c1a2

You are awesome, thank you!
diff mbox series

Patch

diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
index 457590746..2f5be8e32 100644
--- a/profiles/audio/transport.c
+++ b/profiles/audio/transport.c
@@ -734,15 +734,22 @@  static void set_volume(const GDBusPropertyTable *property,
 	uint16_t arg;
 	int8_t volume;
 	bool notify;
+	int err;
 
-	if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_UINT16)
-		goto error;
+	if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_UINT16) {
+		g_dbus_pending_property_error(id,
+				ERROR_INTERFACE ".InvalidArguments",
+				"Expected UINT16");
+		return;
+	}
 
 	dbus_message_iter_get_basic(iter, &arg);
-	if (arg > INT8_MAX)
-		goto error;
-
-	g_dbus_pending_property_success(id);
+	if (arg > INT8_MAX) {
+		g_dbus_pending_property_error(id,
+				ERROR_INTERFACE ".InvalidArguments",
+				"Volume must not be larger than 127");
+		return;
+	}
 
 	volume = (int8_t)arg;
 	if (a2dp->volume == volume)
@@ -757,12 +764,17 @@  static void set_volume(const GDBusPropertyTable *property,
 						"Volume");
 	}
 
-	avrcp_set_volume(transport->device, volume, notify);
-	return;
+	err = avrcp_set_volume(transport->device, volume, notify);
+	if (err) {
+		error("avrcp_set_volume returned %s (%d)", strerror(-err), err);
+		g_dbus_pending_property_error(id,
+				ERROR_INTERFACE ".Failed",
+				"Internal error %s (%d)",
+				strerror(-err), err);
+		return;
+	}
 
-error:
-	g_dbus_pending_property_error(id, ERROR_INTERFACE ".InvalidArguments",
-					"Invalid arguments in method call");
+	g_dbus_pending_property_success(id);
 }
 
 static gboolean endpoint_exists(const GDBusPropertyTable *property, void *data)