Repository: ceph/go-ceph
Pull request #1013 (2024-07-12 16:57:48)

Detect `rbd_clone4` support by using `dlsym()`
The dlsym package provides LookupSymbol() which resolves a named symbol
(like "rbd_clone4") and returns a unsafe.Pointer to it. The caller is
expected to cast the symbol to function that can be called.

Some versions of librbd provide the rbd_clone4 function, and others do
not. Squid will have the function backported in the 1st update, the
initial release of Squid does not have it. This makes checking for the
function based on the named Ceph version impractical.

With the new dlsym.LookupSymbol() function, it is now possible to check
the availability of rbd_clone4 during runtime. If the symbol is not
found ErrNotImplemented is returned, which can be used to detect the
unavailability of the function.

## Checklist
- [x] Added tests for features and functional changes
- [x] Public functions and types are documented
- [x] Standard formatting is applied to Go code
- [ ] Is this a new API? Added a new file that begins with `//go:build ceph_preview`
- [ ] Ran `make api-update` to record new APIs

New or infrequent contributors may want to review the go-ceph [Developer's Guide](https://github.com/ceph/go-ceph/blob/master/docs/development.md) including the section on how we track [API Status](https://github.com/ceph/go-ceph/blob/master/docs/development.md#api-status) and the [API Stability Plan](https://github.com/ceph/go-ceph/blob/master/docs/api-stability.md).

The go-ceph project uses mergify. View the [mergify command guide](https://docs.mergify.com/commands/#commands) for information on how to interact with mergify. Add a comment with `@Mergifyio` `rebase` to rebase your PR when github indicates that the PR is out of date with the base branch.


nixpanic: **pre-squid** runs the test cases with success:
![image](https://github.com/user-attachments/assets/e086a870-ba63-4535-a6e6-0ed9aa5af4a5)

**quincy** skips the test cases as expected:
![image](https://github.com/user-attachments/assets/760cbdcf-41a7-46b1-b5e4-1c72265802cb)

phlogistonjohn: Very interesting. I will try to dig into it in more detail soon!

also cc: @ansiwen 
ansiwen: No specific concerns, technically well done.

My only concern is, that I think this is a case of over-engineering to add the libdl dependency in general for such an edge-case. I'm not really sure about the side effects on non-Linux and MUSL systems. But I will not veto it, if a majority prefers this change. But I wanted to note here that I would prefer to just not support the first release of Squid.
phlogistonjohn: > My only concern is, that I think this is a case of over-engineering to add the libdl dependency in general for such an edge-case. I'm not really sure about the side effects on non-Linux and MUSL systems. 

That's a fair point, would a tag like `nodlsym` that disables this file be a viable addition for those kinds of platforms? Then if we see people file issues related to dlsym we can say 'try with build tag "nodlsym"' but default it to on for the more common users of Linux+glibc?

anoopcs9: > The `rbd_clone4` function is just the 1st where this happens, #1010 is related to a 2nd candidate for backporting to Squid.

Looking at the nature of APIs suggested via #1010 I was wondering whether "dlsym" approach can help us with all those variables/enums coming along with it.

nixpanic: > > The `rbd_clone4` function is just the 1st where this happens, #1010 is related to a 2nd candidate for backporting to Squid.
> 
> Looking at the nature of APIs suggested via #1010 I was wondering whether "dlsym" approach can help us with all those variables/enums coming along with it.

It works for new functions and structs, not enums or functions that were changed. Modifying existing public functions is not something that should be done in any case, it will break existing applications that use them. Introduction of new functions while keeping (possibly deprecating) the old ones is a more sane approach.
nixpanic: @ansiwen and @phlogistonjohn I've dropped the CGo linker directive to link against `libdl`. It seems the CGo linker is quite forgiving, and it works without explicitly mentioning `-ldl`.

Do you have any other concerns that I should address?
nixpanic: > @ansiwen and @phlogistonjohn I've dropped the CGo linker directive to link against `libdl`. It seems the CGo linker is quite forgiving, and it works without explicitly mentioning `-ldl`.
> 
> Do you have any other concerns that I should address?

Oh, interesting! Local testing with the `make test-container CEPH_VERSION=...` worked. Not sure whats different in these CI jobs, but obviously the CGo linker is more strict there :thinking: 
anoopcs9: > > > The `rbd_clone4` function is just the 1st where this happens, #1010 is related to a 2nd candidate for backporting to Squid.
> > 
> > 
> > Looking at the nature of APIs suggested via #1010 I was wondering whether "dlsym" approach can help us with all those variables/enums coming along with it.
> 
> It works for new functions and structs, not enums or functions that were changed. Modifying existing public functions is not something that should be done in any case, it will break existing applications that use them. Introduction of new functions while keeping (possibly deprecating) the old ones is a more sane approach.

Shouldn't we document the situation with enums/variables from original Ceph API referenced in the corresponding go-ceph API?

Nevertheless I think its good to have this new approach documented within [development.md](https://github.com/ceph/go-ceph/blob/master/docs/development.md).
nixpanic: > Shouldn't we document the situation with enums/variables from original Ceph API referenced in the corresponding go-ceph API?
> 
> Nevertheless I think its good to have this new approach documented within [development.md](https://github.com/ceph/go-ceph/blob/master/docs/development.md).


anoopcs9: > > Shouldn't we document the situation with enums/variables from original Ceph API referenced in the corresponding go-ceph API?
>>
> > Nevertheless I think its good to have this new approach documented within [development.md](https://github.com/ceph/go-ceph/blob/master/docs/development.md).
> 
> @anoopcs9 , I think enums and other `#define`s work are well understood by developers that are familiar with C. They are not symbols in a shared library, so `dlsym()` can not detect them either. This PR does not use enums or new librbd `#define`s, so I think documenting a potential workaround for those is out of scope here?

Even otherwise I was more interested in documenting the _LookupSymbol()_ helper from `dlysm` package as a hint to someone who could end up in such a situation where an API is only available with further minor updates. I won't insist though as others haven't shown interest in such a documentation effort yet.

Patch:
diff --git a/internal/dlsym/dlsym.go b/internal/dlsym/dlsym.go
new file mode 100644
index 0000000..9e5782c
--- /dev/null
+++ b/internal/dlsym/dlsym.go
@@ -0,0 +1,41 @@
+package dlsym
+
+// #cgo LDFLAGS: -ldl
+//
+// #include <stdlib.h>
+// #include <dlfcn.h>
+//
+// #ifndef RTLD_DEFAULT /* from dlfcn.h */
+// #define RTLD_DEFAULT ((void *) 0)
+// #endif
+import "C"
+
+import (
+	"errors"
+	"fmt"
+	"unsafe"
+)
+
+// ErrUndefinedSymbol is returned by LookupSymbol when the requested symbol
+// could not be found.
+var ErrUndefinedSymbol = errors.New("symbol not found")
+
+// LookupSymbol resolves the named symbol from the already dynamically loaded
+// libraries. If the symbol is found, a pointer to it is returned, in case of a
+// failure, the message provided by dlerror() is included in the error message.
+func LookupSymbol(symbol string) (unsafe.Pointer, error) {
+	cSymName := C.CString(symbol)
+	defer C.free(unsafe.Pointer(cSymName))
+
+	// clear dlerror before looking up the symbol
+	C.dlerror()
+	// resolve the address of the symbol
+	sym := C.dlsym(C.RTLD_DEFAULT, cSymName)
+	e := C.dlerror()
+	dlerr := C.GoString(e)
+	if dlerr != "" {
+		return nil, fmt.Errorf("%w: %s", ErrUndefinedSymbol, dlerr)
+	}
+
+	return sym, nil
+}
diff --git a/rbd/clone_image_by_id.go b/rbd/clone_image_by_id.go
index a73c99b..63cf552 100644
--- a/rbd/clone_image_by_id.go
+++ b/rbd/clone_image_by_id.go
@@ -1,20 +1,46 @@
-//go:build !(nautilus || octopus || pacific || quincy || reef) && ceph_preview
+//go:build ceph_preview
 
 package rbd
 
-// #cgo LDFLAGS: -lrbd
-// #include <errno.h>
-// #include <stdlib.h>
-// #include <rados/librados.h>
-// #include <rbd/librbd.h>
+/*
+#cgo LDFLAGS: -lrbd
+#include <errno.h>
+#include <stdlib.h>
+#include <rados/librados.h>
+#include <rbd/librbd.h>
+
+// rbd_clone4_fn matches the rbd_clone4 function signature.
+typedef int(*rbd_clone4_fn)(rados_ioctx_t p_ioctx, const char *p_name,
+                            uint64_t p_snap_id, rados_ioctx_t c_ioctx,
+			    const char *c_name, rbd_image_options_t c_opts);
+
+// rbd_clone4_dlsym take *fn as rbd_clone4_fn and calls the dynamically loaded
+// rbd_clone4 function passed as 1st argument.
+static inline int rbd_clone4_dlsym(void *fn, rados_ioctx_t p_ioctx,
+				   const char *p_name, uint64_t p_snap_id,
+				   rados_ioctx_t c_ioctx, const char *c_name,
+                                   rbd_image_options_t c_opts) {
+  // cast function pointer fn to rbd_clone4 and call the function
+  return ((rbd_clone4_fn) fn)(p_ioctx, p_name, p_snap_id, c_ioctx, c_name, c_opts);
+}
+*/
 import "C"
 
 import (
+	"fmt"
+	"sync"
 	"unsafe"
 
+	"github.com/ceph/go-ceph/internal/dlsym"
 	"github.com/ceph/go-ceph/rados"
 )
 
+var (
+	rbdClone4Once sync.Once
+	rbdClone4     unsafe.Pointer
+	rbdClone4Err  error
+)
+
 // CloneImageByID creates a clone of the image from a snapshot with the given
 // ID in the provided io-context with the given name and image options.
 //
@@ -25,22 +51,33 @@ import (
 //	               const char *c_name, rbd_image_options_t c_opts);
 func CloneImageByID(ioctx *rados.IOContext, parentName string, snapID uint64,
 	destctx *rados.IOContext, name string, rio *ImageOptions) error {
-
 	if rio == nil {
 		return rbdError(C.EINVAL)
 	}
 
+	rbdClone4Once.Do(func() {
+		rbdClone4, rbdClone4Err = dlsym.LookupSymbol("rbd_clone4")
+	})
+
+	if rbdClone4Err != nil {
+		return fmt.Errorf("%w: %w", ErrNotImplemented, rbdClone4Err)
+	}
+
 	cParentName := C.CString(parentName)
 	defer C.free(unsafe.Pointer(cParentName))
 	cCloneName := C.CString(name)
 	defer C.free(unsafe.Pointer(cCloneName))
 
-	ret := C.rbd_clone4(
+	// call rbd_clone4_dlsym with the function pointer to rbd_clone4 as 1st
+	// argument
+	ret := C.rbd_clone4_dlsym(
+		rbdClone4,
 		cephIoctx(ioctx),
 		cParentName,
 		C.uint64_t(snapID),
 		cephIoctx(destctx),
 		cCloneName,
 		C.rbd_image_options_t(rio.options))
+
 	return getError(ret)
 }
diff --git a/rbd/errors.go b/rbd/errors.go
index 3469333..4d98cce 100644
--- a/rbd/errors.go
+++ b/rbd/errors.go
@@ -75,6 +75,8 @@ var (
 const (
 	// ErrNotExist indicates a non-specific missing resource.
 	ErrNotExist = rbdError(-C.ENOENT)
+	// ErrNotImplemented indicates a function is not implemented in by librbd.
+	ErrNotImplemented = rbdError(-C.ENOSYS)
 )
 
 // Private errors:


Test patch:
diff --git a/internal/dlsym/dlsym_test.go b/internal/dlsym/dlsym_test.go
new file mode 100644
index 0000000..c33eccc
--- /dev/null
+++ b/internal/dlsym/dlsym_test.go
@@ -0,0 +1,22 @@
+package dlsym
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestLookupSymbol(t *testing.T) {
+	t.Run("ValidSymbol", func(t *testing.T) {
+		sym, err := LookupSymbol("dlsym")
+		assert.NotNil(t, sym)
+		assert.NoError(t, err)
+	})
+
+	t.Run("InvalidSymbol", func(t *testing.T) {
+		sym, err := LookupSymbol("go_ceph_dlsym")
+		assert.Nil(t, sym)
+		assert.True(t, errors.Is(err, ErrUndefinedSymbol))
+	})
+}
diff --git a/rbd/clone_image_by_id_test.go b/rbd/clone_image_by_id_test.go
index db944ec..9a50b71 100644
--- a/rbd/clone_image_by_id_test.go
+++ b/rbd/clone_image_by_id_test.go
@@ -1,8 +1,9 @@
-//go:build !(nautilus || octopus || pacific || quincy || reef) && ceph_preview
+//go:build ceph_preview
 
 package rbd
 
 import (
+	"errors"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -91,6 +92,9 @@ func TestCloneImageByID(t *testing.T) {
 
 		// Create a clone of the image using the snapshot.
 		err = CloneImageByID(ioctx, name1, snapID, ioctx, cloneName, optionsClone)
+		if errors.Is(err, ErrNotImplemented) {
+			t.Skipf("CloneImageByID is not supported: %v", err)
+		}
 		assert.NoError(t, err)
 		defer func() { assert.NoError(t, RemoveImage(ioctx, cloneName)) }()
 
@@ -112,6 +116,9 @@ func TestCloneImageByID(t *testing.T) {
 	t.Run("CloneFromGroupSnap", func(t *testing.T) {
 		err := GroupSnapCreate(ioctx, gname, "groupsnap")
 		assert.NoError(t, err)
+		defer func() {
+			assert.NoError(t, GroupSnapRemove(ioctx, gname, "groupsnap"))
+		}()
 
 		cloneName := "img-clone"
 		optionsClone := NewRbdImageOptions()
@@ -132,6 +139,9 @@ func TestCloneImageByID(t *testing.T) {
 
 		// Create a clone of the image using the snapshot.
 		err = CloneImageByID(ioctx, name1, snapID, ioctx, cloneName, optionsClone)
+		if errors.Is(err, ErrNotImplemented) {
+			t.Skipf("CloneImageByID is not supported: %v", err)
+		}
 		assert.NoError(t, err)
 		defer func() { assert.NoError(t, RemoveImage(ioctx, cloneName)) }()
 
@@ -147,8 +157,5 @@ func TestCloneImageByID(t *testing.T) {
 		assert.Equal(t, parentInfo.Snap.ID, snapID)
 		assert.Equal(t, parentInfo.Image.PoolName, poolname)
 		assert.False(t, parentInfo.Image.Trash)
-
-		err = GroupSnapRemove(ioctx, gname, "groupsnap")
-		assert.NoError(t, err)
 	})
 }