2011-03-16 12:01:12

by Dmitriy Paliy

[permalink] [raw]
Subject: [PATCH] Add 'Protocol not supported' error in a2dp_add_sep

'Protocol not supported' error code is added to registeration of A2DP
end-points. Error response org.bluez.Error.NotSupported instead of
org.bluez.Error.InvalidArguments is used when SEP registration fails
due to disabled corresponding interface in audio.conf.
---
audio/a2dp.c | 34 ++++++++++++++++++++++++++--------
audio/a2dp.h | 2 +-
audio/media.c | 23 +++++++++++++++++------
3 files changed, 44 insertions(+), 15 deletions(-)

diff --git a/audio/a2dp.c b/audio/a2dp.c
index 3407d6f..8c3698a 100644
--- a/audio/a2dp.c
+++ b/audio/a2dp.c
@@ -1486,22 +1486,23 @@ proceed:
if (source) {
for (i = 0; i < sbc_srcs; i++)
a2dp_add_sep(src, AVDTP_SEP_TYPE_SOURCE,
- A2DP_CODEC_SBC, delay_reporting, NULL);
+ A2DP_CODEC_SBC, delay_reporting, NULL, NULL);

for (i = 0; i < mpeg12_srcs; i++)
a2dp_add_sep(src, AVDTP_SEP_TYPE_SOURCE,
- A2DP_CODEC_MPEG12, delay_reporting, NULL);
+ A2DP_CODEC_MPEG12, delay_reporting,
+ NULL, NULL);
}
server->sink_enabled = sink;
if (sink) {
for (i = 0; i < sbc_sinks; i++)
a2dp_add_sep(src, AVDTP_SEP_TYPE_SINK,
- A2DP_CODEC_SBC, delay_reporting, NULL);
+ A2DP_CODEC_SBC, delay_reporting, NULL, NULL);

for (i = 0; i < mpeg12_sinks; i++)
a2dp_add_sep(src, AVDTP_SEP_TYPE_SINK,
A2DP_CODEC_MPEG12, delay_reporting,
- NULL);
+ NULL, NULL);
}

return 0;
@@ -1541,7 +1542,7 @@ void a2dp_unregister(const bdaddr_t *src)

struct a2dp_sep *a2dp_add_sep(const bdaddr_t *src, uint8_t type,
uint8_t codec, gboolean delay_reporting,
- struct media_endpoint *endpoint)
+ struct media_endpoint *endpoint, int *err)
{
struct a2dp_server *server;
struct a2dp_sep *sep;
@@ -1551,14 +1552,23 @@ struct a2dp_sep *a2dp_add_sep(const bdaddr_t *src, uint8_t type,
struct avdtp_sep_ind *ind;

server = find_server(servers, src);
- if (server == NULL)
+ if (server == NULL) {
+ if (err)
+ *err = -EINVAL;
return NULL;
+ }

- if (type == AVDTP_SEP_TYPE_SINK && !server->sink_enabled)
+ if (type == AVDTP_SEP_TYPE_SINK && !server->sink_enabled) {
+ if (err)
+ *err = -EPROTONOSUPPORT;
return NULL;
+ }

- if (type == AVDTP_SEP_TYPE_SOURCE && !server->source_enabled)
+ if (type == AVDTP_SEP_TYPE_SOURCE && !server->source_enabled) {
+ if (err)
+ *err = -EPROTONOSUPPORT;
return NULL;
+ }

sep = g_new0(struct a2dp_sep, 1);

@@ -1575,6 +1585,8 @@ proceed:
delay_reporting, ind, &cfm, sep);
if (sep->lsep == NULL) {
g_free(sep);
+ if (err)
+ *err = -EINVAL;
return NULL;
}

@@ -1600,6 +1612,8 @@ proceed:
error("Unable to allocate new service record");
avdtp_unregister_sep(sep->lsep);
g_free(sep);
+ if (err)
+ *err = -EINVAL;
return NULL;
}

@@ -1608,6 +1622,8 @@ proceed:
sdp_record_free(record);
avdtp_unregister_sep(sep->lsep);
g_free(sep);
+ if (err)
+ *err = -EINVAL;
return NULL;
}
*record_id = record->handle;
@@ -1615,6 +1631,8 @@ proceed:
add:
*l = g_slist_append(*l, sep);

+ if (err)
+ *err = 0;
return sep;
}

diff --git a/audio/a2dp.h b/audio/a2dp.h
index 21fccaa..5c4232d 100644
--- a/audio/a2dp.h
+++ b/audio/a2dp.h
@@ -138,7 +138,7 @@ void a2dp_unregister(const bdaddr_t *src);

struct a2dp_sep *a2dp_add_sep(const bdaddr_t *src, uint8_t type,
uint8_t codec, gboolean delay_reporting,
- struct media_endpoint *endpoint);
+ struct media_endpoint *endpoint, int *err);
void a2dp_remove_sep(struct a2dp_sep *sep);

