2011-11-15 14:44:54

by Bartosz Szatkowski

[permalink] [raw]
Subject: [PATCH] gobex: Fix setpath to match one from OBEX spec

OBEX spec states that when bit 0 is set in flags, server should backup a
level before applying name. It's accomplished by moving parsing of the
path to upper layers (as gobex is low level lib and I think it should be
based on level defined in OBEX spec, upper profiles may wrap that as
defined in correspondent specifications) and exposing *cdup* flag in
function parameters.
---
client/ftp.c | 6 +++++-
client/pbap.c | 4 ++--
gobex/gobex.c | 16 ++++++++++------
gobex/gobex.h | 5 +++--
unit/test-gobex.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
5 files changed, 64 insertions(+), 13 deletions(-)

diff --git a/client/ftp.c b/client/ftp.c
index 336fc26..d9e31ad 100644
--- a/client/ftp.c
+++ b/client/ftp.c
@@ -82,7 +82,11 @@ static DBusMessage *change_folder(DBusConnection *connection,
return g_dbus_create_error(message,
"org.openobex.Error.InvalidArguments", NULL);

- g_obex_setpath(obex, folder, async_cb, message, &err);
+ if (g_strcmp0("..", folder) == 0)
+ g_obex_setpath(obex, NULL, TRUE, async_cb, message, &err);
+ else
+ g_obex_setpath(obex, folder, FALSE, async_cb, message, &err);
+
if (err != NULL) {
DBusMessage *reply;
reply = g_dbus_create_error(message,
diff --git a/client/pbap.c b/client/pbap.c
index 9e9eb05..5055ad9 100644
--- a/client/pbap.c
+++ b/client/pbap.c
@@ -277,7 +277,7 @@ static void setpath_cb(GObex *obex, GError *err, GObexPacket *rsp,

data->index++;

- g_obex_setpath(obex, next, setpath_cb, data, &err);
+ g_obex_setpath(obex, next, FALSE, setpath_cb, data, &err);
if (err != NULL) {
setpath_complete(err, data);
g_error_free(err);
@@ -292,7 +292,7 @@ static gboolean setpath(GObex *obex, const char *path, size_t max_elem,

data = g_new0(struct setpath_data, 1);

- g_obex_setpath(obex, "", setpath_cb, data, &err);
+ g_obex_setpath(obex, NULL, FALSE, setpath_cb, data, &err);
if (err != NULL) {
error("set_path: %s", err->message);
g_error_free(err);
diff --git a/gobex/gobex.c b/gobex/gobex.c
index 7840304..1085e78 100644
--- a/gobex/gobex.c
+++ b/gobex/gobex.c
@@ -1083,11 +1083,13 @@ guint g_obex_connect(GObex *obex, GObexResponseFunc func, gpointer user_data,
return g_obex_send_req(obex, req, -1, func, user_data, err);
}

-guint g_obex_setpath(GObex *obex, const char *path, GObexResponseFunc func,
- gpointer user_data, GError **err)
+guint g_obex_setpath(GObex *obex, const char *folder, gboolean cdup,
+ GObexResponseFunc func, gpointer user_data,
+ GError **err)
{
GObexPacket *req;
struct setpath_data data;
+ GObexHeader *hdr;

g_obex_debug(G_OBEX_DEBUG_COMMAND, "conn %u", obex->conn_id);

@@ -1095,12 +1097,14 @@ guint g_obex_setpath(GObex *obex, const char *path, GObexResponseFunc func,

memset(&data, 0, sizeof(data));

- if (strcmp(path, "..") == 0)
+ if (cdup)
data.flags = 0x03;
- else {
- GObexHeader *hdr;
+ else
data.flags = 0x02;
- hdr = g_obex_header_new_unicode(G_OBEX_HDR_NAME, path);
+
+ if (!cdup || (folder != NULL && folder[0] != '\0')) {
+ hdr = g_obex_header_new_unicode(G_OBEX_HDR_NAME,
+ folder != NULL ? folder : "");
g_obex_packet_add_header(req, hdr);
}

diff --git a/gobex/gobex.h b/gobex/gobex.h
index 1b20333..d9d470d 100644
--- a/gobex/gobex.h
+++ b/gobex/gobex.h
@@ -73,8 +73,9 @@ void g_obex_unref(GObex *obex);
guint g_obex_connect(GObex *obex, GObexResponseFunc func, gpointer user_data,
GError **err, guint8 first_hdr_id, ...);

-guint g_obex_setpath(GObex *obex, const char *path, GObexResponseFunc func,
- gpointer user_data, GError **err);
+guint g_obex_setpath(GObex *obex, const char *folder, gboolean cdup,
+ GObexResponseFunc func, gpointer user_data,
+ GError **err);

guint g_obex_mkdir(GObex *obex, const char *path, GObexResponseFunc func,
gpointer user_data, GError **err);
diff --git a/unit/test-gobex.c b/unit/test-gobex.c
index 62443db..8b86dda 100644
--- a/unit/test-gobex.c
+++ b/unit/test-gobex.c
@@ -46,6 +46,10 @@ static uint8_t pkt_setpath_req[] = { G_OBEX_OP_SETPATH | FINAL_BIT, 0x00, 0x10,
0, 'd', 0, 'i', 0, 'r', 0, 0 };
static uint8_t pkt_setpath_up_req[] = { G_OBEX_OP_SETPATH | FINAL_BIT,
0x00, 0x05, 0x03, 0x00 };
+static uint8_t pkt_setpath_up_down_req[] = { G_OBEX_OP_SETPATH | FINAL_BIT,
+ 0x00, 0x10, 0x03, 0x00,
+ G_OBEX_HDR_NAME, 0x00, 0x0b,
+ 0, 'd', 0, 'i', 0, 'r', 0, 0 };
static uint8_t pkt_success_rsp[] = { 0x20 | FINAL_BIT, 0x00, 0x03 };

static uint8_t pkt_mkdir_req[] = { G_OBEX_OP_SETPATH | FINAL_BIT, 0x00, 0x10,
@@ -890,7 +894,7 @@ static void test_setpath(void)

timer_id = g_timeout_add_seconds(1, test_timeout, &d);

- g_obex_setpath(obex, "dir", req_complete, &d, &d.err);
+ g_obex_setpath(obex, "dir", FALSE, req_complete, &d, &d.err);
g_assert_no_error(d.err);

g_main_loop_run(d.mainloop);
@@ -926,7 +930,44 @@ static void test_setpath_up(void)

timer_id = g_timeout_add_seconds(1, test_timeout, &d);

- g_obex_setpath(obex, "..", req_complete, &d, &d.err);
+ g_obex_setpath(obex, "", TRUE, req_complete, &d, &d.err);
+ g_assert_no_error(d.err);
+
+ g_main_loop_run(d.mainloop);
+
+ g_assert_cmpuint(d.count, ==, 1);
+
+ g_main_loop_unref(d.mainloop);
+
+ g_source_remove(timer_id);
+ g_io_channel_unref(io);
+ g_source_remove(io_id);
+ g_obex_unref(obex);
+
+ g_assert_no_error(d.err);
+}
+
+static void test_setpath_up_down(void)
+{
+ GIOChannel *io;
+ GIOCondition cond;
+ guint io_id, timer_id;
+ GObex *obex;
+ struct test_data d = { 0, NULL, {
+ { pkt_setpath_up_down_req,
+ sizeof(pkt_setpath_up_down_req) } }, {
+ { pkt_success_rsp, sizeof(pkt_success_rsp) } } };
+
+ create_endpoints(&obex, &io, SOCK_STREAM);
+
+ cond = G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL;
+ io_id = g_io_add_watch(io, cond, test_io_cb, &d);
+
+ d.mainloop = g_main_loop_new(NULL, FALSE);
+
+ timer_id = g_timeout_add_seconds(1, test_timeout, &d);
+
+ g_obex_setpath(obex, "dir", TRUE, req_complete, &d, &d.err);
g_assert_no_error(d.err);

g_main_loop_run(d.mainloop);
@@ -1142,6 +1183,7 @@ int main(int argc, char *argv[])

g_test_add_func("/gobex/test_setpath", test_setpath);
g_test_add_func("/gobex/test_setpath_up", test_setpath_up);
+ g_test_add_func("/gobex/test_setpath_up_down", test_setpath_up_down);

g_test_add_func("/gobex/test_mkdir", test_mkdir);

--
1.7.4.1



2011-11-16 12:00:43

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH] gobex: Fix setpath to match one from OBEX spec

Hi, Slawek,

On Wed, Nov 16, 2011, Slawomir Bochenski wrote:
> The function itself is quite useful for MAP clients. There is this
> fixed part of directories structure in MAP, which is:
>
> telecom
> '- msg
> |- inbox
> |- outbox
> |- sent
> |- deleted
> '- draft
>
> There are no messages in msg, thus going from e.g. outbox to sent
> directly seems convenient.
>
> I do not understand why you are so reluctant to have function for
> setting path in gobex that actually offers what OBEX offers. As I
> mentioned already in discussion about MAP API proposal, this also
> gives additional restrictions on what characters one can use in folder
> name.
>
> You at least have one example of profile that could have make use of
> it. So it won't be in effect anything that is not used.

I don't as such have anything against the possibility of supporting a
"cd ../foo" operation through the API, but the way that the OBEX spec
implements it feels like a hack that's been put in place as an
afterthought (they thought they'd get away with restricting path changes
to one level at a time and when they realized ../foo might make sense
instead of extending the name header they added this to flags (I don't
know if this is how things actually happened but it's the impression I
get). In OBEX 1.4 the DestName header was defined to tackle this
shortcoming of the Name header, but at that point it was too late to
have SetPath use it due to backwards compatibility reasons.

Anyway, there's no reason to expose this ugliness in our APIs and we can
still support the feature by allowing the application to pass "../foo"
as the path name.

Johan

2011-11-16 11:49:09

by Slawomir Bochenski

[permalink] [raw]
Subject: Re: [PATCH] gobex: Fix setpath to match one from OBEX spec

On Wed, Nov 16, 2011 at 12:12 PM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Bartosz,
>
> On Wed, Nov 16, 2011 at 10:24 AM, Bartosz Szatkowski <[email protected]> wrote:
>>
>> Come on did You even read it?? This function is OBEX level (an it
>> works before in similar way - only one dir at a time) - we may don't
>> like specification, but seriously? Just read the patch, i exposed on
>> OBEX level stuff defined in the OBEX specification (as MAP needs
>> operation like "go one level up and than down" in one step) and as it
>> would be possible to just add some "if" for "../dir" in previous
>> version, but it is better to fix broken things than to propagate them
>> (it's obex level function not high level abstraction - deal with it).
>
> Yeah, lets create an API that we don't use, really? Your MAP API need
> it but that doesn't mean we agree on using it as it is, that table you
> use to explain how it works is already a sign that it is

I guess the table I've seen in Bartek's proposal is rather graphical
representation that serves an example. I think that simply saying that
"cdup" goes up one level before going to child directory would be also
understandable and not really confusing.

> over-engineered and will be easily misinterpreted by applications. We
> will not break MAP by not exposing this complexity since we can still
> do it transparently, even doing cdup and another SetPath although
> inefficient would still work.

The function itself is quite useful for MAP clients. There is this
fixed part of directories structure in MAP, which is:

telecom
'- msg
|- inbox
|- outbox
|- sent
|- deleted
'- draft

There are no messages in msg, thus going from e.g. outbox to sent
directly seems convenient.

I do not understand why you are so reluctant to have function for
setting path in gobex that actually offers what OBEX offers. As I
mentioned already in discussion about MAP API proposal, this also
gives additional restrictions on what characters one can use in folder
name.

You at least have one example of profile that could have make use of
it. So it won't be in effect anything that is not used.

>> Each profile may use it multiple times for paths delivered through
>> dbus (but it provokes some questions, that i don't like). Generally
>> there is REALLY many gui that works like that (one level down or up at
>> a time) just look around (eg all file explorers) and it's all you
>> need, because its basically same context!
>
> Same context? What context, the folder listing is per folder, besides
> nowadays the file explores have abandon the tree view in favor of more
> simple one level like nautilus. But even with gui that uses tree view
> we can only jump one level or to the root so it is not that it would
> save that much of time.

--
Slawomir Bochenski

2011-11-16 11:12:49

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH] gobex: Fix setpath to match one from OBEX spec

Hi Bartosz,

On Wed, Nov 16, 2011 at 10:24 AM, Bartosz Szatkowski <[email protected]> wrote:
>
> Come on did You even read it?? This function is OBEX level (an it
> works before in similar way - only one dir at a time) - we may don't
> like specification, but seriously? Just read the patch, i exposed on
> OBEX level stuff defined in the OBEX specification (as MAP needs
> operation like "go one level up and than down" in one step) and as it
> would be possible to just add some "if" for "../dir" in previous
> version, but it is better to fix broken things than to propagate them
> (it's obex level function not high level abstraction - deal with it).

Yeah, lets create an API that we don't use, really? Your MAP API need
it but that doesn't mean we agree on using it as it is, that table you
use to explain how it works is already a sign that it is
over-engineered and will be easily misinterpreted by applications. We
will not break MAP by not exposing this complexity since we can still
do it transparently, even doing cdup and another SetPath although
inefficient would still work.

> Each profile may use it multiple times for paths delivered through
> dbus (but it provokes some questions, that i don't like). Generally
> there is REALLY many gui that works like that (one level down or up at
> a time) just look around (eg all file explorers) and it's all you
> need, because its basically same context!

Same context? What context, the folder listing is per folder, besides
nowadays the file explores have abandon the tree view in favor of more
simple one level like nautilus. But even with gui that uses tree view
we can only jump one level or to the root so it is not that it would
save that much of time.

--
Luiz Augusto von Dentz

2011-11-16 08:24:59

by Bartosz Szatkowski

[permalink] [raw]
Subject: Re: [PATCH] gobex: Fix setpath to match one from OBEX spec

On Tue, Nov 15, 2011 at 11:29 PM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Bartosz,
>
> On Tue, Nov 15, 2011 at 4:44 PM, Bartosz Szatkowski <[email protected]> wrote:
>> OBEX spec states that when bit 0 is set in flags, server should backup a
>> level before applying name. It's accomplished by moving parsing of the
>> path to upper layers (as gobex is low level lib and I think it should be
>> based on level defined in OBEX spec, upper profiles may wrap that as
>> defined in correspondent specifications) and exposing *cdup* flag in
>> function parameters.
>> ---
>>  client/ftp.c      |    6 +++++-
>>  client/pbap.c     |    4 ++--
>>  gobex/gobex.c     |   16 ++++++++++------
>>  gobex/gobex.h     |    5 +++--
>>  unit/test-gobex.c |   46 ++++++++++++++++++++++++++++++++++++++++++++--
>>  5 files changed, 64 insertions(+), 13 deletions(-)
>>
>> diff --git a/client/ftp.c b/client/ftp.c
>> index 336fc26..d9e31ad 100644
>> --- a/client/ftp.c
>> +++ b/client/ftp.c
>> @@ -82,7 +82,11 @@ static DBusMessage *change_folder(DBusConnection *connection,
>>                return g_dbus_create_error(message,
>>                                "org.openobex.Error.InvalidArguments", NULL);
>>
>> -       g_obex_setpath(obex, folder, async_cb, message, &err);
>> +       if (g_strcmp0("..", folder) == 0)
>> +               g_obex_setpath(obex, NULL, TRUE, async_cb, message, &err);
>> +       else
>> +               g_obex_setpath(obex, folder, FALSE, async_cb, message, &err);
>> +
>>        if (err != NULL) {
>>                DBusMessage *reply;
>>                reply =  g_dbus_create_error(message,
>> diff --git a/client/pbap.c b/client/pbap.c
>> index 9e9eb05..5055ad9 100644
>> --- a/client/pbap.c
>> +++ b/client/pbap.c
>> @@ -277,7 +277,7 @@ static void setpath_cb(GObex *obex, GError *err, GObexPacket *rsp,
>>
>>        data->index++;
>>
>> -       g_obex_setpath(obex, next, setpath_cb, data, &err);
>> +       g_obex_setpath(obex, next, FALSE, setpath_cb, data, &err);
>>        if (err != NULL) {
>>                setpath_complete(err, data);
>>                g_error_free(err);
>> @@ -292,7 +292,7 @@ static gboolean setpath(GObex *obex, const char *path, size_t max_elem,
>>
>>        data = g_new0(struct setpath_data, 1);
>>
>> -       g_obex_setpath(obex, "", setpath_cb, data, &err);
>> +       g_obex_setpath(obex, NULL, FALSE, setpath_cb, data, &err);
>>        if (err != NULL) {
>>                error("set_path: %s", err->message);
>>                g_error_free(err);
>> diff --git a/gobex/gobex.c b/gobex/gobex.c
>> index 7840304..1085e78 100644
>> --- a/gobex/gobex.c
>> +++ b/gobex/gobex.c
>> @@ -1083,11 +1083,13 @@ guint g_obex_connect(GObex *obex, GObexResponseFunc func, gpointer user_data,
>>        return g_obex_send_req(obex, req, -1, func, user_data, err);
>>  }
>>
>> -guint g_obex_setpath(GObex *obex, const char *path, GObexResponseFunc func,
>> -                                       gpointer user_data, GError **err)
>> +guint g_obex_setpath(GObex *obex, const char *folder, gboolean cdup,
>> +                               GObexResponseFunc func, gpointer user_data,
>> +                               GError **err)
>>  {
>>        GObexPacket *req;
>>        struct setpath_data data;
>> +       GObexHeader *hdr;
>>
>>        g_obex_debug(G_OBEX_DEBUG_COMMAND, "conn %u", obex->conn_id);
>>
>> @@ -1095,12 +1097,14 @@ guint g_obex_setpath(GObex *obex, const char *path, GObexResponseFunc func,
>>
>>        memset(&data, 0, sizeof(data));
>>
>> -       if (strcmp(path, "..") == 0)
>> +       if (cdup)
>>                data.flags = 0x03;
>> -       else {
>> -               GObexHeader *hdr;
>> +       else
>>                data.flags = 0x02;
>> -               hdr = g_obex_header_new_unicode(G_OBEX_HDR_NAME, path);
>> +
>> +       if (!cdup || (folder != NULL && folder[0] != '\0')) {
>> +               hdr = g_obex_header_new_unicode(G_OBEX_HDR_NAME,
>> +                                               folder != NULL ? folder : "");
>>                g_obex_packet_add_header(req, hdr);
>>        }
>>
>> diff --git a/gobex/gobex.h b/gobex/gobex.h
>> index 1b20333..d9d470d 100644
>> --- a/gobex/gobex.h
>> +++ b/gobex/gobex.h
>> @@ -73,8 +73,9 @@ void g_obex_unref(GObex *obex);
>>  guint g_obex_connect(GObex *obex, GObexResponseFunc func, gpointer user_data,
>>                                GError **err, guint8 first_hdr_id, ...);
>>
>> -guint g_obex_setpath(GObex *obex, const char *path, GObexResponseFunc func,
>> -                                       gpointer user_data, GError **err);
>> +guint g_obex_setpath(GObex *obex, const char *folder, gboolean cdup,
>> +                               GObexResponseFunc func, gpointer user_data,
>> +                               GError **err);
>>
>>  guint g_obex_mkdir(GObex *obex, const char *path, GObexResponseFunc func,
>>                                        gpointer user_data, GError **err);
>> diff --git a/unit/test-gobex.c b/unit/test-gobex.c
>> index 62443db..8b86dda 100644
>> --- a/unit/test-gobex.c
>> +++ b/unit/test-gobex.c
>> @@ -46,6 +46,10 @@ static uint8_t pkt_setpath_req[] = { G_OBEX_OP_SETPATH | FINAL_BIT, 0x00, 0x10,
>>                                        0, 'd', 0, 'i', 0, 'r', 0, 0 };
>>  static uint8_t pkt_setpath_up_req[] = { G_OBEX_OP_SETPATH | FINAL_BIT,
>>                                        0x00, 0x05, 0x03, 0x00 };
>> +static uint8_t pkt_setpath_up_down_req[] = { G_OBEX_OP_SETPATH | FINAL_BIT,
>> +                                       0x00, 0x10, 0x03, 0x00,
>> +                                       G_OBEX_HDR_NAME, 0x00, 0x0b,
>> +                                       0, 'd', 0, 'i', 0, 'r', 0, 0 };
>>  static uint8_t pkt_success_rsp[] = { 0x20 | FINAL_BIT, 0x00, 0x03 };
>>
>>  static uint8_t pkt_mkdir_req[] = { G_OBEX_OP_SETPATH | FINAL_BIT, 0x00, 0x10,
>> @@ -890,7 +894,7 @@ static void test_setpath(void)
>>
>>        timer_id = g_timeout_add_seconds(1, test_timeout, &d);
>>
>> -       g_obex_setpath(obex, "dir", req_complete, &d, &d.err);
>> +       g_obex_setpath(obex, "dir", FALSE, req_complete, &d, &d.err);
>>        g_assert_no_error(d.err);
>>
>>        g_main_loop_run(d.mainloop);
>> @@ -926,7 +930,44 @@ static void test_setpath_up(void)
>>
>>        timer_id = g_timeout_add_seconds(1, test_timeout, &d);
>>
>> -       g_obex_setpath(obex, "..", req_complete, &d, &d.err);
>> +       g_obex_setpath(obex, "", TRUE, req_complete, &d, &d.err);
>> +       g_assert_no_error(d.err);
>> +
>> +       g_main_loop_run(d.mainloop);
>> +
>> +       g_assert_cmpuint(d.count, ==, 1);
>> +
>> +       g_main_loop_unref(d.mainloop);
>> +
>> +       g_source_remove(timer_id);
>> +       g_io_channel_unref(io);
>> +       g_source_remove(io_id);
>> +       g_obex_unref(obex);
>> +
>> +       g_assert_no_error(d.err);
>> +}
>> +
>> +static void test_setpath_up_down(void)
>> +{
>> +       GIOChannel *io;
>> +       GIOCondition cond;
>> +       guint io_id, timer_id;
>> +       GObex *obex;
>> +       struct test_data d = { 0, NULL, {
>> +                       { pkt_setpath_up_down_req,
>> +                                       sizeof(pkt_setpath_up_down_req) } }, {
>> +                       { pkt_success_rsp, sizeof(pkt_success_rsp) } } };
>> +
>> +       create_endpoints(&obex, &io, SOCK_STREAM);
>> +
>> +       cond = G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL;
>> +       io_id = g_io_add_watch(io, cond, test_io_cb, &d);
>> +
>> +       d.mainloop = g_main_loop_new(NULL, FALSE);
>> +
>> +       timer_id = g_timeout_add_seconds(1, test_timeout, &d);
>> +
>> +       g_obex_setpath(obex, "dir", TRUE, req_complete, &d, &d.err);
>>        g_assert_no_error(d.err);
>>
>>        g_main_loop_run(d.mainloop);
>> @@ -1142,6 +1183,7 @@ int main(int argc, char *argv[])
>>
>>        g_test_add_func("/gobex/test_setpath", test_setpath);
>>        g_test_add_func("/gobex/test_setpath_up", test_setpath_up);
>> +       g_test_add_func("/gobex/test_setpath_up_down", test_setpath_up_down);
>>
>>        g_test_add_func("/gobex/test_mkdir", test_mkdir);
>>
>> --
>> 1.7.4.1
>
> I don't understand why you want to carry the burden of operating in
> non standard paths? Or you are also planning to change the D-Bus API?
> OBEX spec is just broke in this aspect, it should have allowed
> standard paths and not this one level + cdup and now you are trying to
> push this broken concept up to the user, why? How many uis have you
> seem that can use do cdup + dir in practice? I don't remember seeing
> any, and if that exist that would probably need to cache the folder
> listing of the previous folder to be able to present to the user and
> trust it wont get change in the meantime.
>
> --
> Luiz Augusto von Dentz
>

Come on did You even read it?? This function is OBEX level (an it
works before in similar way - only one dir at a time) - we may don't
like specification, but seriously? Just read the patch, i exposed on
OBEX level stuff defined in the OBEX specification (as MAP needs
operation like "go one level up and than down" in one step) and as it
would be possible to just add some "if" for "../dir" in previous
version, but it is better to fix broken things than to propagate them
(it's obex level function not high level abstraction - deal with it).

Each profile may use it multiple times for paths delivered through
dbus (but it provokes some questions, that i don't like). Generally
there is REALLY many gui that works like that (one level down or up at
a time) just look around (eg all file explorers) and it's all you
need, because its basically same context!

I left profiles alone, just added extra parameter to be ok with new definition.

--
Pozdrowienia - Cheers,
Bartosz Szatkowski

2011-11-15 22:29:27

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH] gobex: Fix setpath to match one from OBEX spec

Hi Bartosz,

On Tue, Nov 15, 2011 at 4:44 PM, Bartosz Szatkowski <[email protected]> wrote:
> OBEX spec states that when bit 0 is set in flags, server should backup a
> level before applying name. It's accomplished by moving parsing of the
> path to upper layers (as gobex is low level lib and I think it should be
> based on level defined in OBEX spec, upper profiles may wrap that as
> defined in correspondent specifications) and exposing *cdup* flag in
> function parameters.
> ---
> ?client/ftp.c ? ? ?| ? ?6 +++++-
> ?client/pbap.c ? ? | ? ?4 ++--
> ?gobex/gobex.c ? ? | ? 16 ++++++++++------
> ?gobex/gobex.h ? ? | ? ?5 +++--
> ?unit/test-gobex.c | ? 46 ++++++++++++++++++++++++++++++++++++++++++++--
> ?5 files changed, 64 insertions(+), 13 deletions(-)
>
> diff --git a/client/ftp.c b/client/ftp.c
> index 336fc26..d9e31ad 100644
> --- a/client/ftp.c
> +++ b/client/ftp.c
> @@ -82,7 +82,11 @@ static DBusMessage *change_folder(DBusConnection *connection,
> ? ? ? ? ? ? ? ?return g_dbus_create_error(message,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"org.openobex.Error.InvalidArguments", NULL);
>
> - ? ? ? g_obex_setpath(obex, folder, async_cb, message, &err);
> + ? ? ? if (g_strcmp0("..", folder) == 0)
> + ? ? ? ? ? ? ? g_obex_setpath(obex, NULL, TRUE, async_cb, message, &err);
> + ? ? ? else
> + ? ? ? ? ? ? ? g_obex_setpath(obex, folder, FALSE, async_cb, message, &err);
> +
> ? ? ? ?if (err != NULL) {
> ? ? ? ? ? ? ? ?DBusMessage *reply;
> ? ? ? ? ? ? ? ?reply = ?g_dbus_create_error(message,
> diff --git a/client/pbap.c b/client/pbap.c
> index 9e9eb05..5055ad9 100644
> --- a/client/pbap.c
> +++ b/client/pbap.c
> @@ -277,7 +277,7 @@ static void setpath_cb(GObex *obex, GError *err, GObexPacket *rsp,
>
> ? ? ? ?data->index++;
>
> - ? ? ? g_obex_setpath(obex, next, setpath_cb, data, &err);
> + ? ? ? g_obex_setpath(obex, next, FALSE, setpath_cb, data, &err);
> ? ? ? ?if (err != NULL) {
> ? ? ? ? ? ? ? ?setpath_complete(err, data);
> ? ? ? ? ? ? ? ?g_error_free(err);
> @@ -292,7 +292,7 @@ static gboolean setpath(GObex *obex, const char *path, size_t max_elem,
>
> ? ? ? ?data = g_new0(struct setpath_data, 1);
>
> - ? ? ? g_obex_setpath(obex, "", setpath_cb, data, &err);
> + ? ? ? g_obex_setpath(obex, NULL, FALSE, setpath_cb, data, &err);
> ? ? ? ?if (err != NULL) {
> ? ? ? ? ? ? ? ?error("set_path: %s", err->message);
> ? ? ? ? ? ? ? ?g_error_free(err);
> diff --git a/gobex/gobex.c b/gobex/gobex.c
> index 7840304..1085e78 100644
> --- a/gobex/gobex.c
> +++ b/gobex/gobex.c
> @@ -1083,11 +1083,13 @@ guint g_obex_connect(GObex *obex, GObexResponseFunc func, gpointer user_data,
> ? ? ? ?return g_obex_send_req(obex, req, -1, func, user_data, err);
> ?}
>
> -guint g_obex_setpath(GObex *obex, const char *path, GObexResponseFunc func,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? gpointer user_data, GError **err)
> +guint g_obex_setpath(GObex *obex, const char *folder, gboolean cdup,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? GObexResponseFunc func, gpointer user_data,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? GError **err)
> ?{
> ? ? ? ?GObexPacket *req;
> ? ? ? ?struct setpath_data data;
> + ? ? ? GObexHeader *hdr;
>
> ? ? ? ?g_obex_debug(G_OBEX_DEBUG_COMMAND, "conn %u", obex->conn_id);
>
> @@ -1095,12 +1097,14 @@ guint g_obex_setpath(GObex *obex, const char *path, GObexResponseFunc func,
>
> ? ? ? ?memset(&data, 0, sizeof(data));
>
> - ? ? ? if (strcmp(path, "..") == 0)
> + ? ? ? if (cdup)
> ? ? ? ? ? ? ? ?data.flags = 0x03;
> - ? ? ? else {
> - ? ? ? ? ? ? ? GObexHeader *hdr;
> + ? ? ? else
> ? ? ? ? ? ? ? ?data.flags = 0x02;
> - ? ? ? ? ? ? ? hdr = g_obex_header_new_unicode(G_OBEX_HDR_NAME, path);
> +
> + ? ? ? if (!cdup || (folder != NULL && folder[0] != '\0')) {
> + ? ? ? ? ? ? ? hdr = g_obex_header_new_unicode(G_OBEX_HDR_NAME,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? folder != NULL ? folder : "");
> ? ? ? ? ? ? ? ?g_obex_packet_add_header(req, hdr);
> ? ? ? ?}
>
> diff --git a/gobex/gobex.h b/gobex/gobex.h
> index 1b20333..d9d470d 100644
> --- a/gobex/gobex.h
> +++ b/gobex/gobex.h
> @@ -73,8 +73,9 @@ void g_obex_unref(GObex *obex);
> ?guint g_obex_connect(GObex *obex, GObexResponseFunc func, gpointer user_data,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?GError **err, guint8 first_hdr_id, ...);
>
> -guint g_obex_setpath(GObex *obex, const char *path, GObexResponseFunc func,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? gpointer user_data, GError **err);
> +guint g_obex_setpath(GObex *obex, const char *folder, gboolean cdup,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? GObexResponseFunc func, gpointer user_data,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? GError **err);
>
> ?guint g_obex_mkdir(GObex *obex, const char *path, GObexResponseFunc func,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?gpointer user_data, GError **err);
> diff --git a/unit/test-gobex.c b/unit/test-gobex.c
> index 62443db..8b86dda 100644
> --- a/unit/test-gobex.c
> +++ b/unit/test-gobex.c
> @@ -46,6 +46,10 @@ static uint8_t pkt_setpath_req[] = { G_OBEX_OP_SETPATH | FINAL_BIT, 0x00, 0x10,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?0, 'd', 0, 'i', 0, 'r', 0, 0 };
> ?static uint8_t pkt_setpath_up_req[] = { G_OBEX_OP_SETPATH | FINAL_BIT,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?0x00, 0x05, 0x03, 0x00 };
> +static uint8_t pkt_setpath_up_down_req[] = { G_OBEX_OP_SETPATH | FINAL_BIT,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0x00, 0x10, 0x03, 0x00,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? G_OBEX_HDR_NAME, 0x00, 0x0b,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0, 'd', 0, 'i', 0, 'r', 0, 0 };
> ?static uint8_t pkt_success_rsp[] = { 0x20 | FINAL_BIT, 0x00, 0x03 };
>
> ?static uint8_t pkt_mkdir_req[] = { G_OBEX_OP_SETPATH | FINAL_BIT, 0x00, 0x10,
> @@ -890,7 +894,7 @@ static void test_setpath(void)
>
> ? ? ? ?timer_id = g_timeout_add_seconds(1, test_timeout, &d);
>
> - ? ? ? g_obex_setpath(obex, "dir", req_complete, &d, &d.err);
> + ? ? ? g_obex_setpath(obex, "dir", FALSE, req_complete, &d, &d.err);
> ? ? ? ?g_assert_no_error(d.err);
>
> ? ? ? ?g_main_loop_run(d.mainloop);
> @@ -926,7 +930,44 @@ static void test_setpath_up(void)
>
> ? ? ? ?timer_id = g_timeout_add_seconds(1, test_timeout, &d);
>
> - ? ? ? g_obex_setpath(obex, "..", req_complete, &d, &d.err);
> + ? ? ? g_obex_setpath(obex, "", TRUE, req_complete, &d, &d.err);
> + ? ? ? g_assert_no_error(d.err);
> +
> + ? ? ? g_main_loop_run(d.mainloop);
> +
> + ? ? ? g_assert_cmpuint(d.count, ==, 1);
> +
> + ? ? ? g_main_loop_unref(d.mainloop);
> +
> + ? ? ? g_source_remove(timer_id);
> + ? ? ? g_io_channel_unref(io);
> + ? ? ? g_source_remove(io_id);
> + ? ? ? g_obex_unref(obex);
> +
> + ? ? ? g_assert_no_error(d.err);
> +}
> +
> +static void test_setpath_up_down(void)
> +{
> + ? ? ? GIOChannel *io;
> + ? ? ? GIOCondition cond;
> + ? ? ? guint io_id, timer_id;
> + ? ? ? GObex *obex;
> + ? ? ? struct test_data d = { 0, NULL, {
> + ? ? ? ? ? ? ? ? ? ? ? { pkt_setpath_up_down_req,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? sizeof(pkt_setpath_up_down_req) } }, {
> + ? ? ? ? ? ? ? ? ? ? ? { pkt_success_rsp, sizeof(pkt_success_rsp) } } };
> +
> + ? ? ? create_endpoints(&obex, &io, SOCK_STREAM);
> +
> + ? ? ? cond = G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL;
> + ? ? ? io_id = g_io_add_watch(io, cond, test_io_cb, &d);
> +
> + ? ? ? d.mainloop = g_main_loop_new(NULL, FALSE);
> +
> + ? ? ? timer_id = g_timeout_add_seconds(1, test_timeout, &d);
> +
> + ? ? ? g_obex_setpath(obex, "dir", TRUE, req_complete, &d, &d.err);
> ? ? ? ?g_assert_no_error(d.err);
>
> ? ? ? ?g_main_loop_run(d.mainloop);
> @@ -1142,6 +1183,7 @@ int main(int argc, char *argv[])
>
> ? ? ? ?g_test_add_func("/gobex/test_setpath", test_setpath);
> ? ? ? ?g_test_add_func("/gobex/test_setpath_up", test_setpath_up);
> + ? ? ? g_test_add_func("/gobex/test_setpath_up_down", test_setpath_up_down);
>
> ? ? ? ?g_test_add_func("/gobex/test_mkdir", test_mkdir);
>
> --
> 1.7.4.1

I don't understand why you want to carry the burden of operating in
non standard paths? Or you are also planning to change the D-Bus API?
OBEX spec is just broke in this aspect, it should have allowed
standard paths and not this one level + cdup and now you are trying to
push this broken concept up to the user, why? How many uis have you
seem that can use do cdup + dir in practice? I don't remember seeing
any, and if that exist that would probably need to cache the folder
listing of the previous folder to be able to present to the user and
trust it wont get change in the meantime.

--
Luiz Augusto von Dentz