[sheepdog] [PATCH v4 1/2] sheep/http: add range support for swift insterface

[Liu Yuan]

On Mon, Jan 20, 2014 at 09:42:29PM +0800, Robin Dong wrote:
> From: Robin Dong <sanbai at taobao.com>
> 
> In swift spec, "Range: bytes=13-15" in http header means "read the three bytes
> of data after a 13-byte offset". We add this support by parsing http header and
> read specific bytes in range.
> 
> We don't support multi-ranges at present.
> 
> Signed-off-by: Robin Dong <sanbai at taobao.com>
> ---
> v3-->v4:
>     1. do not check "off" and "onode->size" twice
>     2. check return value of http_request_write() with "len"
> v2-->v3:
>     1. add scissor lines for patch header
>     2. change "%lu" to PRIx64 for format of output
> v1-->v2:
>     1. use req->offset and req->data_length instead of req->range[2]
> 
>  sheep/http/http.c | 32 ++++++++++++++++++++++++++++++++
>  sheep/http/http.h |  1 +
>  sheep/http/kv.c   | 43 +++++++++++++++++++++++++++++++++----------
>  3 files changed, 66 insertions(+), 10 deletions(-)
> 
> diff --git a/sheep/http/http.c b/sheep/http/http.c
> index 165c818..08f894b 100644
> --- a/sheep/http/http.c
> +++ b/sheep/http/http.c
> @@ -169,10 +169,42 @@ static int request_init_operation(struct http_request *req)
>  	req->uri = FCGX_GetParam("DOCUMENT_URI", env);
>  	if (!req->uri)
>  		return BAD_REQUEST;
> +	p = FCGX_GetParam("HTTP_RANGE", env);
> +	if (p && p[0] != '\0') {
> +		const char prefix[] = "bytes=";
> +		char *left, *right, num[64];
> +		uint64_t max;
> +		left = strstr(p, prefix);
> +		if (!p)
> +			goto invalid_range;
> +		right = strchr(left, '-');
> +		strncpy(num, left + sizeof(prefix) - 1, right - left);
> +		req->offset = strtoll(num, &endp, 10);
> +		if (num == endp)
> +			goto invalid_range;
> +		strcpy(num, right + 1);
> +		/*
> +		 * In swift spec, the second number of RANGE should be included
> +		 * which means [num1, num2], but our common means for read and
> +		 * write data by 'offset' and 'len' is [num1, num2), so we
> +		 * should add 1 to num2.
> +		 */
> +		max = strtoll(num, &endp, 10) + 1;
> +		if (num == endp)
> +			goto invalid_range;
> +		if (max <= req->offset)
> +			goto invalid_range;

I think you should return "Requested Range Not Satisfiable"

> +		req->data_length = max - req->offset;
> +		sd_debug("HTTP_RANGE: %"PRIx64" %"PRIx64, req->offset, max);
> +	}
>  
>  	req->status = UNKNOWN;
>  
>  	return OK;
> +
> +invalid_range:
> +	sd_err("invalid range %s", p);
> +	return BAD_REQUEST;
>  }
>  
>  static int http_init_request(struct http_request *req)
> diff --git a/sheep/http/http.h b/sheep/http/http.h
> index 20246dc..f437f82 100644
> --- a/sheep/http/http.h
> +++ b/sheep/http/http.h
> @@ -48,6 +48,7 @@ struct http_request {
>  	enum http_opcode opcode;
>  	enum http_status status;
>  	uint64_t data_length;
> +	uint64_t offset;
>  };
>  
>  struct http_driver {
> diff --git a/sheep/http/kv.c b/sheep/http/kv.c
> index 182b263..4e9b921 100644
> --- a/sheep/http/kv.c
> +++ b/sheep/http/kv.c
> @@ -858,24 +858,34 @@ static int onode_free_data(struct kv_onode *onode)
>  	return ret;
>  }
>  
> -static int onode_read_extents(struct kv_onode *onode, struct http_request *req)
> +static int onode_read_extents(struct kv_onode *onode, struct http_request *req,
> +			      uint64_t off, uint64_t len)
>  {
>  	struct onode_extent *ext;
> -	uint64_t size, total, total_size, offset, done = 0, i;
> +	uint64_t size, total, total_size, offset, done = 0, i, ext_len;
>  	int ret;
>  	char *data_buf = NULL;
>  	uint64_t read_buffer_size = MIN(MAX_RW_BUFFER, onode->size);
>  
>  	data_buf = xmalloc(read_buffer_size);
> -	total_size = onode->size;
> +	total_size = len;
>  	for (i = 0; i < onode->nr_extent; i++) {
>  		ext = onode->o_extent + i;
> -		total = min(ext->count * SD_DATA_OBJ_SIZE, total_size);
> -		offset = ext->start * SD_DATA_OBJ_SIZE;
> +		ext_len = ext->count * SD_DATA_OBJ_SIZE;
> +		if (off >= ext_len) {
> +			off -= ext_len;
> +			continue;
> +		}
> +		total = min(ext_len - off, total_size);
> +		offset = ext->start * SD_DATA_OBJ_SIZE + off;
> +		off = 0;
> +		done = 0;
>  		while (done < total) {
>  			size = MIN(total - done, read_buffer_size);
>  			ret = vdi_read_write(onode->data_vid, data_buf,
>  					     size, offset, true);
> +			sd_debug("vdi_read_write size: %"PRIx64", offset: %"
> +				 PRIx64, size, offset);
>  			if (ret != SD_RES_SUCCESS) {
>  				sd_err("Failed to read for vid %"PRIx32,
>  				       onode->data_vid);
> @@ -953,12 +963,27 @@ out:
>  static int onode_read_data(struct kv_onode *onode, struct http_request *req)
>  {
>  	int ret;
> +	uint64_t off = 0, len = onode->size;
> +
> +	if (req->offset && req->data_length) {
> +		off = req->offset;
> +		len = req->data_length;
> +		if ((off + len - 1) > onode->size)
> +			len = onode->size - off;
> +		if (off >= onode->size)

I think this check is redundant. Check (off + len - 1) > onode->size is enough

> +			len = 0;
> +	}
> +	req->data_length = len;
> +	http_response_header(req, OK);
> +

I tried the Range: bytes=0-1, but it returns the whole file to me. Also please
add this to test script.

Also if offset + len - 1 > onode-size, I think you should return
"Requested Range Not Satisfiable"

Thanks
Yuan