struct a2dp_sep *a2dp_get(struct avdtp *session, struct avdtp_remote_sep *sep);
diff --git a/audio/media.c b/audio/media.c
index 9cfbe0e..d5fb29c 100644
--- a/audio/media.c
+++ b/audio/media.c
@@ -185,7 +185,8 @@ static struct media_endpoint *media_endpoint_create(struct media_adapter *adapte
gboolean delay_reporting,
uint8_t codec,
uint8_t *capabilities,
- int size)
+ int size,
+ int *err)
{
struct media_endpoint *endpoint;

@@ -206,13 +207,13 @@ static struct media_endpoint *media_endpoint_create(struct media_adapter *adapte
if (strcasecmp(uuid, A2DP_SOURCE_UUID) == 0) {
endpoint->sep = a2dp_add_sep(&adapter->src,
AVDTP_SEP_TYPE_SOURCE, codec,
- delay_reporting, endpoint);
+ delay_reporting, endpoint, err);
if (endpoint->sep == NULL)
goto failed;
} else if (strcasecmp(uuid, A2DP_SINK_UUID) == 0) {
endpoint->sep = a2dp_add_sep(&adapter->src,
AVDTP_SEP_TYPE_SINK, codec,
- delay_reporting, endpoint);
+ delay_reporting, endpoint, err);
if (endpoint->sep == NULL)
goto failed;
} else if (strcasecmp(uuid, HFP_AG_UUID) == 0 ||
@@ -227,8 +228,11 @@ static struct media_endpoint *media_endpoint_create(struct media_adapter *adapte
media_endpoint_set_configuration(endpoint, dev, NULL,
0, headset_setconf_cb,
dev);
- } else
+ } else {
+ if (err)
+ *err = -EINVAL;
goto failed;
+ }

endpoint->watch = g_dbus_add_disconnect_watch(adapter->conn, sender,
media_endpoint_exit, endpoint,
@@ -237,6 +241,8 @@ static struct media_endpoint *media_endpoint_create(struct media_adapter *adapte
adapter->endpoints = g_slist_append(adapter->endpoints, endpoint);
info("Endpoint registered: sender=%s path=%s", sender, path);

+ if (err)
+ *err = 0;
return endpoint;

failed:
@@ -335,6 +341,7 @@ static DBusMessage *register_endpoint(DBusConnection *conn, DBusMessage *msg,
uint8_t codec;
uint8_t *capabilities;
int size = 0;
+ int err;

sender = dbus_message_get_sender(msg);

@@ -355,8 +362,12 @@ static DBusMessage *register_endpoint(DBusConnection *conn, DBusMessage *msg,
return btd_error_invalid_args(msg);

if (media_endpoint_create(adapter, sender, path, uuid, delay_reporting,
- codec, capabilities, size) == FALSE)
- return btd_error_invalid_args(msg);
+ codec, capabilities, size, &err) == FALSE) {
+ if (err == -EPROTONOSUPPORT)
+ return btd_error_not_supported(msg);
+ else
+ return btd_error_invalid_args(msg);
+ }

return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
}
--
1.7.1



2011-03-17 12:44:23

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH] Add 'Protocol not supported' error in a2dp_add_sep

Hi Dmitriy,

On Thu, Mar 17, 2011, Dmitriy Paliy wrote:
> On Wed, 2011-03-16 at 14:01 +0200, ext Dmitriy Paliy wrote:
> > 'Protocol not supported' error code is added to registeration of A2DP
> > end-points. Error response org.bluez.Error.NotSupported instead of
> > org.bluez.Error.InvalidArguments is used when SEP registration fails
> > due to disabled corresponding interface in audio.conf.
>
> Seems like there were some comment about this patch that I missed. Could
> anybody post a reply with them to this email?

The main concern raised on IRC was about the convention of returning an
integer error through a pointer parameter instead of the return value of
the function. Usually it's better to have integer errors in the return
value, but as the main purpose of this function is to create and return
and object and checking for the exact error is a secondary concern
(which isn't always needed). So I think it's kind of ok in this case and
I've therefore pushed the patch upstream. It'd be nice if we had similar
macros as the kernel has for encoding POSIX errno's into a pointer
value. Then this kind of issue wouldn't exist.

Johan

2011-03-17 07:45:49

by Dmitriy Paliy

[permalink] [raw]
Subject: Re: [PATCH] Add 'Protocol not supported' error in a2dp_add_sep

Hi,

On Wed, 2011-03-16 at 14:01 +0200, ext Dmitriy Paliy wrote:
> 'Protocol not supported' error code is added to registeration of A2DP
> end-points. Error response org.bluez.Error.NotSupported instead of
> org.bluez.Error.InvalidArguments is used when SEP registration fails
> due to disabled corresponding interface in audio.conf.

Seems like there were some comment about this patch that I missed. Could
anybody post a reply with them to this email?

Thanks,
Dmitriy