[PATCH 0/4] obexd: Fix setting message folder

[PATCH 0/4] obexd: Fix setting message folder

[Christian Fetzer]

From: Christian Fetzer <christian.fetzer@bmw-carit.de>

This fixes an issue that the folder property for messages was set incorrectly
when ListMessages got called with a subfolder parameter.

Calling ListMessages within /telecom/msg for the subfolder inbox would set the
folder property to /telecom/msg instead of /telecom/msg/inbox.

The patchset therefore changes map_msg_create to not set the folder property
to the current folder, but to a folder given as parameter. This change will be
needed as well for new message notifications (that can indicate new messages
for any folder).

In addition I've updated the documentation to clarify that the folder property
for ListMessages and PushMessage can be used only for a subfolder of the
current folder. It's not possible to specify an absolute or relative path.

Christian Fetzer (4):
  obexd: Add folder property to map_msg_create
  obexd: Fix setting message folder for relative folder in ListMessages
  obexd: Clarify the folder property of ListMessages
  obexd: Clarify the folder property of PushMessage

 doc/obex-api.txt   |  8 +++++---
 obexd/client/map.c | 23 ++++++++++++++++++++---
 2 files changed, 25 insertions(+), 6 deletions(-)

-- 
1.8.3.4

[PATCH 1/4] obexd: Add folder property to map_msg_create

[Christian Fetzer]

From: Christian Fetzer <christian.fetzer@bmw-carit.de>

Message interfaces are not necessarily created for the current folder,
therefore the folder needs to be specified in a parameter.

For example, messages can be created for sub folders when using the folder
parameter in ListMessages.
---
 obexd/client/map.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/obexd/client/map.c b/obexd/client/map.c
index f0dcf72..9a1b140 100644
--- a/obexd/client/map.c
+++ b/obexd/client/map.c
@@ -779,7 +779,8 @@ static const GDBusPropertyTable map_msg_properties[] = {
 	{ }
 };
 
