From 97cbebc91e36a4d5cbd2b8248d4c460ce95919f3 Mon Sep 17 00:00:00 2001 From: Andrew Bettison Date: Fri, 11 Oct 2013 16:06:12 +1030 Subject: [PATCH] Issue #11: improve types in file i/o functions Use open(2)/read(2)/write(2) instead of fopen(3)/fread(3)/fwrite(3) in several places to avoid unnecessary buffering Fix a bug in Rhizome HTTP add's handling of unconfigured manifest template file Improve some debug and error logging for file i/o --- commandline.c | 2 +- net.c | 8 +++--- rhizome.h | 2 +- rhizome_bundle.c | 57 +++++++++++++++++++++-------------------- rhizome_database.c | 4 +-- rhizome_direct_http.c | 13 ++++++---- rhizome_fetch.c | 3 +-- rhizome_packetformats.c | 6 ++--- 8 files changed, 49 insertions(+), 46 deletions(-) diff --git a/commandline.c b/commandline.c index 877314f7..74ed1da3 100644 --- a/commandline.c +++ b/commandline.c @@ -1559,7 +1559,7 @@ int app_rhizome_append_manifest(const struct cli_parsed *parsed, struct cli_cont return WHY("Out of manifests."); int ret=0; - if (rhizome_read_manifest_file(m, manifestpath, 0)) + if (rhizome_read_manifest_file(m, manifestpath, 0) == -1) ret=-1; // TODO why doesn't read manifest file set finalised??? m->finalised=1; diff --git a/net.c b/net.c index 6f6f81c5..a26acc8a 100644 --- a/net.c +++ b/net.c @@ -77,11 +77,11 @@ ssize_t _write_all(int fd, const void *buf, size_t len, struct __sourceloc __whe { ssize_t written = write(fd, buf, len); if (written == -1) - return WHYF_perror("write_all: write(%d,%p %s,%lu)", - fd, buf, alloca_toprint(30, buf, len), (unsigned long)len); + return WHYF_perror("write_all: write(%d,%p %s,%zu)", + fd, buf, alloca_toprint(30, buf, len), len); if (written != len) - return WHYF_perror("write_all: write(%d,%p %s,%lu) returned %ld", - fd, buf, alloca_toprint(30, buf, len), (unsigned long)len, (long)written); + return WHYF_perror("write_all: write(%d,%p %s,%zu) returned %zd", + fd, buf, alloca_toprint(30, buf, len), len, (size_t)written); return written; } diff --git a/rhizome.h b/rhizome.h index bc21f05b..fa64ee98 100644 --- a/rhizome.h +++ b/rhizome.h @@ -288,7 +288,7 @@ int rhizome_write_manifest_file(rhizome_manifest *m, const char *filename, char int rhizome_manifest_selfsign(rhizome_manifest *m); int rhizome_drop_stored_file(const rhizome_filehash_t *hashp, int maximum_priority); int rhizome_manifest_priority(sqlite_retry_state *retry, const rhizome_bid_t *bidp); -int rhizome_read_manifest_file(rhizome_manifest *m, const char *filename, int bufferPAndSize); +int rhizome_read_manifest_file(rhizome_manifest *m, const char *filename, size_t bufferPAndSize); int rhizome_hash_file(rhizome_manifest *m, const char *path, rhizome_filehash_t *hash_out, uint64_t *size_out); char *rhizome_manifest_get(const rhizome_manifest *m, const char *var, char *out, int maxlen); int64_t rhizome_manifest_get_ll(rhizome_manifest *m, const char *var); diff --git a/rhizome_bundle.c b/rhizome_bundle.c index 3eedbab3..cb9400eb 100644 --- a/rhizome_bundle.c +++ b/rhizome_bundle.c @@ -85,16 +85,16 @@ int rhizome_manifest_verify(rhizome_manifest *m) else return 0; } -int read_whole_file(const char *filename, unsigned char *buffer, int buffer_size) +ssize_t read_whole_file(const char *path, unsigned char *buffer, size_t buffer_size) { - FILE *f = fopen(filename, "r"); - if (f == NULL) - return WHYF("Could not open file %s for reading", filename); - int ret = fread(buffer, 1, buffer_size, f); + int fd = open(path, O_RDONLY); + if (fd == -1) + return WHYF_perror("open(%s,O_RDONLY)", alloca_str_toprint(path)); + ssize_t ret = read(fd, buffer, buffer_size); if (ret == -1) - ret = WHY_perror("fread"); - if (fclose(f) == EOF) - ret = WHY_perror("fclose"); + ret = WHYF_perror("read(%s,%u)", alloca_str_toprint(path), buffer_size); + if (close(fd) == -1) + ret = WHY_perror("close"); return ret; } @@ -325,7 +325,7 @@ int rhizome_manifest_parse(rhizome_manifest *m) OUT(); } -int rhizome_read_manifest_file(rhizome_manifest *m, const char *filename, int bufferP) +int rhizome_read_manifest_file(rhizome_manifest *m, const char *filename, size_t bufferP) { if (!m) return WHY("Null manifest"); @@ -336,9 +336,10 @@ int rhizome_read_manifest_file(rhizome_manifest *m, const char *filename, int bu m->manifest_bytes=bufferP; memcpy(m->manifestdata, filename, m->manifest_bytes); } else { - m->manifest_bytes = read_whole_file(filename, m->manifestdata, sizeof(m->manifestdata)); - if (m->manifest_bytes == -1) + ssize_t bytes = read_whole_file(filename, m->manifestdata, sizeof m->manifestdata); + if (bytes == -1) return -1; + m->manifest_bytes = bytes; } return rhizome_manifest_parse(m); } @@ -643,30 +644,30 @@ int rhizome_manifest_selfsign(rhizome_manifest *m) return 0; } -int rhizome_write_manifest_file(rhizome_manifest *m, const char *filename, char append) +int rhizome_write_manifest_file(rhizome_manifest *m, const char *path, char append) { - if (config.debug.rhizome) DEBUGF("write manifest (%d bytes) to %s", m->manifest_all_bytes, filename); - if (!m) return WHY("Manifest is null."); - if (!m->finalised) return WHY("Manifest must be finalised before it can be written."); - FILE *f = fopen(filename, (append?"a":"w")); - if (f == NULL) - return WHYF_perror("Cannot write manifest to %s", filename); - + if (config.debug.rhizome) + DEBUGF("write manifest (%d bytes) to %s", m->manifest_all_bytes, path); + if (!m) + return WHY("Manifest is null."); + if (!m->finalised) + return WHY("Manifest must be finalised before it can be written."); + int fd = open(path, O_WRONLY | O_CREAT | (append ? O_APPEND : 0), 0666); + if (fd == -1) + return WHYF_perror("open(%s,O_WRONLY|O_CREAT%s,0666)", alloca_str_toprint(path), append ? "|O_APPEND" : ""); int ret = 0; - if (fwrite(m->manifestdata, m->manifest_all_bytes, 1, f)!=1) - ret=WHYF_perror("fwrite(%s)", filename); - - if (ret==0 && append){ + if (write_all(fd, m->manifestdata, m->manifest_all_bytes) == -1) + ret = -1; + else if (append) { unsigned char marker[4]; write_uint16(marker, m->manifest_all_bytes); marker[2]=0x41; marker[3]=0x10; - if (fwrite(marker, 4,1,f)!=1) - ret=WHYF_perror("fwrite(%s)", filename); + if (write_all(fd, marker, sizeof marker) == -1) + ret = -1; } - - if (fclose(f)) - ret=WHYF_perror("fclose(%s)", filename); + if (close(fd) == -1) + ret = WHY_perror("close"); return ret; } diff --git a/rhizome_database.c b/rhizome_database.c index d73fc360..d559cd15 100644 --- a/rhizome_database.c +++ b/rhizome_database.c @@ -151,7 +151,7 @@ void verify_bundles(){ while(sqlite_step_retry(&retry, statement)==SQLITE_ROW){ sqlite3_int64 rowid = sqlite3_column_int64(statement, 0); const void *manifest = sqlite3_column_blob(statement, 1); - int manifest_length = sqlite3_column_bytes(statement, 1); + size_t manifest_length = sqlite3_column_bytes(statement, 1); rhizome_manifest *m=rhizome_new_manifest(); int ret=0; @@ -1734,7 +1734,7 @@ static int unpack_manifest_row(rhizome_manifest *m, sqlite3_stmt *statement) int64_t q_inserttime = sqlite3_column_int64(statement, 3); const char *q_author = (const char *) sqlite3_column_text(statement, 4); size_t q_blobsize = sqlite3_column_bytes(statement, 1); // must call after sqlite3_column_blob() - if (rhizome_read_manifest_file(m, q_blob, q_blobsize)) + if (rhizome_read_manifest_file(m, q_blob, q_blobsize) == -1) return WHYF("Manifest %s exists but is invalid", q_id); if (q_author) { if (str_to_sid_t(&m->author, q_author) == -1) diff --git a/rhizome_direct_http.c b/rhizome_direct_http.c index 72229568..ef6a9430 100644 --- a/rhizome_direct_http.c +++ b/rhizome_direct_http.c @@ -208,11 +208,14 @@ int rhizome_direct_form_received(rhizome_http_request *r) snprintf(filepath,1024,"rhizomedirect.%d.data",r->alarm.poll.fd); char manifestTemplate[1024]; - strbuf b = strbuf_local(manifestTemplate, sizeof manifestTemplate); - strbuf_path_join(b, serval_instancepath(), config.rhizome.api.addfile.manifest_template_file, NULL); - if (manifestTemplate[0] && access(manifestTemplate, R_OK) != 0) { - rhizome_direct_clear_temporary_files(r); - return rhizome_server_simple_http_response(r,500,"rhizome.api.addfile.manifesttemplate points to a file I could not read."); + manifestTemplate[0] = '\0'; + if (config.rhizome.api.addfile.manifest_template_file[0]) { + strbuf b = strbuf_local(manifestTemplate, sizeof manifestTemplate); + strbuf_path_join(b, serval_instancepath(), config.rhizome.api.addfile.manifest_template_file, NULL); + if (access(manifestTemplate, R_OK) != 0) { + rhizome_direct_clear_temporary_files(r); + return rhizome_server_simple_http_response(r,500,"rhizome.api.addfile.manifest_template_file points to a file I could not read."); + } } rhizome_manifest *m = rhizome_new_manifest(); diff --git a/rhizome_fetch.c b/rhizome_fetch.c index b66a9c38..20a51f26 100644 --- a/rhizome_fetch.c +++ b/rhizome_fetch.c @@ -1287,8 +1287,7 @@ int rhizome_write_complete(struct rhizome_fetch_slot *slot) call schedule queued items. */ rhizome_manifest *m = rhizome_new_manifest(); if (m) { - if (rhizome_read_manifest_file(m, slot->manifest_buffer, - slot->manifest_bytes) == -1) { + if (rhizome_read_manifest_file(m, slot->manifest_buffer, (size_t)slot->manifest_bytes) == -1) { DEBUGF("Couldn't read manifest"); rhizome_manifest_free(m); } else { diff --git a/rhizome_packetformats.c b/rhizome_packetformats.c index 2c5ca869..0dbf4477 100644 --- a/rhizome_packetformats.c +++ b/rhizome_packetformats.c @@ -290,7 +290,7 @@ int overlay_rhizome_saw_advertisements(int i, struct decode_context *context, st int ad_frame_type=ob_get(f->payload); struct sockaddr_in httpaddr = context->addr; httpaddr.sin_port = htons(RHIZOME_HTTP_PORT); - int manifest_length; + size_t manifest_length; rhizome_manifest *m=NULL; int (*oldfunc)() = sqlite_set_tracefunc(is_debug_rhizome_ads); @@ -307,12 +307,12 @@ int overlay_rhizome_saw_advertisements(int i, struct decode_context *context, st break; } - manifest_length=ob_get_ui16(f->payload); + manifest_length = ob_get_ui16(f->payload); if (manifest_length==0) continue; unsigned char *data = ob_get_bytes_ptr(f->payload, manifest_length); if (!data) { - WHYF("Illegal manifest length field in rhizome advertisement frame %d vs %d.", + WHYF("Illegal manifest length field in rhizome advertisement frame %zu vs %d", manifest_length, f->payload->sizeLimit - f->payload->position); break; }