On Tue, Nov 15, 2011 at 11:29 PM, Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
> Hi Bartosz,
>
> On Tue, Nov 15, 2011 at 4:44 PM, Bartosz Szatkowski <bulislaw@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