-static struct map_msg *map_msg_create(struct map_data *data, const char *handle)
+static struct map_msg *map_msg_create(struct map_data *data, const char *handle,
+							const char *folder)
 {
 	struct map_msg *msg;
 
@@ -788,7 +789,7 @@ static struct map_msg *map_msg_create(struct map_data *data, const char *handle)
 	msg->path = g_strdup_printf("%s/message%s",
 					obc_session_get_path(data->session),
 					handle);
-	msg->folder = g_strdup(obc_session_get_folder(data->session));
+	msg->folder = g_strdup(folder);
 
 	if (!g_dbus_register_interface(conn, msg->path, MAP_MSG_INTERFACE,
 						map_msg_methods, NULL,
@@ -1057,7 +1058,8 @@ static void msg_element(GMarkupParseContext *ctxt, const char *element,
 
 	msg = g_hash_table_lookup(data->messages, values[i]);
 	if (msg == NULL) {
-		msg = map_msg_create(data, values[i]);
+		msg = map_msg_create(data, values[i],
+					obc_session_get_folder(data->session));
 		if (msg == NULL)
 			return;
 	}
-- 
1.8.3.4

Re: [PATCH 1/4] obexd: Add folder property to map_msg_create

[Luiz Augusto von Dentz]

Hi Christian,

On Fri, Sep 13, 2013 at 12:23 PM, Christian Fetzer
<christian.fetzer@oss.bmw-carit.de> wrote:
> From: Christian Fetzer <christian.fetzer@bmw-carit.de>
>
> Message interfaces are not necessarily created for the current folder,
> therefore the folder needs to be specified in a parameter.
>
> For example, messages can be created for sub folders when using the folder
> parameter in ListMessages.
> ---
>  obexd/client/map.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/obexd/client/map.c b/obexd/client/map.c
> index f0dcf72..9a1b140 100644
> --- a/obexd/client/map.c
> +++ b/obexd/client/map.c
> @@ -779,7 +779,8 @@ static const GDBusPropertyTable map_msg_properties[] = {
>         { }
>  };
>
> -static struct map_msg *map_msg_create(struct map_data *data, const char *handle)
> +static struct map_msg *map_msg_create(struct map_data *data, const char *handle,
> +                                                       const char *folder)
>  {
>         struct map_msg *msg;
>
> @@ -788,7 +789,7 @@ static struct map_msg *map_msg_create(struct map_data *data, const char *handle)
>         msg->path = g_strdup_printf("%s/message%s",
>                                         obc_session_get_path(data->session),
>                                         handle);
> -       msg->folder = g_strdup(obc_session_get_folder(data->session));
> +       msg->folder = g_strdup(folder);
>
>         if (!g_dbus_register_interface(conn, msg->path, MAP_MSG_INTERFACE,
>                                                 map_msg_methods, NULL,
> @@ -1057,7 +1058,8 @@ static void msg_element(GMarkupParseContext *ctxt, const char *element,
>
>         msg = g_hash_table_lookup(data->messages, values[i]);
>         if (msg == NULL) {
> -               msg = map_msg_create(data, values[i]);
> +               msg = map_msg_create(data, values[i],
> +                                       obc_session_get_folder(data->session));

Is this really fixing anything? Because it seems it is just changing
places where obc_session_get_folder is called when what you should
probably be doing is to store the folder parameter given to
ListMessages. I actually regret to have this parameter as it makes
things a little bit more complicated just to avoid SetFolder.

Probably Session.SetPath would make more sense than having each
profile interface treating it differently but that would require to
break APIs so it is better not to do it right now.



-- 
Luiz Augusto von Dentz

Re: [PATCH 1/4] obexd: Add folder property to map_msg_create

[Christian Fetzer]

Hi Luiz,

On 09/13/2013 12:11 PM, Luiz Augusto von Dentz wrote:
> Hi Christian,
> 
> On Fri, Sep 13, 2013 at 12:23 PM, Christian Fetzer
> <christian.fetzer@oss.bmw-carit.de> wrote:
>> From: Christian Fetzer <christian.fetzer@bmw-carit.de>
>>
>> Message interfaces are not necessarily created for the current folder,
>> therefore the folder needs to be specified in a parameter.
>>
>> For example, messages can be created for sub folders when using the folder
>> parameter in ListMessages.
>> ---
>>  obexd/client/map.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/obexd/client/map.c b/obexd/client/map.c
>> index f0dcf72..9a1b140 100644
>> --- a/obexd/client/map.c
>> +++ b/obexd/client/map.c
>> @@ -779,7 +779,8 @@ static const GDBusPropertyTable map_msg_properties[] = {
>>         { }
>>  };
>>
>> -static struct map_msg *map_msg_create(struct map_data *data, const char *handle)
>> +static struct map_msg *map_msg_create(struct map_data *data, const char *handle,
>> +                                                       const char *folder)
>>  {
>>         struct map_msg *msg;
>>
>> @@ -788,7 +789,7 @@ static struct map_msg *map_msg_create(struct map_data *data, const char *handle)
>>         msg->path = g_strdup_printf("%s/message%s",
>>                                         obc_session_get_path(data->session),
>>                                         handle);
>> -       msg->folder = g_strdup(obc_session_get_folder(data->session));
>> +       msg->folder = g_strdup(folder);
>>
>>         if (!g_dbus_register_interface(conn, msg->path, MAP_MSG_INTERFACE,
>>                                                 map_msg_methods, NULL,
>> @@ -1057,7 +1058,8 @@ static void msg_element(GMarkupParseContext *ctxt, const char *element,
>>
>>         msg = g_hash_table_lookup(data->messages, values[i]);
>>         if (msg == NULL) {
>> -               msg = map_msg_create(data, values[i]);
>> +               msg = map_msg_create(data, values[i],
>> +                                       obc_session_get_folder(data->session));
> 
> Is this really fixing anything? Because it seems it is just changing
> places where obc_session_get_folder is called when what you should
> probably be doing is to store the folder parameter given to
> ListMessages. 


The fix itself is in patch 2. But as explained in the cover letter,
I'll need the parameter as well when creating new messages from
event reports. (I have a first notification API patchset ready but
wanted to wait until this is applied.)


> I actually regret to have this parameter as it makes
> things a little bit more complicated just to avoid SetFolder.
> 
> Probably Session.SetPath would make more sense than having each
> profile interface treating it differently but that would require to
> break APIs so it is better not to do it right now.
> 


Fully agree. Maybe it's a good thing to keep 
those points in mind for future API changes.

Christian

[PATCH 2/4] obexd: Fix setting message folder for relative folder in ListMessages

[Christian Fetzer]

From: Christian Fetzer <christian.fetzer@bmw-carit.de>

The method ListMessages allows to specify a relative subfolder.
This subfolder needs to be added to the current path when registering
a new message interface.
---
 obexd/client/map.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/obexd/client/map.c b/obexd/client/map.c
index 9a1b140..54011d8 100644
--- a/obexd/client/map.c
+++ b/obexd/client/map.c
@@ -96,6 +96,7 @@ static const char * const filter_list[] = {
 struct map_data {
 	struct obc_session *session;
 	DBusMessage *msg;
+	char *folder;
 	GHashTable *messages;
 	int16_t mas_instance_id;
 	uint8_t supported_message_types;
@@ -1058,8 +1059,7 @@ static void msg_element(GMarkupParseContext *ctxt, const char *element,
 
 	msg = g_hash_table_lookup(data->messages, values[i]);
 	if (msg == NULL) {
-		msg = map_msg_create(data, values[i],
-					obc_session_get_folder(data->session));
+		msg = map_msg_create(data, values[i], data->folder);
 		if (msg == NULL)
 			return;
 	}
@@ -1153,6 +1153,19 @@ static void message_listing_cb(struct obc_session *session,
 done:
 	g_dbus_send_message(conn, reply);
 	dbus_message_unref(map->msg);
+	g_free(map->folder);
+	map->folder = NULL;
+}
+
+static char *get_absolute_folder(const char *root, const char *subfolder)
+{
+	if (!subfolder || strlen(subfolder) == 0)
+		return g_strdup(root);
+	else
+		if (g_str_has_suffix(root, "/"))
+			return g_strconcat(root, subfolder, NULL);
+		else
+			return g_strconcat(root, "/", subfolder, NULL);
 }
 
 static DBusMessage *get_message_listing(struct map_data *map,
@@ -1175,6 +1188,8 @@ static DBusMessage *get_message_listing(struct map_data *map,
 	if (obc_session_queue(map->session, transfer, message_listing_cb, map,
 								&err)) {
 		map->msg = dbus_message_ref(message);
+		map->folder = get_absolute_folder(obc_session_get_folder(
+							map->session), folder);
 		return NULL;
 	}
 
-- 
1.8.3.4

Re: [PATCH 2/4] obexd: Fix setting message folder for relative folder in ListMessages

[Luiz Augusto von Dentz]

Hi Christian,

On Fri, Sep 13, 2013 at 12:23 PM, Christian Fetzer
<christian.fetzer@oss.bmw-carit.de> wrote:
> From: Christian Fetzer <christian.fetzer@bmw-carit.de>
>
> The method ListMessages allows to specify a relative subfolder.
> This subfolder needs to be added to the current path when registering
> a new message interface.
> ---
>  obexd/client/map.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/obexd/client/map.c b/obexd/client/map.c
> index 9a1b140..54011d8 100644
> --- a/obexd/client/map.c
> +++ b/obexd/client/map.c
> @@ -96,6 +96,7 @@ static const char * const filter_list[] = {
>  struct map_data {
>         struct obc_session *session;
>         DBusMessage *msg;
> +       char *folder;
>         GHashTable *messages;
>         int16_t mas_instance_id;
>         uint8_t supported_message_types;
> @@ -1058,8 +1059,7 @@ static void msg_element(GMarkupParseContext *ctxt, const char *element,
>
>         msg = g_hash_table_lookup(data->messages, values[i]);
>         if (msg == NULL) {
> -               msg = map_msg_create(data, values[i],
> -                                       obc_session_get_folder(data->session));
> +               msg = map_msg_create(data, values[i], data->folder);
>                 if (msg == NULL)
>                         return;
>         }
> @@ -1153,6 +1153,19 @@ static void message_listing_cb(struct obc_session *session,
>  done:
>         g_dbus_send_message(conn, reply);
>         dbus_message_unref(map->msg);
> +       g_free(map->folder);
> +       map->folder = NULL;
> +}
> +
> +static char *get_absolute_folder(const char *root, const char *subfolder)
> +{
> +       if (!subfolder || strlen(subfolder) == 0)
> +               return g_strdup(root);
> +       else
> +               if (g_str_has_suffix(root, "/"))
> +                       return g_strconcat(root, subfolder, NULL);
> +               else
> +                       return g_strconcat(root, "/", subfolder, NULL);
>  }
>
>  static DBusMessage *get_message_listing(struct map_data *map,
> @@ -1175,6 +1188,8 @@ static DBusMessage *get_message_listing(struct map_data *map,
>         if (obc_session_queue(map->session, transfer, message_listing_cb, map,
>                                                                 &err)) {
>                 map->msg = dbus_message_ref(message);
> +               map->folder = get_absolute_folder(obc_session_get_folder(
> +                                                       map->session), folder);
>                 return NULL;
>         }
>
> --
> 1.8.3.4

This will probably not work in case of multiple outstanding requests
the last will always overwrite the folder, which btw will leak, so
probably we need a per request data.


-- 
Luiz Augusto von Dentz

Re: [PATCH 2/4] obexd: Fix setting message folder for relative folder in ListMessages

[Christian Fetzer]

Hi Luiz,

On 09/13/2013 12:47 PM, Luiz Augusto von Dentz wrote:
> Hi Christian,
> 
> On Fri, Sep 13, 2013 at 12:23 PM, Christian Fetzer
> <christian.fetzer@oss.bmw-carit.de> wrote:
>> From: Christian Fetzer <christian.fetzer@bmw-carit.de>
>>
>> The method ListMessages allows to specify a relative subfolder.
>> This subfolder needs to be added to the current path when registering
>> a new message interface.
>> ---
>>  obexd/client/map.c | 19 +++++++++++++++++--
>>  1 file changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/obexd/client/map.c b/obexd/client/map.c
>> index 9a1b140..54011d8 100644
>> --- a/obexd/client/map.c
>> +++ b/obexd/client/map.c
>> @@ -96,6 +96,7 @@ static const char * const filter_list[] = {
>>  struct map_data {
>>         struct obc_session *session;
>>         DBusMessage *msg;
>> +       char *folder;
>>         GHashTable *messages;
>>         int16_t mas_instance_id;
>>         uint8_t supported_message_types;
>> @@ -1058,8 +1059,7 @@ static void msg_element(GMarkupParseContext *ctxt, const char *element,
>>
>>         msg = g_hash_table_lookup(data->messages, values[i]);
>>         if (msg == NULL) {
>> -               msg = map_msg_create(data, values[i],
>> -                                       obc_session_get_folder(data->session));
>> +               msg = map_msg_create(data, values[i], data->folder);
>>                 if (msg == NULL)
>>                         return;
>>         }
>> @@ -1153,6 +1153,19 @@ static void message_listing_cb(struct obc_session *session,
>>  done:
>>         g_dbus_send_message(conn, reply);
>>         dbus_message_unref(map->msg);
>> +       g_free(map->folder);
>> +       map->folder = NULL;
>> +}
>> +
>> +static char *get_absolute_folder(const char *root, const char *subfolder)
>> +{
>> +       if (!subfolder || strlen(subfolder) == 0)
>> +               return g_strdup(root);
>> +       else
>> +               if (g_str_has_suffix(root, "/"))
>> +                       return g_strconcat(root, subfolder, NULL);
>> +               else
>> +                       return g_strconcat(root, "/", subfolder, NULL);
>>  }
>>
>>  static DBusMessage *get_message_listing(struct map_data *map,
>> @@ -1175,6 +1188,8 @@ static DBusMessage *get_message_listing(struct map_data *map,
>>         if (obc_session_queue(map->session, transfer, message_listing_cb, map,
>>                                                                 &err)) {
>>                 map->msg = dbus_message_ref(message);
>> +               map->folder = get_absolute_folder(obc_session_get_folder(
>> +                                                       map->session), folder);
>>                 return NULL;
>>         }
>>
>> --
>> 1.8.3.4
> 
> This will probably not work in case of multiple outstanding requests
> the last will always overwrite the folder, which btw will leak, so
> probably we need a per request data.
> 
> 

Yes, the issue exists already in the current code base, because the stored
dbus message is overridden if any MAP function is called before the previous
one is finished.

I've been able to reproduce a crash with it and already started to write a patch
that adds a pending_request on top of this patch.

Do you prefer to have a fix first and rebase this patchset on it, or do you
prefer to get the notification API patchset first? (which is already ready to be sent)

Christian

Re: [PATCH 2/4] obexd: Fix setting message folder for relative folder in ListMessages

[Luiz Augusto von Dentz]

Hi Christian,

On Fri, Sep 13, 2013 at 2:45 PM, Christian Fetzer
<christian.fetzer@oss.bmw-carit.de> wrote:
> Hi Luiz,
>
> On 09/13/2013 12:47 PM, Luiz Augusto von Dentz wrote:
>> Hi Christian,
>>
>> On Fri, Sep 13, 2013 at 12:23 PM, Christian Fetzer
>> <christian.fetzer@oss.bmw-carit.de> wrote:
>>> From: Christian Fetzer <christian.fetzer@bmw-carit.de>
>>>
>>> The method ListMessages allows to specify a relative subfolder.
>>> This subfolder needs to be added to the current path when registering
>>> a new message interface.
>>> ---
>>>  obexd/client/map.c | 19 +++++++++++++++++--
>>>  1 file changed, 17 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/obexd/client/map.c b/obexd/client/map.c
>>> index 9a1b140..54011d8 100644
>>> --- a/obexd/client/map.c
>>> +++ b/obexd/client/map.c
>>> @@ -96,6 +96,7 @@ static const char * const filter_list[] = {
>>>  struct map_data {
>>>         struct obc_session *session;
>>>         DBusMessage *msg;
>>> +       char *folder;
>>>         GHashTable *messages;
>>>         int16_t mas_instance_id;
>>>         uint8_t supported_message_types;
>>> @@ -1058,8 +1059,7 @@ static void msg_element(GMarkupParseContext *ctxt, const char *element,
>>>
>>>         msg = g_hash_table_lookup(data->messages, values[i]);
>>>         if (msg == NULL) {
>>> -               msg = map_msg_create(data, values[i],
>>> -                                       obc_session_get_folder(data->session));
>>> +               msg = map_msg_create(data, values[i], data->folder);
>>>                 if (msg == NULL)
>>>                         return;
>>>         }
>>> @@ -1153,6 +1153,19 @@ static void message_listing_cb(struct obc_session *session,
>>>  done:
>>>         g_dbus_send_message(conn, reply);
>>>         dbus_message_unref(map->msg);
>>> +       g_free(map->folder);
>>> +       map->folder = NULL;
>>> +}
>>> +
>>> +static char *get_absolute_folder(const char *root, const char *subfolder)
>>> +{
>>> +       if (!subfolder || strlen(subfolder) == 0)
>>> +               return g_strdup(root);
>>> +       else
>>> +               if (g_str_has_suffix(root, "/"))
>>> +                       return g_strconcat(root, subfolder, NULL);
>>> +               else
>>> +                       return g_strconcat(root, "/", subfolder, NULL);
>>>  }
>>>
>>>  static DBusMessage *get_message_listing(struct map_data *map,
>>> @@ -1175,6 +1188,8 @@ static DBusMessage *get_message_listing(struct map_data *map,
>>>         if (obc_session_queue(map->session, transfer, message_listing_cb, map,
>>>                                                                 &err)) {
>>>                 map->msg = dbus_message_ref(message);
>>> +               map->folder = get_absolute_folder(obc_session_get_folder(
>>> +                                                       map->session), folder);
>>>                 return NULL;
>>>         }
>>>
>>> --
>>> 1.8.3.4
>>
>> This will probably not work in case of multiple outstanding requests
>> the last will always overwrite the folder, which btw will leak, so
>> probably we need a per request data.
>>
>>
>
> Yes, the issue exists already in the current code base, because the stored
> dbus message is overridden if any MAP function is called before the previous
> one is finished.
>
> I've been able to reproduce a crash with it and already started to write a patch
> that adds a pending_request on top of this patch.
>
> Do you prefer to have a fix first and rebase this patchset on it, or do you
> prefer to get the notification API patchset first? (which is already ready to be sent)

Fixes should take precedence over regular patches specially if it is
fixing crashes.


-- 
Luiz Augusto von Dentz

[PATCH 3/4] obexd: Clarify the folder property of ListMessages

[Christian Fetzer]

From: Christian Fetzer <christian.fetzer@bmw-carit.de>

The folder property of ListMessages does not accept path information,
it allows only to request the messages of a subfolder of the current
folder.
---
 doc/obex-api.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/doc/obex-api.txt b/doc/obex-api.txt
index 0a8e632..28343f6 100644
--- a/doc/obex-api.txt
+++ b/doc/obex-api.txt
@@ -538,7 +538,8 @@ Methods		void SetFolder(string name)
 		array{object, dict} ListMessages(string folder, dict filter)
 
 			Returns an array containing the messages found in the
-			given folder.
+			given subfolder of the current folder, or in the
+			current folder if folder is empty.
 
 			Possible Filters: Offset, MaxCount, SubjectLength, Fields,
 			Type, PeriodStart, PeriodEnd, Status, Recipient, Sender,
-- 
1.8.3.4

[PATCH 4/4] obexd: Clarify the folder property of PushMessage

[Christian Fetzer]

From: Christian Fetzer <christian.fetzer@bmw-carit.de>

The folder property of PushMessages does not accept path information,
it allows only to request the messages to be added to a subfolder of the
current folder.
---
 doc/obex-api.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/doc/obex-api.txt b/doc/obex-api.txt
index 28343f6..eda7cb9 100644
--- a/doc/obex-api.txt
+++ b/doc/obex-api.txt
@@ -638,8 +638,9 @@ Methods		void SetFolder(string name)
 			Transfer a message (in bMessage format) to the
 			remote device.
 
-			The message is transferred either to the given folder,
-			or to the current folder if folder is omitted.
+			The message is transferred either to the given
+			subfolder of the current folder, or to the current
+			folder if folder is empty.
 
 			Possible args: Transparent, Retry, Charset
 
-- 
1.8.3.4