[PATCH obexd 1/4] Add initial support for OBEX Action command

[PATCH obexd 1/4] Add initial support for OBEX Action command

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

---
 src/mimetype.h  |    2 +
 src/obex-priv.h |    2 +
 src/obex.c      |  142 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/obex.h      |    6 ++
 src/service.h   |    2 +
 5 files changed, 154 insertions(+), 0 deletions(-)

diff --git a/src/mimetype.h b/src/mimetype.h
index 8472684..f3a2b34 100644
--- a/src/mimetype.h
+++ b/src/mimetype.h
@@ -36,6 +36,8 @@ struct obex_mime_type_driver {
 	ssize_t (*read) (void *object, void *buf, size_t count, uint8_t *hi);
 	ssize_t (*write) (void *object, const void *buf, size_t count);
 	int (*flush) (void *object);
+	int (*copy) (const char *name, const char *destname);
+	int (*move) (const char *name, const char *destname);
 	int (*remove) (const char *name);
 	int (*set_io_watch) (void *object, obex_object_io_func func,
 				void *user_data);
diff --git a/src/obex-priv.h b/src/obex-priv.h
index 0caa0dd..8fcd5d1 100644
--- a/src/obex-priv.h
+++ b/src/obex-priv.h
@@ -28,7 +28,9 @@ struct obex_session {
 	uint16_t tx_mtu;
 	uint16_t rx_mtu;
 	uint8_t cmd;
+	uint8_t action_id;
 	char *name;
+	char *destname;
 	char *type;
 	char *path;
 	time_t time;
diff --git a/src/obex.c b/src/obex.c
index e6585ca..49d9c18 100644
--- a/src/obex.c
+++ b/src/obex.c
@@ -52,6 +52,13 @@
 #include "transport.h"
 #include "btio.h"
 
+#ifndef OBEX_CMD_ACTION
+#define OBEX_CMD_ACTION 0x06
+#define OBEX_HDR_ACTION_ID 0x94
+#define OBEX_HDR_DESTNAME 0x15
+#define OBEX_HDR_PERMISSIONS 0xD6
+#endif /* OBEX_CMD_ACTION */
+
 /* Default MTU's */
 #define DEFAULT_RX_MTU 32767
 #define DEFAULT_TX_MTU 32767
@@ -128,6 +135,7 @@ static struct {
 	{ OBEX_CMD_SETPATH,	"SETPATH"	},
 	{ OBEX_CMD_SESSION,	"SESSION"	},
 	{ OBEX_CMD_ABORT,	"ABORT"		},
+	{ OBEX_CMD_ACTION,	"ACTION"	},
 	{ OBEX_FINAL,		"FINAL"		},
 	{ 0xFF,			NULL		},
 };
@@ -1096,6 +1104,104 @@ static void cmd_put(struct obex_session *os, obex_t *obex, obex_object_t *obj)
 	}
 }
 
