From d5dfe5ee67ca43333951726d9b2a8da8cae6713f Mon Sep 17 00:00:00 2001 From: Patrick LeBlanc Date: Thu, 4 Apr 2019 16:52:47 -0500 Subject: [PATCH] Fixed all compiler warnings. For debugging builds, also made the compiler treat warnings as errors. --- CMakeLists.txt | 2 +- src/AppendTask.cpp | 2 +- src/Cache.cpp | 7 +++---- src/CloudStorage.cpp | 2 +- src/Config.cpp | 2 +- src/CopyTask.cpp | 1 - src/Downloader.cpp | 15 +++++++-------- src/Downloader.h | 4 ++-- src/IOCoordinator.cpp | 18 ++++++++++-------- src/IOCoordinator.h | 4 ++-- src/ListDirectoryTask.cpp | 2 +- src/LocalStorage.cpp | 1 - src/MetadataFile.cpp | 6 +++--- src/ReadTask.cpp | 2 +- src/Replicator.cpp | 6 +++++- src/SessionManager.cpp | 4 +--- src/Utilities.cpp | 4 ++-- src/unit_tests.cpp | 37 ++++++++++++++++--------------------- 18 files changed, 57 insertions(+), 62 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 04ce7a48c..692c13e07 100755 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -40,7 +40,7 @@ if (TRACE) endif() if (CMAKE_BUILD_TYPE STREQUAL Debug) - add_definitions(-DDEBUG) + add_definitions(-DDEBUG -Werror -Wall) set(S3_CONFIGURE_OPT --enable-debug) endif() diff --git a/src/AppendTask.cpp b/src/AppendTask.cpp index 578e71c87..74c1756ad 100644 --- a/src/AppendTask.cpp +++ b/src/AppendTask.cpp @@ -61,7 +61,7 @@ bool AppendTask::run() check_error("AppendTask read data", false); readCount += toRead; uint writePos = 0; - int err; + while (writeCount < readCount) { try diff --git a/src/Cache.cpp b/src/Cache.cpp index 02ca621bb..b28b0e361 100644 --- a/src/Cache.cpp +++ b/src/Cache.cpp @@ -186,11 +186,10 @@ void Cache::read(const vector &keys) s.unlock(); // start downloading the keys to fetch - int dl_err; vector dl_errnos; vector sizes; if (!keysToFetch.empty()) - dl_err = downloader.download(keysToFetch, &dl_errnos, &sizes); + downloader.download(keysToFetch, &dl_errnos, &sizes); size_t sum_sizes = 0; for (size_t &size : sizes) @@ -272,7 +271,7 @@ void Cache::exists(const vector &keys, vector *out) const { out->resize(keys.size()); boost::unique_lock s(lru_mutex); - for (int i = 0; i < keys.size(); i++) + for (uint i = 0; i < keys.size(); i++) (*out)[i] = (m_lru.find(keys[i]) != m_lru.end()); } @@ -370,7 +369,7 @@ void Cache::_makeSpace(size_t size) remove it from our structs update current size */ - assert(currentCacheSize >= statbuf.st_size); + assert(currentCacheSize >= (size_t) statbuf.st_size); currentCacheSize -= statbuf.st_size; thisMuch -= statbuf.st_size; Synchronizer::get()->flushObject(*it); diff --git a/src/CloudStorage.cpp b/src/CloudStorage.cpp index e51833031..c62e8d356 100644 --- a/src/CloudStorage.cpp +++ b/src/CloudStorage.cpp @@ -18,7 +18,7 @@ namespace string tolower(const string &s) { string ret(s); - for (int i = 0; i < ret.length(); i++) + for (uint i = 0; i < ret.length(); i++) ret[i] = ::tolower(ret[i]); return ret; } diff --git a/src/Config.cpp b/src/Config.cpp index bd49db887..16801823b 100644 --- a/src/Config.cpp +++ b/src/Config.cpp @@ -52,7 +52,7 @@ Config::Config() : die(false) paths.push_back(cs_install_dir); paths.push_back("/etc"); - for (int i = 0; i < paths.size(); i++) + for (uint i = 0; i < paths.size(); i++) { if (boost::filesystem::exists(paths[i] + "/storagemanager.cnf")) { diff --git a/src/CopyTask.cpp b/src/CopyTask.cpp index 770af3cb6..448d0badd 100644 --- a/src/CopyTask.cpp +++ b/src/CopyTask.cpp @@ -65,7 +65,6 @@ bool CopyTask::run() } sm_response *resp = (sm_response *) buf; - uint payloadLen = 0; resp->returnCode = 0; success = write(*resp, 0); return success; diff --git a/src/Downloader.cpp b/src/Downloader.cpp index 94ce1ff48..2bdeb3277 100644 --- a/src/Downloader.cpp +++ b/src/Downloader.cpp @@ -4,6 +4,7 @@ #include #include #include +#include using namespace std; namespace storagemanager @@ -36,9 +37,8 @@ inline boost::mutex & Downloader::getDownloadMutex() return download_mutex; } -int Downloader::download(const vector &keys, vector *errnos, vector *sizes) +void Downloader::download(const vector &keys, vector *errnos, vector *sizes) { - //volatile uint counter = keys.size(); uint counter = keys.size(); boost::condition condvar; boost::mutex m; @@ -96,19 +96,14 @@ int Downloader::download(const vector &keys, vector *errnos s.unlock(); // check for errors & propagate - int ret = 0; errnos->resize(keys.size()); + char buf[80]; for (i = 0; i < keys.size(); i++) { auto &dl = dls[i]; (*errnos)[i] = dl->dl_errno; if (dl->dl_errno != 0) - { - char buf[80]; - // not sure why yet, but valgrind complains about the call below logger->log(LOG_ERR, "Downloader: failed to download %s, got '%s'", keys[i]->c_str(), strerror_r(dl->dl_errno, buf, 80)); - ret = -1; - } } } @@ -132,7 +127,11 @@ void Downloader::Download::operator()() CloudStorage *storage = CloudStorage::get(); int err = storage->getObject(*key, dler->getDownloadPath() + "/" + *key, &size); if (err != 0) + { + boost::filesystem::remove(dler->getDownloadPath() + "/" + *key); + size = 0; dl_errno = errno; + } boost::unique_lock s(dler->getDownloadMutex()); for (auto &listener : listeners) diff --git a/src/Downloader.h b/src/Downloader.h index 0a1a790a0..fdf326f0c 100644 --- a/src/Downloader.h +++ b/src/Downloader.h @@ -22,9 +22,9 @@ class Downloader Downloader(); virtual ~Downloader(); - // returns 0 on success. If != 0, errnos will contains the errno associated with the failure // caller owns the memory for the strings. - int download(const std::vector &keys, std::vector *errnos, std::vector *sizes); + // errors are reported through errnos + void download(const std::vector &keys, std::vector *errnos, std::vector *sizes); void setDownloadPath(const std::string &path); const std::string & getDownloadPath() const; diff --git a/src/IOCoordinator.cpp b/src/IOCoordinator.cpp index e948fdf14..db3e55ae9 100755 --- a/src/IOCoordinator.cpp +++ b/src/IOCoordinator.cpp @@ -263,7 +263,7 @@ int IOCoordinator::write(const char *filename, const uint8_t *data, off_t offset for (std::vector::const_iterator i = objects.begin(); i != objects.end(); ++i) { // figure out how much data to write to this object - if (count == 0 && offset > i->offset) + if (count == 0 && (uint64_t) offset > i->offset) { // first object in the list so start at offset and // write to end of oject or all the data @@ -335,11 +335,13 @@ int IOCoordinator::append(const char *filename, const uint8_t *data, size_t leng size_t count = 0; while (count < length) { err = ::write(fd, &data[count], length - count); - if (err <= 0) + if (err <= 0) + { if (count > 0) // return what was successfully written return count; else return err; + } count += err; } @@ -809,10 +811,10 @@ boost::shared_array IOCoordinator::mergeJournal(const char *object, con if (*len == 0) // read to the end of the file - *len = max(maxJournalOffset, objStat.st_size) - offset; + *len = max(maxJournalOffset, (size_t) objStat.st_size) - offset; else // make sure len is within the bounds of the data - *len = min(*len, (max(maxJournalOffset, objStat.st_size) - offset)); + *len = min(*len, (max(maxJournalOffset, (size_t) objStat.st_size) - offset)); ret.reset(new uint8_t[*len]); // read the object into memory @@ -852,9 +854,9 @@ boost::shared_array IOCoordinator::mergeJournal(const char *object, con // if this entry overlaps, read the overlapping section uint64_t lastJournalOffset = offlen[0] + offlen[1]; uint64_t lastBufOffset = offset + *len; - if (offlen[0] <= lastBufOffset && lastJournalOffset >= offset) + if (offlen[0] <= lastBufOffset && lastJournalOffset >= (uint64_t) offset) { - uint64_t startReadingAt = max(offlen[0], offset); + uint64_t startReadingAt = max(offlen[0], (uint64_t) offset); uint64_t lengthOfRead = min(lastBufOffset, lastJournalOffset) - startReadingAt; if (startReadingAt != offlen[0]) @@ -958,7 +960,7 @@ void IOCoordinator::renameObject(const string &oldKey, const string &newKey) } -bool IOCoordinator::readLock(const string &filename) +void IOCoordinator::readLock(const string &filename) { boost::unique_lock s(lockMutex); @@ -981,7 +983,7 @@ void IOCoordinator::readUnlock(const string &filename) } } -bool IOCoordinator::writeLock(const string &filename) +void IOCoordinator::writeLock(const string &filename) { boost::unique_lock s(lockMutex); diff --git a/src/IOCoordinator.h b/src/IOCoordinator.h index 267355946..6b3bcdcd3 100644 --- a/src/IOCoordinator.h +++ b/src/IOCoordinator.h @@ -59,8 +59,8 @@ class IOCoordinator : public boost::noncopyable /* Lock manipulation fcns. They can lock on any param given to them. For convention's sake, the parameter should mostly be the abs filename being accessed. */ void renameObject(const std::string &oldKey, const std::string &newKey); - bool readLock(const std::string &filename); - bool writeLock(const std::string &filename); + void readLock(const std::string &filename); + void writeLock(const std::string &filename); void readUnlock(const std::string &filename); void writeUnlock(const std::string &filename); diff --git a/src/ListDirectoryTask.cpp b/src/ListDirectoryTask.cpp index 9f7e36136..41bf70c34 100644 --- a/src/ListDirectoryTask.cpp +++ b/src/ListDirectoryTask.cpp @@ -37,7 +37,7 @@ bool ListDirectoryTask::writeString(uint8_t *buf, int *offset, int size, const s check_error("ListDirectoryTask::writeString()", false); *offset = 0; } - uint count = 0, len = str.length(); + int count = 0, len = str.length(); *((uint32_t *) &buf[*offset]) = len; *offset += 4; while (count < len) diff --git a/src/LocalStorage.cpp b/src/LocalStorage.cpp index a0d4e1d71..3165770f0 100644 --- a/src/LocalStorage.cpp +++ b/src/LocalStorage.cpp @@ -72,7 +72,6 @@ int LocalStorage::getObject(const string &source, const string &dest, size_t *si int LocalStorage::getObject(const std::string &sourceKey, boost::shared_array *data, size_t *size) { - int ret; bf::path source = prefix / sourceKey; const char *c_source = source.string().c_str(); char buf[80]; diff --git a/src/MetadataFile.cpp b/src/MetadataFile.cpp index cbf794578..1bfa6f9a8 100755 --- a/src/MetadataFile.cpp +++ b/src/MetadataFile.cpp @@ -225,7 +225,7 @@ vector MetadataFile::metadataRead(off_t offset, size_t length) c // find the first object in range while (i != mObjects.end()) { - if (offset <= (i->offset + i->length - 1)) + if ((uint64_t) offset <= (i->offset + i->length - 1)) break; ++i; } @@ -313,7 +313,7 @@ string MetadataFile::getNewKey(string sourceName, size_t offset, size_t length) boost::uuids::uuid u = boost::uuids::random_generator()(); stringstream ss; - for (int i = 0; i < sourceName.length(); i++) + for (uint i = 0; i < sourceName.length(); i++) { if (sourceName[i] == '/') { @@ -339,7 +339,7 @@ string MetadataFile::getSourceFromKey(const string &key) // this is to convert the munged filenames back to regular filenames // for consistent use in IOC locks - for (int i = 0; i < split[3].length(); i++) + for (uint i = 0; i < split[3].length(); i++) if (split[3][i] == '~') split[3][i] = '/'; diff --git a/src/ReadTask.cpp b/src/ReadTask.cpp index 73c1cff96..61e0e2cae 100644 --- a/src/ReadTask.cpp +++ b/src/ReadTask.cpp @@ -58,7 +58,7 @@ bool ReadTask::run() // todo: need to make this use O_DIRECT on the IOC side ioc->willRead(cmd->filename, cmd->offset, cmd->count); int err; - while (resp->returnCode < cmd->count) + while ((uint) resp->returnCode < cmd->count) { try { diff --git a/src/Replicator.cpp b/src/Replicator.cpp index 166240ef2..3230f27fd 100755 --- a/src/Replicator.cpp +++ b/src/Replicator.cpp @@ -94,10 +94,12 @@ int Replicator::newObject(const char *filename, const uint8_t *data, size_t leng while (count < length) { err = ::write(fd, &data[count], length - count); if (err <= 0) + { if (count > 0) // return what was successfully written return count; else return err; + } count += err; } @@ -107,7 +109,7 @@ int Replicator::newObject(const char *filename, const uint8_t *data, size_t leng int Replicator::addJournalEntry(const char *filename, const uint8_t *data, off_t offset, size_t length) { int fd, err; - uint64_t offlen[] = {offset,length}; + uint64_t offlen[] = {(uint64_t) offset,length}; size_t count = 0; int version = 1; string journalFilename = msJournalPath + "/" + string(filename) + ".journal"; @@ -150,10 +152,12 @@ int Replicator::addJournalEntry(const char *filename, const uint8_t *data, off_t while (count < length) { err = ::write(fd, &data[count], length - count); if (err <= 0) + { if (count > 0) // return what was successfully written return count; else return err; + } count += err; } diff --git a/src/SessionManager.cpp b/src/SessionManager.cpp index e838264d0..608a23485 100644 --- a/src/SessionManager.cpp +++ b/src/SessionManager.cpp @@ -231,9 +231,7 @@ int SessionManager::start() uint remainingBytes = 0; uint endOfData, i; int peakLength,len; - struct timespec ts; - ts.tv_sec = 0; - ts.tv_nsec = 100000000; // .1 sec + //logger->log(LOG_DEBUG,"reading from fd %i index is %i", fds[socketIncr].fd,socketIncr); if (sockState.find(fds[socketIncr].fd) != sockState.end()) { diff --git a/src/Utilities.cpp b/src/Utilities.cpp index ee9000d73..b6a6d66de 100644 --- a/src/Utilities.cpp +++ b/src/Utilities.cpp @@ -4,7 +4,7 @@ namespace storagemanager { -ScopedReadLock::ScopedReadLock(IOCoordinator *i, const std::string &k) : ioc(i), key(k), locked(false) +ScopedReadLock::ScopedReadLock(IOCoordinator *i, const std::string &k) : ioc(i), locked(false), key(k) { lock(); } @@ -29,7 +29,7 @@ void ScopedReadLock::unlock() } } -ScopedWriteLock::ScopedWriteLock(IOCoordinator *i, const std::string &k) : ioc(i), key(k), locked(false) +ScopedWriteLock::ScopedWriteLock(IOCoordinator *i, const std::string &k) : ioc(i), locked(false), key(k) { lock(); } diff --git a/src/unit_tests.cpp b/src/unit_tests.cpp index 21e0c345c..a774d42eb 100755 --- a/src/unit_tests.cpp +++ b/src/unit_tests.cpp @@ -125,7 +125,7 @@ void acceptConnection() sa.sun_family = AF_UNIX; memcpy(&sa.sun_path[1], "testing", 7); - int err = ::bind(serverSock, (struct sockaddr *) &sa, sizeof(sa)); + err = ::bind(serverSock, (struct sockaddr *) &sa, sizeof(sa)); assert(err == 0); err = ::listen(serverSock, 2); assert(err == 0); @@ -208,7 +208,6 @@ bool replicatorTest() Replicator *repli = Replicator::get(); int err,fd; const char *newobject = "newobjectTest"; - const char *newobjectJournal = "newobjectTest.journal"; string newObjectJournalFullPath = journalPath + "/" + "newobjectTest.journal"; uint8_t buf[1024]; uint8_t data[1024]; @@ -234,7 +233,7 @@ bool replicatorTest() fd = ::open(newObjectJournalFullPath.c_str(), O_RDONLY); err = ::read(fd, buf, 1024); - assert(err == (header.length() + 1 + 16 + 10)); + assert((uint) err == (header.length() + 1 + 16 + 10)); buf[err] = 0; assert(!strcmp("1234567890", (const char *) buf + header.length() + 1 + 16)); cout << "replicator addJournalEntry OK" << endl; @@ -271,22 +270,22 @@ bool metadataJournalTest(std::size_t size, off_t offset) hdr->type = SM_MSG_START; hdr->payloadLen = sizeof(*cmd) + cmd->flen + cmd->count; WriteTask w(clientSock, hdr->payloadLen); - int error = ::write(sessionSock, cmd, hdr->payloadLen); + int err = ::write(sessionSock, cmd, hdr->payloadLen); w.run(); // verify response - int err = ::recv(sessionSock, buf, 1024, MSG_DONTWAIT); + err = ::recv(sessionSock, buf, 1024, MSG_DONTWAIT); sm_response *resp = (sm_response *) buf; assert(err == sizeof(*resp)); assert(resp->header.type == SM_MSG_START); assert(resp->header.payloadLen == 4); assert(resp->header.flags == 0); - assert(resp->returnCode == size); + assert(resp->returnCode == (int) size); MetadataFile mdfTest(filename); mdfTest.printObjects(); - + return true; } void metadataJournalTestCleanup(std::size_t size) @@ -405,7 +404,7 @@ bool appendtask() return true; } -bool unlinktask() +void unlinktask() { // make a meta file and delete it const char *filename = "unlinktest1"; @@ -418,7 +417,6 @@ bool unlinktask() uint8_t buf[1024]; unlink_cmd *cmd = (unlink_cmd *) buf; - uint8_t *data; cmd->opcode = UNLINK; cmd->flen = strlen(filename); @@ -721,7 +719,7 @@ bool listdirtask() listdir_resp *r = (listdir_resp *) resp->payload; assert(r->elements == 10); int off = sizeof(sm_response) + sizeof(listdir_resp); - int fileCounter = 0; + uint fileCounter = 0; while (off < err) { listdir_resp_entry *e = (listdir_resp_entry *) &buf[off]; @@ -738,7 +736,7 @@ bool listdirtask() return true; } -bool pingtask() +void pingtask() { uint8_t buf[1024]; ping_cmd *cmd = (ping_cmd *) buf; @@ -850,7 +848,7 @@ bool cacheTest1() cache->exists(v_bogus, &exists); assert(exists.size() == 1); assert(exists[0]); - ssize_t currentSize = cache->getCurrentCacheSize(); + size_t currentSize = cache->getCurrentCacheSize(); assert(currentSize == bf::file_size(cachePath / realFile)); // lie about the file being deleted and then replaced @@ -866,10 +864,9 @@ bool cacheTest1() bf::remove(cachePath / realFile); bf::remove(storagePath / realFile); cout << "cache test 1 OK" << endl; + return true; } - - bool mergeJournalTest() { /* @@ -1005,7 +1002,6 @@ bool syncTest1() foundIt = (MetadataFile::getSourceFromKey(newKey) == "test-file"); if (foundIt) { - size_t fsize = bf::file_size(dir->path()); assert(cache->exists(newKey)); cs->deleteObject(newKey); break; @@ -1120,9 +1116,9 @@ void s3storageTest1() uint8_t *data1 = new uint8_t[len]; uint8_t *data2 = new uint8_t[len]; err = read(fd1, data1, len); - assert(err == len); + assert(err == (int) len); err = read(fd2, data2, len); - assert(err == len); + assert(err == (int) len); assert(!memcmp(data1, data2, len)); close(fd1); close(fd2); @@ -1194,7 +1190,7 @@ void IOCReadTest1() makeTestMetadata(metaFilename.c_str()); size_t objSize = bf::file_size(objFilename); err = ioc->read(testFile, data.get(), 0, 1<<20); - assert(err == objSize); + assert(err == (int) objSize); // verify the data int *data32 = (int *) data.get(); @@ -1207,7 +1203,7 @@ void IOCReadTest1() makeTestJournal(journalFilename.c_str()); err = ioc->read(testFile, data.get(), 0, 1<<20); - assert(err == objSize); + assert(err == (int) objSize); for (i = 0; i < 5; i++) assert(data32[i] == i); for (; i < 10; i++) @@ -1362,9 +1358,8 @@ void IOCCopyFile3() call ioc::copyFile() verify dest file exists */ - IOCoordinator *ioc = IOCoordinator::get(); + IOCoordinator *ioc = IOCoordinator::get(); Cache *cache = Cache::get(); - CloudStorage *cs = CloudStorage::get(); bf::path metaPath = ioc->getMetadataPath(); bf::path journalPath = ioc->getJournalPath();