2012-01-13 10:48:29

by Slawomir Bochenski

[permalink] [raw]
Subject: [PATCH obexd 1/4] MAP: Implementation of MAP AP core functions

This adds implementation for creation and destruction of map_ap_t and
the definitions of MAP supported application parameters.
---
src/map_ap.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 105 insertions(+), 1 deletions(-)

diff --git a/src/map_ap.c b/src/map_ap.c
index 9d13adf..bec2cc5 100644
--- a/src/map_ap.c
+++ b/src/map_ap.c
@@ -27,13 +27,117 @@

#include "map_ap.h"

+enum ap_type {
+ APT_UINT8,
+ APT_UINT16,
+ APT_UINT32,
+ APT_STR
+};
+
+static const struct ap_def {
+ enum map_ap_tag tag;
+ const char *name;
+ enum ap_type type;
+} ap_defs[] = {
+ { MAP_AP_MAXLISTCOUNT, "MAXLISTCOUNT",
+ APT_UINT16 },
+ { MAP_AP_STARTOFFSET, "STARTOFFSET",
+ APT_UINT16 },
+ { MAP_AP_FILTERMESSAGETYPE, "FILTERMESSAGETYPE",
+ APT_UINT8 },
+ { MAP_AP_FILTERPERIODBEGIN, "FILTERPERIODBEGIN",
+ APT_STR },
+ { MAP_AP_FILTERPERIODEND, "FILTERPERIODEND",
+ APT_STR },
+ { MAP_AP_FILTERREADSTATUS, "FILTERREADSTATUS",
+ APT_UINT8 },
+ { MAP_AP_FILTERRECIPIENT, "FILTERRECIPIENT",
+ APT_STR },
+ { MAP_AP_FILTERORIGINATOR, "FILTERORIGINATOR",
+ APT_STR },
+ { MAP_AP_FILTERPRIORITY, "FILTERPRIORITY",
+ APT_UINT8 },
+ { MAP_AP_ATTACHMENT, "ATTACHMENT",
+ APT_UINT8 },
+ { MAP_AP_TRANSPARENT, "TRANSPARENT",
+ APT_UINT8 },
+ { MAP_AP_RETRY, "RETRY",
+ APT_UINT8 },
+ { MAP_AP_NEWMESSAGE, "NEWMESSAGE",
+ APT_UINT8 },
+ { MAP_AP_NOTIFICATIONSTATUS, "NOTIFICATIONSTATUS",
+ APT_UINT8 },
+ { MAP_AP_MASINSTANCEID, "MASINSTANCEID",
+ APT_UINT8 },
+ { MAP_AP_PARAMETERMASK, "PARAMETERMASK",
+ APT_UINT32 },
+ { MAP_AP_FOLDERLISTINGSIZE, "FOLDERLISTINGSIZE",
+ APT_UINT16 },
+ { MAP_AP_MESSAGESLISTINGSIZE, "MESSAGESLISTINGSIZE",
+ APT_UINT16 },
+ { MAP_AP_SUBJECTLENGTH, "SUBJECTLENGTH",
+ APT_UINT8 },
+ { MAP_AP_CHARSET, "CHARSET",
+ APT_UINT8 },
+ { MAP_AP_FRACTIONREQUEST, "FRACTIONREQUEST",
+ APT_UINT8 },
+ { MAP_AP_FRACTIONDELIVER, "FRACTIONDELIVER",
+ APT_UINT8 },
+ { MAP_AP_STATUSINDICATOR, "STATUSINDICATOR",
+ APT_UINT8 },
+ { MAP_AP_STATUSVALUE, "STATUSVALUE",
+ APT_UINT8 },
+ { MAP_AP_MSETIME, "MSETIME",
+ APT_STR },
+ { MAP_AP_INVALID, NULL,
+ 0 },
+};
+
+struct ap_entry {
+ enum map_ap_tag tag;
+ union {
+ uint32_t val32u;
+ uint16_t val16u;
+ uint8_t val8u;
+ char *valstr;
+ };
+};
+
+static int find_ap_def(uint8_t tag)
+{
+ int i;
+
+ for (i = 0; ap_defs[i].tag != MAP_AP_INVALID; ++i)
+ if (ap_defs[i].tag == tag)
+ return i;
+
+ return -1;
+}
+
+static void ap_entry_free(gpointer val)
+{
+ struct ap_entry *entry = val;
+ int tago;
+
+ tago = find_ap_def(entry->tag);
+
+ if (tago >= 0 && ap_defs[tago].type == APT_STR)
+ g_free(entry->valstr);
+
+ g_free(entry);
+}
+
map_ap_t *map_ap_new(void)
{
- return NULL;
+ return g_hash_table_new_full(NULL, NULL, NULL, ap_entry_free);
}