+static void cmd_action(struct obex_session *os, obex_t *obex,
+							obex_object_t *obj)
+{
+	obex_headerdata_t hd;
+	unsigned int hlen;
+	uint8_t hi;
+	int err;
+
+	if (!os->service) {
+		OBEX_ObjectSetRsp(obj, OBEX_RSP_FORBIDDEN, OBEX_RSP_FORBIDDEN);
+		return;
+	} else if (!os->service->action) {
+		OBEX_ObjectSetRsp(obj, OBEX_RSP_NOT_IMPLEMENTED,
+				OBEX_RSP_NOT_IMPLEMENTED);
+		return;
+	}
+
+	g_return_if_fail(chk_cid(obex, obj, os->cid));
+
+	if (os->name) {
+		g_free(os->name);
+		os->name = NULL;
+	}
+
+	if (os->destname) {
+		g_free(os->destname);
+		os->destname = NULL;
+	}
+
+	while (OBEX_ObjectGetNextHeader(obex, obj, &hi, &hd, &hlen)) {
+		switch (hi) {
+		case OBEX_HDR_NAME:
+			if (os->name) {
+				DBG("Ignoring multiple name headers");
+				break;
+			}
+
+			if (hlen == 0)
+				continue;
+
+			os->name = g_convert((const char *) hd.bs, hlen,
+					"UTF8", "UTF16BE", NULL, NULL, NULL);
+			DBG("OBEX_HDR_NAME: %s", os->name);
+			break;
+
+		case OBEX_HDR_DESTNAME:
+			if (os->destname) {
+				DBG("Ignoring multiple destination headers");
+				break;
+			}
+
+			if (hlen == 0)
+				continue;
+
+			os->destname = g_convert((const char *) hd.bs, hlen,
+					"UTF8", "UTF16BE", NULL, NULL, NULL);
+			DBG("OBEX_HDR_DESTNAME: %s", os->destname);
+			break;
+
+		case OBEX_HDR_ACTION_ID:
+			if (hlen == 0)
+				continue;
+
+			os->action_id = hd.bq1;
+
+			DBG("OBEX_HDR_ACTIONID: %u", os->action_id);
+			break;
+
+		case OBEX_HDR_PERMISSIONS:
+			if (hlen == 0)
+				continue;
+
+			DBG("OBEX_HDR_PERMISSIONS: %d", hd.bq4);
+			break;
+		}
+	}
+
+	os->driver = obex_mime_type_driver_find(os->service->target,
+						os->service->target_size,
+						NULL,
+						os->service->who,
+						os->service->who_size);
+
+	if (!os->driver || !os->service->action) {
+		OBEX_ObjectSetRsp(obj, OBEX_RSP_NOT_IMPLEMENTED,
+				OBEX_RSP_NOT_IMPLEMENTED);
+		return;
+	}
+
+	err = os->service->action(os, obj, os->service_data);
+	if (err < 0) {
+		os_set_response(obj, err);
+		return;
+	}
+
+	return;
+}
+
 static void obex_event_cb(obex_t *obex, obex_object_t *obj, int mode,
 					int evt, int cmd, int rsp)
 {
@@ -1141,6 +1247,7 @@ static void obex_event_cb(obex_t *obex, obex_object_t *obj, int mode,
 		case OBEX_CMD_SETPATH:
 		case OBEX_CMD_CONNECT:
 		case OBEX_CMD_DISCONNECT:
+		case OBEX_CMD_ACTION:
 			OBEX_ObjectSetRsp(obj, OBEX_RSP_CONTINUE,
 					OBEX_RSP_SUCCESS);
 			break;
@@ -1176,6 +1283,9 @@ static void obex_event_cb(obex_t *obex, obex_object_t *obj, int mode,
 		case OBEX_CMD_PUT:
 			cmd_put(os, obex, obj);
 			break;
+		case OBEX_CMD_ACTION:
+			cmd_action(os, obex, obj);
+			break;
 		default:
 			DBG("Unknown request: 0x%X", cmd);
 			OBEX_ObjectSetRsp(obj, OBEX_RSP_NOT_IMPLEMENTED,
@@ -1309,6 +1419,11 @@ const char *obex_get_name(struct obex_session *os)
 	return os->name;
 }
 
+const char *obex_get_destname(struct obex_session *os)
+{
+	return os->destname;
+}
+
 void obex_set_name(struct obex_session *os, const char *name)
 {
 	g_free(os->name);
@@ -1359,6 +1474,33 @@ int obex_remove(struct obex_session *os, const char *path)
 	return os->driver->remove(path);
 }
 
+int obex_copy(struct obex_session *os, const char *source,
+						const char *destination)
+{
+	if (os->driver == NULL || os->driver->copy == NULL)
+		return -EINVAL;
+
+	DBG("%s %s", source, destination);
+
+	return os->driver->copy(source, destination);
+}
+
+int obex_move(struct obex_session *os, const char *source,
+						const char *destination)
+{
+	if (os->driver == NULL || os->driver->move == NULL)
+		return -EINVAL;
+
+	DBG("%s %s", source, destination);
+
+	return os->driver->move(source, destination);
+}
+
+uint8_t obex_get_action_id(struct obex_session *os)
+{
+	return os->action_id;
+}
+
 /* TODO: find a way to do this for tty or fix syncevolution */
 char *obex_get_id(struct obex_session *os)
 {
diff --git a/src/obex.h b/src/obex.h
index 94274c2..03243f1 100644
--- a/src/obex.h
+++ b/src/obex.h
@@ -47,6 +47,7 @@ void obex_connect_cb(GIOChannel *io, GError *err, void *user_data);
 int obex_get_stream_start(struct obex_session *os, const char *filename);
 int obex_put_stream_start(struct obex_session *os, const char *filename);
 const char *obex_get_name(struct obex_session *os);
+const char *obex_get_destname(struct obex_session *os);
 void obex_set_name(struct obex_session *os, const char *name);
 ssize_t obex_get_size(struct obex_session *os);
 const char *obex_get_type(struct obex_session *os);
@@ -56,6 +57,11 @@ gboolean obex_get_symlinks(struct obex_session *os);
 const char *obex_get_capability_path(struct obex_session *os);
 gboolean obex_get_auto_accept(struct obex_session *os);
 int obex_remove(struct obex_session *os, const char *path);
+int obex_copy(struct obex_session *os, const char *source,
+						const char *destination);
+int obex_move(struct obex_session *os, const char *source,
+						const char *destination);
+uint8_t obex_get_action_id(struct obex_session *os);
 char *obex_get_id(struct obex_session *os);
 ssize_t obex_aparam_read(struct obex_session *os, obex_object_t *obj,
 						const uint8_t **buffer);
diff --git a/src/service.h b/src/service.h
index a844885..61c0dcc 100644
--- a/src/service.h
+++ b/src/service.h
@@ -39,6 +39,8 @@ struct obex_service_driver {
 	int (*chkput) (struct obex_session *os, void *user_data);
 	int (*setpath) (struct obex_session *os, obex_object_t *obj,
 							void *user_data);
+	int (*action) (struct obex_session *os, obex_object_t *obj,
+							void *user_data);
 	void (*disconnect) (struct obex_session *os, void *user_data);
 	void (*reset) (struct obex_session *os, void *user_data);
 };
-- 
1.7.5.4

[PATCH obexd 2/4] Add basic support for action commands on ftp driver

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This add basic support for copy and move actions for ftp service driver
---
 plugins/ftp.c |   99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 99 insertions(+), 0 deletions(-)

diff --git a/plugins/ftp.c b/plugins/ftp.c
index 79223bf..7e2f1e4 100644
--- a/plugins/ftp.c
+++ b/plugins/ftp.c
@@ -403,6 +403,104 @@ done:
 	return err;
 }
 
+static char *ftp_build_filename(struct ftp_session *ftp, const char *destname)
+{
+	char *filename;
+
+	/* DestName can either be relative or absolute (FTP style) */
+	if (g_path_is_absolute(destname))
+		filename = g_build_filename(destname, NULL);
+	else
+		filename = g_build_filename(ftp->folder, destname, NULL);
+
+	/* Check if destination is inside root path */
+	if (g_str_has_prefix(filename, ftp->folder))
+		return filename;
+
+	g_free(filename);
+
+	return NULL;
+}
+
+static int ftp_copy(struct ftp_session *ftp, const char *name,
+							const char *destname)
+{
+	char *source, *destination;
+	int ret;
+
+	DBG("%p name %s destination %s", ftp, name, destname);
+
+	if (ftp->folder == NULL) {
+		error("No folder set");
+		return -ENOENT;
+	}
+
+	if (name == NULL || destname == NULL)
+		return -EINVAL;
+
+	destination = ftp_build_filename(ftp, destname);
+
+	source = g_build_filename(ftp->folder, name, NULL);
+
+	ret = obex_copy(ftp->os, source, destination);
+
+	g_free(source);
+	g_free(destination);
+
+	return ret;
+}
+
+static int ftp_move(struct ftp_session *ftp, const char *name,
+							const char *destname)
+{
+	char *source, *destination;
+	int ret;
+
+	DBG("%p name %s destname %s", ftp, name, destname);
+
+	if (ftp->folder == NULL) {
+		error("No folder set");
+		return -ENOENT;
+	}
+
+	if (name == NULL || destname == NULL)
+		return -EINVAL;
+
+	destination = ftp_build_filename(ftp, destname);
+
+	source = g_build_filename(ftp->folder, name, NULL);
+
+	ret = obex_move(ftp->os, source, destination);
+
+	g_free(source);
+	g_free(destination);
+
+	return ret;
+}
+
+static int ftp_action(struct obex_session *os, obex_object_t *obj,
+							void *user_data)
+{
+	struct ftp_session *ftp = user_data;
+	const char *name, *destname;
+	uint8_t action_id;
+
+	name = obex_get_name(os);
+	destname = obex_get_destname(os);
+	action_id = obex_get_action_id(os);
+
+	DBG("%p action 0x%x", ftp, action_id);
+
+	switch (action_id) {
+	case 0x00: /* Copy Object */
+		return ftp_copy(ftp, name, destname);
+	case 0x01: /* Move/Rename Object */
+		return ftp_move(ftp, name, destname);
+	default:
+		return -EINVAL;
+	}
+}
+
 void ftp_disconnect(struct obex_session *os, void *user_data)
 {
 	struct ftp_session *ftp = user_data;
@@ -427,6 +525,7 @@ static struct obex_service_driver ftp = {
 	.put = ftp_put,
 	.chkput = ftp_chkput,
 	.setpath = ftp_setpath,
+	.action = ftp_action,
 	.disconnect = ftp_disconnect
 };
 
-- 
1.7.5.4

[PATCH obexd 3/4] Add support for Action command to pcsuite plugin

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

PCSuite service relies on ftp driver
---
 plugins/ftp.c     |    3 +--
 plugins/ftp.h     |    1 +
 plugins/pcsuite.c |   11 +++++++++++
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/plugins/ftp.c b/plugins/ftp.c
index 7e2f1e4..4cf0135 100644
--- a/plugins/ftp.c
+++ b/plugins/ftp.c
@@ -478,8 +478,7 @@ static int ftp_move(struct ftp_session *ftp, const char *name,
 	return ret;
 }
 
-static int ftp_action(struct obex_session *os, obex_object_t *obj,
-							void *user_data)
+int ftp_action(struct obex_session *os, obex_object_t *obj, void *user_data)
 {
 	struct ftp_session *ftp = user_data;
 	const char *name, *destname;
diff --git a/plugins/ftp.h b/plugins/ftp.h
index 2374125..710bfa4 100644
--- a/plugins/ftp.h
+++ b/plugins/ftp.h
@@ -28,3 +28,4 @@ int ftp_chkput(struct obex_session *os, void *user_data);
 int ftp_put(struct obex_session *os, obex_object_t *obj, void *user_data);
 int ftp_setpath(struct obex_session *os, obex_object_t *obj, void *user_data);
 void ftp_disconnect(struct obex_session *os, void *user_data);
+int ftp_action(struct obex_session *os, obex_object_t *obj, void *user_data);
diff --git a/plugins/pcsuite.c b/plugins/pcsuite.c
index 5545917..125f25a 100644
--- a/plugins/pcsuite.c
+++ b/plugins/pcsuite.c
@@ -219,6 +219,16 @@ static int pcsuite_setpath(struct obex_session *os, obex_object_t *obj,
 	return ftp_setpath(os, obj, pcsuite->ftp);
 }
 
+static int pcsuite_action(struct obex_session *os, obex_object_t *obj,
+							void *user_data)
+{
+	struct pcsuite_session *pcsuite = user_data;
+
+	DBG("%p", pcsuite);
+
+	return ftp_action(os, obj, pcsuite->ftp);
+}
+
 static void pcsuite_disconnect(struct obex_session *os, void *user_data)
 {
 	struct pcsuite_session *pcsuite = user_data;
@@ -253,6 +263,7 @@ static struct obex_service_driver pcsuite = {
 	.put = pcsuite_put,
 	.chkput = pcsuite_chkput,
 	.setpath = pcsuite_setpath,
+	.action = pcsuite_action,
 	.disconnect = pcsuite_disconnect
 };
 
-- 
1.7.5.4

[PATCH obexd 4/4] Add copy and move support for filesystem plugin

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Move is implemented using rename and copy uses sendfile, both part of
posix.
---
 plugins/filesystem.c |   93 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 93 insertions(+), 0 deletions(-)

diff --git a/plugins/filesystem.c b/plugins/filesystem.c
index b4ff556..53e7e94 100644
--- a/plugins/filesystem.c
+++ b/plugins/filesystem.c
@@ -36,6 +36,7 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <sys/statvfs.h>
+#include <sys/sendfile.h>
 #include <fcntl.h>
 #include <wait.h>
 
@@ -234,6 +235,96 @@ static ssize_t filesystem_write(void *object, const void *buf, size_t count)
 	return ret;
 }
 
+static int filesystem_rename(const char *name, const char *destname)
+{
+	int ret;
+
+	ret = rename(name, destname);
+	if (ret < 0) {
+		error("rename(%s, %s): %s (%d)", name, destname,
+						strerror(errno), errno);
+		return -errno;
+	}
+
+	return ret;
+}
+
+static int sendfile_async(void *out, void *in, off_t *offset, size_t count)
+{
+	int pid;
+
+	/* Run sendfile on child process */
+	pid = fork();
+	switch (pid) {
+		case 0:
+			break;
+		case -1:
+			error("fork() %s (%d)", strerror(errno), errno);
+			return -errno;
+		default:
+			DBG("child %d forked", pid);
+			return pid;
+	}
+
+	/* At child */
+	if (sendfile(GPOINTER_TO_INT(out), GPOINTER_TO_INT(in), offset, count) < 0)
+		error("sendfile(): %s (%d)", strerror(errno), errno);
+
+	filesystem_close(in);
+	filesystem_close(out);
+
+	exit(errno);
+}
+
+static int filesystem_copy(const char *name, const char *destname)
+{
+	void *in, *out;
+	ssize_t ret;
+	size_t size;
+	struct stat fstat;
+	int err;
+
+	in = filesystem_open(name, O_RDONLY, 0, NULL, &size, &err);
+	if (in == NULL) {
+		error("open(%s): %s (%d)", name, strerror(-err), -err);
+		return -err;
+	}
+
+	ret = stat(name, &fstat);
+	if (ret < 0) {
+		error("stat(%s): %s (%d)", name, strerror(errno), errno);
+		return -errno;
+	}
+
+	out = filesystem_open(destname, O_WRONLY | O_CREAT | O_TRUNC,
+					fstat.st_mode, NULL, &size, &err);
+	if (out == NULL) {
+		error("open(%s): %s (%d)", destname, strerror(-err), -err);
+		filesystem_close(in);
+		return -errno;
+	}
+
+	/* Check if sendfile is supported */
+	ret = sendfile(GPOINTER_TO_INT(out), GPOINTER_TO_INT(in), NULL, 0);
+	if (ret < 0) {
+		ret = -errno;
+		error("sendfile: %s (%zd)", strerror(-ret), -ret);
+		goto done;
+	}
+
+	ret = sendfile_async(out, in, NULL, fstat.st_size);
+	if (ret < 0)
+		goto done;
+
+	return 0;
+
+done:
+	filesystem_close(in);
+	filesystem_close(out);
+
+	return ret;
+}
+
 struct capability_object {
 	int pid;
 	int output;
@@ -555,6 +646,8 @@ static struct obex_mime_type_driver file = {
 	.read = filesystem_read,
 	.write = filesystem_write,
 	.remove = remove,
+	.move = filesystem_rename,
+	.copy = filesystem_copy,
 };
 
 static struct obex_mime_type_driver capability = {
-- 
1.7.5.4

Re: [PATCH obexd 2/4] Add basic support for action commands on ftp driver

[Hendrik Sattler]

Zitat von Luiz Augusto von Dentz <luiz.dentz@gmail.com>:
> +static char *ftp_build_filename(struct ftp_session *ftp, const char  
> *destname)
> +{
> +	char *filename;
> +
> +	/* DestName can either be relative or absolute (FTP style) */
> +	if (g_path_is_absolute(destname))
> +		filename = g_build_filename(destname, NULL);
> +	else
> +		filename = g_build_filename(ftp->folder, destname, NULL);
> +
> +	/* Check if destination is inside root path */
> +	if (g_str_has_prefix(filename, ftp->folder))
> +		return filename;
> +
> +	g_free(filename);
> +
> +	return NULL;
> +}

Except that this interprets the destination header in a wrong way:
Absolute destination means that it is based on the path that you  
export to the _client_ as root, not the local filesystem root.

You should also catch stuff like the UNC path stuff and drive letter  
prefixes. No need to support all the sick stuff :-/

HS

Re: [PATCH obexd 4/4] Add copy and move support for filesystem plugin

[Hendrik Sattler]

Zitat von Luiz Augusto von Dentz <luiz.dentz@gmail.com>:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> Move is implemented using rename and copy uses sendfile, both part of
> posix.

Did you ever take a look at the "cp" implementation in coreutils? [1]
Additionally, the obex spec says that the copy/move actions also  
copies/moves directories recursively.

HS

[1]: http://git.savannah.gnu.org/cgit/coreutils.git/tree/src/copy.c

Re: [PATCH obexd 4/4] Add copy and move support for filesystem plugin

[Johan Hedberg]

Hi Hendrik,

On Fri, Jun 17, 2011, Hendrik Sattler wrote:
> Additionally, the obex spec says that the copy/move actions also
> copies/moves directories recursively.

Where exactly do you see that? To my understanding Action isn't intended
for directories at all. E.g. in section 3.4.8.1 of IrOBEX 1.4 there's
just the following:

"The Name header specifies the source file name and the DestName header
specifies the destination file name. These two headers are mandatory for
this action."

I.e. it doesn't talk about directories at all.

Johan

Re: [PATCH obexd 4/4] Add copy and move support for filesystem plugin

[Luiz Augusto von Dentz]

Hi Hendric,

On Fri, Jun 17, 2011 at 1:28 PM, Hendrik Sattler
<post@hendrik-sattler.de> wrote:
> Zitat von Luiz Augusto von Dentz <luiz.dentz@gmail.com>:
>>
>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>
>> Move is implemented using rename and copy uses sendfile, both part of
>> posix.
>
> Did you ever take a look at the "cp" implementation in coreutils? [1]
> Additionally, the obex spec says that the copy/move actions also
> copies/moves directories recursively.

Well need to check in the spec because for deleting recursively is not
mandatory so I supposed the copy/move wouldn't be either.  As for cp,
I did look, obviously I won't incorporate that much of code in obexd
and sendfile does actually do a lot of the checks there without
copying the data to the userspace. So what really is your point here,
that I should call system cp instead of sendfile?

-- 
Luiz Augusto von Dentz

Re: [PATCH obexd 2/4] Add basic support for action commands on ftp driver

[Luiz Augusto von Dentz]

Hi Hendrik,

On Fri, Jun 17, 2011 at 1:19 PM, Hendrik Sattler
<post@hendrik-sattler.de> wrote:
> Zitat von Luiz Augusto von Dentz <luiz.dentz@gmail.com>:
>>
>> +static char *ftp_build_filename(struct ftp_session *ftp, const char
>> *destname)
>> +{
>> +       char *filename;
>> +
>> +       /* DestName can either be relative or absolute (FTP style) */
>> +       if (g_path_is_absolute(destname))
>> +               filename = g_build_filename(destname, NULL);
>> +       else
>> +               filename = g_build_filename(ftp->folder, destname, NULL);
>> +
>> +       /* Check if destination is inside root path */
>> +       if (g_str_has_prefix(filename, ftp->folder))
>> +               return filename;
>> +
>> +       g_free(filename);
>> +
>> +       return NULL;
>> +}
>
> Except that this interprets the destination header in a wrong way:
> Absolute destination means that it is based on the path that you export to
> the _client_ as root, not the local filesystem root.

This one is actually a valid point, I will fix it.

> You should also catch stuff like the UNC path stuff and drive letter
> prefixes. No need to support all the sick stuff :-/

If g_build_filename doesn't care I don't see why we should care.

-- 
Luiz Augusto von Dentz

Re: [PATCH obexd 4/4] Add copy and move support for filesystem plugin

[Hendrik Sattler]

Zitat von Johan Hedberg <johan.hedberg@gmail.com>:

> Hi Hendrik,
>
> On Fri, Jun 17, 2011, Hendrik Sattler wrote:
>> Additionally, the obex spec says that the copy/move actions also
>> copies/moves directories recursively.
>
> Where exactly do you see that? To my understanding Action isn't intended
> for directories at all. E.g. in section 3.4.8.1 of IrOBEX 1.4 there's
> just the following:

See section 3.4.8.4:
"A Move Action of a folder should also move the contents of all the  
subfolders beneath it."
"A Copy Action of a folder should also copy the contents of all the  
subfolders beneath it."

HS

Re: [PATCH obexd 2/4] Add basic support for action commands on ftp driver

[Hendrik Sattler]

Hi,

Zitat von Luiz Augusto von Dentz <luiz.dentz@gmail.com>:
> On Fri, Jun 17, 2011 at 1:19 PM, Hendrik Sattler
> <post@hendrik-sattler.de> wrote:
>> Zitat von Luiz Augusto von Dentz <luiz.dentz@gmail.com>:
>>>
>>> +static char *ftp_build_filename(struct ftp_session *ftp, const char
>>> *destname)
>>> +{
>>> +       char *filename;
>>> +
>>> +       /* DestName can either be relative or absolute (FTP style) */
>>> +       if (g_path_is_absolute(destname))
>>> +               filename = g_build_filename(destname, NULL);
>>> +       else
>>> +               filename = g_build_filename(ftp->folder, destname, NULL);
>>> +
>>> +       /* Check if destination is inside root path */
>>> +       if (g_str_has_prefix(filename, ftp->folder))
>>> +               return filename;
>>> +
>>> +       g_free(filename);
>>> +
>>> +       return NULL;
>>> +}
>>
>> You should also catch stuff like the UNC path stuff and drive letter
>> prefixes. No need to support all the sick stuff :-/
>
> If g_build_filename doesn't care I don't see why we should care.

Because strings like "\\..." and "C:\..." may return 0 with  
g_path_is_absolute(destname) and you then use a wrong directory that  
is not according to spec.

HS

Re: [PATCH obexd 4/4] Add copy and move support for filesystem plugin

[Johan Hedberg]

Hi Hendrik,

On Fri, Jun 17, 2011, Hendrik Sattler wrote:
> >On Fri, Jun 17, 2011, Hendrik Sattler wrote:
> >>Additionally, the obex spec says that the copy/move actions also
> >>copies/moves directories recursively.
> >
> >Where exactly do you see that? To my understanding Action isn't intended
> >for directories at all. E.g. in section 3.4.8.1 of IrOBEX 1.4 there's
> >just the following:
> 
> See section 3.4.8.4:
> "A Move Action of a folder should also move the contents of all the
> subfolders beneath it."
> "A Copy Action of a folder should also copy the contents of all the
> subfolders beneath it."

Thanks. I should just have read a little bit further. However, since the
spec. is using the term "should" here we could at least for initial
implementation simplicity return simply FORBIDDEN for folders (at least
for copy).

Johan

Re: [PATCH obexd 4/4] Add copy and move support for filesystem plugin

[Johan Hedberg]

Hi Luiz,

On Fri, Jun 17, 2011, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> Move is implemented using rename and copy uses sendfile, both part of
> posix.
> ---
>  plugins/filesystem.c |   93 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 93 insertions(+), 0 deletions(-)

The first three patches have been pushed upstream, but could you please
fix a few things with this one:

> +static int filesystem_copy(const char *name, const char *destname)
> +{
> +	void *in, *out;
> +	ssize_t ret;
> +	size_t size;
> +	struct stat fstat;
> +	int err;
> +
> +	in = filesystem_open(name, O_RDONLY, 0, NULL, &size, &err);
> +	if (in == NULL) {
> +		error("open(%s): %s (%d)", name, strerror(-err), -err);
> +		return -err;
> +	}

Could you create local int type variables here (in_fd and out_fd) so
that it's clear what filesystem_open has returned and you don't need to
do the GPOINTER_TO_INT cast in every place.

> +	ret = stat(name, &fstat);

Since you already have the fd, please use fstat instead.

Johan