Repository: ceph/go-ceph
Pull request #350 (2020-08-25 00:02:17)

rbd: remove unnecessary C wrapper functions
Signed-off-by: Sven Anderson <sven@redhat.com>

## Checklist
- [x] Added tests for features and functional changes
- [x] Public functions and types are documented
- [x] Standard formatting is applied to Go code


ansiwen: > ```
> void * void_ptr(uintptr_t v) {
>     return (void*)v;
> }
> ```
> 
> Would that work with less risk?

Ha, it might be even easier than that! Let my try something....
ansiwen: Unfortunately, what I had in mind, that is a `typedef void_ptr (void *);` and then using a cast like `C.void_ptr(...)` didn't work. But I agree with you, it's safer to use a C function for that. It was not so easy to implement that only at a single place for the whole project. Now I just put everything in a single C header file. PTAL.
ansiwen: @phlogistonjohn Ok, after thinking about it a bit more: is it really less risky? Of course one could still store the result of `C.void_ptr(10)` in a variable and would have exactly the same problem with the GC. We just obfuscated the same function a bit, by moving it into the C space. So now, and also because the header file solution is not so pretty, I think I prefer the Go function with a clear warning again. 😅
WDYT?
phlogistonjohn: > @phlogistonjohn Ok, after thinking about it a bit more: is it really less risky? Of course one could still store the result of `C.void_ptr(10)` in a variable and would have exactly the same problem with the GC. 

Would it? Honest question, not rhetorical. I would have thought that keeping it "outside" the go memory realm the GC woudln't touch it. If I'm wrong about this that certainly changes my opinion.

> We just obfuscated the same function a bit, by moving it into the C space. So now, and also because the header file solution is not so pretty, I think I prefer the Go function with a clear warning again. sweat_smile
> WDYT?

Yeah, if the GC will touch it and there are no advantages around reducing the risk of a crash I agree with this and would prefer the earlier approach.


ansiwen: > > @phlogistonjohn Ok, after thinking about it a bit more: is it really less risky? Of course one could still store the result of `C.void_ptr(10)` in a variable and would have exactly the same problem with the GC.
> 
> Would it? Honest question, not rhetorical. I would have thought that keeping it "outside" the go memory realm the GC woudln't touch it. If I'm wrong about this that certainly changes my opinion.

Yeah, because by storing the value in an `unsafe.Pointer` variable, it's not "outside" of go memory realm anymore. The GC will check it occasionally and will bail out with something like
```
runtime: bad pointer in frame main.main.func1 at 0xc000050fb0: 0x1e4
fatal error: invalid pointer found on stack
```
I wrote reproducers for both variants, try for yourself: https://gist.github.com/ansiwen/77c1d53ff17bbc6c53424f17c1b59090

> 
> > We just obfuscated the same function a bit, by moving it into the C space. So now, and also because the header file solution is not so pretty, I think I prefer the Go function with a clear warning again. sweat_smile
> > WDYT?
> 
> Yeah, if the GC will touch it and there are no advantages around reducing the risk of a crash I agree with this and would prefer the earlier approach.

Not "touch" it, but look at it and become unhappy. Which is enough to crash. ;-)

phlogistonjohn: > > > @phlogistonjohn Ok, after thinking about it a bit more: is it really less risky? Of course one could still store the result of `C.void_ptr(10)` in a variable and would have exactly the same problem with the GC.
> > 
> > 
> > Would it? Honest question, not rhetorical. I would have thought that keeping it "outside" the go memory realm the GC woudln't touch it. If I'm wrong about this that certainly changes my opinion.
> 
> Yeah, because by storing the value in an `unsafe.Pointer` variable, it's not "outside" of go memory realm anymore. The GC will check it occasionally and will bail out with something like
> 
> ```
> runtime: bad pointer in frame main.main.func1 at 0xc000050fb0: 0x1e4
> fatal error: invalid pointer found on stack
> ```
> 

Thanks for that. It's unfortunate, but based on this I'd rather go with your original Go approach then. I think that's easier to maintain & work with. The extra pointer arithmetic thing done just to avoid go vet is unfortunate, but I can live with it. Could you add a comment to that function to serve as a reminder that's the reason for the extra variable and math, please?


> I wrote reproducers for both variants, try for yourself: https://gist.github.com/ansiwen/77c1d53ff17bbc6c53424f17c1b59090
> 
> > > We just obfuscated the same function a bit, by moving it into the C space. So now, and also because the header file solution is not so pretty, I think I prefer the Go function with a clear warning again. sweat_smile
> > > WDYT?
> > 
> > 
> > Yeah, if the GC will touch it and there are no advantages around reducing the risk of a crash I agree with this and would prefer the earlier approach.
> 
> Not "touch" it, but look at it and become unhappy. Which is enough to crash. ;-)

Oh, that's too bad. 

Longer term I wonder if this is worth communicating with the go team about. This is the first thing in cgo and unsafe packages that feels actually "broken" to me.
phlogistonjohn: One last stray thought: what if we passed the index via a pointer to the index value. This would cause the unsafe.Pointer value to actually be a pointer value. It's a tad inefficient IMO but if it's less risky it might be better. Or would that violate the pointer passing rules?
ansiwen: > One last stray thought: what if we passed the index via a pointer to the index value. This would cause the unsafe.Pointer value to actually be a pointer value. It's a tad inefficient IMO but if it's less risky it might be better. Or would that violate the pointer passing rules?