void map_ap_free(map_ap_t *ap)
{
+ if (!ap)
+ return;
+
+ g_hash_table_destroy(ap);
}

map_ap_t *map_ap_decode(const uint8_t *buffer, size_t length)
--
1.7.4.1



2012-01-13 12:55:23

by Slawomir Bochenski

[permalink] [raw]
Subject: Re: [PATCH obexd 1/4] MAP: Implementation of MAP AP core functions

On Fri, Jan 13, 2012 at 1:37 PM, Slawomir Bochenski <[email protected]> wrote:
> On Fri, Jan 13, 2012 at 1:02 PM, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> Hi Slawomir,
>>
>> On Fri, Jan 13, 2012 at 12:48 PM, Slawomir Bochenski <[email protected]> wrote:
>>> ?map_ap_t *map_ap_new(void)
>>> ?{
>>> - ? ? ? return NULL;
>>> + ? ? ? return g_hash_table_new_full(NULL, NULL, NULL, ap_entry_free);
>>
>> I supposed you would like to use g_direct_hash and g_direct_equal
>> here, even if NULL mean the same it easier to understand and maintain
>> that way. Also it is not clear why the return of map_ap_new is
>> map_ap_t not a GHashTable?
>
> _I_ would like to use NULL - as you said yourself this is easier to
> understand and maintain. And this is explicitly documented default

I have misread you. You said fully spelled function are easier to
maintain. Although I do not believe so, I can see that both in BlueZ
and in oFono it is used that way, so I can make it that way for the
sake of consistency between projects.

> behavior. map_ap_t was agreed earlier for the implementation to be
> interchangeable - map_ap_t is opaque from the map_ap_* API user point
> of view.
>
>>
>>> ?}
>>>
>>> ?void map_ap_free(map_ap_t *ap)
>>> ?{
>>> + ? ? ? if (!ap)
>>> + ? ? ? ? ? ? ? return;
>>
>> Are you sure you need this check, iirc most functions of glib does
>> NULL check already and sometime it even print warnings, so if this
>> check is to prevent warning I would suggest no to have because ?it
>> will hide real bugs behind it.
>
> Destroy function here would print critical error message on NULL
> GHashTable. I like map_ap_free(NULL) call to be valid just like
> g_free(NULL).
>
>>
>> --
>> Luiz Augusto von Dentz
>
>
>
> --
> Slawomir Bochenski



--
Slawomir Bochenski

2012-01-13 12:40:59

by Slawomir Bochenski

[permalink] [raw]
Subject: Re: [PATCH obexd 3/4] map_ap.c: Add implementation for map_ap_decode()

On Fri, Jan 13, 2012 at 1:09 PM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Slawomir,
>
> On Fri, Jan 13, 2012 at 12:48 PM, Slawomir Bochenski <[email protected]> wrote:
>> + ? ? ? ? ? ? ? switch (ap_defs[tago].type) {
>> + ? ? ? ? ? ? ? case APT_UINT8:
>> + ? ? ? ? ? ? ? ? ? ? ? if (hdr->len != 1) {
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? DBG("Value of tag %s (%d) is %d bytes long "
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "instead of expected 1 byte - "
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "skipped!", ap_defs[tago].name,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? hdr->tag, hdr->len);
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? ? ? ? ? ? }
>> +
>> + ? ? ? ? ? ? ? ? ? ? ? map_ap_set_u8(ap, hdr->tag, hdr->val[0]);
>> +
>> + ? ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? ? case APT_UINT16:
>> + ? ? ? ? ? ? ? ? ? ? ? if (hdr->len != 2) {
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? DBG("Value of tag %s (%d) is %d bytes long "
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "instead of expected 2 bytes - "
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "skipped!", ap_defs[tago].name,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? hdr->tag, hdr->len);
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? ? ? ? ? ? }
>> +
>> + ? ? ? ? ? ? ? ? ? ? ? memcpy(&val16, hdr->val, sizeof(val16));
>> + ? ? ? ? ? ? ? ? ? ? ? map_ap_set_u16(ap, hdr->tag, GUINT16_FROM_BE(val16));
>> +
>> + ? ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? ? case APT_UINT32:
>> + ? ? ? ? ? ? ? ? ? ? ? if (hdr->len != 4) {
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? DBG("Value of tag %s (%d) is %d bytes long "
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "instead of expected 4 bytes - "
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "skipped!", ap_defs[tago].name,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? hdr->tag, hdr->len);
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? ? ? ? ? ? }
>> +
>> + ? ? ? ? ? ? ? ? ? ? ? memcpy(&val32, hdr->val, sizeof(val32));
>> + ? ? ? ? ? ? ? ? ? ? ? map_ap_set_u32(ap, hdr->tag, GUINT32_FROM_BE(val32));
>> +
>> + ? ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? ? case APT_STR:
>> + ? ? ? ? ? ? ? ? ? ? ? valstr = g_malloc0(hdr->len + 1);
>> + ? ? ? ? ? ? ? ? ? ? ? memcpy(valstr, hdr->val, hdr->len);
>> + ? ? ? ? ? ? ? ? ? ? ? map_ap_set_string(ap, hdr->tag, valstr);
>> + ? ? ? ? ? ? ? ? ? ? ? g_free(valstr);
>> +
>> + ? ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? ? }
>
> Perhaps this parts can be integrated directly in their respective
> map_ap_set_* functions or if you are planing to reuse them not only
> for decoding then you can use dedicated decoder function e.g.
> map_ap_decode_u8.

Looking at the map_ap.h one can use find out that map_ap_set_*
functions are going to be used for other purposes (those are even
documented there). Additional decoding function could be added if you
think that this will really increase readability.

>
> --
> Luiz Augusto von Dentz



--
Slawomir Bochenski

2012-01-13 12:37:50

by Slawomir Bochenski

[permalink] [raw]
Subject: Re: [PATCH obexd 1/4] MAP: Implementation of MAP AP core functions

On Fri, Jan 13, 2012 at 1:02 PM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Slawomir,
>
> On Fri, Jan 13, 2012 at 12:48 PM, Slawomir Bochenski <[email protected]> wrote:
>> ?map_ap_t *map_ap_new(void)
>> ?{
>> - ? ? ? return NULL;
>> + ? ? ? return g_hash_table_new_full(NULL, NULL, NULL, ap_entry_free);
>
> I supposed you would like to use g_direct_hash and g_direct_equal
> here, even if NULL mean the same it easier to understand and maintain
> that way. Also it is not clear why the return of map_ap_new is
> map_ap_t not a GHashTable?

_I_ would like to use NULL - as you said yourself this is easier to
understand and maintain. And this is explicitly documented default
behavior. map_ap_t was agreed earlier for the implementation to be
interchangeable - map_ap_t is opaque from the map_ap_* API user point
of view.

>
>> ?}
>>
>> ?void map_ap_free(map_ap_t *ap)
>> ?{
>> + ? ? ? if (!ap)
>> + ? ? ? ? ? ? ? return;
>
> Are you sure you need this check, iirc most functions of glib does
> NULL check already and sometime it even print warnings, so if this
> check is to prevent warning I would suggest no to have because ?it
> will hide real bugs behind it.

Destroy function here would print critical error message on NULL
GHashTable. I like map_ap_free(NULL) call to be valid just like
g_free(NULL).

>
> --
> Luiz Augusto von Dentz



--
Slawomir Bochenski

2012-01-13 12:24:37

by Slawomir Bochenski

[permalink] [raw]
Subject: Re: [PATCH obexd 4/4] map_ap.c: Add dumping of map_ap_t after decoding

On Fri, Jan 13, 2012 at 1:13 PM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Slawomir,
>
> On Fri, Jan 13, 2012 at 12:48 PM, Slawomir Bochenski <[email protected]> wrote:
>> ---
>> ?src/map_ap.c | ? 25 +++++++++++++++++++++++++
>> ?1 files changed, 25 insertions(+), 0 deletions(-)
>>
>> diff --git a/src/map_ap.c b/src/map_ap.c
>> index 2e7758e..38e9500 100644
>> --- a/src/map_ap.c
>> +++ b/src/map_ap.c
>> @@ -125,6 +125,29 @@ static int find_ap_def(uint8_t tag)
>> ? ? ? ?return -1;
>> ?}
>>
>> +static void ap_entry_dump(gpointer tag, gpointer val, gpointer user_data)
>> +{
>> + ? ? ? struct ap_entry *entry = val;
>> + ? ? ? int tago;
>> +
>> + ? ? ? tago = find_ap_def(GPOINTER_TO_INT(tag));
>> +
>> + ? ? ? switch (ap_defs[tago].type) {
>> + ? ? ? case APT_UINT8:
>> + ? ? ? ? ? ? ? DBG("%-30s %08x", ap_defs[tago].name, entry->val8u);
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? case APT_UINT16:
>> + ? ? ? ? ? ? ? DBG("%-30s %08x", ap_defs[tago].name, entry->val16u);
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? case APT_UINT32:
>> + ? ? ? ? ? ? ? DBG("%-30s %08x", ap_defs[tago].name, entry->val32u);
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? case APT_STR:
>> + ? ? ? ? ? ? ? DBG("%-30s %s", ap_defs[tago].name, entry->valstr);
>> + ? ? ? ? ? ? ? break;
>> + ? ? ? }
>> +}
>> +
>> ?static void ap_entry_free(gpointer val)
>> ?{
>> ? ? ? ?struct ap_entry *entry = val;
>> @@ -228,6 +251,8 @@ skip:
>> ? ? ? ? ? ? ? ?done += hdr->len + sizeof(struct obex_ap_header);
>> ? ? ? ?}
>>
>> + ? ? ? g_hash_table_foreach(ap, ap_entry_dump, NULL);
>> +
>
> You are dumping on free isn't that more efficient to dump while decoding?

On free? After decode, no free here.

>
> --
> Luiz Augusto von Dentz



--
Slawomir Bochenski

2012-01-13 12:13:24

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH obexd 4/4] map_ap.c: Add dumping of map_ap_t after decoding

