2010-10-29 09:30:19

by Lukasz Pawlik

[permalink] [raw]
Subject: [PATCH] Fix handling nco affiliation fields

Previously when contact record was divided into separate replies,
phonebook-tracker would use first reply and merge only phone numbers,
addresses and emails from next. Now it's merging all fields dependent on the
nco:hasAffiliation as well.
---
plugins/phonebook-tracker.c | 15 +++++++++++++++
1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/plugins/phonebook-tracker.c b/plugins/phonebook-tracker.c
index 96290a4..f37ed5a 100644
--- a/plugins/phonebook-tracker.c
+++ b/plugins/phonebook-tracker.c
@@ -982,6 +982,14 @@ static void add_phone_number(struct phonebook_contact *contact,
contact->numbers = g_slist_append(contact->numbers, number);
}

+static void add_affiliation(char **field, const char *value)
+{
+ if(strlen(*field) != 0 || value == NULL || strlen(value) == 0)
+ return;
+
+ *field = g_strdup(value);
+}
+
static struct phonebook_email *find_email(GSList *emails, const char *address,
int type)
{
@@ -1196,6 +1204,13 @@ add_numbers:
g_free(home_addr);
g_free(work_addr);

+ /* Adding fields connected by nco:hasAffiliation - they may be in
+ * separate replies */
+ add_affiliation(&contact->title, reply[33]);
+ add_affiliation(&contact->company, reply[22]);
+ add_affiliation(&contact->department, reply[23]);
+ add_affiliation(&contact->role, reply[24]);
+
DBG("contact %p", contact);

/* Adding contacts data to wrapper struct - this data will be used to
--
1.7.0.4



2010-10-29 13:53:04

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH] Fix handling nco affiliation fields

Hi Lukasz,

On Fri, Oct 29, 2010, Lukasz Pawlik wrote:
> Previously when contact record was divided into separate replies,
> phonebook-tracker would use first reply and merge only phone numbers,
> addresses and emails from next. Now it's merging all fields dependent on the
> nco:hasAffiliation as well.
> ---
> plugins/phonebook-tracker.c | 15 +++++++++++++++
> 1 files changed, 15 insertions(+), 0 deletions(-)

You'll need to fix the coding style before this goes upstream:

> +static void add_affiliation(char **field, const char *value)
> +{
> + if(strlen(*field) != 0 || value == NULL || strlen(value) == 0)

Space between "if" and "(". Also, I'd prefer > 0 instead of != 0 for the
first strlen check. Then, you're also using spaces instead of tabs for
the indentation of all code in this patch. Did you forget to switch your
editor into bluez/kernel mode?

Johan

2010-10-29 12:46:51

by Anderson Lizardo

[permalink] [raw]
Subject: Re: [PATCH] Fix handling nco affiliation fields

On Fri, Oct 29, 2010 at 5:30 AM, Lukasz Pawlik <[email protected]> wrote:
> Previously when contact record was divided into separate replies,
> phonebook-tracker would use first reply and merge only phone numbers,
> addresses and emails from next. Now it's merging all fields dependent on the
> nco:hasAffiliation as well.
> ---
> ?plugins/phonebook-tracker.c | ? 15 +++++++++++++++
> ?1 files changed, 15 insertions(+), 0 deletions(-)
>
> diff --git a/plugins/phonebook-tracker.c b/plugins/phonebook-tracker.c
> index 96290a4..f37ed5a 100644
> --- a/plugins/phonebook-tracker.c
> +++ b/plugins/phonebook-tracker.c
> @@ -982,6 +982,14 @@ static void add_phone_number(struct phonebook_contact *contact,
> ? ? ? ?contact->numbers = g_slist_append(contact->numbers, number);
> ?}
>
> +static void add_affiliation(char **field, const char *value)
> +{
> + ? ? ? ?if(strlen(*field) != 0 || value == NULL || strlen(value) == 0)
> + ? ? ? ? ? ? ? ?return;

Is *field always non-NULL ? If yes, then the next g_strdup() will leak
one byte (the "\0"). If not, strlen() will segfault on a NULL pointer.

> +
> + ? ? ? ?*field = g_strdup(value);
> +}
> +
> ?static struct phonebook_email *find_email(GSList *emails, const char *address,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?int type)
> ?{
> @@ -1196,6 +1204,13 @@ add_numbers:
> ? ? ? ?g_free(home_addr);
> ? ? ? ?g_free(work_addr);
>
> + ? ? ? ?/* Adding fields connected by nco:hasAffiliation - they may be in
> + ? ? ? ? * separate replies */
> + ? ? ? ?add_affiliation(&contact->title, reply[33]);
> + ? ? ? ?add_affiliation(&contact->company, reply[22]);
> + ? ? ? ?add_affiliation(&contact->department, reply[23]);
> + ? ? ? ?add_affiliation(&contact->role, reply[24]);
> +
> ? ? ? ?DBG("contact %p", contact);
>
> ? ? ? ?/* Adding contacts data to wrapper struct - this data will be used to
> --
> 1.7.0.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>


Regards,
--
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil