librbd: change diff_iterate interface to be more C-friendly

Use int instead of bool for the callback, and make it represent
whether the data exists, rather than the opposite, since callers
are likely to test for whether it's data instead of whether its zeroes.

Change the return value to 0, since an int64_t will wrap around
for large reads, and there's no value in reporting the length
read when it will always be the length requested clipped to the
size of the image.

Signed-off-by: Josh Durgin <josh.durgin@inktank.com>

src/include/rbd/librbd.hpp

@@ -161,22 +161,23 @@ class Image
    *
    * This will return the differences between two versions of an image
    * via a callback, which gets the offset and length and a flag
-   * indicating whether the extent is known/defined to be zeros (a
-   * hole).  If the source snapshot name is NULL, we interpret that as
-   * the beginning of time and return all allocated regions of the
-   * image.  The end version is whatever is currently selected for the
-   * image handle (either a snapshot or the writeable head).
+   * indicating whether the extent exists (1), or is known/defined to
+   * be zeros (a hole, 0).  If the source snapshot name is NULL, we
+   * interpret that as the beginning of time and return all allocated
+   * regions of the image.  The end version is whatever is currently
+   * selected for the image handle (either a snapshot or the writeable
+   * head).
    *
    * @param fromsnapname start snapshot name, or NULL
    * @param ofs start offset
    * @param len len in bytes of region to report on
    * @param cb callback to call for each allocated region
    * @param arg argument to pass to the callback
-   * @returns len on success, or negative error code on error
+   * @returns 0 on success, or negative error code on error
    */
-  int64_t diff_iterate(const char *fromsnapname,
-		       uint64_t ofs, size_t len,
-		       int (*cb)(uint64_t, size_t, bool, void *), void *arg);
+  int diff_iterate(const char *fromsnapname,
+		   uint64_t ofs, size_t len,
+		   int (*cb)(uint64_t, size_t, int, void *), void *arg);
   ssize_t write(uint64_t ofs, size_t len, ceph::bufferlist& bl);
   int discard(uint64_t ofs, uint64_t len);
 

src/librbd/internal.cc

@@ -2269,18 +2269,22 @@ namespace librbd {
     return total_read;
   }
 
-  int simple_diff_cb(uint64_t off, size_t len, bool exists, void *arg)
+  int simple_diff_cb(uint64_t off, size_t len, int exists, void *arg)
   {
+    // This reads the existing extents in a parent from the beginning
+    // of time.  Since images are thin-provisioned, the extents will
+    // always represent data, not holes.
+    assert(exists);
     interval_set<uint64_t> *diff = static_cast<interval_set<uint64_t> *>(arg);
     diff->insert(off, len);
     return 0;
   }
 
 
-  int64_t diff_iterate(ImageCtx *ictx, const char *fromsnapname,
-		       uint64_t off, uint64_t len,
-		       int (*cb)(uint64_t, size_t, bool, void *),
-		       void *arg)
+  int diff_iterate(ImageCtx *ictx, const char *fromsnapname,
+		   uint64_t off, uint64_t len,
+		   int (*cb)(uint64_t, size_t, int, void *),
+		   void *arg)
   {
     utime_t start_time, elapsed;
 
@@ -2291,8 +2295,7 @@ namespace librbd {
     if (r < 0)
       return r;
 
-    uint64_t mylen = len;
-    r = clip_io(ictx, off, &mylen);
+    r = clip_io(ictx, off, &len);
     if (r < 0)
       return r;
 
@@ -2347,9 +2350,8 @@ namespace librbd {
 	return r;
     }
 
-    int64_t total_read = 0;
     uint64_t period = ictx->get_stripe_period();
-    uint64_t left = mylen;
+    uint64_t left = len;
 
     while (left > 0) {
       uint64_t period_off = off - (off % period);
@@ -2385,7 +2387,7 @@ namespace librbd {
 		o.intersection_of(parent_diff);
 		ldout(ictx->cct, 20) << " reporting parent overlap " << o << dendl;
 		for (interval_set<uint64_t>::iterator s = o.begin(); s != o.end(); ++s) {
-		  cb(s.get_start(), s.get_len(), false, arg);
+		  cb(s.get_start(), s.get_len(), true, arg);
 		}
 	      }
 	    }
@@ -2431,20 +2433,19 @@ namespace librbd {
 				   << " logical "
 				   << logical_off << "~" << s.get_len()
 				   << dendl;
-	      cb(logical_off, s.get_len(), !end_exists, arg);
+	      cb(logical_off, s.get_len(), end_exists, arg);
 	    }
 	    opos += r->second;
 	  }
 	  assert(opos == q->offset + q->length);
 	}
       }
 
-      total_read += read_len;
       left -= read_len;
       off += read_len;
     }
 
-    return total_read;
+    return 0;
   }
 
   int simple_read_cb(uint64_t ofs, size_t len, const char *buf, void *arg)

src/librbd/internal.h