Hi Slawomir,

On Fri, Jan 13, 2012 at 12:48 PM, Slawomir Bochenski <[email protected]> wrote:
> ---
> ?src/map_ap.c | ? 25 +++++++++++++++++++++++++
> ?1 files changed, 25 insertions(+), 0 deletions(-)
>
> diff --git a/src/map_ap.c b/src/map_ap.c
> index 2e7758e..38e9500 100644
> --- a/src/map_ap.c
> +++ b/src/map_ap.c
> @@ -125,6 +125,29 @@ static int find_ap_def(uint8_t tag)
> ? ? ? ?return -1;
> ?}
>
> +static void ap_entry_dump(gpointer tag, gpointer val, gpointer user_data)
> +{
> + ? ? ? struct ap_entry *entry = val;
> + ? ? ? int tago;
> +
> + ? ? ? tago = find_ap_def(GPOINTER_TO_INT(tag));
> +
> + ? ? ? switch (ap_defs[tago].type) {
> + ? ? ? case APT_UINT8:
> + ? ? ? ? ? ? ? DBG("%-30s %08x", ap_defs[tago].name, entry->val8u);
> + ? ? ? ? ? ? ? break;
> + ? ? ? case APT_UINT16:
> + ? ? ? ? ? ? ? DBG("%-30s %08x", ap_defs[tago].name, entry->val16u);
> + ? ? ? ? ? ? ? break;
> + ? ? ? case APT_UINT32:
> + ? ? ? ? ? ? ? DBG("%-30s %08x", ap_defs[tago].name, entry->val32u);
> + ? ? ? ? ? ? ? break;
> + ? ? ? case APT_STR:
> + ? ? ? ? ? ? ? DBG("%-30s %s", ap_defs[tago].name, entry->valstr);
> + ? ? ? ? ? ? ? break;
> + ? ? ? }
> +}
> +
> ?static void ap_entry_free(gpointer val)
> ?{
> ? ? ? ?struct ap_entry *entry = val;
> @@ -228,6 +251,8 @@ skip:
> ? ? ? ? ? ? ? ?done += hdr->len + sizeof(struct obex_ap_header);
> ? ? ? ?}
>
> + ? ? ? g_hash_table_foreach(ap, ap_entry_dump, NULL);
> +

