Skip to content

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history 
Signed-off-by: Cole Miller <[email protected]>
  • Loading branch information
@cole-miller
cole-miller committed Apr 16, 2024
1 parent abb9b39 commit c7464b5
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 27 deletions.
5 changes: 5 additions & 0 deletions src/utils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#define DQLITE_UTILS_H_

#include <assert.h>
#include <stdbool.h>
#include <stdint.h>

/* Various utility functions and macros */
Expand All @@ -21,4 +22,8 @@
#define POST(cond) assert((cond))
#define ERGO(a, b) (!(a) || (b))

static inline bool is_po2(unsigned long n) {
return n > 0 && (n & (n - 1)) == 0;
}

#endif /* DQLITE_UTILS_H_ */
73 changes: 46 additions & 27 deletions src/vfs2.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ static uint32_t native_magic(void)
return is_bigendian() ? BE_MAGIC : LE_MAGIC;

Check warning on line 120 in src/vfs2.c

View check run for this annotation

Codecov / codecov/patch

src/vfs2.c#L120 

Added line #L120 was not covered by tests
}

void update_cksums(uint32_t magic, const uint8_t *p, size_t len, struct cksums *sums)
static void update_cksums(uint32_t magic, const uint8_t *p, size_t len, struct cksums *sums)
{
PRE(magic == BE_MAGIC || magic == LE_MAGIC);
PRE(len % 8 == 0);
Expand All @@ -136,7 +136,7 @@ void update_cksums(uint32_t magic, const uint8_t *p, size_t len, struct cksums *
}
}

