Repository: ceph/go-ceph
Pull request #643 (2022-02-10 20:25:31)

rados: use omap v2 bindings
This PR is a follow-up to our previous discussion in https://github.com/ceph/go-ceph/pull/629#discussion_r800915702 .

All RADOS bindings that handle OMap keys now use their "more flexible" librados versions that consume byte strings with explicit lengths instead of expecting them to be null-terminated.

I have done a bit of refactoring too -- creating new C-allocated arrays to store the key sizes seemed like too much boilerplate. To that end I have added `cutil.GoCSLenSlice` that handles byte arrays and size arrays in one go.

Also, it seems that some of the `*Step` structs were unnecessary: these particular C functions in librados make copies of their args anyway, so it seems there is no need to keep them around for longer in the `*Step` structs...

I've tried to split this PR into multiple commits for easier review. Please see commit messages for more details about the individual changes.

## 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? Is this new API marked PREVIEW?


gman0: * `NewBufferGroupStrings()` now uses `C.CBytes()`
* Added a TODO comment regarding PtrGuard to buffergroup.go
* Renamed `BufferGroup.CStrings` to `BufferGroup.Buffers`
gman0: If you are ok with the changes please let me know if the commits need to be squashed.
ansiwen: > If you are ok with the changes please let me know if the commits need to be squashed.

No, I'm fine with it as is. Only if there is a back and forth or fix-ups in the commits, I would prefer them to be squashed, but it seems to me, that your commits are not that.
gman0: @mergifyio rebase
mergify[bot]: > rebase

#### ✅ Branch has been successfully rebased



<!--
DO NOT EDIT
-*- Mergify Payload -*-
{"command": "rebase", "conclusion": "success"}
-*- Mergify Payload End -*-
-->


Patch:
diff --git a/docs/api-status.json b/docs/api-status.json
index c101f86..4ee6641 100644
--- a/docs/api-status.json
+++ b/docs/api-status.json
@@ -983,7 +983,7 @@
       },
       {
         "name": "ReadOp.GetOmapValuesByKeys",
-        "comment": "GetOmapValuesByKeys starts iterating over specific key/value pairs.\n PREVIEW\n\nImplements:\n void rados_read_op_omap_get_vals_by_keys(rados_read_op_t read_op,\n                                          char const * const * keys,\n                                          size_t keys_len,\n                                          rados_omap_iter_t * iter,\n                                          int * prval)\n",
+        "comment": "GetOmapValuesByKeys starts iterating over specific key/value pairs.\n PREVIEW\n\nImplements:\n void rados_read_op_omap_get_vals_by_keys2(rados_read_op_t read_op,\n                                           char const * const * keys,\n                                           size_t num_keys,\n                                           const size_t * key_lens,\n                                           rados_omap_iter_t * iter,\n                                           int * prval)\n",
         "added_in_version": "v0.14.0",
         "expected_stable_version": "v0.16.0"
       },
