From 42b6305964cb3a5461402652773ceacf5dec49fd Mon Sep 17 00:00:00 2001 From: antirez Date: Wed, 17 Jul 2019 17:30:02 +0200 Subject: RDB: try to make error handling code more readable. --- src/rdb.c | 86 +++++++++++++++++++++++++++------------------------ src/rdb.h | 4 +-- src/redis-check-rdb.c | 9 +++--- 3 files changed, 52 insertions(+), 47 deletions(-) diff --git a/src/rdb.c b/src/rdb.c index 6d5c1f2e5..ac9973385 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -101,12 +101,13 @@ int rdbLoadType(rio *rdb) { /* This is only used to load old databases stored with the RDB_OPCODE_EXPIRETIME * opcode. New versions of Redis store using the RDB_OPCODE_EXPIRETIME_MS - * opcode. */ -int rdbLoadTime(rio *rdb, time_t *t) { + * opcode. On error -1 is returned, however this could be a valid time, so + * to check for loading errors the caller should call rioGetReadError() after + * calling this function. */ +time_t rdbLoadTime(rio *rdb) { int32_t t32; - if (rioRead(rdb,&t32,4) == 0) return C_ERR; - *t = (time_t)t32; - return C_OK;; + if (rioRead(rdb,&t32,4) == 0) return -1; + return (time_t)t32; } int rdbSaveMillisecondTime(rio *rdb, long long t) { @@ -125,14 +126,17 @@ int rdbSaveMillisecondTime(rio *rdb, long long t) { * after upgrading to Redis version 5 they will no longer be able to load their * own old RDB files. Because of that, we instead fix the function only for new * RDB versions, and load older RDB versions as we used to do in the past, - * allowing big endian systems to load their own old RDB files. */ -int rdbLoadMillisecondTime(rio *rdb, long long *t, int rdbver) { + * allowing big endian systems to load their own old RDB files. + * + * On I/O error the function returns LLONG_MAX, however if this is also a + * valid stored value, the caller should use rioGetReadError() to check for + * errors after calling this function. */ +long long rdbLoadMillisecondTime(rio *rdb, int rdbver) { int64_t t64; - if (rioRead(rdb,&t64,8) == 0) return C_ERR; + if (rioRead(rdb,&t64,8) == 0) return LLONG_MAX; if (rdbver >= 9) /* Check the top comment of this function. */ memrev64ifbe(&t64); /* Convert in big endian if the system is BE. */ - *t = (long long)t64; - return C_OK; + return (long long)t64; } /* Saves an encoded length. The first two bits in the first byte are used to @@ -1692,15 +1696,14 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, robj *key) { rdbExitReportCorruptRDB("Listpack re-added with existing key"); } /* Load total number of items inside the stream. */ - if (rdbLoadLenByRef(rdb,NULL,&s->length)) { - rdbReportReadError("Stream item count loading failed."); - decrRefCount(o); - return NULL; - } + s->length = rdbLoadLen(rdb,NULL); + /* Load the last entry ID. */ - if (rdbLoadLenByRef(rdb,NULL,&s->last_id.ms) || - rdbLoadLenByRef(rdb,NULL,&s->last_id.seq)) { - rdbReportReadError("Stream last entry ID loading failed."); + s->last_id.ms = rdbLoadLen(rdb,NULL); + s->last_id.seq = rdbLoadLen(rdb,NULL); + + if (rioGetReadError(rdb)) { + rdbReportReadError("Stream object metadata loading failed."); decrRefCount(o); return NULL; } @@ -1724,13 +1727,16 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, robj *key) { decrRefCount(o); return NULL; } - if (rdbLoadLenByRef(rdb,NULL,&cg_id.ms) || - rdbLoadLenByRef(rdb,NULL,&cg_id.seq)) { + + cg_id.ms = rdbLoadLen(rdb,NULL); + cg_id.seq = rdbLoadLen(rdb,NULL); + if (rioGetReadError(rdb)) { rdbReportReadError("Stream cgroup ID loading failed."); sdsfree(cgname); decrRefCount(o); return NULL; } + streamCG *cgroup = streamCreateCG(s,cgname,sdslen(cgname),&cg_id); if (cgroup == NULL) rdbExitReportCorruptRDB("Duplicated consumer group name %s", @@ -1756,14 +1762,10 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, robj *key) { return NULL; } streamNACK *nack = streamCreateNACK(NULL); - if (rdbLoadMillisecondTime(rdb, &nack->delivery_time,RDB_VERSION) == C_ERR) { - rdbReportReadError("Stream PEL nack loading failed."); - decrRefCount(o); - streamFreeNACK(nack); - return NULL; - } - if ((nack->delivery_count = rdbLoadLen(rdb,NULL)) == RDB_LENERR) { - rdbReportReadError("Stream nack deliveries loading failed."); + nack->delivery_time = rdbLoadMillisecondTime(rdb,RDB_VERSION); + nack->delivery_count = rdbLoadLen(rdb,NULL); + if (rioGetReadError(rdb)) { + rdbReportReadError("Stream PEL NACK loading failed."); decrRefCount(o); streamFreeNACK(nack); return NULL; @@ -1792,7 +1794,8 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, robj *key) { streamConsumer *consumer = streamLookupConsumer(cgroup,cname, 1); sdsfree(cname); - if (rdbLoadMillisecondTime(rdb, &consumer->seen_time,RDB_VERSION) == C_ERR) { + consumer->seen_time = rdbLoadMillisecondTime(rdb,RDB_VERSION); + if (rioGetReadError(rdb)) { rdbReportReadError("Stream short read reading seen time."); decrRefCount(o); return NULL; @@ -1802,14 +1805,16 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, robj *key) { * consumer. */ pel_size = rdbLoadLen(rdb,NULL); if (pel_size == RDB_LENERR) { - rdbReportReadError("Stream consumer PEL num loading failed."); + rdbReportReadError( + "Stream consumer PEL num loading failed."); decrRefCount(o); return NULL; } while(pel_size--) { unsigned char rawid[sizeof(streamID)]; if (rioRead(rdb,rawid,sizeof(rawid)) == 0) { - rdbReportReadError("Stream short read reading PEL streamID."); + rdbReportReadError( + "Stream short read reading PEL streamID."); decrRefCount(o); return NULL; } @@ -1830,10 +1835,8 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, robj *key) { } } } else if (rdbtype == RDB_TYPE_MODULE || rdbtype == RDB_TYPE_MODULE_2) { - uint64_t moduleid; - if (rdbLoadLenByRef(rdb,NULL, &moduleid)) { - return NULL; - } + uint64_t moduleid = rdbLoadLen(rdb,NULL); + if (rioGetReadError(rdb)) return NULL; moduleType *mt = moduleTypeLookupModuleByID(moduleid); char name[10]; @@ -1977,14 +1980,15 @@ int rdbLoadRio(rio *rdb, rdbSaveInfo *rsi, int loading_aof) { /* EXPIRETIME: load an expire associated with the next key * to load. Note that after loading an expire we need to * load the actual type, and continue. */ - time_t t; - if (rdbLoadTime(rdb, &t) == C_ERR) goto eoferr; - expiretime = t * 1000; + expiretime = rdbLoadTime(rdb); + expiretime *= 1000; + if (rioGetReadError(rdb)) goto eoferr; continue; /* Read next opcode. */ } else if (type == RDB_OPCODE_EXPIRETIME_MS) { /* EXPIRETIME_MS: milliseconds precision expire times introduced * with RDB v3. Like EXPIRETIME but no with more precision. */ - if (rdbLoadMillisecondTime(rdb, &expiretime, rdbver) == C_ERR) goto eoferr; + expiretime = rdbLoadMillisecondTime(rdb,rdbver); + if (rioGetReadError(rdb)) goto eoferr; continue; /* Read next opcode. */ } else if (type == RDB_OPCODE_FREQ) { /* FREQ: LFU frequency. */ @@ -2093,8 +2097,8 @@ int rdbLoadRio(rio *rdb, rdbSaveInfo *rsi, int loading_aof) { * we have the ability to read a MODULE_AUX opcode followed by an * identifier of the module, and a serialized value in "MODULE V2" * format. */ - uint64_t moduleid; - if (rdbLoadLenByRef(rdb,NULL,&moduleid)) goto eoferr; + uint64_t moduleid = rdbLoadLen(rdb,NULL); + if (rioGetReadError(rdb)) goto eoferr; moduleType *mt = moduleTypeLookupModuleByID(moduleid); char name[10]; moduleTypeNameByID(name,moduleid); diff --git a/src/rdb.h b/src/rdb.h index 2bec0171b..0acddf9ab 100644 --- a/src/rdb.h +++ b/src/rdb.h @@ -127,10 +127,10 @@ int rdbSaveType(rio *rdb, unsigned char type); int rdbLoadType(rio *rdb); int rdbSaveTime(rio *rdb, time_t t); -int rdbLoadTime(rio *rdb, time_t *t); +time_t rdbLoadTime(rio *rdb); int rdbSaveLen(rio *rdb, uint64_t len); int rdbSaveMillisecondTime(rio *rdb, long long t); -int rdbLoadMillisecondTime(rio *rdb, long long *t, int rdbver); +long long rdbLoadMillisecondTime(rio *rdb, int rdbver); uint64_t rdbLoadLen(rio *rdb, int *isencoded); int rdbLoadLenByRef(rio *rdb, int *isencoded, uint64_t *lenptr); int rdbSaveObjectType(rio *rdb, robj *o); diff --git a/src/redis-check-rdb.c b/src/redis-check-rdb.c index fd3f07b0a..5e7415046 100644 --- a/src/redis-check-rdb.c +++ b/src/redis-check-rdb.c @@ -212,19 +212,20 @@ int redis_check_rdb(char *rdbfilename, FILE *fp) { /* Handle special types. */ if (type == RDB_OPCODE_EXPIRETIME) { - time_t t; rdbstate.doing = RDB_CHECK_DOING_READ_EXPIRE; /* EXPIRETIME: load an expire associated with the next key * to load. Note that after loading an expire we need to * load the actual type, and continue. */ - if (rdbLoadTime(&rdb, &t) == C_ERR) goto eoferr; - expiretime = t * 1000; + expiretime = rdbLoadTime(&rdb); + expiretime *= 1000; + if (rioGetReadError(&rdb)) goto eoferr; continue; /* Read next opcode. */ } else if (type == RDB_OPCODE_EXPIRETIME_MS) { /* EXPIRETIME_MS: milliseconds precision expire times introduced * with RDB v3. Like EXPIRETIME but no with more precision. */ rdbstate.doing = RDB_CHECK_DOING_READ_EXPIRE; - if (rdbLoadMillisecondTime(&rdb, &expiretime, rdbver) == C_ERR) goto eoferr; + expiretime = rdbLoadMillisecondTime(&rdb, rdbver); + if (rioGetReadError(&rdb)) goto eoferr; continue; /* Read next opcode. */ } else if (type == RDB_OPCODE_FREQ) { /* FREQ: LFU frequency. */ -- cgit v1.2.1