--- a/cache.h	Sat Aug 09 01:00:35 2008 +0300
+++ b/cache.h	Sat Aug 09 14:42:59 2008 +0300
@@ -43,13 +43,16 @@
     CACHE_STATE_INVALID,
 
     CACHE_STATE_LOOKUP,
-    CACHE_STATE_WRITE_BEGIN,
-    CACHE_STATE_READ_BEGIN,
+    CACHE_STATE_HIT,
+    CACHE_STATE_MISS,
+
+    CACHE_STATE_OPEN_READ,
+    CACHE_STATE_READ,
+
+    CACHE_STATE_OPEN_WRITE,
     CACHE_STATE_WRITE,
     CACHE_STATE_WRITE_PAUSE,
-    CACHE_STATE_READ,
 
-    CACHE_STATE_DONE,
     CACHE_STATE_ERROR,
 };
 
@@ -128,7 +131,12 @@
 /*
  * Prepare this cache req for writing in the data. Hint, if nonzero, is used to pre-allocate resources for the entry.
  */
-int cache_req_begin_write(struct cache_req *req, size_t hint);
+int cache_req_begin_write (struct cache_req *req, size_t hint);
+
+/*
+ * Prepare this cache req for use with cache_req_pull.
+ */
+int cache_req_begin_read (struct cache_req *req);
 
 /*
  * Add some data into this cache entry, reading from the given fd. This is only valid for cache_req's in

--- a/cache/engine.h	Sat Aug 09 01:00:35 2008 +0300
+++ b/cache/engine.h	Sat Aug 09 14:42:59 2008 +0300
@@ -17,10 +17,15 @@
     /*
      * Return some information about the available data.
      */
-    int (*fn_op_available) (struct cache_op *op, size_t *size, size_t *offset);
+    int (*fn_op_available) (struct cache_op *, size_t *size, size_t *offset);
+    
+    /*
+     * Prepare to read from this cache entry.
+     */
+    int (*fn_op_begin_read) (struct cache_op *);
 
     /*
-     * Prepare to write and possibly read this cache entry.
+     * Prepare to write to this cache entry.
      *
      * size_hint, if nonzero, provides a guess at the size of the cache entry that can be used to optimize stuff
      */
@@ -31,18 +36,18 @@
      * bytes should be read, or zero to just read a suitable amount. Should be updated to how many bytes were actually
      * written (may be zero if writes are currently paused).
      */
-    int (*fn_op_push) (struct cache_op *op, int fd, size_t *size);
+    int (*fn_op_push) (struct cache_op *, int fd, size_t *size);
 
     /*
      * No more calls to fn_op_push will take place. The cache entry now contains the complete data.
      */
-    int (*fn_op_done) (struct cache_op *op);
+    int (*fn_op_done) (struct cache_op *);
 
     /*
      * The op is not needed for any operations anymore. Free any resources and memory associated with this op,
      * including the op itself.
      */
-    int (*fn_op_close) (struct cache_op *op);
+    int (*fn_op_close) (struct cache_op *);
 };
 
 #endif /* CACHE_ENGINE_H */

--- a/cache/engines/fs.c	Sat Aug 09 01:00:35 2008 +0300
+++ b/cache/engines/fs.c	Sat Aug 09 14:42:59 2008 +0300
@@ -158,7 +158,7 @@
         ERROR("calloc");
     
     // init it
-    if (cache_op_init(&op->base, cache, key))
+    if (_cache_op_init(&op->base, cache, key))
         goto error;
     
     // mark it as being in the lookup state...
@@ -181,7 +181,7 @@
     }
 
     // indicate that the key was found/not found
-    if (cache_op_lookup_done(&op->base, found))
+    if (_cache_op_lookup_done(&op->base, found))
         goto error;
     
     *op_ptr = &op->base;
@@ -232,7 +232,7 @@
         goto error;
     
     // great
-    if (cache_op_write_ready(&op->base))
+    if (_cache_op_write_ready(&op->base))
         goto error;
 
     // done