You are dumping on free isn't that more efficient to dump while decoding?

--
Luiz Augusto von Dentz

2012-01-13 12:09:47

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH obexd 3/4] map_ap.c: Add implementation for map_ap_decode()

SGkgU2xhd29taXIsCgpPbiBGcmksIEphbiAxMywgMjAxMiBhdCAxMjo0OCBQTSwgU2xhd29taXIg
Qm9jaGVuc2tpIDxsa3NsYXdla0BnbWFpbC5jb20+IHdyb3RlOgo+ICsgoCCgIKAgoCCgIKAgoCBz
d2l0Y2ggKGFwX2RlZnNbdGFnb10udHlwZSkgewo+ICsgoCCgIKAgoCCgIKAgoCBjYXNlIEFQVF9V
SU5UODoKPiArIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCBpZiAoaGRyLT5sZW4gIT0gMSkgewo+ICsg
oCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgREJHKCJWYWx1ZSBvZiB0YWcgJXMgKCVkKSBp
cyAlZCBieXRlcyBsb25nICIKPiArIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCg
IKAgoCCgIKAgoCAiaW5zdGVhZCBvZiBleHBlY3RlZCAxIGJ5dGUgLSAiCj4gKyCgIKAgoCCgIKAg
oCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgInNraXBwZWQhIiwgYXBfZGVmc1t0
YWdvXS5uYW1lLAo+ICsgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAg
oCCgIGhkci0+dGFnLCBoZHItPmxlbik7Cj4gKyCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAg
oCBicmVhazsKPiArIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCB9Cj4gKwo+ICsgoCCgIKAgoCCgIKAg
oCCgIKAgoCCgIG1hcF9hcF9zZXRfdTgoYXAsIGhkci0+dGFnLCBoZHItPnZhbFswXSk7Cj4gKwo+
ICsgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIGJyZWFrOwo+ICsgoCCgIKAgoCCgIKAgoCBjYXNlIEFQ
VF9VSU5UMTY6Cj4gKyCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgaWYgKGhkci0+bGVuICE9IDIpIHsK
PiArIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIERCRygiVmFsdWUgb2YgdGFnICVzICgl
ZCkgaXMgJWQgYnl0ZXMgbG9uZyAiCj4gKyCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCg
IKAgoCCgIKAgoCCgIKAgImluc3RlYWQgb2YgZXhwZWN0ZWQgMiBieXRlcyAtICIKPiArIKAgoCCg
IKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCAic2tpcHBlZCEiLCBhcF9k
ZWZzW3RhZ29dLm5hbWUsCj4gKyCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCg
IKAgoCCgIKAgaGRyLT50YWcsIGhkci0+bGVuKTsKPiArIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCg
IKAgoCCgIGJyZWFrOwo+ICsgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIH0KPiArCj4gKyCgIKAgoCCg
IKAgoCCgIKAgoCCgIKAgbWVtY3B5KCZ2YWwxNiwgaGRyLT52YWwsIHNpemVvZih2YWwxNikpOwo+
ICsgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIG1hcF9hcF9zZXRfdTE2KGFwLCBoZHItPnRhZywgR1VJ
TlQxNl9GUk9NX0JFKHZhbDE2KSk7Cj4gKwo+ICsgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIGJyZWFr
Owo+ICsgoCCgIKAgoCCgIKAgoCBjYXNlIEFQVF9VSU5UMzI6Cj4gKyCgIKAgoCCgIKAgoCCgIKAg
oCCgIKAgaWYgKGhkci0+bGVuICE9IDQpIHsKPiArIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAg
oCCgIERCRygiVmFsdWUgb2YgdGFnICVzICglZCkgaXMgJWQgYnl0ZXMgbG9uZyAiCj4gKyCgIKAg
oCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgImluc3RlYWQgb2YgZXhw
ZWN0ZWQgNCBieXRlcyAtICIKPiArIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCg
IKAgoCCgIKAgoCAic2tpcHBlZCEiLCBhcF9kZWZzW3RhZ29dLm5hbWUsCj4gKyCgIKAgoCCgIKAg
oCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgaGRyLT50YWcsIGhkci0+bGVuKTsK
PiArIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIGJyZWFrOwo+ICsgoCCgIKAgoCCgIKAg
oCCgIKAgoCCgIH0KPiArCj4gKyCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgbWVtY3B5KCZ2YWwzMiwg
aGRyLT52YWwsIHNpemVvZih2YWwzMikpOwo+ICsgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIG1hcF9h
cF9zZXRfdTMyKGFwLCBoZHItPnRhZywgR1VJTlQzMl9GUk9NX0JFKHZhbDMyKSk7Cj4gKwo+ICsg
oCCgIKAgoCCgIKAgoCCgIKAgoCCgIGJyZWFrOwo+ICsgoCCgIKAgoCCgIKAgoCBjYXNlIEFQVF9T
VFI6Cj4gKyCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgdmFsc3RyID0gZ19tYWxsb2MwKGhkci0+bGVu
ICsgMSk7Cj4gKyCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgbWVtY3B5KHZhbHN0ciwgaGRyLT52YWws
IGhkci0+bGVuKTsKPiArIKAgoCCgIKAgoCCgIKAgoCCgIKAgoCBtYXBfYXBfc2V0X3N0cmluZyhh
cCwgaGRyLT50YWcsIHZhbHN0cik7Cj4gKyCgIKAgoCCgIKAgoCCgIKAgoCCgIKAgZ19mcmVlKHZh
bHN0cik7Cj4gKwo+ICsgoCCgIKAgoCCgIKAgoCCgIKAgoCCgIGJyZWFrOwo+ICsgoCCgIKAgoCCg
IKAgoCB9CgpQZXJoYXBzIHRoaXMgcGFydHMgY2FuIGJlIGludGVncmF0ZWQgZGlyZWN0bHkgaW4g
dGhlaXIgcmVzcGVjdGl2ZQptYXBfYXBfc2V0XyogZnVuY3Rpb25zIG9yIGlmIHlvdSBhcmUgcGxh
bmluZyB0byByZXVzZSB0aGVtIG5vdCBvbmx5CmZvciBkZWNvZGluZyB0aGVuIHlvdSBjYW4gdXNl
IGRlZGljYXRlZCBkZWNvZGVyIGZ1bmN0aW9uIGUuZy4KbWFwX2FwX2RlY29kZV91OC4KCi0tIApM
dWl6IEF1Z3VzdG8gdm9uIERlbnR6Cg==

