Repository: ceph/go-ceph
Pull request #313 (2020-06-22 14:30:10)

Create common utilty functions for splitting C buffers

There's a recurring pattern in some of the Ceph APIs where a single large buffer & length is passed to the function and the call fills that buffer with one or more C-strings. There was one helper function in rbd for this as well as a bunch of copy and pastes of a similar approach.

One instance of this pattern was recently merged and another one that I am working on is on deck.


These changes add SplitBuffer and SplitSparseBuffer functions for extracting a list     of strings from a single buffer, typically returned in C code, from     a single Go buffer. The SplitBuffer variant will return empty strings    if multiple nulls are found in sequence, assuming that the C code
    packs data between on single null byte (expect the final byte).     The SplitSparseBuffer variant assumes that the C code may not     tightly pack the data with single null bytes and thus will not
    return any empty strings (unless the input buffer is empty     or only contains nulls).
    Most of the code in the go-ceph codebase is doing the latter but     probably should have been doing the former. Thus both approaches     are provided.






## Checklist
- [x] Added tests for features and functional changes
- [x] Public functions and types are documented
- [x] Standard formatting is applied to Go code



Patch:
diff --git a/internal/cutil/splitbuf.go b/internal/cutil/splitbuf.go
new file mode 100644
index 0000000..2f439b0
--- /dev/null
+++ b/internal/cutil/splitbuf.go
@@ -0,0 +1,49 @@
+package cutil
+
+import "C"
+
+import (
+	"bytes"
+)
+
+// SplitBuffer splits a byte-slice buffer, typically returned from C code,
+// into a slice of strings.
+// The contents of the buffer are assumed to be null-byte separated.
+// If the buffer contains a sequence of null-bytes it will assume that the
+// "space" between the bytes are meant to be empty strings.
+func SplitBuffer(b []byte) []string {
+	return splitBufStrings(b, true)
+}
+
+// SplitSparseBuffer splits a byte-slice buffer, typically returned from C code,
+// into a slice of strings.
+// The contents of the buffer are assumed to be null-byte separated.
+// This function assumes that buffer to be "sparse" such that only non-null-byte
+// strings will be returned, and no "empty" strings exist if null-bytes
+// are found adjacent to each other.
+func SplitSparseBuffer(b []byte) []string {
+	return splitBufStrings(b, false)
+}
+
+// If keepEmpty is true, empty substrings will be returned, by default they are
+// excluded from the results.
+// This is almost certainly a suboptimal implementation, especially for
+// keepEmpty=true case. Optimizing the functions is a job for another day.
+func splitBufStrings(b []byte, keepEmpty bool) []string {
+	values := make([]string, 0)
+	// the final null byte should be the terminating null in C
+	// we never want to preserve the empty string after it
+	if len(b) > 0 && b[len(b)-1] == 0 {
+		b = b[:len(b)-1]
+	}
+	if len(b) == 0 {
+		return values
+	}
+	for _, s := range bytes.Split(b, []byte{0}) {
+		if !keepEmpty && len(s) == 0 {
+			continue
+		}
+		values = append(values, string(s))
+	}
+	return values
+}
diff --git a/rados/conn.go b/rados/conn.go
index 69a29bc..8bacb1c 100644
--- a/rados/conn.go
+++ b/rados/conn.go
@@ -6,9 +6,9 @@ package rados
 import "C"
 
 import (
-	"bytes"
 	"unsafe"
 
+	"github.com/ceph/go-ceph/internal/cutil"
 	"github.com/ceph/go-ceph/internal/retry"
 )
 
@@ -117,14 +117,7 @@ func (c *Conn) ListPools() (names []string, err error) {
 			continue
 		}
 
-		tmp := bytes.SplitAfter(buf[:ret-1], []byte{0})
-		for _, s := range tmp {
-			if len(s) > 0 {
-				name := C.GoString((*C.char)(unsafe.Pointer(&s[0])))
-				names = append(names, name)
-			}
-		}
-
+		names = cutil.SplitSparseBuffer(buf[:ret])
 		return names, nil
 	}
 }