@@ -168,10 +168,10 @@ namespace librbd {
   int64_t read_iterate(ImageCtx *ictx, uint64_t off, size_t len,
 		       int (*cb)(uint64_t, size_t, const char *, void *),
 		       void *arg);
-  int64_t diff_iterate(ImageCtx *ictx, const char *fromsnapname,
-		       uint64_t off, uint64_t len,
-		       int (*cb)(uint64_t, size_t, bool, void *),
-		       void *arg);
+  int diff_iterate(ImageCtx *ictx, const char *fromsnapname,
+		   uint64_t off, uint64_t len,
+		   int (*cb)(uint64_t, size_t, int, void *),
+		   void *arg);
   ssize_t read(ImageCtx *ictx, uint64_t off, size_t len, char *buf);
   ssize_t read(ImageCtx *ictx, const vector<pair<uint64_t,uint64_t> >& image_extents,
 	       char *buf, bufferlist *pbl);

src/librbd/librbd.cc

@@ -442,10 +442,10 @@ namespace librbd {
     return librbd::read_iterate(ictx, ofs, len, cb, arg);
   }
 
-  int64_t Image::diff_iterate(const char *fromsnapname,
-			      uint64_t ofs, uint64_t len,
-			      int (*cb)(uint64_t, size_t, bool, void *),
-			      void *arg)
+  int Image::diff_iterate(const char *fromsnapname,
+			  uint64_t ofs, uint64_t len,
+			  int (*cb)(uint64_t, size_t, int, void *),
+			  void *arg)
   {
     ImageCtx *ictx = (ImageCtx *)ctx;
     return librbd::diff_iterate(ictx, fromsnapname, ofs, len, cb, arg);
@@ -1032,6 +1032,16 @@ extern "C" int64_t rbd_read_iterate(rbd_image_t image, uint64_t ofs, size_t len,
   return librbd::read_iterate(ictx, ofs, len, cb, arg);
 }
 
+extern "C" int diff_iterate(rbd_image_t image,
+			    const char *fromsnapname,
+			    uint64_t ofs, uint64_t len,
+			    int (*cb)(uint64_t, size_t, int, void *),
+			    void *arg)
+{
+  librbd::ImageCtx *ictx = (librbd::ImageCtx *)image;
+  return librbd::diff_iterate(ictx, fromsnapname, ofs, len, cb, arg);
+}
+
 extern "C" ssize_t rbd_write(rbd_image_t image, uint64_t ofs, size_t len,
 			     const char *buf)
 {

src/rbd.cc

@@ -1035,14 +1035,14 @@ static int do_export(librbd::Image& image, const char *path)
   return r;
 }
 
-static int export_diff_cb(uint64_t ofs, size_t _len, bool zero, void *arg)
+static int export_diff_cb(uint64_t ofs, size_t _len, int exists, void *arg)
 {
   ExportContext *ec = static_cast<ExportContext *>(arg);
   int r;
 
   // extent
   bufferlist bl;
-  __u8 tag = zero ? 'z' : 'w';
+  __u8 tag = exists ? 'w' : 'z';
   ::encode(tag, bl);
   ::encode(ofs, bl);
   uint64_t len = _len;
@@ -1051,7 +1051,7 @@ static int export_diff_cb(uint64_t ofs, size_t _len, bool zero, void *arg)
   if (r < 0)
     return r;
 
-  if (!zero) {
+  if (exists) {
     // read block
     bl.clear();
     r = ec->image->read(ofs, len, bl);
@@ -1071,7 +1071,7 @@ static int do_export_diff(librbd::Image& image, const char *fromsnapname,
 			  const char *endsnapname,
 			  const char *path)
 {
-  int64_t r;
+  int r;
   librbd::image_info_t info;
   int fd;
 
@@ -1139,7 +1139,7 @@ static int do_export_diff(librbd::Image& image, const char *fromsnapname,
   return r;
 }
 
-static int diff_cb(uint64_t ofs, size_t len, bool zero, void *arg)
+static int diff_cb(uint64_t ofs, size_t len, int exists, void *arg)
 {
   cout << ofs << "\t" << len << "\t"
        << (zero ? "zero" : "data") << "\n";
@@ -1148,7 +1148,7 @@ static int diff_cb(uint64_t ofs, size_t len, bool zero, void *arg)
 
 static int do_diff(librbd::Image& image, const char *fromsnapname)
 {
-  int64_t r;
+  int r;
   librbd::image_info_t info;
 
   r = image.stat(info, sizeof(info));

src/test/librbd/test_librbd.cc

@@ -1503,7 +1503,7 @@ TEST(LibRBD, FlushAioPP)
 }
 
 
-int iterate_cb(uint64_t off, size_t len, bool zero, void *arg)
+int iterate_cb(uint64_t off, size_t len, int exists, void *arg)
 {
   cout << "iterate_cb " << off << "~" << len << std::endl;
   interval_set<uint64_t> *diff = static_cast<interval_set<uint64_t> *>(arg);
@@ -1564,7 +1564,7 @@ TEST(LibRBD, DiffIterate)
     cout << " wrote " << two << std::endl;
 
     interval_set<uint64_t> diff;
-    ASSERT_EQ((int)size, image.diff_iterate("one", 0, size, iterate_cb, (void *)&diff));
+    ASSERT_EQ(0, image.diff_iterate("one", 0, size, iterate_cb, (void *)&diff));
     cout << " diff was " << diff << std::endl;
     if (!two.subset_of(diff)) {
       interval_set<uint64_t> i;
@@ -1620,7 +1620,7 @@ TEST(LibRBD, DiffIterateStress)
 	cout << "from " << i << " to " << j << " diff " << diff << std::endl;
 
 	image.snap_set(snap[j].c_str());
-	ASSERT_EQ((int)size, image.diff_iterate(snap[i].c_str(), 0, size, iterate_cb, (void *)&actual));
+	ASSERT_EQ(0, image.diff_iterate(snap[i].c_str(), 0, size, iterate_cb, (void *)&actual));
 	cout << " actual was " << actual << std::endl;
 	if (!diff.subset_of(actual)) {
 	  interval_set<uint64_t> i;