bool cksums_equal(struct cksums a, struct cksums b)
static bool cksums_equal(struct cksums a, struct cksums b)
{
return a.cksum1 == b.cksum1 && a.cksum2 == b.cksum2;
}
Expand Down Expand Up @@ -203,6 +203,12 @@ struct entry {
/* e.g. /path/to/some.db */
char *main_db_name;

/* The WALs are represented by two physical files (inodes)
* and three filenames. For each of the two physical files
* there is a "fixed name" that always points to that file.
* The "moving name" always points to one of the two physical
* files, but switches between them on every WAL swap. */

/* e.g. /path/to/some.db-wal */
char *wal_moving_name;
/* e.g. /path/to/some.db-xwal1 */
Expand Down Expand Up @@ -236,7 +242,8 @@ struct entry {
/* Zero for unlocked, positive for read-locked, UINT_MAX for write-locked */
unsigned shm_locks[SQLITE_SHM_NLOCK];

/* For ACTIVE, HIDDEN: the pending txn */
/* For ACTIVE, HIDDEN: the pending txn. start and len
* are in units of frames. */
struct vfs2_wal_frame *pending_txn_frames;
uint32_t pending_txn_start;
uint32_t pending_txn_len;
Expand Down Expand Up @@ -305,12 +312,12 @@ static struct wal_index_full_hdr *get_full_hdr(struct entry *e)
return e->shm_regions[0];
}

static bool no_pending_txn(struct entry *e)
static bool no_pending_txn(const struct entry *e)
{
return e->pending_txn_len == 0 && e->pending_txn_frames == NULL && e->pending_txn_last_frame_commit == 0;
}

static bool write_lock_held(struct entry *e)
static bool write_lock_held(const struct entry *e)
{
return e->shm_locks[WAL_WRITE_LOCK] == VFS2_EXCLUSIVE;
}
Expand Down Expand Up @@ -339,8 +346,10 @@ static bool wal_index_basic_hdr_advanced(struct wal_index_basic_hdr new,
&& new.mxFrame > old.mxFrame;
}

/* TODO this is very weak, should check that the hash tables have been constructed */
static bool wal_index_recovered(struct entry *e)
/* Check that the hash tables in the WAL index have been initialized
* by looking for nonzero bytes after the WAL index header. (TODO:
* actually parse the hash tables?) */
static bool wal_index_recovered(const struct entry *e)
{
PRE(e->shm_regions_len > 0);
char *p = e->shm_regions[0];
Expand All @@ -354,10 +363,10 @@ static bool wal_index_recovered(struct entry *e)

static bool is_valid_page_size(unsigned long n)
{
return n >= 1 << 9 && n <= 1 << 16 && (n & (n - 1)) == 0;
return n >= 1 << 9 && n <= 1 << 16 && is_po2(n);
}

static bool is_open(struct entry *e)
static bool is_open(const struct entry *e)
{
return e->main_db_name != NULL
&& e->wal_moving_name != NULL
Expand All @@ -381,7 +390,7 @@ static bool basic_hdr_valid(struct wal_index_basic_hdr bhdr)
&& cksums_equal(sums, bhdr.cksums);
}

static bool full_hdr_valid(struct wal_index_full_hdr *ihdr)
static bool full_hdr_valid(const struct wal_index_full_hdr *ihdr)
{
return basic_hdr_valid(ihdr->basic[0]) && wal_index_basic_hdr_equal(ihdr->basic[0], ihdr->basic[1]);
}
Expand Down Expand Up @@ -494,11 +503,7 @@ static int check_wal_integrity(sqlite3_file *f)

static sqlite3_file *get_orig(struct file *f)
{
if (f->flags & SQLITE_OPEN_WAL) {
return f->entry->wal_cur;
} else {
return f->orig;
}
return (f->flags & SQLITE_OPEN_WAL) ? f->entry->wal_cur : f->orig;
}

static void maybe_close_entry(struct entry *e)
Expand Down Expand Up @@ -568,6 +573,9 @@ static int wal_swap(struct entry *e,

e->page_size = ByteGetBe32(wal_hdr->page_size);

/* Terminology: the outgoing WAL is the one that's moving
* from cur to prev. The incoming WAL is the one that's moving
* from prev to cur. */
sqlite3_file *phys_outgoing = e->wal_cur;
char *name_outgoing = e->wal_cur_fixed_name;
sqlite3_file *phys_incoming = e->wal_prev;
Expand Down Expand Up @@ -666,11 +674,10 @@ static int vfs2_wal_post_write(struct entry *e,
{
uint32_t frame_size = VFS2_WAL_FRAME_HDR_SIZE + e->page_size;
if (amt == VFS2_WAL_FRAME_HDR_SIZE) {
sqlite3_int64 x =
ofst - (sqlite3_int64)sizeof(struct wal_hdr);
assert(x % frame_size == 0);
x /= frame_size;
return vfs2_wal_write_frame_hdr(e, buf, (uint32_t)x);
ofst -= (sqlite3_int64)sizeof(struct wal_hdr);
assert(ofst % frame_size == 0);
sqlite3_int64 frame_ofst = ofst / (sqlite3_int64)frame_size;
return vfs2_wal_write_frame_hdr(e, buf, (uint32_t)frame_ofst);
} else if (amt == (int)e->page_size) {
sqlite3_int64 x = ofst - VFS2_WAL_FRAME_HDR_SIZE -
(sqlite3_int64)sizeof(struct wal_hdr);
Expand Down Expand Up @@ -912,12 +919,9 @@ static __attribute__((noinline)) int busy(void)
static int vfs2_shm_lock(sqlite3_file *file, int ofst, int n, int flags)
{
struct file *xfile = (struct file *)file;
PRE(xfile != NULL);
struct entry *e = xfile->entry;

assert(file != NULL);
assert(ofst >= 0);
assert(n >= 0);

assert(ofst >= 0 && ofst + n <= SQLITE_SHM_NLOCK);
assert(n >= 1);
assert(n == 1 || (flags & SQLITE_SHM_EXCLUSIVE) != 0);
Expand Down Expand Up @@ -1087,7 +1091,7 @@ static struct wal_index_full_hdr initial_full_hdr(struct wal_hdr whdr)
ihdr.basic[0].bigEndCksum = is_bigendian();
ihdr.basic[0].szPage = (uint16_t)ByteGetBe32(whdr.page_size);
struct cksums sums = {};
update_cksums(native_magic(), (const void *)&ihdr.basic[0], 40, &sums);
update_cksums(native_magic(), (const void *)&ihdr.basic[0], offsetof(struct wal_index_basic_hdr, cksums), &sums);
ihdr.basic[0].cksums = sums;
ihdr.basic[1] = ihdr.basic[0];
ihdr.marks[0] = 0;
Expand Down Expand Up @@ -1725,7 +1729,7 @@ static int write_one_frame(struct entry *e, struct wal_frame_hdr hdr, void *data
return SQLITE_OK;

Check warning on line 1729 in src/vfs2.c

View check run for this annotation

Codecov / codecov/patch

src/vfs2.c#L1728-L1729 

Added lines #L1728 - L1729 were not covered by tests
}

static struct wal_hdr next_wal_hdr(struct entry *e)
static struct wal_hdr next_wal_hdr(const struct entry *e)

Check warning on line 1732 in src/vfs2.c

View check run for this annotation

Codecov / codecov/patch

src/vfs2.c#L1732 

Added line #L1732 was not covered by tests
{
struct wal_hdr ret;
struct wal_hdr old = e->wal_cur_hdr;
Expand Down Expand Up @@ -1773,7 +1777,17 @@ int vfs2_apply_uncommitted(sqlite3_file *file, uint32_t page_size, const struct
PRE(page_size == e->page_size);
int rv;

Check warning on line 1778 in src/vfs2.c

View check run for this annotation

Codecov / codecov/patch

src/vfs2.c#L1778 

Added line #L1778 was not covered by tests

/* It's okay if the write lock is held already */
/* The write lock is always held if there is at least one
* uncommitted frame in WAL-cur. In FOLLOWING state, we allow
* adding more frames to WAL-cur even if there are already
* some uncommitted frames. Hence we don't check the write
* lock here before "acquiring" it, we just make sure that
* it's held before returning.
*
* The write lock will be released when a call to vfs2_commit
* or vfs2_unapply causes the number of committed frames in
* WAL-cur (mxFrame) to equal the number of applies frames
* (wal_cursor). */
e->shm_locks[WAL_WRITE_LOCK] = VFS2_EXCLUSIVE;

Check warning on line 1791 in src/vfs2.c

View check run for this annotation

Codecov / codecov/patch

src/vfs2.c#L1791 

Added line #L1791 was not covered by tests

struct wal_index_full_hdr *ihdr = get_full_hdr(e);
Expand Down Expand Up @@ -1829,8 +1843,13 @@ int vfs2_apply_uncommitted(sqlite3_file *file, uint32_t page_size, const struct
return 0;

Check warning on line 1843 in src/vfs2.c

View check run for this annotation

Codecov / codecov/patch

src/vfs2.c#L1840-L1843 

Added lines #L1840 - L1843 were not covered by tests
}

/* Get the index of the `i`th read lock in the array of
* shm locks. There are five read locks, and three non-read
* locks that come before the read locks.
* See https://sqlite.org/walformat.html#wal_locks. */
static unsigned read_lock(unsigned i)

Check warning on line 1850 in src/vfs2.c

View check run for this annotation

Codecov / codecov/patch

src/vfs2.c#L1850 

Added line #L1850 was not covered by tests
{
PRE(i < 5);
return 3 + i;

Check warning on line 1853 in src/vfs2.c

View check run for this annotation

Codecov / codecov/patch

src/vfs2.c#L1853 

Added line #L1853 was not covered by tests
}

Expand Down

0 comments on commit c7464b5

Please sign in to comment.