From eaf58bd32a56adab5252f380df627a7c31d121f2 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Wed, 20 Apr 2011 16:54:12 +0200 Subject: [PATCH] Use pre-allocated buffer for error strings in reader --- hiredis.c | 149 +++++++++++++++++++++++++++++++----------------------- hiredis.h | 9 ++-- util.h | 25 +++++++++ 3 files changed, 116 insertions(+), 67 deletions(-) diff --git a/hiredis.c b/hiredis.c index f9e8488..ef89e4a 100644 --- a/hiredis.c +++ b/hiredis.c @@ -41,13 +41,10 @@ #include "sds.h" #include "util.h" -#define REDIS_READER_OOM -2 -#define REDIS_READER_NEED_MORE_DATA -1 -#define REDIS_READER_OK 0 - typedef struct redisReader { + int err; /* Error flags, 0 when there is no error */ + char errstr[128]; /* String representation of error when applicable */ struct redisReplyObjectFunctions *fn; - sds error; /* holds optional error */ void *reply; /* holds temporary reply */ sds buf; /* read buffer */ @@ -64,7 +61,6 @@ static void *createStringObject(const redisReadTask *task, char *str, size_t len static void *createArrayObject(const redisReadTask *task, int elements); static void *createIntegerObject(const redisReadTask *task, long long value); static void *createNilObject(const redisReadTask *task); -static void redisSetReplyReaderError(redisReader *r, sds err); /* Default set of functions to build the reply. Keep in mind that such a * function returning NULL is interpreted as OOM. */ @@ -200,6 +196,45 @@ static void *createNilObject(const redisReadTask *task) { return r; } +static void __redisReplyReaderSetError(redisReader *r, int type, const char *str) { + size_t len; + + if (r->reply != NULL && r->fn && r->fn->freeObject) { + r->fn->freeObject(r->reply); + r->reply = NULL; + } + + /* Clear input buffer on errors. */ + if (r->buf != NULL) { + sdsfree(r->buf); + r->buf = NULL; + r->pos = r->len = 0; + } + + /* Reset task stack. */ + r->ridx = -1; + + /* Set error. */ + r->err = type; + len = strlen(str); + len = len < (sizeof(r->errstr)-1) ? len : (sizeof(r->errstr)-1); + memcpy(r->errstr,str,len); + r->errstr[len] = '\0'; +} + +static void __redisReplyReaderSetErrorProtocolByte(redisReader *r, char byte) { + char cbuf[8], sbuf[128]; + + chrtos(cbuf,sizeof(cbuf),byte); + snprintf(sbuf,sizeof(sbuf), + "Protocol error, got %s as reply type byte", cbuf); + __redisReplyReaderSetError(r,REDIS_ERR_PROTOCOL,sbuf); +} + +static void __redisReplyReaderSetErrorOOM(redisReader *r) { + __redisReplyReaderSetError(r,REDIS_ERR_OOM,"Out of memory"); +} + static char *readBytes(redisReader *r, unsigned int bytes) { char *p; if (r->len-r->pos >= bytes) { @@ -326,16 +361,18 @@ static int processLineItem(redisReader *r) { obj = (void*)(size_t)(cur->type); } - if (obj == NULL) - return REDIS_READER_OOM; + if (obj == NULL) { + __redisReplyReaderSetErrorOOM(r); + return REDIS_ERR; + } /* Set reply if this is the root object. */ if (r->ridx == 0) r->reply = obj; moveToNextTask(r); - return REDIS_READER_OK; + return REDIS_OK; } - return REDIS_READER_NEED_MORE_DATA; + return REDIS_ERR; } static int processBulkItem(redisReader *r) { @@ -374,19 +411,21 @@ static int processBulkItem(redisReader *r) { /* Proceed when obj was created. */ if (success) { - if (obj == NULL) - return REDIS_READER_OOM; + if (obj == NULL) { + __redisReplyReaderSetErrorOOM(r); + return REDIS_ERR; + } r->pos += bytelen; /* Set reply if this is the root object. */ if (r->ridx == 0) r->reply = obj; moveToNextTask(r); - return REDIS_READER_OK; + return REDIS_OK; } } - return REDIS_READER_NEED_MORE_DATA; + return REDIS_ERR; } static int processMultiBulkItem(redisReader *r) { @@ -398,9 +437,9 @@ static int processMultiBulkItem(redisReader *r) { /* Set error for nested multi bulks with depth > 1 */ if (r->ridx == 2) { - redisSetReplyReaderError(r,sdscatprintf(sdsempty(), - "No support for nested multi bulk replies with depth > 1")); - return -1; + __redisReplyReaderSetError(r,REDIS_ERR_PROTOCOL, + "No support for nested multi bulk replies with depth > 1"); + return REDIS_ERR; } if ((p = readLine(r,NULL)) != NULL) { @@ -413,8 +452,10 @@ static int processMultiBulkItem(redisReader *r) { else obj = (void*)REDIS_REPLY_NIL; - if (obj == NULL) - return REDIS_READER_OOM; + if (obj == NULL) { + __redisReplyReaderSetErrorOOM(r); + return REDIS_ERR; + } moveToNextTask(r); } else { @@ -423,8 +464,10 @@ static int processMultiBulkItem(redisReader *r) { else obj = (void*)REDIS_REPLY_ARRAY; - if (obj == NULL) - return REDIS_READER_OOM; + if (obj == NULL) { + __redisReplyReaderSetErrorOOM(r); + return REDIS_ERR; + } /* Modify task stack when there are more than 0 elements. */ if (elements > 0) { @@ -444,16 +487,15 @@ static int processMultiBulkItem(redisReader *r) { /* Set reply if this is the root object. */ if (root) r->reply = obj; - return REDIS_READER_OK; + return REDIS_OK; } - return REDIS_READER_NEED_MORE_DATA; + return REDIS_ERR; } static int processItem(redisReader *r) { redisReadTask *cur = &(r->rstack[r->ridx]); char *p; - sds byte; /* check if we need to read type */ if (cur->type < 0) { @@ -475,15 +517,12 @@ static int processItem(redisReader *r) { cur->type = REDIS_REPLY_ARRAY; break; default: - byte = sdscatrepr(sdsempty(),p,1); - redisSetReplyReaderError(r,sdscatprintf(sdsempty(), - "Protocol error, got %s as reply type byte", byte)); - sdsfree(byte); - return -1; + __redisReplyReaderSetErrorProtocolByte(r,*p); + return REDIS_ERR; } } else { /* could not consume 1 byte */ - return -1; + return REDIS_ERR; } } @@ -499,13 +538,14 @@ static int processItem(redisReader *r) { return processMultiBulkItem(r); default: assert(NULL); - return -1; + return REDIS_ERR; /* Avoid warning. */ } } void *redisReplyReaderCreate(void) { redisReader *r = calloc(sizeof(redisReader),1); - r->error = NULL; + r->err = 0; + r->errstr[0] = '\0'; r->fn = &defaultFunctions; r->buf = sdsempty(); r->ridx = -1; @@ -545,8 +585,6 @@ void *redisReplyReaderGetObject(void *reader) { void redisReplyReaderFree(void *reader) { redisReader *r = reader; - if (r->error != NULL) - sdsfree(r->error); if (r->reply != NULL && r->fn) r->fn->freeObject(r->reply); if (r->buf != NULL) @@ -554,23 +592,9 @@ void redisReplyReaderFree(void *reader) { free(r); } -static void redisSetReplyReaderError(redisReader *r, sds err) { - if (r->reply != NULL) - r->fn->freeObject(r->reply); - - /* Clear remaining buffer when we see a protocol error. */ - if (r->buf != NULL) { - sdsfree(r->buf); - r->buf = sdsempty(); - r->pos = r->len = 0; - } - r->ridx = -1; - r->error = err; -} - char *redisReplyReaderGetError(void *reader) { redisReader *r = reader; - return r->error; + return r->errstr; } void redisReplyReaderFeed(void *reader, const char *buf, size_t len) { @@ -592,9 +616,14 @@ void redisReplyReaderFeed(void *reader, const char *buf, size_t len) { int redisReplyReaderGetReply(void *reader, void **reply) { redisReader *r = reader; - int ret = REDIS_READER_OK; - if (reply != NULL) *reply = NULL; + /* Default target pointer to NULL. */ + if (reply != NULL) + *reply = NULL; + + /* Return early when this reader is in an erroneous state. */ + if (r->err) + return REDIS_ERR; /* When the buffer is empty, there will never be a reply. */ if (r->len == 0) @@ -613,11 +642,11 @@ int redisReplyReaderGetReply(void *reader, void **reply) { /* Process items in reply. */ while (r->ridx >= 0) - if ((ret = processItem(r)) != REDIS_READER_OK) + if (processItem(r) != REDIS_OK) break; - /* Set errors on OOM. */ - if (ret == REDIS_READER_OOM) + /* Return ASAP when an error occurred. */ + if (r->err) return REDIS_ERR; /* Discard part of the buffer when we've consumed at least 1k, to avoid @@ -630,15 +659,9 @@ int redisReplyReaderGetReply(void *reader, void **reply) { /* Emit a reply when there is one. */ if (r->ridx == -1) { - void *aux = r->reply; + if (reply != NULL) + *reply = r->reply; r->reply = NULL; - - /* Check if there actually *is* a reply. */ - if (r->error != NULL) { - return REDIS_ERR; - } else { - if (reply != NULL) *reply = aux; - } } return REDIS_OK; } @@ -1024,7 +1047,7 @@ int redisGetReplyFromReader(redisContext *c, void **reply) { __redisCreateReplyReader(c); if (redisReplyReaderGetReply(c->reader,reply) == REDIS_ERR) { __redisSetError(c,REDIS_ERR_PROTOCOL, - sdsnew(((redisReader*)c->reader)->error)); + sdsnew(((redisReader*)c->reader)->errstr)); return REDIS_ERR; } return REDIS_OK; diff --git a/hiredis.h b/hiredis.h index f445209..858ec8d 100644 --- a/hiredis.h +++ b/hiredis.h @@ -46,10 +46,11 @@ * error that occured. REDIS_ERR_IO means there was an I/O error and you * should use the "errno" variable to find out what is wrong. * For other values, the "errstr" field will hold a description. */ -#define REDIS_ERR_IO 1 /* error in read or write */ -#define REDIS_ERR_EOF 3 /* eof */ -#define REDIS_ERR_PROTOCOL 4 /* protocol error */ -#define REDIS_ERR_OTHER 2 /* something else */ +#define REDIS_ERR_IO 1 /* Error in read or write */ +#define REDIS_ERR_EOF 3 /* End of file */ +#define REDIS_ERR_PROTOCOL 4 /* Protocol error */ +#define REDIS_ERR_OOM 5 /* Out of memory */ +#define REDIS_ERR_OTHER 2 /* Everything else... */ /* Connection type can be blocking or non-blocking and is set in the * least significant bit of the flags field in redisContext. */ diff --git a/util.h b/util.h index f192990..bf50659 100644 --- a/util.h +++ b/util.h @@ -30,6 +30,7 @@ #ifndef __UTIL_H #define __UTIL_H #include +#include /* Abort on out of memory */ static void redisOOM(void) { @@ -37,4 +38,28 @@ static void redisOOM(void) { exit(1); } +static size_t chrtos(char *buf, size_t size, char byte) { + size_t len = 0; + + switch(byte) { + case '\\': + case '"': + len = snprintf(buf,size,"\"\\%c\"",byte); + break; + case '\n': len = snprintf(buf,size,"\"\\n\""); break; + case '\r': len = snprintf(buf,size,"\"\\r\""); break; + case '\t': len = snprintf(buf,size,"\"\\t\""); break; + case '\a': len = snprintf(buf,size,"\"\\a\""); break; + case '\b': len = snprintf(buf,size,"\"\\b\""); break; + default: + if (isprint(byte)) + len = snprintf(buf,size,"\"%c\"",byte); + else + len = snprintf(buf,size,"\"\\x%02x\"",(unsigned char)byte); + break; + } + + return len; +} + #endif