diff --git a/rbd/namespace_nautilus.go b/rbd/namespace_nautilus.go
index fb78a9a..5b0e201 100644
--- a/rbd/namespace_nautilus.go
+++ b/rbd/namespace_nautilus.go
@@ -13,9 +13,9 @@ package rbd
 import "C"
 
 import (
-	"bytes"
 	"unsafe"
 
+	"github.com/ceph/go-ceph/internal/cutil"
 	"github.com/ceph/go-ceph/internal/retry"
 	"github.com/ceph/go-ceph/rados"
 )
@@ -100,12 +100,7 @@ func NamespaceList(ioctx *rados.IOContext) (names []string, err error) {
 	if err != nil {
 		return nil, err
 	}
-	tmpList := bytes.Split(buf[:cSize-1], []byte{0})
-	for _, s := range tmpList {
-		if len(s) > 0 {
-			name := C.GoString((*C.char)(unsafe.Pointer(&s[0])))
-			names = append(names, name)
-		}
-	}
+
+	names = cutil.SplitSparseBuffer(buf[:cSize])
 	return names, nil
 }
diff --git a/rbd/rbd.go b/rbd/rbd.go
index 1f041b4..12e7a5a 100644
--- a/rbd/rbd.go
+++ b/rbd/rbd.go
@@ -11,12 +11,12 @@ package rbd
 import "C"
 
 import (
-	"bytes"
 	"errors"
 	"io"
 	"time"
 	"unsafe"
 
+	"github.com/ceph/go-ceph/internal/cutil"
 	"github.com/ceph/go-ceph/internal/retry"
 	ts "github.com/ceph/go-ceph/internal/timespec"
 	"github.com/ceph/go-ceph/rados"
@@ -95,18 +95,6 @@ type TrashInfo struct {
 	DefermentEndTime time.Time // Date / time after which the trashed RBD may be permanently deleted.
 }
 
-//
-func split(buf []byte) (values []string) {
-	tmp := bytes.Split(buf[:len(buf)-1], []byte{0})
-	for _, s := range tmp {
-		if len(s) > 0 {
-			go_s := C.GoString((*C.char)(unsafe.Pointer(&s[0])))
-			values = append(values, go_s)
-		}
-	}
-	return values
-}
-
 // cephIoctx returns a ceph rados_ioctx_t given a go-ceph rados IOContext.
 func cephIoctx(radosIoctx *rados.IOContext) C.rados_ioctx_t {
 	return C.rados_ioctx_t(radosIoctx.Pointer())
@@ -605,9 +593,9 @@ func (image *Image) ListLockers() (tag string, lockers []Locker, err error) {
 		return "", nil, rbdError(c_locker_cnt)
 	}
 
-	clients := split(clients_buf)
-	cookies := split(cookies_buf)
-	addrs := split(addrs_buf)
+	clients := cutil.SplitSparseBuffer(clients_buf)
+	cookies := cutil.SplitSparseBuffer(cookies_buf)
+	addrs := cutil.SplitSparseBuffer(addrs_buf)
 
 	lockers = make([]Locker, c_locker_cnt)
 	for i := 0; i < int(c_locker_cnt); i++ {
diff --git a/rbd/rbd_mimic.go b/rbd/rbd_mimic.go
index d25ac74..1c4a123 100644
--- a/rbd/rbd_mimic.go
+++ b/rbd/rbd_mimic.go
@@ -12,9 +12,9 @@ package rbd
 import "C"
 
 import (
-	"bytes"
 	"unsafe"
 
+	"github.com/ceph/go-ceph/internal/cutil"
 	"github.com/ceph/go-ceph/internal/retry"
 	"github.com/ceph/go-ceph/rados"
 )
@@ -37,12 +37,6 @@ func GetImageNames(ioctx *rados.IOContext) (names []string, err error) {
 	if err != nil {
 		return nil, err
 	}
-	tmp := bytes.Split(buf[:csize-1], []byte{0})
-	for _, s := range tmp {
-		if len(s) > 0 {
-			name := C.GoString((*C.char)(unsafe.Pointer(&s[0])))
-			names = append(names, name)
-		}
-	}
+	names = cutil.SplitSparseBuffer(buf[:csize])
 	return names, nil
 }
diff --git a/rbd/snapshot_mimic.go b/rbd/snapshot_mimic.go
index 04190d5..9c12d9f 100644
--- a/rbd/snapshot_mimic.go
+++ b/rbd/snapshot_mimic.go
@@ -12,8 +12,9 @@ package rbd
 import "C"
 
 import (
-	"bytes"
 	"unsafe"
+
+	"github.com/ceph/go-ceph/internal/cutil"
 )
 
 // GetParentInfo looks for the parent of the image and stores the pool, name
@@ -81,21 +82,7 @@ func (image *Image) ListChildren() (pools []string, images []string, err error)
 		return nil, nil, rbdError(ret)
 	}
 
-	tmp := bytes.Split(pools_buf[:c_pools_len-1], []byte{0})
-	for _, s := range tmp {
-		if len(s) > 0 {
-			name := C.GoString((*C.char)(unsafe.Pointer(&s[0])))
-			pools = append(pools, name)
-		}
-	}
-
-	tmp = bytes.Split(images_buf[:c_images_len-1], []byte{0})
-	for _, s := range tmp {
-		if len(s) > 0 {
-			name := C.GoString((*C.char)(unsafe.Pointer(&s[0])))
-			images = append(images, name)
-		}
-	}
-
+	pools = cutil.SplitSparseBuffer(pools_buf[:c_pools_len])
+	images = cutil.SplitSparseBuffer(images_buf[:c_images_len])
 	return pools, images, nil
 }


Test patch:
diff --git a/internal/cutil/splitbuf_test.go b/internal/cutil/splitbuf_test.go
new file mode 100644
index 0000000..a2c2c17
--- /dev/null
+++ b/internal/cutil/splitbuf_test.go
@@ -0,0 +1,70 @@
+package cutil
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+var tbl = []struct {
+	val  []byte
+	res1 []string
+	res2 []string
+}{
+	// simple inputs
+	{
+		val:  []byte("foo\x00bar\x00baz\x00"),
+		res1: []string{"foo", "bar", "baz"},
+		res2: []string{"foo", "bar", "baz"},
+	},
+	// no trailing null bytes
+	{
+		val:  []byte("meow mix"),
+		res1: []string{"meow mix"},
+		res2: []string{"meow mix"},
+	},
+	// one item
+	{
+		val:  []byte("fancy feast\x00"),
+		res1: []string{"fancy feast"},
+		res2: []string{"fancy feast"},
+	},
+	// nuttin dare
+	{
+		val:  []byte(""),
+		res1: []string{},
+		res2: []string{},
+	},
+	// almost nuttin
+	{
+		val:  []byte("\x00"),
+		res1: []string{},
+		res2: []string{},
+	},
+	// how multiple adjacent nulls are handled
+	{
+		val:  []byte("kibbles\x00\x00and\x00bits"),
+		res1: []string{"kibbles", "and", "bits"},
+		res2: []string{"kibbles", "", "and", "bits"},
+	},
+	{
+		val:  []byte("dinki\x00\x00\x00di\x00\x00"),
+		res1: []string{"dinki", "di"},
+		res2: []string{"dinki", "", "", "di", ""},
+	},
+	// starting with a null
+	{
+		val:  []byte("\x00caesar\x00"),
+		res1: []string{"caesar"},
+		res2: []string{"", "caesar"},
+	},
+}
+
+func TestSplitBufStrings(t *testing.T) {
+	for _, x := range tbl {
+		assert.Equal(t, x.res1, SplitSparseBuffer(x.val))
+	}
+	for _, x := range tbl {
+		assert.Equal(t, x.res2, SplitBuffer(x.val))
+	}
+}