2012-01-13 12:02:04

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH obexd 1/4] MAP: Implementation of MAP AP core functions

Hi Slawomir,

On Fri, Jan 13, 2012 at 12:48 PM, Slawomir Bochenski <[email protected]> wrote:
> ?map_ap_t *map_ap_new(void)
> ?{
> - ? ? ? return NULL;
> + ? ? ? return g_hash_table_new_full(NULL, NULL, NULL, ap_entry_free);

I supposed you would like to use g_direct_hash and g_direct_equal
here, even if NULL mean the same it easier to understand and maintain
that way. Also it is not clear why the return of map_ap_new is
map_ap_t not a GHashTable?

> ?}
>
> ?void map_ap_free(map_ap_t *ap)
> ?{
> + ? ? ? if (!ap)
> + ? ? ? ? ? ? ? return;

Are you sure you need this check, iirc most functions of glib does
NULL check already and sometime it even print warnings, so if this
check is to prevent warning I would suggest no to have because it
will hide real bugs behind it.

--
Luiz Augusto von Dentz

2012-01-13 10:48:32

by Slawomir Bochenski

[permalink] [raw]
Subject: [PATCH obexd 4/4] map_ap.c: Add dumping of map_ap_t after decoding

---
src/map_ap.c | 25 +++++++++++++++++++++++++
1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/src/map_ap.c b/src/map_ap.c
index 2e7758e..38e9500 100644
--- a/src/map_ap.c
+++ b/src/map_ap.c
@@ -125,6 +125,29 @@ static int find_ap_def(uint8_t tag)
return -1;
}

