diff mbox

[OpenWrt-Devel,1/3,rpcd,v2] file: add support for base64

Message ID 1431296789-11294-1-git-send-email-luka@openwrt.org
State Changes Requested
Headers show

Commit Message

Luka Perkov May 10, 2015, 10:26 p.m. UTC 
Signed-off-by: Luka Perkov <luka@openwrt.org>
---
=> changes in v2:

Use new libubox base64 provided API.

 file.c | 118 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 107 insertions(+), 11 deletions(-)

Comments

Felix Fietkau May 11, 2015, 9:36 a.m. UTC | #1 
On 2015-05-11 00:26, Luka Perkov wrote:
> Signed-off-by: Luka Perkov <luka@openwrt.org>
> ---
> => changes in v2:
> 
> Use new libubox base64 provided API.
> 
>  file.c | 118 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 107 insertions(+), 11 deletions(-)
> 
> diff --git a/file.c b/file.c
> index 9c1b301..c3671bb 100644
> --- a/file.c
> +++ b/file.c
> @@ -182,7 +206,17 @@ rpc_file_read(struct ubus_context *ctx, struct ubus_object *obj,
>  
>  	blob_buf_init(&buf, 0);
>  
> -	wbuf = blobmsg_alloc_string_buffer(&buf, "data", s.st_size + 1);
> +	if (tb[RPC_F_RB_BASE64])
> +		base64 = blobmsg_get_bool(tb[RPC_F_RB_BASE64]);
> +
> +	if (base64)
> +	{
> +		wbuf = blobmsg_alloc_string_buffer(&buf, "data", B64_ENCODE_LEN(s.st_size));
> +	}
> +	else
> +	{
> +		wbuf = blobmsg_alloc_string_buffer(&buf, "data", s.st_size + 1);
> +	}
How about using the 'len' variable to avoid duplicating most of the code
here. You can also get rid of unnecessary {} lines.

> @@ -196,14 +230,35 @@ rpc_file_read(struct ubus_context *ctx, struct ubus_object *obj,
>  		goto out;
>  	}
>  
> +	if (base64)
> +	{
> +		uint8_t *data = calloc(B64_ENCODE_LEN(len), sizeof(uint8_t));
> +		if (!data)
> +		{
> +			rv = UBUS_STATUS_UNKNOWN_ERROR;
> +			goto out;
> +		}
You can reduce allocation/copy size if you copy wbuf to a temporary
buffer and then use it as output for b64_encode.

> +		len = b64_encode(wbuf, len, data, B64_ENCODE_LEN(len));
> +		if (len < 0)
> +		{
> +			free(data);
> +			rv = UBUS_STATUS_UNKNOWN_ERROR;
> +			goto out;
> +		}
> +
> +		memcpy(wbuf, data, len);
> +		free(data);
> +	}
> +
>  	*(wbuf + len) = 0;
>  	blobmsg_add_string_buffer(&buf);
>  
>  	ubus_send_reply(ctx, req, buf.head);
> -	blob_buf_free(&buf);
>  	rv = UBUS_STATUS_OK;
>  
>  out:
> +	blob_buf_free(&buf);
>  	close(fd);
>  	return rv;
>  }
> @@ -222,18 +282,54 @@ rpc_file_write(struct ubus_context *ctx, struct ubus_object *obj,
>  	if (!tb[RPC_F_RW_PATH] || !tb[RPC_F_RW_DATA])
>  		return UBUS_STATUS_INVALID_ARGUMENT;
>  
> +	data = blobmsg_data(tb[RPC_F_RW_DATA]);
> +	data_len = blobmsg_data_len(tb[RPC_F_RW_DATA]) - 1;
> +
>  	if ((fd = open(blobmsg_data(tb[RPC_F_RW_PATH]), O_CREAT | O_TRUNC | O_WRONLY, 0666)) < 0)
>  		return rpc_errno_status();
>  
> -	if (write(fd, blobmsg_data(tb[RPC_F_RW_DATA]), blobmsg_data_len(tb[RPC_F_RW_DATA])) < 0)
> -		return rpc_errno_status();
> +	if (tb[RPC_F_RW_BASE64])
> +		base64 = blobmsg_get_bool(tb[RPC_F_RW_BASE64]);
> +
> +	if (base64)
> +	{
> +		rbuf_len = B64_DECODE_LEN(data_len);
> +		rbuf = calloc(rbuf_len, sizeof(uint8_t));
> +		if (!rbuf)
> +		{
> +			rv = UBUS_STATUS_UNKNOWN_ERROR;
> +			goto out;
> +		}
> +
> +		rbuf_len = b64_decode(data, rbuf, rbuf_len);
> +		if (rbuf_len < 0)
> +		{
> +			rv = UBUS_STATUS_UNKNOWN_ERROR;
> +			goto out;
> +		}
> +	}
> +	else
> +	{
> +		rbuf = data;
> +		rbuf_len = data_len;
> +	}
It is safe to overwrite the data area in this function, as long as you
don't overstep attribute bounds. This means you can reuse the input
buffer as output buffer for b64_decode and get rid of the temporary
allocation.

- Felix
Luka Perkov May 11, 2015, 9:26 p.m. UTC | #2 
Hi Felix,

On Mon, May 11, 2015 at 11:36:46AM +0200, Felix Fietkau wrote:
> On 2015-05-11 00:26, Luka Perkov wrote:
> > Signed-off-by: Luka Perkov <luka@openwrt.org>
> > ---
> > => changes in v2:
> > 
> > Use new libubox base64 provided API.
> > 
> >  file.c | 118 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 107 insertions(+), 11 deletions(-)
> > 
> > diff --git a/file.c b/file.c
> > index 9c1b301..c3671bb 100644
> > --- a/file.c
> > +++ b/file.c
> > @@ -182,7 +206,17 @@ rpc_file_read(struct ubus_context *ctx, struct ubus_object *obj,
> >  
> >  	blob_buf_init(&buf, 0);
> >  
> > -	wbuf = blobmsg_alloc_string_buffer(&buf, "data", s.st_size + 1);
> > +	if (tb[RPC_F_RB_BASE64])
> > +		base64 = blobmsg_get_bool(tb[RPC_F_RB_BASE64]);
> > +
> > +	if (base64)
> > +	{
> > +		wbuf = blobmsg_alloc_string_buffer(&buf, "data", B64_ENCODE_LEN(s.st_size));
> > +	}
> > +	else
> > +	{
> > +		wbuf = blobmsg_alloc_string_buffer(&buf, "data", s.st_size + 1);
> > +	}
> How about using the 'len' variable to avoid duplicating most of the code
> here.

Can you be more specific here please? I don't see how by using 'len' we
can reduce more code here.

> You can also get rid of unnecessary {} lines.

I have fixed this and all other comments below. New series will follow
shortly.

Luka


> 
> > @@ -196,14 +230,35 @@ rpc_file_read(struct ubus_context *ctx, struct ubus_object *obj,
> >  		goto out;
> >  	}
> >  
> > +	if (base64)
> > +	{
> > +		uint8_t *data = calloc(B64_ENCODE_LEN(len), sizeof(uint8_t));
> > +		if (!data)
> > +		{
> > +			rv = UBUS_STATUS_UNKNOWN_ERROR;
> > +			goto out;
> > +		}
> You can reduce allocation/copy size if you copy wbuf to a temporary
> buffer and then use it as output for b64_encode.
> 
> > +		len = b64_encode(wbuf, len, data, B64_ENCODE_LEN(len));
> > +		if (len < 0)
> > +		{
> > +			free(data);
> > +			rv = UBUS_STATUS_UNKNOWN_ERROR;
> > +			goto out;
> > +		}
> > +
> > +		memcpy(wbuf, data, len);
> > +		free(data);
> > +	}
> > +
> >  	*(wbuf + len) = 0;
> >  	blobmsg_add_string_buffer(&buf);
> >  
> >  	ubus_send_reply(ctx, req, buf.head);
> > -	blob_buf_free(&buf);
> >  	rv = UBUS_STATUS_OK;
> >  
> >  out:
> > +	blob_buf_free(&buf);
> >  	close(fd);
> >  	return rv;
> >  }
> > @@ -222,18 +282,54 @@ rpc_file_write(struct ubus_context *ctx, struct ubus_object *obj,
> >  	if (!tb[RPC_F_RW_PATH] || !tb[RPC_F_RW_DATA])
> >  		return UBUS_STATUS_INVALID_ARGUMENT;
> >  
> > +	data = blobmsg_data(tb[RPC_F_RW_DATA]);
> > +	data_len = blobmsg_data_len(tb[RPC_F_RW_DATA]) - 1;
> > +
> >  	if ((fd = open(blobmsg_data(tb[RPC_F_RW_PATH]), O_CREAT | O_TRUNC | O_WRONLY, 0666)) < 0)
> >  		return rpc_errno_status();
> >  
> > -	if (write(fd, blobmsg_data(tb[RPC_F_RW_DATA]), blobmsg_data_len(tb[RPC_F_RW_DATA])) < 0)
> > -		return rpc_errno_status();
> > +	if (tb[RPC_F_RW_BASE64])
> > +		base64 = blobmsg_get_bool(tb[RPC_F_RW_BASE64]);
> > +
> > +	if (base64)
> > +	{
> > +		rbuf_len = B64_DECODE_LEN(data_len);
> > +		rbuf = calloc(rbuf_len, sizeof(uint8_t));
> > +		if (!rbuf)
> > +		{
> > +			rv = UBUS_STATUS_UNKNOWN_ERROR;
> > +			goto out;
> > +		}
> > +
> > +		rbuf_len = b64_decode(data, rbuf, rbuf_len);
> > +		if (rbuf_len < 0)
> > +		{
> > +			rv = UBUS_STATUS_UNKNOWN_ERROR;
> > +			goto out;
> > +		}
> > +	}
> > +	else
> > +	{
> > +		rbuf = data;
> > +		rbuf_len = data_len;
> > +	}
> It is safe to overwrite the data area in this function, as long as you
> don't overstep attribute bounds. This means you can reuse the input
> buffer as output buffer for b64_decode and get rid of the temporary
> allocation.
> 
> - Felix
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
Felix Fietkau May 11, 2015, 9:52 p.m. UTC | #3 
On 2015-05-11 23:26, Luka Perkov wrote:
> Hi Felix,
> 
> On Mon, May 11, 2015 at 11:36:46AM +0200, Felix Fietkau wrote:
>> On 2015-05-11 00:26, Luka Perkov wrote:
>> > Signed-off-by: Luka Perkov <luka@openwrt.org>
>> > ---
>> > => changes in v2:
>> > 
>> > Use new libubox base64 provided API.
>> > 
>> >  file.c | 118 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
>> >  1 file changed, 107 insertions(+), 11 deletions(-)
>> > 
>> > diff --git a/file.c b/file.c
>> > index 9c1b301..c3671bb 100644
>> > --- a/file.c
>> > +++ b/file.c
>> > @@ -182,7 +206,17 @@ rpc_file_read(struct ubus_context *ctx, struct ubus_object *obj,
>> >  
>> >  	blob_buf_init(&buf, 0);
>> >  
>> > -	wbuf = blobmsg_alloc_string_buffer(&buf, "data", s.st_size + 1);
>> > +	if (tb[RPC_F_RB_BASE64])
>> > +		base64 = blobmsg_get_bool(tb[RPC_F_RB_BASE64]);
>> > +
>> > +	if (base64)
>> > +	{
>> > +		wbuf = blobmsg_alloc_string_buffer(&buf, "data", B64_ENCODE_LEN(s.st_size));
>> > +	}
>> > +	else
>> > +	{
>> > +		wbuf = blobmsg_alloc_string_buffer(&buf, "data", s.st_size + 1);
>> > +	}
>> How about using the 'len' variable to avoid duplicating most of the code
>> here.
> 
> Can you be more specific here please? I don't see how by using 'len' we
> can reduce more code here.
len = s.st_size + 1;
if (base64)
	len = B64_ENCODE_LEN(s.st_size);
wbuf = blobmsg_alloc_string_buffer(&buf, "data", len);

- Felix
diff mbox

Patch

diff --git a/file.c b/file.c
index 9c1b301..c3671bb 100644
--- a/file.c
+++ b/file.c
@@ -29,6 +29,7 @@ 
 #include <libubox/blobmsg.h>
 #include <libubox/md5.h>
 #include <libubox/ustream.h>
+#include <libubox/utils.h>
 
 #include <rpcd/plugin.h>
 
@@ -79,14 +80,27 @@  static const struct blobmsg_policy rpc_file_r_policy[__RPC_F_R_MAX] = {
 };
 
 enum {
+	RPC_F_RB_PATH,
+	RPC_F_RB_BASE64,
+	__RPC_F_RB_MAX,
+};
+
+static const struct blobmsg_policy rpc_file_rb_policy[__RPC_F_RB_MAX] = {
+	[RPC_F_RB_PATH]   = { .name = "path",   .type = BLOBMSG_TYPE_STRING },
+	[RPC_F_RB_BASE64] = { .name = "base64", .type = BLOBMSG_TYPE_BOOL   },
+};
+
+enum {
 	RPC_F_RW_PATH,
 	RPC_F_RW_DATA,
+	RPC_F_RW_BASE64,
 	__RPC_F_RW_MAX,
 };
 
 static const struct blobmsg_policy rpc_file_rw_policy[__RPC_F_RW_MAX] = {
-	[RPC_F_RW_PATH] = { .name = "path", .type = BLOBMSG_TYPE_STRING },
-	[RPC_F_RW_DATA] = { .name = "data", .type = BLOBMSG_TYPE_STRING },
+	[RPC_F_RW_PATH]   = { .name = "path",   .type = BLOBMSG_TYPE_STRING },
+	[RPC_F_RW_DATA]   = { .name = "data",   .type = BLOBMSG_TYPE_STRING },
+	[RPC_F_RW_BASE64] = { .name = "base64", .type = BLOBMSG_TYPE_BOOL   },
 };
 
 enum {
@@ -162,12 +176,22 @@  rpc_file_read(struct ubus_context *ctx, struct ubus_object *obj,
               struct ubus_request_data *req, const char *method,
               struct blob_attr *msg)
 {
-	int fd, rv, len;
+	static struct blob_attr *tb[__RPC_F_RB_MAX];
+	bool base64 = false;
+	int fd, rv;
+	ssize_t len;
 	char *path;
 	struct stat s;
 	char *wbuf;
 
-	if (!rpc_check_path(msg, &path, &s))
+	blobmsg_parse(rpc_file_rb_policy, __RPC_F_RB_MAX, tb, blob_data(msg), blob_len(msg));
+
+	if (!tb[RPC_F_RB_PATH])
+		return rpc_errno_status();
+
+	path = blobmsg_data(tb[RPC_F_RB_PATH]);
+
+	if (stat(path, &s))
 		return rpc_errno_status();
 
 	if (s.st_size >= RPC_FILE_MAX_SIZE)
@@ -182,7 +206,17 @@  rpc_file_read(struct ubus_context *ctx, struct ubus_object *obj,
 
 	blob_buf_init(&buf, 0);
 
-	wbuf = blobmsg_alloc_string_buffer(&buf, "data", s.st_size + 1);
+	if (tb[RPC_F_RB_BASE64])
+		base64 = blobmsg_get_bool(tb[RPC_F_RB_BASE64]);
+
+	if (base64)
+	{
+		wbuf = blobmsg_alloc_string_buffer(&buf, "data", B64_ENCODE_LEN(s.st_size));
+	}
+	else
+	{
+		wbuf = blobmsg_alloc_string_buffer(&buf, "data", s.st_size + 1);
+	}
 
 	if (!wbuf)
 	{
@@ -196,14 +230,35 @@  rpc_file_read(struct ubus_context *ctx, struct ubus_object *obj,
 		goto out;
 	}
 
+	if (base64)
+	{
+		uint8_t *data = calloc(B64_ENCODE_LEN(len), sizeof(uint8_t));
+		if (!data)
+		{
+			rv = UBUS_STATUS_UNKNOWN_ERROR;
+			goto out;
+		}
+
+		len = b64_encode(wbuf, len, data, B64_ENCODE_LEN(len));
+		if (len < 0)
+		{
+			free(data);
+			rv = UBUS_STATUS_UNKNOWN_ERROR;
+			goto out;
+		}
+
+		memcpy(wbuf, data, len);
+		free(data);
+	}
+
 	*(wbuf + len) = 0;
 	blobmsg_add_string_buffer(&buf);
 
 	ubus_send_reply(ctx, req, buf.head);
-	blob_buf_free(&buf);
 	rv = UBUS_STATUS_OK;
 
 out:
+	blob_buf_free(&buf);
 	close(fd);
 	return rv;
 }
@@ -213,8 +268,13 @@  rpc_file_write(struct ubus_context *ctx, struct ubus_object *obj,
                struct ubus_request_data *req, const char *method,
                struct blob_attr *msg)
 {
-	int fd;
 	struct blob_attr *tb[__RPC_F_RW_MAX];
+	bool base64 = false;
+	int fd, rv = 0;
+	void *data = NULL;
+	size_t data_len = 0;
+	void *rbuf = NULL;
+	ssize_t rbuf_len = 0;
 
 	blobmsg_parse(rpc_file_rw_policy, __RPC_F_RW_MAX, tb,
 	              blob_data(msg), blob_len(msg));
@@ -222,18 +282,54 @@  rpc_file_write(struct ubus_context *ctx, struct ubus_object *obj,
 	if (!tb[RPC_F_RW_PATH] || !tb[RPC_F_RW_DATA])
 		return UBUS_STATUS_INVALID_ARGUMENT;
 
+	data = blobmsg_data(tb[RPC_F_RW_DATA]);
+	data_len = blobmsg_data_len(tb[RPC_F_RW_DATA]) - 1;
+
 	if ((fd = open(blobmsg_data(tb[RPC_F_RW_PATH]), O_CREAT | O_TRUNC | O_WRONLY, 0666)) < 0)
 		return rpc_errno_status();
 
-	if (write(fd, blobmsg_data(tb[RPC_F_RW_DATA]), blobmsg_data_len(tb[RPC_F_RW_DATA])) < 0)
-		return rpc_errno_status();
+	if (tb[RPC_F_RW_BASE64])
+		base64 = blobmsg_get_bool(tb[RPC_F_RW_BASE64]);
+
+	if (base64)
+	{
+		rbuf_len = B64_DECODE_LEN(data_len);
+		rbuf = calloc(rbuf_len, sizeof(uint8_t));
+		if (!rbuf)
+		{
+			rv = UBUS_STATUS_UNKNOWN_ERROR;
+			goto out;
+		}
+
+		rbuf_len = b64_decode(data, rbuf, rbuf_len);
+		if (rbuf_len < 0)
+		{
+			rv = UBUS_STATUS_UNKNOWN_ERROR;
+			goto out;
+		}
+	}
+	else
+	{
+		rbuf = data;
+		rbuf_len = data_len;
+	}
+
+	if (write(fd, rbuf, rbuf_len) < 0)
+		rv = -1;
+
+out:
+	if (base64)
+		free(rbuf);
 
 	if (fsync(fd) < 0)
-		return rpc_errno_status();
+		rv = -1;
 
 	close(fd);
 	sync();
 
+	if (rv)
+		return rpc_errno_status();
+
 	return 0;
 }
 
@@ -641,7 +737,7 @@  static int
 rpc_file_api_init(const struct rpc_daemon_ops *o, struct ubus_context *ctx)
 {
 	static const struct ubus_method file_methods[] = {
-		UBUS_METHOD("read",    rpc_file_read,  rpc_file_r_policy),
+		UBUS_METHOD("read",    rpc_file_read,  rpc_file_rb_policy),
 		UBUS_METHOD("write",   rpc_file_write, rpc_file_rw_policy),
 		UBUS_METHOD("list",    rpc_file_list,  rpc_file_r_policy),
 		UBUS_METHOD("stat",    rpc_file_stat,  rpc_file_r_policy),