diff --git a/internal/cutil/buffergroup.go b/internal/cutil/buffergroup.go
new file mode 100644
index 0000000..447ec11
--- /dev/null
+++ b/internal/cutil/buffergroup.go
@@ -0,0 +1,89 @@
+package cutil
+
+// #include <stdlib.h>
+import "C"
+
+import (
+	"unsafe"
+)
+
+// BufferGroup is a helper structure that holds Go-allocated slices of
+// C-allocated strings and their respective lengths. Useful for C functions
+// that consume byte buffers with explicit length instead of null-terminated
+// strings. When used as input arguments in C functions, caller must make sure
+// the C code will not hold any pointers to either of the struct's attributes
+// after that C function returns.
+type BufferGroup struct {
+	// C-allocated buffers.
+	Buffers []CharPtr
+	// Lengths of C buffers, where Lengths[i] = length(Buffers[i]).
+	Lengths []SizeT
+}
+
+// TODO: should BufferGroup implementation change and the slices would contain
+//       nested Go pointers, they must be pinned with PtrGuard.
+
+// NewBufferGroupStrings returns new BufferGroup constructed from strings.
+func NewBufferGroupStrings(strs []string) *BufferGroup {
+	s := &BufferGroup{
+		Buffers: make([]CharPtr, len(strs)),
+		Lengths: make([]SizeT, len(strs)),
+	}
+
+	for i, str := range strs {
+		bs := []byte(str)
+		s.Buffers[i] = CharPtr(C.CBytes(bs))
+		s.Lengths[i] = SizeT(len(bs))
+	}
+
+	return s
+}
+
+// NewBufferGroupBytes returns new BufferGroup constructed
+// from slice of byte slices.
+func NewBufferGroupBytes(bss [][]byte) *BufferGroup {
+	s := &BufferGroup{
+		Buffers: make([]CharPtr, len(bss)),
+		Lengths: make([]SizeT, len(bss)),
+	}
+
+	for i, bs := range bss {
+		s.Buffers[i] = CharPtr(C.CBytes(bs))
+		s.Lengths[i] = SizeT(len(bs))
+	}
+
+	return s
+}
+
+// Free free()s the C-allocated memory.
+func (s *BufferGroup) Free() {
+	for _, ptr := range s.Buffers {
+		C.free(unsafe.Pointer(ptr))
+	}
+
+	s.Buffers = nil
+	s.Lengths = nil
+}
+
+// BuffersPtr returns a pointer to the beginning of the Buffers slice.
+func (s *BufferGroup) BuffersPtr() CharPtrPtr {
+	if len(s.Buffers) == 0 {
+		return nil
+	}
+
+	return CharPtrPtr(&s.Buffers[0])
+}
+
+// LengthsPtr returns a pointer to the beginning of the Lengths slice.
+func (s *BufferGroup) LengthsPtr() SizeTPtr {
+	if len(s.Lengths) == 0 {
+		return nil
+	}
+
+	return SizeTPtr(&s.Lengths[0])
+}
+
+func testBufferGroupGet(s *BufferGroup, index int) (str string, length int) {
+	bs := C.GoBytes(unsafe.Pointer(s.Buffers[index]), C.int(s.Lengths[index]))
+	return string(bs), int(s.Lengths[index])
+}
diff --git a/rados/omap.go b/rados/omap.go
index c2756ce..525826b 100644
--- a/rados/omap.go
+++ b/rados/omap.go
@@ -10,69 +10,8 @@ import "C"
 import (
 	"runtime"
 	"unsafe"
-
-	"github.com/ceph/go-ceph/internal/cutil"
 )
 