+static void ap_entry_dump(gpointer tag, gpointer val, gpointer user_data)
+{
+ struct ap_entry *entry = val;
+ int tago;
+
+ tago = find_ap_def(GPOINTER_TO_INT(tag));
+
+ switch (ap_defs[tago].type) {
+ case APT_UINT8:
+ DBG("%-30s %08x", ap_defs[tago].name, entry->val8u);
+ break;
+ case APT_UINT16:
+ DBG("%-30s %08x", ap_defs[tago].name, entry->val16u);
+ break;
+ case APT_UINT32:
+ DBG("%-30s %08x", ap_defs[tago].name, entry->val32u);
+ break;
+ case APT_STR:
+ DBG("%-30s %s", ap_defs[tago].name, entry->valstr);
+ break;
+ }
+}
+
static void ap_entry_free(gpointer val)
{
struct ap_entry *entry = val;
@@ -228,6 +251,8 @@ skip:
done += hdr->len + sizeof(struct obex_ap_header);
}

+ g_hash_table_foreach(ap, ap_entry_dump, NULL);
+
return ap;
}

--
1.7.4.1


2012-01-13 10:48:31

by Slawomir Bochenski

[permalink] [raw]
Subject: [PATCH obexd 3/4] map_ap.c: Add implementation for map_ap_decode()