@phlogistonjohn Yes, AFAIK that would violate the rules. If the memory has been allocated by the Go memory management, pointers are not necessarily constant. The GC might move the memory and adjust all pointers referencing it. If you pass such a pointer into C code, it might become invalid. A way to avoid that is to allocate memory with malloc, which will not be touched by the GC. And that is exactly what https://github.com/mattn/go-pointer does, it allocates 1 byte for each pointer it needs. So if we want to go that direction, we should just use that package, I guess.


Patch:
diff --git a/internal/cutil/cutil.go b/internal/cutil/cutil.go
new file mode 100644
index 0000000..ae55f86
--- /dev/null
+++ b/internal/cutil/cutil.go
@@ -0,0 +1,14 @@
+package cutil
+
+import "unsafe"
+
+// VoidPtr casts a uintptr value to an unsafe.Pointer value in order to use it
+// directly as a void* argument in a C function call.
+// CAUTION: NEVER store the result in a variable, or the Go GC could panic.
+func VoidPtr(i uintptr) unsafe.Pointer {
+	var nullPtr unsafe.Pointer
+	// It's not possible to cast uintptr directly to unsafe.Pointer. Therefore we
+	// cast a null pointer to uintptr and apply pointer arithmetic on it, which
+	// allows us to cast it back to unsafe.Pointer.
+	return unsafe.Pointer(uintptr(nullPtr) + i)
+}
diff --git a/rbd/diff_iterate.go b/rbd/diff_iterate.go
index 3a46179..41ab9ef 100644
--- a/rbd/diff_iterate.go
+++ b/rbd/diff_iterate.go
@@ -7,18 +7,8 @@ package rbd
 #include <stdlib.h>
 #include <rbd/librbd.h>
 
+typedef int (*diff_iterate_callback_t)(uint64_t, size_t, int, void *);
 extern int diffIterateCallback(uint64_t, size_t, int, void *);
-
-// cgo is having trouble converting the callback from the librbd header
-// to a unsafe.Pointer. This shim exists solely to help it along.
-static inline int wrap_rbd_diff_iterate2(
-			rbd_image_t image,
-			const char *fromsnapname,
-			uint64_t ofs, uint64_t len,
-			uint8_t include_parent, uint8_t whole_object,
-			uintptr_t index) {
-	return rbd_diff_iterate2(image, fromsnapname, ofs, len, include_parent, whole_object, diffIterateCallback, (void*)index);
-}
 */
 import "C"
 
@@ -26,6 +16,7 @@ import (
 	"unsafe"
 
 	"github.com/ceph/go-ceph/internal/callbacks"
+	"github.com/ceph/go-ceph/internal/cutil"
 )
 
 var diffIterateCallbacks = callbacks.New()
@@ -110,14 +101,15 @@ func (image *Image) DiffIterate(config DiffIterateConfig) error {
 	cbIndex := diffIterateCallbacks.Add(config)
 	defer diffIterateCallbacks.Remove(cbIndex)
 
-	ret := C.wrap_rbd_diff_iterate2(
+	ret := C.rbd_diff_iterate2(
 		image.image,
 		cSnapName,
 		C.uint64_t(config.Offset),
 		C.uint64_t(config.Length),
 		C.uint8_t(config.IncludeParent),
 		C.uint8_t(config.WholeObject),
-		C.uintptr_t(cbIndex))
+		C.diff_iterate_callback_t(C.diffIterateCallback),
+		cutil.VoidPtr(cbIndex))
 
 	return getError(ret)
 }
diff --git a/rbd/watchers_mimic.go b/rbd/watchers_mimic.go
index 7c9a222..96c31af 100644
--- a/rbd/watchers_mimic.go
+++ b/rbd/watchers_mimic.go
@@ -9,18 +9,6 @@ package rbd
 #include <rbd/librbd.h>
 
 extern void imageWatchCallback(void *);
-
-// cgo has trouble converting the types of the callback and data arg defined in
-// librbd header. It wants the callback function to be a byte pointer and
-// the arg to be a pointer, which is pretty much the opposite of what we
-// actually want. This shim exists to help coerce the auto-type-conversion
-// to do the right thing for us.
-static inline int wrap_rbd_update_watch(
-			rbd_image_t image,
-			uint64_t *handle,
-			uintptr_t index) {
-	return rbd_update_watch(image, handle, imageWatchCallback, (void*)index);
-}
 */
 import "C"
 
@@ -28,6 +16,7 @@ import (
 	"unsafe"
 
 	"github.com/ceph/go-ceph/internal/callbacks"
+	"github.com/ceph/go-ceph/internal/cutil"
 	"github.com/ceph/go-ceph/internal/retry"
 )
 
@@ -118,10 +107,11 @@ func (image *Image) UpdateWatch(cb WatchCallback, data interface{}) (*Watch, err
 		cbIndex: watchCallbacks.Add(wcc),
 	}
 
-	ret := C.wrap_rbd_update_watch(
+	ret := C.rbd_update_watch(
 		image.image,
 		&w.handle,
-		C.uintptr_t(w.cbIndex))
+		C.rbd_update_callback_t(C.imageWatchCallback),
+		cutil.VoidPtr(w.cbIndex))
 	if ret != 0 {
 		return nil, getError(ret)
 	}


Test patch:
diff --git a/internal/cutil/cutil_test.go b/internal/cutil/cutil_test.go
new file mode 100644
index 0000000..f996c63
--- /dev/null
+++ b/internal/cutil/cutil_test.go
@@ -0,0 +1,13 @@
+package cutil
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestVoidPtr(t *testing.T) {
+	i := uintptr(42)
+	j := uintptr(VoidPtr(i))
+	assert.Equal(t, i, j)
+}