@@ -277,7 +277,7 @@
     *size_ptr = ret;
 
     // notify newly available data
-    if (cache_op_data_available(&op->base))
+    if (_cache_op_data_available(&op->base))
         goto error;
 
     // great
@@ -306,7 +306,7 @@
         goto error;
 
     // notify that data is complete
-    if (cache_op_write_done(&op->base))
+    if (_cache_op_write_done(&op->base))
         goto error;
 
     // great

--- a/cache/op.c	Sat Aug 09 01:00:35 2008 +0300
+++ b/cache/op.c	Sat Aug 09 14:42:59 2008 +0300
@@ -22,7 +22,7 @@
         WARNING("fn_op_close failed");
 }
 
-int cache_op_init (struct cache_op *op, struct cache *cache, struct cache_key *key) {
+int _cache_op_init (struct cache_op *op, struct cache *cache, struct cache_key *key) {
     op->cache = cache;
     op->key = key;
     op->state = OP_STATE_INVALID;
@@ -83,19 +83,35 @@
     return op->cache->engine->fn_op_available(op, size, offset);
 }
 
+int cache_op_begin_read (struct cache_op *op) {
+    assert(op->state == OP_STATE_HIT);
+
+    op->state = OP_STATE_OPEN_READ;
+
+    return op->cache->engine->fn_op_begin_read(op);
+}
+
 int cache_op_begin_write (struct cache_op *op, size_t size_hint) {
+    assert(op->state == OP_STATE_MISS);
+
+    op->state = OP_STATE_OPEN_WRITE;
+
     return op->cache->engine->fn_op_begin_write(op, size_hint);
 }
 
 int cache_op_push (struct cache_op *op, int fd, size_t *size) {
+    assert(op->state == OP_STATE_WRITE);
+
     return op->cache->engine->fn_op_push(op, fd, size);
 }
 
 int cache_op_done (struct cache_op *op) {
+    assert(op->state == OP_STATE_WRITE);
+
     return op->cache->engine->fn_op_done(op);
 }
 
-int cache_op_lookup_done (struct cache_op *op, int found) {
+int _cache_op_lookup_done (struct cache_op *op, int found) {
     // modify state
     op->state = found ? OP_STATE_HIT : OP_STATE_MISS;
 
@@ -103,7 +119,7 @@
     return _cache_op_notify(op);
 }
 
-int cache_op_write_ready (struct cache_op *op) {
+int _cache_op_write_ready (struct cache_op *op) {
     // modify state
     op->state = OP_STATE_WRITE;
 
@@ -111,12 +127,12 @@
     return _cache_op_notify(op);
 }
 
-int cache_op_data_available (struct cache_op *op) {
+int _cache_op_data_available (struct cache_op *op) {
     // notify waiting reqs
     return _cache_op_notify(op);
 }
 
-int cache_op_write_done (struct cache_op *op) {
+int _cache_op_write_done (struct cache_op *op) {
     op->state = OP_STATE_READ;
 
     // notify waiting reqs

--- a/cache/op.h	Sat Aug 09 01:00:35 2008 +0300
+++ b/cache/op.h	Sat Aug 09 14:42:59 2008 +0300
@@ -3,37 +3,83 @@
 
 #include <sys/queue.h>
 
+// this duplicates the cache_state enum, but is indeed interpreted differently:
 enum cache_op_state {
+    /*
+     * The op is still in the state of being constructed, and it not yet valid.
+     *
+     * This should not be encountered other than if the callback is called from within fn_op_start.
+     */
     OP_STATE_INVALID,
-    OP_STATE_LOOKUP,
 
+    /*
+     * In the process of looking up the key. The op should enter this state as soon as it is constructed.
+     */
+    OP_STATE_LOOKUP,
+    
+    /*
+     * The key was not found in the cache. You should call cache_op_begin_write next.
+     */
     OP_STATE_MISS,
+
+    /*
+     * The key was found in the cache. You should call cache_op_begin_read next.
+     */
     OP_STATE_HIT,
+ 
+    /*
+     * cache_op_begin_read has been called, but the op is not yet read for cache_op_pull.
+     */
+    OP_STATE_OPEN_READ,
+   
+    /*
+     * cache_op_begin_write has been called, but the op is not yet read for cache_op_push.
+     */
+    OP_STATE_OPEN_WRITE,
 
+    /*
+     * cache_op_begin_read has completed succesfully, and you may now call cache_op_pull.
+     */
     OP_STATE_READ,
+
+    /*
+     * cache_op_begin_write has completed succesfully, and you may now call cache_op_push/cache_op_done.
+     */
     OP_STATE_WRITE,
+    
+    /*
+     * XXX: not used yet
+     */
+    OP_STATE_ERROR,
 };
 
 struct cache_op {
+    // reference to what cache this belongs to
     struct cache *cache;
-
+    
+    // used to store this in the cache->op_list
     LIST_ENTRY(cache_op) node;
-
+    
+    // a pointer to the key we are using
+    // XXX: object lifetime is currently broken (cache_req.key_copy of the first req atm)
+    // XXX: the key should probably be copied into this.
     struct cache_key *key;
-
+    
+    // the list of cache_req's that are using this op.
     LIST_HEAD(cache_op_req_list_head, cache_req) req_list;
-
+    
+    // our current state.
     enum cache_op_state state;
 };
 
 /*
- * Initialize the basic cache op state. Refcount is set to one.
+ * Used by the engine's fn_op_start implementation to initialize the basic cache op state.
  */
-int cache_op_init(struct cache_op *op, struct cache *cache, struct cache_key *key);
+int _cache_op_init(struct cache_op *op, struct cache *cache, struct cache_key *key);
 
 /*
- * Look for an existing cache_op in the given cache. Return NULL if not found, return pointer and increment
- * refcount if found.
+ * Look for an existing cache_op with the given key in the given cache. Return NULL if not found, return pointer
+ * if found.
  */
 struct cache_op *cache_op_find (struct cache *cache, struct cache_key *key);
 
@@ -43,30 +89,88 @@
 int cache_op_register (struct cache_op *op, struct cache_req *req);
 
 /*
- * The given req is not interested in this op anymore. The op is removed if it was the only req
+ * The given req is done and not using this op anymore. The op is closed if it was the only req left
+ *
+ * The op would normally be in the OP_STATE_READ state.
  */
 int cache_op_deregister (struct cache_op *op, struct cache_req *req);
 
+/*
+ * Get info about the data available in this cache entry.
+ *  size    - total size of the cache entry. 0 if unknown (not yet fully written).
+ *  offset  - how many bytes of the cache entry are currently stored. May be 0
+ *
+ *  size will be nonzero in the (OP_STATE_HIT | (OP_STATE_MISS -> OP_STATE_WRITE) -> ) OP_STATE_READ state.
+ *
+ *  This is valid in all states except OP_STATE_INVALID, OP_STATE_LOOKUP and OP_STATE_ERROR, but both values may very
+ *  well be zero in many of these states.
+ */
 int cache_op_available (struct cache_op *op, size_t *size, size_t *offset);
 
 /*
- * Prepare op for writing data to it, size_hint can be used to preallocate resources
+ * Prepare op for reading the data from it.
+ *
+ * The op must be in the OP_SATE_HIT state, and will move ino the OP_STATE_OPEN_READ state, possibly also
+ * directly into the OP_STATE_READ state.
+ */
+int cache_op_begin_read (struct cache_op *op);
+
+/*
+ * Prepare op for writing data to it. If size_hint is nonzero, it is used to optimize resource preallocation.
+ *
+ * The op must be in the OP_STATE_MISS state, and will move into the OP_STATE_OPEN_WRITE state, possibly also
+ * directly into the OP_STATE_WRITE state.
  */
 int cache_op_begin_write (struct cache_op *op, size_t size_hint);
 
+/*
+ * Write some data into the cache by reading from the given fd. Size, if nonzero, tells how many bytes of data to read,
+ * otherwise, as much data as possible will be read. Size will be updated to the number of bytes actually pushed on
+ * return. This may be zero bytes (XXX: OP_STATE_PAUSE_WRITE).
+ *
+ * The op must be in the OP_STATE_WRITE state. All registered reqs will be notified, even if zero bytes were pushed.
+ */
 int cache_op_push (struct cache_op *op, int fd, size_t *size);
+
+/*
+ * Indicate that the freshly written cache entry is now complete. This should be called after the last cache_op_push
+ * call, and no more cache_op_push calls may ensue.
+ *
+ * The op must be in the OP_STATE_WRITE state, and will move into the OP_STATE_READ state (i.e. the op may still be
+ * read from).
+ */
 int cache_op_done (struct cache_op *op);
 
 /*
- * Used by the engines to notify that the key lookup completed
+ * Used by the engines to notify of state transitions
  */
-int cache_op_lookup_done (struct cache_op *op, int found);
 
 /*
- * in OP_STATE_WRITE
+ * cache_op_start completed, and the cache op is ready for begin_read/begin_write, based on `found`.
+ *
+ * OP_STATE_LOOKUP -> (OP_STATE_HIT | OP_STATE_MISS)
  */
-int cache_op_write_ready (struct cache_op *op);
-int cache_op_data_available (struct cache_op *op);
-int cache_op_write_done (struct cache_op *op);
+int _cache_op_lookup_done (struct cache_op *op, int found);
+
+/*
+ * cache_op_begin_write completed, and the cache op is ready for cache_op_push.
+ *
+ * OP_STATE_OPEN_WRITE -> OP_STATE_WRITE
+ */
+int _cache_op_write_ready (struct cache_op *op);
+
+/*
+ * There is new data available for reading.
+ *
+ * OP_STATE_READ | OP_STATE_WRITE 
+ */
+int _cache_op_data_available (struct cache_op *op);
+
+/*
+ * cache_op_done completed, and the cache op now contains all the data that it ever will.
+ *
+ * OP_STATE_WRITE -> OP_STATE_READ
+ */
+int _cache_op_write_done (struct cache_op *op);
 
 #endif /* CACHE_OP_H */

--- a/cache/req.c	Sat Aug 09 01:00:35 2008 +0300
+++ b/cache/req.c	Sat Aug 09 14:42:59 2008 +0300
@@ -50,8 +50,8 @@
         // none available, start a new cache op
         if (cache->engine->fn_op_start(cache, &req->op, req->key))
             goto error;
-
-        // since we were the one that created it, we take care of writing it
+        
+        // since we were the one that created it, we take care of writing it if needed
         req->is_write = 1;
         
     } else {
@@ -139,17 +139,27 @@
             return CACHE_STATE_LOOKUP;
         
         case OP_STATE_HIT:
-            return CACHE_STATE_READ_BEGIN;
+            return CACHE_STATE_HIT;
 
         case OP_STATE_MISS:
-            return req->is_write ? CACHE_STATE_WRITE_BEGIN : CACHE_STATE_READ_BEGIN;
+            return req->is_write ? CACHE_STATE_MISS : CACHE_STATE_OPEN_READ;
         
+        case OP_STATE_OPEN_READ:
+            return CACHE_STATE_OPEN_READ;
+        
+        case OP_STATE_OPEN_WRITE:
+            return req->is_write ? CACHE_STATE_OPEN_WRITE : CACHE_STATE_READ;
+
         case OP_STATE_READ:
             return CACHE_STATE_READ;
 
         case OP_STATE_WRITE:
             return req->is_write ? CACHE_STATE_WRITE : CACHE_STATE_READ;
         
+        case OP_STATE_ERROR:
+            // XXX: error handling
+            assert(0);
+
         default:
             assert(0);
     }
@@ -160,12 +170,13 @@
 }
 
 int cache_req_available (struct cache_req *req, size_t *size, size_t *offset, size_t *available) {
-    if (req->op->state != OP_STATE_READ && req->op->state != OP_STATE_WRITE)
+    if (req->op->state == OP_STATE_INVALID || req->op->state == OP_STATE_LOOKUP || req->op->state == OP_STATE_ERROR)
         ERROR("req is not readable");
 
     if (cache_op_available(req->op, size, offset))
         goto error;
-
+    
+    // this is based on our own read offset
     *available = (*offset - req->read_offset);
 
     return 0;
@@ -184,6 +195,27 @@
     return -1;
 }
 
+int cache_req_begin_read (struct cache_req *req) {
+    if (req->op->state == OP_STATE_HIT) {
+        // needs to be done
+        return cache_op_begin_read(req->op);
+
+    } else if (req->op->state == OP_STATE_OPEN_READ || req->op->state == OP_STATE_READ) {
+        WARNING("req is already in OP_STATE_READ/OP_STATE_OPEN_READ");
+
+    } else if (req->op->state == OP_STATE_OPEN_WRITE || req->op->state == OP_STATE_WRITE) {
+        WARNING("req is already in OP_STATE_WRITE/OP_STATE_OPEN_WRITE");
+
+    } else {
+        ERROR("req is not readable");
+    }
+
+    return 0;
+
+error:
+    return -1;
+}
+
 int cache_req_push (struct cache_req *req, int fd, size_t *size) {
     if (req->op->state != OP_STATE_WRITE || !req->is_write)
         ERROR("req not in write mode");

--- a/cache/req.h	Sat Aug 09 01:00:35 2008 +0300
+++ b/cache/req.h	Sat Aug 09 14:42:59 2008 +0300
@@ -7,18 +7,27 @@
 #include "op.h"
 
 struct cache_req {
+    // a pointer to the relevant key
     struct cache_key *key;
-    struct cache_key key_copy;  // XXX: who keeps the copy around?
+
+    // a copy
+    // XXX: this should be moved to cache_op
+    struct cache_key key_copy;
     
+    // we are part of cache_op.req_list
     LIST_ENTRY(cache_req) node;
     
+    // the user callback
     cache_callback cb_func;
     void *cb_data;
     
+    // the op we are using
     struct cache_op *op;
-
+    
+    // were we the origional req that created the op, i.e. the one that handles open_write in case of OP_STATE_MISS?
     int is_write;
-
+    
+    // our own read offset into the cache entry.
     off_t read_offset;
 };
 

--- a/cache_test.c	Sat Aug 09 01:00:35 2008 +0300
+++ b/cache_test.c	Sat Aug 09 14:42:59 2008 +0300
@@ -29,13 +29,18 @@
 const char *cache_state_str (enum cache_req_state state) {
     switch (state) {
         case CACHE_STATE_INVALID:       return "INVALID";
+
         case CACHE_STATE_LOOKUP:        return "LOOKUP";
-        case CACHE_STATE_READ_BEGIN:    return "READ_BEGIN";
-        case CACHE_STATE_WRITE_BEGIN:   return "WRITE_BEGIN";
+        case CACHE_STATE_HIT:           return "HIT";
+        case CACHE_STATE_MISS:          return "MISS";
+
+        case CACHE_STATE_OPEN_READ:     return "OPEN_READ";
+        case CACHE_STATE_READ:          return "READ";
+
+        case CACHE_STATE_OPEN_WRITE:    return "OPEN_WRITE";
         case CACHE_STATE_WRITE:         return "WRITE";
         case CACHE_STATE_WRITE_PAUSE:   return "WRITE_PAUSE";
-        case CACHE_STATE_READ:          return "READ";
-        case CACHE_STATE_DONE:          return "DONE";
+
         case CACHE_STATE_ERROR:         return "ERROR";
         default:                        return "???";
     }