---
src/map_ap.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 87 insertions(+), 1 deletions(-)

diff --git a/src/map_ap.c b/src/map_ap.c
index 0ed097f..2e7758e 100644
--- a/src/map_ap.c
+++ b/src/map_ap.c
@@ -25,6 +25,10 @@
#include <config.h>
#endif

+#include <string.h>
+
+#include "log.h"
+
#include "map_ap.h"

enum ap_type {
@@ -103,6 +107,13 @@ struct ap_entry {
};
};

+/* This comes from OBEX specs */
+struct obex_ap_header {
+ uint8_t tag;
+ uint8_t len;
+ uint8_t val[0];
+} __attribute__ ((packed));
+
static int find_ap_def(uint8_t tag)
{
int i;
@@ -142,7 +153,82 @@ void map_ap_free(map_ap_t *ap)

map_ap_t *map_ap_decode(const uint8_t *buffer, size_t length)
{
- return NULL;
+ GHashTable *ap;
+ struct obex_ap_header *hdr;
+ uint32_t done = 0;
+ uint16_t val16;
+ uint32_t val32;
+ char *valstr;
+ int tago;
+
+ ap = map_ap_new();
+ if (!ap)
+ return NULL;
+
+ while (done < length) {
+ hdr = (void *) buffer + done;
+
+ tago = find_ap_def(hdr->tag);
+
+ if (tago < 0) {
+ DBG("Unknown tag %d (length %d) - skipped.",
+ hdr->tag, hdr->len);
+ goto skip;
+ }
+
+ switch (ap_defs[tago].type) {
+ case APT_UINT8:
+ if (hdr->len != 1) {
+ DBG("Value of tag %s (%d) is %d bytes long "
+ "instead of expected 1 byte - "
+ "skipped!", ap_defs[tago].name,
+ hdr->tag, hdr->len);
+ break;
+ }
+
+ map_ap_set_u8(ap, hdr->tag, hdr->val[0]);
+
+ break;
+ case APT_UINT16:
+ if (hdr->len != 2) {
+ DBG("Value of tag %s (%d) is %d bytes long "
+ "instead of expected 2 bytes - "
+ "skipped!", ap_defs[tago].name,
+ hdr->tag, hdr->len);
+ break;
+ }
+
+ memcpy(&val16, hdr->val, sizeof(val16));
+ map_ap_set_u16(ap, hdr->tag, GUINT16_FROM_BE(val16));
+
+ break;
+ case APT_UINT32:
+ if (hdr->len != 4) {
+ DBG("Value of tag %s (%d) is %d bytes long "
+ "instead of expected 4 bytes - "
+ "skipped!", ap_defs[tago].name,
+ hdr->tag, hdr->len);
+ break;
+ }
+
+ memcpy(&val32, hdr->val, sizeof(val32));
+ map_ap_set_u32(ap, hdr->tag, GUINT32_FROM_BE(val32));
+
+ break;
+ case APT_STR:
+ valstr = g_malloc0(hdr->len + 1);
+ memcpy(valstr, hdr->val, hdr->len);
+ map_ap_set_string(ap, hdr->tag, valstr);
+ g_free(valstr);
+
+ break;
+ }
+
+skip:
+ done += hdr->len + sizeof(struct obex_ap_header);
+ }
+
+ return ap;
}

uint8_t *map_ap_encode(map_ap_t *ap, size_t *length)
--
1.7.4.1


2012-01-13 10:48:30

by Slawomir Bochenski

[permalink] [raw]
Subject: [PATCH obexd 2/4] map_ap.c: Add implementation for map_ap_set_*

---
src/map_ap.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
1 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/src/map_ap.c b/src/map_ap.c
index bec2cc5..0ed097f 100644
--- a/src/map_ap.c
+++ b/src/map_ap.c
@@ -174,20 +174,68 @@ const char *map_ap_get_string(map_ap_t *ap, enum map_ap_tag tag)

gboolean map_ap_set_u8(map_ap_t *ap, enum map_ap_tag tag, uint8_t val)
{
- return FALSE;
+ struct ap_entry *entry;
+ int tago = find_ap_def(tag);
+
+ if (tago < 0 || ap_defs[tago].type != APT_UINT8)
+ return FALSE;
+
+ entry = g_new0(struct ap_entry, 1);
+ entry->tag = tag;
+ entry->val8u = val;
+
+ g_hash_table_insert(ap, GINT_TO_POINTER(tag), entry);
+
+ return TRUE;
}

gboolean map_ap_set_u16(map_ap_t *ap, enum map_ap_tag tag, uint16_t val)
{
- return FALSE;
+ struct ap_entry *entry;
+ int tago = find_ap_def(tag);
+
+ if (tago < 0 || ap_defs[tago].type != APT_UINT16)
+ return FALSE;
+
+ entry = g_new0(struct ap_entry, 1);
+ entry->tag = tag;
+ entry->val16u = val;
+
+ g_hash_table_insert(ap, GINT_TO_POINTER(tag), entry);
+
+ return TRUE;
}

gboolean map_ap_set_u32(map_ap_t *ap, enum map_ap_tag tag, uint32_t val)
{
- return FALSE;
+ struct ap_entry *entry;
+ int tago = find_ap_def(tag);
+
+ if (tago < 0 || ap_defs[tago].type != APT_UINT32)
+ return FALSE;
+
+ entry = g_new0(struct ap_entry, 1);
+ entry->tag = tag;
+ entry->val32u = val;
+
+ g_hash_table_insert(ap, GINT_TO_POINTER(tag), entry);
+
+ return TRUE;
}

gboolean map_ap_set_string(map_ap_t *ap, enum map_ap_tag tag, const char *val)
{
- return FALSE;
+ struct ap_entry *entry;
+ int tago = find_ap_def(tag);
+
+ if (tago < 0 || ap_defs[tago].type != APT_STR)
+ return FALSE;
+
+ entry = g_new0(struct ap_entry, 1);
+ entry->tag = tag;
+ entry->valstr = g_strdup(val);
+
+ g_hash_table_insert(ap, GINT_TO_POINTER(tag), entry);
+
+ return TRUE;
}
--
1.7.4.1