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

[Luka Perkov]

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 at 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 at lists.openwrt.org
> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel