[PATCH 0/2] obexd: Fix bug in irmc phonebook and prevent to reintroduce it

[PATCH 0/2] obexd: Fix bug in irmc phonebook and prevent to reintroduce it

[Harald Schmitt]

This patchset fixes a bug in irmc plugin the same way as it was sent
today, but with a commit message. The second patch defines macros in
phonebook.h for the well known phonbook folders and names and replaces
those magic values.

Harald Schmitt (2):
  irmc: Fix phonebook contacts query
  phonebook: Replace magic strings for phonebook names and folders

 plugins/irmc.c              |    4 ++--
 plugins/phonebook-ebook.c   |   14 ++++++-------
 plugins/phonebook-tracker.c |   48 +++++++++++++++++++++----------------------
 plugins/phonebook.h         |   13 ++++++++++++
 4 files changed, 46 insertions(+), 33 deletions(-)

-- 
1.7.9.5

[PATCH 1/2] irmc: Fix phonebook contacts query

[Harald Schmitt]

At the moment IRMC fails to connect for phonebook_ebook and
phonebook_tracker usage because the name parameter for phonebook_pull
is not an absolute path.
---
 plugins/irmc.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/plugins/irmc.c b/plugins/irmc.c
index 87f3b6c..2283fe6 100644
--- a/plugins/irmc.c
+++ b/plugins/irmc.c
@@ -226,7 +226,7 @@ static void *irmc_connect(struct obex_session *os, int *err)
 	param->maxlistcount = 0; /* to count the number of vcards... */
 	param->filter = 0x200085; /* UID TEL N VERSION */
 	irmc->params = param;