-// setOmapStep is a write op step. It holds C memory used in the operation.
-type setOmapStep struct {
-	withRefs
-	withoutUpdate
-
-	// C arguments
-	cKeys    cutil.CPtrCSlice
-	cValues  cutil.CPtrCSlice
-	cLengths cutil.SizeTCSlice
-	cNum     C.size_t
-}
-
-func newSetOmapStep(pairs map[string][]byte) *setOmapStep {
-
-	maplen := len(pairs)
-	cKeys := cutil.NewCPtrCSlice(maplen)
-	cValues := cutil.NewCPtrCSlice(maplen)
-	cLengths := cutil.NewSizeTCSlice(maplen)
-
-	sos := &setOmapStep{
-		cKeys:    cKeys,
-		cValues:  cValues,
-		cLengths: cLengths,
-		cNum:     C.size_t(maplen),
-	}
-
-	var i uintptr
-	for key, value := range pairs {
-		// key
-		ck := C.CString(key)
-		sos.add(unsafe.Pointer(ck))
-		cKeys[i] = cutil.CPtr(ck)
-
-		// value and its length
-		vlen := cutil.SizeT(len(value))
-		if vlen > 0 {
-			cv := C.CBytes(value)
-			sos.add(cv)
-			cValues[i] = cutil.CPtr(cv)
-		} else {
-			cValues[i] = nil
-		}
-
-		cLengths[i] = vlen
-
-		i++
-	}
-
-	runtime.SetFinalizer(sos, opStepFinalizer)
-	return sos
-}
-
-func (sos *setOmapStep) free() {
-	sos.cKeys.Free()
-	sos.cValues.Free()
-	sos.cLengths.Free()
-	sos.withRefs.free()
-}
-
 // OmapKeyValue items are returned by the GetOmapStep's Next call.
 type OmapKeyValue struct {
 	Key   string
@@ -88,15 +27,6 @@ type OmapKeyValue struct {
 // Release method is called the public methods of the step must no longer be
 // used and may return errors.
 type GetOmapStep struct {
-	// inputs:
-	startAfter   string
-	filterPrefix string
-	maxReturn    uint64
-
-	// arguments:
-	cStartAfter   *C.char
-	cFilterPrefix *C.char
-
 	// C returned data:
 	iter C.rados_omap_iter_t
 	more *C.uchar
@@ -109,15 +39,10 @@ type GetOmapStep struct {
 	canIterate bool
 }
 
-func newGetOmapStep(startAfter, filterPrefix string, maxReturn uint64) *GetOmapStep {
+func newGetOmapStep() *GetOmapStep {
 	gos := &GetOmapStep{
-		startAfter:    startAfter,
-		filterPrefix:  filterPrefix,
-		maxReturn:     maxReturn,
-		cStartAfter:   C.CString(startAfter),
-		cFilterPrefix: C.CString(filterPrefix),
-		more:          (*C.uchar)(C.malloc(C.sizeof_uchar)),
-		rval:          (*C.int)(C.malloc(C.sizeof_int)),
+		more: (*C.uchar)(C.malloc(C.sizeof_uchar)),
+		rval: (*C.int)(C.malloc(C.sizeof_int)),
 	}
 	runtime.SetFinalizer(gos, opStepFinalizer)
 	return gos
@@ -133,10 +58,6 @@ func (gos *GetOmapStep) free() {
 	gos.more = nil
 	C.free(unsafe.Pointer(gos.rval))
 	gos.rval = nil
-	C.free(unsafe.Pointer(gos.cStartAfter))
-	gos.cStartAfter = nil
-	C.free(unsafe.Pointer(gos.cFilterPrefix))
-	gos.cFilterPrefix = nil
 }
 
 func (gos *GetOmapStep) update() error {
@@ -151,11 +72,12 @@ func (gos *GetOmapStep) Next() (*OmapKeyValue, error) {
 		return nil, ErrOperationIncomplete
 	}
 	var (
-		cKey *C.char
-		cVal *C.char
-		cLen C.size_t
+		cKey    *C.char
+		cVal    *C.char
+		cKeyLen C.size_t
+		cValLen C.size_t
 	)
-	ret := C.rados_omap_get_next(gos.iter, &cKey, &cVal, &cLen)
+	ret := C.rados_omap_get_next2(gos.iter, &cKey, &cVal, &cKeyLen, &cValLen)
 	if ret != 0 {
 		return nil, getError(ret)
 	}
@@ -163,8 +85,8 @@ func (gos *GetOmapStep) Next() (*OmapKeyValue, error) {
 		return nil, nil
 	}
 	return &OmapKeyValue{
-		Key:   C.GoString(cKey),
-		Value: C.GoBytes(unsafe.Pointer(cVal), C.int(cLen)),
+		Key:   string(C.GoBytes(unsafe.Pointer(cKey), C.int(cKeyLen))),
+		Value: C.GoBytes(unsafe.Pointer(cVal), C.int(cValLen)),
 	}, nil
 }
 
@@ -175,40 +97,6 @@ func (gos *GetOmapStep) More() bool {
 	return *gos.more != 0
 }
 
-// removeOmapKeysStep is a write operation step used to track state, especially
-// C memory, across the setup and use of a WriteOp.
-type removeOmapKeysStep struct {
-	withRefs
-	withoutUpdate
-
-	// arguments:
-	cKeys cutil.CPtrCSlice
-	cNum  C.size_t
-}
-
-func newRemoveOmapKeysStep(keys []string) *removeOmapKeysStep {
-	cKeys := cutil.NewCPtrCSlice(len(keys))
-	roks := &removeOmapKeysStep{
-		cKeys: cKeys,
-		cNum:  C.size_t(len(keys)),
-	}
-
-	i := 0
-	for _, key := range keys {
-		cKeys[i] = cutil.CPtr(C.CString(key))
-		roks.add(unsafe.Pointer(cKeys[i]))
-		i++
-	}
-
-	runtime.SetFinalizer(roks, opStepFinalizer)
-	return roks
-}
-
-func (roks *removeOmapKeysStep) free() {
-	roks.cKeys.Free()
-	roks.withRefs.free()
-}
-
 // SetOmap appends the map `pairs` to the omap `oid`
 func (ioctx *IOContext) SetOmap(oid string, pairs map[string][]byte) error {
 	op := CreateWriteOp()
diff --git a/rados/read_op.go b/rados/read_op.go
index 74f2eb2..8487664 100644
--- a/rados/read_op.go
+++ b/rados/read_op.go
@@ -69,13 +69,19 @@ func (r *ReadOp) AssertExists() {
 // function. The GetOmapStep may be used to iterate over the key-value
 // pairs after the Operate call has been performed.
 func (r *ReadOp) GetOmapValues(startAfter, filterPrefix string, maxReturn uint64) *GetOmapStep {
-	gos := newGetOmapStep(startAfter, filterPrefix, maxReturn)
+	gos := newGetOmapStep()
 	r.steps = append(r.steps, gos)
+
+	cStartAfter := C.CString(startAfter)
+	cFilterPrefix := C.CString(filterPrefix)
+	defer C.free(unsafe.Pointer(cStartAfter))
+	defer C.free(unsafe.Pointer(cFilterPrefix))
+
 	C.rados_read_op_omap_get_vals2(
 		r.op,
-		gos.cStartAfter,
-		gos.cFilterPrefix,
-		C.uint64_t(gos.maxReturn),
+		cStartAfter,
+		cFilterPrefix,
+		C.uint64_t(maxReturn),
 		&gos.iter,
 		gos.more,
 		gos.rval,
diff --git a/rados/read_op_omap_get_vals_by_keys.go b/rados/read_op_omap_get_vals_by_keys.go
index c328907..feed5d2 100644
--- a/rados/read_op_omap_get_vals_by_keys.go
+++ b/rados/read_op_omap_get_vals_by_keys.go
@@ -11,6 +11,8 @@ import "C"
 
 import (
 	"unsafe"
+
+	"github.com/ceph/go-ceph/internal/cutil"
 )
 
 // ReadOpOmapGetValsByKeysStep holds the result of the
@@ -65,10 +67,11 @@ func (s *ReadOpOmapGetValsByKeysStep) Next() (*OmapKeyValue, error) {
 	var (
 		cKey    *C.char
 		cVal    *C.char
+		cKeyLen C.size_t
 		cValLen C.size_t
 	)
 
-	ret := C.rados_omap_get_next(s.iter, &cKey, &cVal, &cValLen)
+	ret := C.rados_omap_get_next2(s.iter, &cKey, &cVal, &cKeyLen, &cValLen)
 	if ret != 0 {
 		return nil, getError(ret)
 	}
@@ -79,7 +82,7 @@ func (s *ReadOpOmapGetValsByKeysStep) Next() (*OmapKeyValue, error) {
 	}
 
 	return &OmapKeyValue{
-		Key:   C.GoString(cKey),
+		Key:   string(C.GoBytes(unsafe.Pointer(cKey), C.int(cKeyLen))),
 		Value: C.GoBytes(unsafe.Pointer(cVal), C.int(cValLen)),
 	}, nil
 }
@@ -88,30 +91,24 @@ func (s *ReadOpOmapGetValsByKeysStep) Next() (*OmapKeyValue, error) {
 //  PREVIEW
 //
 // Implements:
-//  void rados_read_op_omap_get_vals_by_keys(rados_read_op_t read_op,
-//                                           char const * const * keys,
-//                                           size_t keys_len,
-//                                           rados_omap_iter_t * iter,
-//                                           int * prval)
+//  void rados_read_op_omap_get_vals_by_keys2(rados_read_op_t read_op,
+//                                            char const * const * keys,
+//                                            size_t num_keys,
+//                                            const size_t * key_lens,
+//                                            rados_omap_iter_t * iter,
+//                                            int * prval)
 func (r *ReadOp) GetOmapValuesByKeys(keys []string) *ReadOpOmapGetValsByKeysStep {
 	s := newReadOpOmapGetValsByKeysStep()
 	r.steps = append(r.steps, s)
 
-	cKeys := make([]*C.char, len(keys))
-	defer func() {
-		for _, cKeyPtr := range cKeys {
-			C.free(unsafe.Pointer(cKeyPtr))
-		}
-	}()
-
-	for i, key := range keys {
-		cKeys[i] = C.CString(key)
-	}
+	cKeys := cutil.NewBufferGroupStrings(keys)
+	defer cKeys.Free()
 
-	C.rados_read_op_omap_get_vals_by_keys(
+	C.rados_read_op_omap_get_vals_by_keys2(
 		r.op,
-		&cKeys[0],
+		(**C.char)(cKeys.BuffersPtr()),
 		C.size_t(len(keys)),
+		(*C.size_t)(cKeys.LengthsPtr()),
 		&s.iter,
 		s.prval,
 	)
diff --git a/rados/write_op.go b/rados/write_op.go
index cdb838d..fa25f3d 100644
--- a/rados/write_op.go
+++ b/rados/write_op.go
@@ -10,6 +10,7 @@ import "C"
 import (
 	"unsafe"
 
+	"github.com/ceph/go-ceph/internal/cutil"
 	ts "github.com/ceph/go-ceph/internal/timespec"
 )
 
@@ -92,24 +93,39 @@ func (w *WriteOp) Create(exclusive CreateOption) {
 
 // SetOmap appends the map `pairs` to the omap `oid`.
 func (w *WriteOp) SetOmap(pairs map[string][]byte) {
-	sos := newSetOmapStep(pairs)
-	w.steps = append(w.steps, sos)
-	C.rados_write_op_omap_set(
+	keys := make([]string, len(pairs))
+	values := make([][]byte, len(pairs))
+	idx := 0
+	for k, v := range pairs {
+		keys[idx] = k
+		values[idx] = v
+		idx++
+	}
+
+	cKeys := cutil.NewBufferGroupStrings(keys)
+	cValues := cutil.NewBufferGroupBytes(values)
+	defer cKeys.Free()
+	defer cValues.Free()
+
+	C.rados_write_op_omap_set2(
 		w.op,
-		(**C.char)(sos.cKeys.Ptr()),
-		(**C.char)(sos.cValues.Ptr()),
-		(*C.size_t)(sos.cLengths.Ptr()),
-		sos.cNum)
+		(**C.char)(cKeys.BuffersPtr()),
+		(**C.char)(cValues.BuffersPtr()),
+		(*C.size_t)(cKeys.LengthsPtr()),
+		(*C.size_t)(cValues.LengthsPtr()),
+		(C.size_t)(len(pairs)))
 }
 
 // RmOmapKeys removes the specified `keys` from the omap `oid`.
 func (w *WriteOp) RmOmapKeys(keys []string) {
-	roks := newRemoveOmapKeysStep(keys)
-	w.steps = append(w.steps, roks)
-	C.rados_write_op_omap_rm_keys(
+	cKeys := cutil.NewBufferGroupStrings(keys)
+	defer cKeys.Free()
+
+	C.rados_write_op_omap_rm_keys2(
 		w.op,
-		(**C.char)(roks.cKeys.Ptr()),
-		roks.cNum)
+		(**C.char)(cKeys.BuffersPtr()),
+		(*C.size_t)(cKeys.LengthsPtr()),
+		(C.size_t)(len(keys)))
 }
 
 // CleanOmap clears the omap `oid`.


Test patch:
diff --git a/internal/cutil/buffergroup_test.go b/internal/cutil/buffergroup_test.go
new file mode 100644
index 0000000..9a0eccd
--- /dev/null
+++ b/internal/cutil/buffergroup_test.go
@@ -0,0 +1,53 @@
+package cutil
+
+import (
+	"testing"
+	"unsafe"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestBufferGroup(t *testing.T) {
+	t.Run("Empty", func(t *testing.T) {
+		s := NewBufferGroupStrings(nil)
+		assert.NotPanics(t, func() { s.Free() })
+	})
+	t.Run("NotEmpty", func(t *testing.T) {
+		s := NewBufferGroupStrings([]string{"hello"})
+		assert.NotPanics(t, func() { s.Free() })
+	})
+	t.Run("FreeSetsNil", func(t *testing.T) {
+		s := NewBufferGroupStrings([]string{"hello"})
+		s.Free()
+		assert.Nil(t, s.Buffers)
+		assert.Nil(t, s.Lengths)
+	})
+	t.Run("DoubleFree", func(t *testing.T) {
+		s := NewBufferGroupStrings([]string{"hello"})
+		assert.NotPanics(t, func() { s.Free() })
+		assert.NotPanics(t, func() { s.Free() })
+	})
+	t.Run("ValidPtrs", func(t *testing.T) {
+		s := NewBufferGroupStrings([]string{"hello"})
+		defer s.Free()
+		assert.Equal(t, unsafe.Pointer(&s.Buffers[0]), unsafe.Pointer(s.BuffersPtr()))
+		assert.Equal(t, unsafe.Pointer(&s.Lengths[0]), unsafe.Pointer(s.LengthsPtr()))
+	})
+	t.Run("ValidContents", func(t *testing.T) {
+		values := []string{
+			"1", "12", "123", "世界", "abc\x00", "ab\x00c",
+		}
+
+		s := NewBufferGroupStrings(values)
+		defer s.Free()
+
+		assert.Equal(t, len(values), len(s.Buffers))
+		assert.Equal(t, len(values), len(s.Lengths))
+
+		for i := range values {
+			actualStr, actualLen := testBufferGroupGet(s, i)
+			assert.Equal(t, values[i], actualStr)
+			assert.Equal(t, len(values[i]), actualLen)
+		}
+	})
+}