-	irmc->request = phonebook_pull("telecom/pb.vcf", irmc->params,
+	irmc->request = phonebook_pull("/telecom/pb.vcf", irmc->params,
 					phonebook_size_result, irmc, err);
 	ret = phonebook_pull_read(irmc->request);
 	if (err)
@@ -312,7 +312,7 @@ static void *irmc_open_pb(const char *name, struct irmc_session *irmc,
 
 	if (!g_strcmp0(name, ".vcf")) {
 		/* how can we tell if the vcard count call already finished? */
-		irmc->request = phonebook_pull("telecom/pb.vcf", irmc->params,
+		irmc->request = phonebook_pull("/telecom/pb.vcf", irmc->params,
 						query_result, irmc, &ret);
 		if (ret < 0) {
 			DBG("phonebook_pull failed...");
-- 
1.7.9.5

[PATCH 2/2] phonebook: Replace magic strings for phonebook names and folders

[Harald Schmitt]

---
 plugins/irmc.c              |    4 ++--
 plugins/phonebook-ebook.c   |   14 ++++++-------
 plugins/phonebook-tracker.c |   48 +++++++++++++++++++++----------------------
 plugins/phonebook.h         |   13 ++++++++++++
 4 files changed, 46 insertions(+), 33 deletions(-)

diff --git a/plugins/irmc.c b/plugins/irmc.c
index 2283fe6..2574b22 100644
--- a/plugins/irmc.c
+++ b/plugins/irmc.c
@@ -226,7 +226,7 @@ static void *irmc_connect(struct obex_session *os, int *err)
 	param->maxlistcount = 0; /* to count the number of vcards... */
 	param->filter = 0x200085; /* UID TEL N VERSION */
 	irmc->params = param;
-	irmc->request = phonebook_pull("/telecom/pb.vcf", irmc->params,
+	irmc->request = phonebook_pull(PB_CONTACTS, irmc->params,
 					phonebook_size_result, irmc, err);
 	ret = phonebook_pull_read(irmc->request);
 	if (err)
@@ -312,7 +312,7 @@ static void *irmc_open_pb(const char *name, struct irmc_session *irmc,
 
 	if (!g_strcmp0(name, ".vcf")) {
 		/* how can we tell if the vcard count call already finished? */
-		irmc->request = phonebook_pull("/telecom/pb.vcf", irmc->params,
+		irmc->request = phonebook_pull(PB_CONTACTS, irmc->params,
 						query_result, irmc, &ret);
 		if (ret < 0) {
 			DBG("phonebook_pull failed...");
diff --git a/plugins/phonebook-ebook.c b/plugins/phonebook-ebook.c
index a1f06b5..59a4eac 100644
--- a/plugins/phonebook-ebook.c
+++ b/plugins/phonebook-ebook.c
@@ -459,7 +459,7 @@ char *phonebook_set_folder(const char *current_folder,
 	root = (g_strcmp0("/", current_folder) == 0);
 	child = (new_folder && strlen(new_folder) != 0);
 
-	/* Evolution back-end will support telecom/pb folder only */
+	/* Evolution back-end will support /telecom/pb folder only */
 
 	switch (flags) {
 	case 0x02:
@@ -471,8 +471,8 @@ char *phonebook_set_folder(const char *current_folder,
 
 		/* Go down 1 level */
 		fullname = g_build_filename(current_folder, new_folder, NULL);
-		if (strcmp("/telecom", fullname) != 0 &&
-				strcmp("/telecom/pb", fullname) != 0) {
+		if (strcmp(PB_TELECOM_FOLDER, fullname) != 0 &&
+				strcmp(PB_CONTACTS_FOLDER, fullname) != 0) {
 			g_free(fullname);
 			fullname = NULL;
 			ret = -ENOENT;
@@ -511,8 +511,8 @@ char *phonebook_set_folder(const char *current_folder,
 		}
 
 		fullname = g_build_filename(base, new_folder, NULL);
-		if (strcmp(fullname, "/telecom") != 0 &&
-				strcmp(fullname, "/telecom/pb") != 0) {
+		if (strcmp(fullname, PB_TELECOM_FOLDER) != 0 &&
+				strcmp(fullname, PB_CONTACTS_FOLDER) != 0) {
 			g_free(fullname);
 			fullname = NULL;
 			ret = -ENOENT;
@@ -548,7 +548,7 @@ void *phonebook_pull(const char *name, const struct apparam_field *params,
 {
 	struct query_context *data;
 
-	if (g_strcmp0("/telecom/pb.vcf", name) != 0) {
+	if (g_strcmp0(PB_CONTACTS, name) != 0) {
 		if (err)
 			*err = -ENOENT;
 
@@ -638,7 +638,7 @@ void *phonebook_create_cache(const char *name, phonebook_entry_cb entry_cb,
 	EVCardAttribute *attrib;
 	char *uid, *tel, *cname;
 
-	if (g_strcmp0("/telecom/pb", name) != 0) {
+	if (g_strcmp0(PB_CONTACTS_FOLDER, name) != 0) {
 		if (err)
 			*err = -ENOENT;
 
diff --git a/plugins/phonebook-tracker.c b/plugins/phonebook-tracker.c
index 96635c4..2fd7ba1 100644
--- a/plugins/phonebook-tracker.c
+++ b/plugins/phonebook-tracker.c
@@ -513,15 +513,15 @@ static TrackerSparqlConnection *connection = NULL;
 
 static const char *name2query(const char *name)
 {
-	if (g_str_equal(name, "/telecom/pb.vcf"))
+	if (g_str_equal(name, PB_CONTACTS))
 		return CONTACTS_QUERY_ALL;
-	else if (g_str_equal(name, "/telecom/ich.vcf"))
+	else if (g_str_equal(name, PB_CALLS_INCOMING))
 		return INCOMING_CALLS_QUERY;
-	else if (g_str_equal(name, "/telecom/och.vcf"))
+	else if (g_str_equal(name, PB_CALLS_OUTGOING))
 		return OUTGOING_CALLS_QUERY;
-	else if (g_str_equal(name, "/telecom/mch.vcf"))
+	else if (g_str_equal(name, PB_CALLS_MISSED))
 		return MISSED_CALLS_QUERY;
-	else if (g_str_equal(name, "/telecom/cch.vcf"))
+	else if (g_str_equal(name, PB_CALLS_COMBINED))
 		return COMBINED_CALLS_QUERY;
 
 	return NULL;
@@ -529,15 +529,15 @@ static const char *name2query(const char *name)
 
 static const char *name2count_query(const char *name)
 {
-	if (g_str_equal(name, "/telecom/pb.vcf"))
+	if (g_str_equal(name, PB_CONTACTS))
 		return CONTACTS_COUNT_QUERY;
-	else if (g_str_equal(name, "/telecom/ich.vcf"))
+	else if (g_str_equal(name, PB_CALLS_INCOMING))
 		return INCOMING_CALLS_COUNT_QUERY;
-	else if (g_str_equal(name, "/telecom/och.vcf"))
+	else if (g_str_equal(name, PB_CALLS_OUTGOING))
 		return OUTGOING_CALLS_COUNT_QUERY;
-	else if (g_str_equal(name, "/telecom/mch.vcf"))
+	else if (g_str_equal(name, PB_CALLS_MISSED))
 		return MISSED_CALLS_COUNT_QUERY;
-	else if (g_str_equal(name, "/telecom/cch.vcf"))
+	else if (g_str_equal(name, PB_CALLS_COMBINED))
 		return COMBINED_CALLS_COUNT_QUERY;
 
 	return NULL;
@@ -550,17 +550,17 @@ static gboolean folder_is_valid(const char *folder)
 
 	if (g_str_equal(folder, "/"))
 		return TRUE;
-	else if (g_str_equal(folder, "/telecom"))
+	else if (g_str_equal(folder, PB_TELECOM_FOLDER))
 		return TRUE;
-	else if (g_str_equal(folder, "/telecom/pb"))
+	else if (g_str_equal(folder, PB_CONTACTS_FOLDER))
 		return TRUE;
-	else if (g_str_equal(folder, "/telecom/ich"))
+	else if (g_str_equal(folder, PB_CALLS_INCOMING_FOLDER))
 		return TRUE;
-	else if (g_str_equal(folder, "/telecom/och"))
+	else if (g_str_equal(folder, PB_CALLS_OUTGOING_FOLDER))
 		return TRUE;
-	else if (g_str_equal(folder, "/telecom/mch"))
+	else if (g_str_equal(folder, PB_CALLS_MISSED_FOLDER))
 		return TRUE;
-	else if (g_str_equal(folder, "/telecom/cch"))
+	else if (g_str_equal(folder, PB_CALLS_COMBINED_FOLDER))
 		return TRUE;
 
 	return FALSE;
@@ -568,15 +568,15 @@ static gboolean folder_is_valid(const char *folder)
 
 static const char *folder2query(const char *folder)
 {
-	if (g_str_equal(folder, "/telecom/pb"))
+	if (g_str_equal(folder, PB_CONTACTS_FOLDER))
 		return CONTACTS_QUERY_ALL_LIST;
-	else if (g_str_equal(folder, "/telecom/ich"))
+	else if (g_str_equal(folder, PB_CALLS_INCOMING_FOLDER))
 		return INCOMING_CALLS_LIST;
-	else if (g_str_equal(folder, "/telecom/och"))
+	else if (g_str_equal(folder, PB_CALLS_OUTGOING_FOLDER))
 		return OUTGOING_CALLS_LIST;
-	else if (g_str_equal(folder, "/telecom/mch"))
+	else if (g_str_equal(folder, PB_CALLS_MISSED_FOLDER))
 		return MISSED_CALLS_LIST;
-	else if (g_str_equal(folder, "/telecom/cch"))
+	else if (g_str_equal(folder, PB_CALLS_COMBINED_FOLDER))
 		return COMBINED_CALLS_LIST;
 
 	return NULL;
@@ -1541,11 +1541,11 @@ static int pull_newmissedcalls(const char **reply, int num_fields,
 	}
 
 	if (data->params->maxlistcount == 0) {
-		query = name2count_query("/telecom/mch.vcf");
+		query = name2count_query(PB_CALLS_MISSED);
 		col_amount = COUNT_QUERY_COL_AMOUNT;
 		pull_cb = pull_contacts_size;
 	} else {
-		query = name2query("/telecom/mch.vcf");
+		query = name2query(PB_CALLS_MISSED);
 		col_amount = PULL_QUERY_COL_AMOUNT;
 		pull_cb = pull_contacts;
 	}
@@ -1613,7 +1613,7 @@ int phonebook_pull_read(void *request)
 
 	data->newmissedcalls = 0;
 
-	if (g_strcmp0(data->req_name, "/telecom/mch.vcf") == 0 &&
+	if (g_strcmp0(data->req_name, PB_CALLS_MISSED) == 0 &&
 						data->tracker_index == 0) {
 		/* new missed calls amount should be counted only once - it
 		 * will be done during generating first part of results of
diff --git a/plugins/phonebook.h b/plugins/phonebook.h
index 00abc08..740557c 100644
--- a/plugins/phonebook.h
+++ b/plugins/phonebook.h
@@ -29,6 +29,19 @@
 #define VCARD_LISTING_ELEMENT "<card handle = \"%d.vcf\" name = \"%s\"/>" EOL
 #define VCARD_LISTING_END "</vCard-listing>"
 
+#define PB_TELECOM_FOLDER "/telecom"
+#define PB_CONTACTS_FOLDER "/telecom/pb"
+#define PB_CALLS_COMBINED_FOLDER "/telecom/cch"
+#define PB_CALLS_INCOMING_FOLDER "/telecom/ich"
+#define PB_CALLS_MISSED_FOLDER "/telecom/mch"
+#define PB_CALLS_OUTGOING_FOLDER "/telecom/och"
+
+#define PB_CONTACTS "/telecom/pb.vcf"
+#define PB_CALLS_COMBINED "/telecom/cch.vcf"
+#define PB_CALLS_INCOMING "/telecom/ich.vcf"
+#define PB_CALLS_MISSED "/telecom/mch.vcf"
+#define PB_CALLS_OUTGOING "/telecom/och.vcf"
+
 struct apparam_field {
 	/* list and pull attributes */
 	uint16_t maxlistcount;
-- 
1.7.9.5

Re: [PATCH 0/2] obexd: Fix bug in irmc phonebook and prevent to reintroduce it

[Luiz Augusto von Dentz]

Hi Harald,

On Wed, Jul 18, 2012 at 5:53 PM, Harald Schmitt <linux@hschmitt.de> wrote:
> This patchset fixes a bug in irmc plugin the same way as it was sent
> today, but with a commit message. The second patch defines macros in
> phonebook.h for the well known phonbook folders and names and replaces
> those magic values.
>
> Harald Schmitt (2):
>   irmc: Fix phonebook contacts query
>   phonebook: Replace magic strings for phonebook names and folders
>
>  plugins/irmc.c              |    4 ++--
>  plugins/phonebook-ebook.c   |   14 ++++++-------
>  plugins/phonebook-tracker.c |   48 +++++++++++++++++++++----------------------
>  plugins/phonebook.h         |   13 ++++++++++++
>  4 files changed, 46 insertions(+), 33 deletions(-)
>
> --
> 1.7.9.5

I guess you should take a look at patch
4f48e26fa73217dde9916fe6e857b1de7fae33cc, not sure if is the same
issue though since this here it is used as type, but perhaps it is
worth making it similar so either pbap and sync does accept the path
in both absolute or relative format.


-- 
Luiz Augusto von Dentz

Re: [PATCH 0/2] obexd: Fix bug in irmc phonebook and prevent to reintroduce it

[Harald Schmitt]

Hi Luiz,

Am 24.07.2012 22:20, schrieb Luiz Augusto von Dentz:
> Hi Harald,
> 
> On Wed, Jul 18, 2012 at 5:53 PM, Harald Schmitt <linux@hschmitt.de> wrote:
>> This patchset fixes a bug in irmc plugin the same way as it was sent
>> today, but with a commit message. The second patch defines macros in
>> phonebook.h for the well known phonbook folders and names and replaces
>> those magic values.
>>
>> Harald Schmitt (2):
>>   irmc: Fix phonebook contacts query
>>   phonebook: Replace magic strings for phonebook names and folders
>>
>>  plugins/irmc.c              |    4 ++--
>>  plugins/phonebook-ebook.c   |   14 ++++++-------
>>  plugins/phonebook-tracker.c |   48 +++++++++++++++++++++----------------------
>>  plugins/phonebook.h         |   13 ++++++++++++
>>  4 files changed, 46 insertions(+), 33 deletions(-)
>>
>> --
>> 1.7.9.5
> 
> I guess you should take a look at patch
> 4f48e26fa73217dde9916fe6e857b1de7fae33cc, not sure if is the same
> issue though since this here it is used as type, but perhaps it is
> worth making it similar so either pbap and sync does accept the path
> in both absolute or relative format.
> 
> 
This one is related to that. But I think your proposal should be a
separate patch since it is about the incomming paths from an irmc
client. This one is more about to what irmc converts the incomming paths
to query phonebook-ebook and phonebook-tracker for contacts and call lists.
At the  moment these well known paths in phonebook-*.c are designed to
match 1:1 with pbap spec and they are very similar to irmc, but there
could be a third implementation to query phonebook where this could be
different.
The patch 1/2 fixes irmc to query phonebook implementation for the well
known absolute path. This was changed in pbap.c with the patch you
stated, but forgot to change in irmc.c.
Patch 2/2 just replaces the well-known phonebook paths which
phoenbook-ebook.c and phonebook-tracker.c support with constants.

Re: [PATCH 0/2] obexd: Fix bug in irmc phonebook and prevent to reintroduce it

[Luiz Augusto von Dentz]

Hi Harald,

On Wed, Jul 25, 2012 at 10:18 AM, Harald Schmitt <linux@hschmitt.de> wrote:
> This one is related to that. But I think your proposal should be a
> separate patch since it is about the incomming paths from an irmc
> client. This one is more about to what irmc converts the incomming paths
> to query phonebook-ebook and phonebook-tracker for contacts and call lists.
> At the  moment these well known paths in phonebook-*.c are designed to
> match 1:1 with pbap spec and they are very similar to irmc, but there
> could be a third implementation to query phonebook where this could be
> different.

While the backend indeed take an absolute path this doesnt mean we
have to send as absolute path, not to mention it is not compatible
with mimetypes which is what TYPE header describes, even in case of
PBAP it is wrong to send absolute path in SETPATH.

To avoid this problems we send relative although we do accept
absolute, but to avoid problems with stack interpreting absolute path
as not valid as OBEX spec state we always send relative paths.

> The patch 1/2 fixes irmc to query phonebook implementation for the well
> known absolute path. This was changed in pbap.c with the patch you
> stated, but forgot to change in irmc.c.
> Patch 2/2 just replaces the well-known phonebook paths which
> phoenbook-ebook.c and phonebook-tracker.c support with constants.

Im fine with converting to constants, it is more the sending of
absolute path that Im not comfortable because of the potential
interoperability problems it could cause.

-- 
Luiz Augusto von Dentz

Re: [PATCH 0/2] obexd: Fix bug in irmc phonebook and prevent to reintroduce it

[Harald Schmitt]

Hi Luiz,
Am 25.07.2012 16:22, schrieb Luiz Augusto von Dentz:
> Hi Harald,
> 
> On Wed, Jul 25, 2012 at 10:18 AM, Harald Schmitt <linux@hschmitt.de> wrote:
>> This one is related to that. But I think your proposal should be a
>> separate patch since it is about the incomming paths from an irmc
>> client. This one is more about to what irmc converts the incomming paths
>> to query phonebook-ebook and phonebook-tracker for contacts and call lists.
>> At the  moment these well known paths in phonebook-*.c are designed to
>> match 1:1 with pbap spec and they are very similar to irmc, but there
>> could be a third implementation to query phonebook where this could be
>> different.
> 
> While the backend indeed take an absolute path this doesnt mean we
> have to send as absolute path, not to mention it is not compatible
> with mimetypes which is what TYPE header describes, even in case of
> PBAP it is wrong to send absolute path in SETPATH.

Patch 1/2 only changes the way phonebook_pull is called (with absolute
path). It has nothing changed about the path resolution/interpretation
that is asked from the client. Sorry for my bad explanations.
> 
> To avoid this problems we send relative although we do accept
> absolute, but to avoid problems with stack interpreting absolute path
> as not valid as OBEX spec state we always send relative paths.

In fact irmc implementation at the moment only accepts relative paths
and I did not change this.
> 
>> The patch 1/2 fixes irmc to query phonebook implementation for the well
>> known absolute path. This was changed in pbap.c with the patch you
>> stated, but forgot to change in irmc.c.
>> Patch 2/2 just replaces the well-known phonebook paths which
>> phoenbook-ebook.c and phonebook-tracker.c support with constants.
> 
> Im fine with converting to constants, it is more the sending of
> absolute path that Im not comfortable because of the potential
> interoperability problems it could cause.

I probably used the wrong term "well known" what I meant by well known
are the paths that phonebook-tracker and phonebook-ebook knows that it
should fetch the contacts, etc. and these are absolute paths. It is not
about the "well known" paths from the pbap spec

Re: [PATCH 0/2] obexd: Fix bug in irmc phonebook and prevent to reintroduce it

[Luiz Augusto von Dentz]

Hi Harald,

On Wed, Jul 25, 2012 at 6:03 PM, Harald Schmitt <linux@hschmitt.de> wrote:

>> While the backend indeed take an absolute path this doesnt mean we
>> have to send as absolute path, not to mention it is not compatible
>> with mimetypes which is what TYPE header describes, even in case of
>> PBAP it is wrong to send absolute path in SETPATH.
>
> Patch 1/2 only changes the way phonebook_pull is called (with absolute
> path). It has nothing changed about the path resolution/interpretation
> that is asked from the client. Sorry for my bad explanations.

Yep, only now I realized that this is not the client side, so it is
probably fine.

>>
>> To avoid this problems we send relative although we do accept
>> absolute, but to avoid problems with stack interpreting absolute path
>> as not valid as OBEX spec state we always send relative paths.
>
> In fact irmc implementation at the moment only accepts relative paths
> and I did not change this.

No problem, I will make it less strict to follow PBAP in another patch.

>>
>>> The patch 1/2 fixes irmc to query phonebook implementation for the well
>>> known absolute path. This was changed in pbap.c with the patch you
>>> stated, but forgot to change in irmc.c.
>>> Patch 2/2 just replaces the well-known phonebook paths which
>>> phoenbook-ebook.c and phonebook-tracker.c support with constants.
>>
>> Im fine with converting to constants, it is more the sending of
>> absolute path that Im not comfortable because of the potential
>> interoperability problems it could cause.
>
> I probably used the wrong term "well known" what I meant by well known
> are the paths that phonebook-tracker and phonebook-ebook knows that it
> should fetch the contacts, etc. and these are absolute paths. It is not
> about the "well known" paths from the pbap spec

No problem, I should have looked what the code was doing before
drawing any conclusion, anyway I will apply this patches asap.


-- 
Luiz Augusto von Dentz

Re: [PATCH 0/2] obexd: Fix bug in irmc phonebook and prevent to reintroduce it

[Luiz Augusto von Dentz]

Hi Harald,

On Fri, Jul 27, 2012 at 4:40 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Harald,
>
> On Wed, Jul 25, 2012 at 6:03 PM, Harald Schmitt <linux@hschmitt.de> wrote:
>
>>> While the backend indeed take an absolute path this doesnt mean we
>>> have to send as absolute path, not to mention it is not compatible
>>> with mimetypes which is what TYPE header describes, even in case of
>>> PBAP it is wrong to send absolute path in SETPATH.
>>
>> Patch 1/2 only changes the way phonebook_pull is called (with absolute
>> path). It has nothing changed about the path resolution/interpretation
>> that is asked from the client. Sorry for my bad explanations.
>
> Yep, only now I realized that this is not the client side, so it is
> probably fine.
>
>>>
>>> To avoid this problems we send relative although we do accept
>>> absolute, but to avoid problems with stack interpreting absolute path
>>> as not valid as OBEX spec state we always send relative paths.
>>
>> In fact irmc implementation at the moment only accepts relative paths
>> and I did not change this.
>
> No problem, I will make it less strict to follow PBAP in another patch.
>
>>>
>>>> The patch 1/2 fixes irmc to query phonebook implementation for the well
>>>> known absolute path. This was changed in pbap.c with the patch you
>>>> stated, but forgot to change in irmc.c.
>>>> Patch 2/2 just replaces the well-known phonebook paths which
>>>> phoenbook-ebook.c and phonebook-tracker.c support with constants.
>>>
>>> Im fine with converting to constants, it is more the sending of
>>> absolute path that Im not comfortable because of the potential
>>> interoperability problems it could cause.
>>
>> I probably used the wrong term "well known" what I meant by well known
>> are the paths that phonebook-tracker and phonebook-ebook knows that it
>> should fetch the contacts, etc. and these are absolute paths. It is not
>> about the "well known" paths from the pbap spec
>
> No problem, I should have looked what the code was doing before
> drawing any conclusion, anyway I will apply this patches asap.

Patches are now pushed upstream, thanks.


-- 
Luiz Augusto von Dentz