Repository: ceph/go-ceph
Pull request #226 (2020-04-18 18:51:22)

refactor command funcs arguments set up
Why we may want to do this:
The various *Command and *CommandWithInputBuffer functions all behave similarly but we are effectively copy-n-pasting the argument and result processing over and over. These changes establish a new `cutil` internal package to help with this and other future c-and-go conversions that are common. It makes two new types one for handling the input, and one for the output. Then we convert the functions in rados and cephfs.

Why we may not want to do this:
Go's cgo support is pretty nice when used entirely within one package but the current design of go/cgo is pretty hostile to allowing cgo details to transition across packages. You can not use the cgo types directly and thus have to resort to workarounds, like overuse (abuse?) of unsafe.Pointer.
As this is an internal package I'm less conerened about unsafe.Pointer because we are not advertising these types for general use, and we who work on the library itself should know what we're doing (famous last words).
It does mean that the code reduction is smaller than I'd like.

## 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/Makefile b/Makefile
index ffcd9f1..1fddd7a 100644
--- a/Makefile
+++ b/Makefile
@@ -52,7 +52,14 @@ check:
 
 # Do a quick compile only check of the tests and impliclity the
 # library code as well.
-test-binaries: cephfs.test rados.test rbd.test internal/callbacks.test internal/errutil.test internal/retry.test
+test-binaries: \
+	cephfs.test \
+	internal/callbacks.test \
+	internal/cutil.test \
+	internal/errutil.test \
+	internal/retry.test \
+	rados.test \
+	rbd.test
 test-bins: test-binaries
 
 %.test: % force_go_build
diff --git a/cephfs/command.go b/cephfs/command.go
index 0c4b3b3..9ed23bc 100644
--- a/cephfs/command.go
+++ b/cephfs/command.go
@@ -10,8 +10,14 @@ import "C"
 
 import (
 	"unsafe"
+
+	"github.com/ceph/go-ceph/internal/cutil"
 )
 
+func cephBufferFree(p unsafe.Pointer) {
+	C.ceph_buffer_free((*C.char)(p))
+}
+
 // MdsCommand sends commands to the specified MDS.
 func (mount *MountInfo) MdsCommand(mdsSpec string, args [][]byte) ([]byte, string, error) {
 	return mount.mdsCommand(mdsSpec, args, nil)
@@ -36,51 +42,22 @@ func (mount *MountInfo) MdsCommandWithInputBuffer(mdsSpec string, args [][]byte,
 func (mount *MountInfo) mdsCommand(mdsSpec string, args [][]byte, inputBuffer []byte) (buffer []byte, info string, err error) {
 	spec := C.CString(mdsSpec)
 	defer C.free(unsafe.Pointer(spec))
-
-	argc := len(args)
-	argv := make([]*C.char, argc)
-
-	for i, arg := range args {
-		argv[i] = C.CString(string(arg))
-	}
-	// free all array elements in a single defer
-	defer func() {
-		for i := range argv {
-			C.free(unsafe.Pointer(argv[i]))
-		}
-	}()
-
-	var (
-		outs, outbuf       *C.char
-		outslen, outbuflen C.size_t
-	)
-	inbuf := C.CString(string(inputBuffer))
-	inbufLen := len(inputBuffer)
-	defer C.free(unsafe.Pointer(inbuf))
+	ci := cutil.NewCommandInput(args, inputBuffer)
+	defer ci.Free()
+	co := cutil.NewCommandOutput().SetFreeFunc(cephBufferFree)
+	defer co.Free()
 
 	ret := C.ceph_mds_command(
-		mount.mount,        // cephfs mount ref
-		spec,               // mds spec
-		&argv[0],           // cmd array
-		C.size_t(argc),     // cmd array length
-		inbuf,              // bulk input
-		C.size_t(inbufLen), // length inbuf
-		&outbuf,            // buffer
-		&outbuflen,         // buffer length
-		&outs,              // status string
-		&outslen)
-
-	if outslen > 0 {
-		info = C.GoStringN(outs, C.int(outslen))
-		C.free(unsafe.Pointer(outs))
-	}
-	if outbuflen > 0 {
-		buffer = C.GoBytes(unsafe.Pointer(outbuf), C.int(outbuflen))
-		C.free(unsafe.Pointer(outbuf))
-	}
-	if ret != 0 {
-		return nil, info, getError(ret)
-	}
-
-	return buffer, info, nil
+		mount.mount, // cephfs mount ref
+		spec,        // mds spec
+		(**C.char)(ci.Cmd()),
+		C.size_t(ci.CmdLen()),
+		(*C.char)(ci.InBuf()),
+		C.size_t(ci.InBufLen()),
+		(**C.char)(co.OutBuf()),
+		(*C.size_t)(co.OutBufLen()),
+		(**C.char)(co.Outs()),
+		(*C.size_t)(co.OutsLen()))
+	buf, status := co.GoValues()
+	return buf, status, getError(ret)
 }
diff --git a/entrypoint.sh b/entrypoint.sh
index 7e92045..78f653e 100755
--- a/entrypoint.sh
+++ b/entrypoint.sh
@@ -175,6 +175,7 @@ test_go_ceph() {
     pkgs=(\
         "cephfs" \
         "internal/callbacks" \
+        "internal/cutil" \
         "internal/errutil" \
         "internal/retry" \
         "rados" \
diff --git a/internal/cutil/command_input.go b/internal/cutil/command_input.go
new file mode 100644
index 0000000..fc11b82
--- /dev/null
+++ b/internal/cutil/command_input.go
@@ -0,0 +1,62 @@
+package cutil
+
+/*
+#include <stdlib.h>
+*/
+import "C"
+
+import (
+	"unsafe"
+)
+
+// CommandInput can be used to manage the input to ceph's *_command functions.
+type CommandInput struct {
+	cmd   []*C.char
+	inbuf []byte
+}
+
+// NewCommandInput creates C-level pointers from go byte buffers suitable
+// for passing off to ceph's *_command functions.
+func NewCommandInput(cmd [][]byte, inputBuffer []byte) *CommandInput {
+	ci := &CommandInput{
+		cmd:   make([]*C.char, len(cmd)),
+		inbuf: inputBuffer,
+	}
+	for i := range cmd {
+		ci.cmd[i] = C.CString(string(cmd[i]))
+	}
+	return ci
+}
+
+// Free any C memory managed by this CommandInput.
+func (ci *CommandInput) Free() {
+	for i := range ci.cmd {
+		C.free(unsafe.Pointer(ci.cmd[i]))
+	}
+	ci.cmd = nil
+}
+
+// Cmd returns an unsafe wrapper around an array of C-strings.
+func (ci *CommandInput) Cmd() CharPtrPtr {
+	ptr := &ci.cmd[0]
+	return CharPtrPtr(ptr)
+}
+
+// CmdLen returns the length of the array of C-strings returned by
+// Cmd.
+func (ci *CommandInput) CmdLen() SizeT {
+	return SizeT(len(ci.cmd))
+}
+
+// InBuf returns an unsafe wrapper to a C char*.
+func (ci *CommandInput) InBuf() CharPtr {
+	if len(ci.inbuf) == 0 {
+		return nil
+	}
+	return CharPtr(&ci.inbuf[0])
+}
+
+// InBufLen returns the length of the buffer returned by InBuf.
+func (ci *CommandInput) InBufLen() SizeT {
+	return SizeT(len(ci.inbuf))
+}
diff --git a/internal/cutil/command_output.go b/internal/cutil/command_output.go
new file mode 100644
index 0000000..8b0c6e5
--- /dev/null
+++ b/internal/cutil/command_output.go
@@ -0,0 +1,100 @@
+package cutil
+
+/*
+#include <stdlib.h>
+*/
+import "C"
+
+import (
+	"unsafe"
+)
+
+// CommandOutput can be used to manage the outputs of ceph's *_command
+// functions.
+type CommandOutput struct {
+	free      FreeFunc
+	outBuf    *C.char
+	outBufLen C.size_t
+	outs      *C.char
+	outsLen   C.size_t
+}
+
+// NewCommandOutput returns an empty CommandOutput. The pointers that
+// a CommandOutput provides can be used to get the results of ceph's
+// *_command functions.
+func NewCommandOutput() *CommandOutput {
+	return &CommandOutput{
+		free: free,
+	}
+}
+
+// SetFreeFunc sets the function used to free memory held by CommandOutput.
+// Not all uses of CommandOutput expect to use the basic C.free function
+// and either require or prefer the use of a custom deallocation function.
+// Use SetFreeFunc to change the free function and return the modified
+// CommandOutput object.
+func (co *CommandOutput) SetFreeFunc(f FreeFunc) *CommandOutput {
+	co.free = f
+	return co
+}
+
+// Free any C memory tracked by this object.
+func (co *CommandOutput) Free() {
+	if co.outBuf != nil {
+		co.free(unsafe.Pointer(co.outBuf))
+	}
+	if co.outs != nil {
+		co.free(unsafe.Pointer(co.outs))
+	}
+}
+
+// OutBuf returns an unsafe wrapper around a pointer to a `char*`.
+func (co *CommandOutput) OutBuf() CharPtrPtr {
+	return CharPtrPtr(&co.outBuf)
+}
+
+// OutBufLen returns an unsafe wrapper around a pointer to a size_t.
+func (co *CommandOutput) OutBufLen() SizeTPtr {
+	return SizeTPtr(&co.outBufLen)
+}
+
+// Outs returns an unsafe wrapper around a pointer to a `char*`.
+func (co *CommandOutput) Outs() CharPtrPtr {
+	return CharPtrPtr(&co.outs)
+}
+
+// OutsLen returns an unsafe wrapper around a pointer to a size_t.
+func (co *CommandOutput) OutsLen() SizeTPtr {
+	return SizeTPtr(&co.outsLen)
+}
+
+// GoValues returns native go values converted from the internal C-language
+// values tracked by this object.
+func (co *CommandOutput) GoValues() (buf []byte, status string) {
+	if co.outBufLen > 0 {
+		buf = C.GoBytes(unsafe.Pointer(co.outBuf), C.int(co.outBufLen))
+	}
+	if co.outsLen > 0 {
+		status = C.GoStringN(co.outs, C.int(co.outsLen))
+	}
+	return buf, status
+}
+
+// testSetString is only used by the unit tests for this file.
+// It is located here due to the restriction on having import "C" in
+// go test files. :-(
+// It mimics a C function that takes a pointer to a
+// string and length and allocates memory and sets the pointers
+// to the new string and its length.
+func testSetString(strp CharPtrPtr, lenp SizeTPtr, s string) {
+	sp := (**C.char)(strp)
+	lp := (*C.size_t)(lenp)
+	*sp = C.CString(s)
+	*lp = C.size_t(len(s))
+}
+
+// free wraps C.free.
+// Required for unit tests that may not use cgo directly.
+func free(p unsafe.Pointer) {
+	C.free(p)
+}
diff --git a/internal/cutil/type_aliases.go b/internal/cutil/type_aliases.go
new file mode 100644
index 0000000..79495ef
--- /dev/null
+++ b/internal/cutil/type_aliases.go
@@ -0,0 +1,28 @@
+package cutil
+
+import "C"
+
+import (
+	"unsafe"
+)
+
+// Basic types from C that we can make "public" without too much fuss.
+
+// SizeT wraps size_t from C.
+type SizeT C.size_t
+
+// This section contains a bunch of types that are basically just
+// unsafe.Pointer but have specific types to help "self document" what the
+// underlying pointer is really meant to represent.
+
+// CharPtrPtr is an unsafe pointer wrapping C's `char**`.
+type CharPtrPtr unsafe.Pointer
+
+// CharPtr is an unsafe pointer wrapping C's `char*`.
+type CharPtr unsafe.Pointer
+
+// SizeTPtr is an unsafe pointer wrapping C's `size_t*`.
+type SizeTPtr unsafe.Pointer
+
+// FreeFunc is a wrapper around calls to, or act like, C's free function.
+type FreeFunc func(unsafe.Pointer)
diff --git a/rados/command.go b/rados/command.go
index ec41d1c..1ec0422 100644
--- a/rados/command.go
+++ b/rados/command.go
@@ -7,8 +7,14 @@ import "C"
 
 import (
 	"unsafe"
+
+	"github.com/ceph/go-ceph/internal/cutil"
 )
 
+func radosBufferFree(p unsafe.Pointer) {
+	C.rados_buffer_free((*C.char)(p))
+}
+
 // MonCommand sends a command to one of the monitors
 func (c *Conn) MonCommand(args []byte) ([]byte, string, error) {
 	return c.monCommand(args, nil)
@@ -20,42 +26,23 @@ func (c *Conn) MonCommandWithInputBuffer(args, inputBuffer []byte) ([]byte, stri
 }
 
 func (c *Conn) monCommand(args, inputBuffer []byte) ([]byte, string, error) {
-	argv := C.CString(string(args))
-	defer C.free(unsafe.Pointer(argv))
-
-	var (
-		info               string
-		buffer             []byte
-		outs, outbuf       *C.char
-		outslen, outbuflen C.size_t
-	)
-	inbuf := C.CString(string(inputBuffer))
-	inbufLen := len(inputBuffer)
-	defer C.free(unsafe.Pointer(inbuf))
-
-	ret := C.rados_mon_command(c.cluster,
-		&argv, 1,
-		inbuf,              // bulk input (e.g. crush map)
-		C.size_t(inbufLen), // length inbuf
-		&outbuf,            // buffer
-		&outbuflen,         // buffer length
-		&outs,              // status string
-		&outslen)
-
-	if outslen > 0 {
-		info = C.GoStringN(outs, C.int(outslen))
-		C.free(unsafe.Pointer(outs))
-	}
-	if outbuflen > 0 {
-		buffer = C.GoBytes(unsafe.Pointer(outbuf), C.int(outbuflen))
-		C.free(unsafe.Pointer(outbuf))
-	}
-	if ret != 0 {
-		err := getError(ret)
-		return nil, info, err
-	}
-
-	return buffer, info, nil
+	ci := cutil.NewCommandInput([][]byte{args}, inputBuffer)
+	defer ci.Free()
+	co := cutil.NewCommandOutput().SetFreeFunc(radosBufferFree)
+	defer co.Free()
+
+	ret := C.rados_mon_command(
+		c.cluster,
+		(**C.char)(ci.Cmd()),
+		C.size_t(ci.CmdLen()),
+		(*C.char)(ci.InBuf()),
+		C.size_t(ci.InBufLen()),
+		(**C.char)(co.OutBuf()),
+		(*C.size_t)(co.OutBufLen()),
+		(**C.char)(co.Outs()),
+		(*C.size_t)(co.OutsLen()))
+	buf, status := co.GoValues()
+	return buf, status, getError(ret)
 }
 
 // PGCommand sends a command to one of the PGs
@@ -85,50 +72,24 @@ func (c *Conn) PGCommandWithInputBuffer(pgid []byte, args [][]byte, inputBuffer
 func (c *Conn) pgCommand(pgid []byte, args [][]byte, inputBuffer []byte) ([]byte, string, error) {
 	name := C.CString(string(pgid))
 	defer C.free(unsafe.Pointer(name))
+	ci := cutil.NewCommandInput(args, inputBuffer)
+	defer ci.Free()
+	co := cutil.NewCommandOutput().SetFreeFunc(radosBufferFree)
+	defer co.Free()
 
-	argc := len(args)
-	argv := make([]*C.char, argc)
-
-	for i, arg := range args {
-		argv[i] = C.CString(string(arg))
-		defer C.free(unsafe.Pointer(argv[i]))
-	}
-
-	var (
-		info               string
-		buffer             []byte
-		outs, outbuf       *C.char
-		outslen, outbuflen C.size_t
-	)
-	inbuf := C.CString(string(inputBuffer))
-	inbufLen := len(inputBuffer)
-	defer C.free(unsafe.Pointer(inbuf))
-
-	ret := C.rados_pg_command(c.cluster,
+	ret := C.rados_pg_command(
+		c.cluster,
 		name,
-		&argv[0],
-		C.size_t(argc),
-		inbuf,              // bulk input
-		C.size_t(inbufLen), // length inbuf
-		&outbuf,            // buffer
-		&outbuflen,         // buffer length
-		&outs,              // status string
-		&outslen)
-
-	if outslen > 0 {
-		info = C.GoStringN(outs, C.int(outslen))
-		C.free(unsafe.Pointer(outs))
-	}
-	if outbuflen > 0 {
-		buffer = C.GoBytes(unsafe.Pointer(outbuf), C.int(outbuflen))
-		C.free(unsafe.Pointer(outbuf))
-	}
-	if ret != 0 {
-		err := getError(ret)
-		return nil, info, err
-	}
-
-	return buffer, info, nil
+		(**C.char)(ci.Cmd()),
+		C.size_t(ci.CmdLen()),
+		(*C.char)(ci.InBuf()),
+		C.size_t(ci.InBufLen()),
+		(**C.char)(co.OutBuf()),
+		(*C.size_t)(co.OutBufLen()),
+		(**C.char)(co.Outs()),
+		(*C.size_t)(co.OutsLen()))
+	buf, status := co.GoValues()
+	return buf, status, getError(ret)
 }
 
 // MgrCommand sends a command to a ceph-mgr.
@@ -148,47 +109,21 @@ func (c *Conn) MgrCommandWithInputBuffer(args [][]byte, inputBuffer []byte) ([]b
 //                         size_t *outbuflen, char **outs,
 //                          size_t *outslen);
 func (c *Conn) mgrCommand(args [][]byte, inputBuffer []byte) ([]byte, string, error) {
-	argc := len(args)
-	argv := make([]*C.char, argc)
-
-	for i, arg := range args {
-		argv[i] = C.CString(string(arg))
-		defer C.free(unsafe.Pointer(argv[i]))
-	}
-
-	var (
-		info               string
-		buffer             []byte
-		outs, outbuf       *C.char
-		outslen, outbuflen C.size_t
-	)
-	inbuf := C.CString(string(inputBuffer))
-	inbufLen := len(inputBuffer)
-	defer C.free(unsafe.Pointer(inbuf))
+	ci := cutil.NewCommandInput(args, inputBuffer)
+	defer ci.Free()
+	co := cutil.NewCommandOutput().SetFreeFunc(radosBufferFree)
+	defer co.Free()
 
 	ret := C.rados_mgr_command(
 		c.cluster,
-		&argv[0],
-		C.size_t(argc),
-		inbuf,
-		C.size_t(inbufLen),
-		&outbuf,
-		&outbuflen,
-		&outs,
-		&outslen)
-
-	if outslen > 0 {
-		info = C.GoStringN(outs, C.int(outslen))
-		C.free(unsafe.Pointer(outs))
-	}
-	if outbuflen > 0 {
-		buffer = C.GoBytes(unsafe.Pointer(outbuf), C.int(outbuflen))
-		C.free(unsafe.Pointer(outbuf))
-	}
-	if ret != 0 {
-		err := getError(ret)
-		return nil, info, err
-	}
-
-	return buffer, info, nil
+		(**C.char)(ci.Cmd()),
+		C.size_t(ci.CmdLen()),
+		(*C.char)(ci.InBuf()),
+		C.size_t(ci.InBufLen()),
+		(**C.char)(co.OutBuf()),
+		(*C.size_t)(co.OutBufLen()),
+		(**C.char)(co.Outs()),
+		(*C.size_t)(co.OutsLen()))
+	buf, status := co.GoValues()
+	return buf, status, getError(ret)
 }


Test patch:
diff --git a/internal/cutil/command_input_test.go b/internal/cutil/command_input_test.go
new file mode 100644
index 0000000..60057a0
--- /dev/null
+++ b/internal/cutil/command_input_test.go
@@ -0,0 +1,50 @@
+package cutil
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestCommandInput(t *testing.T) {
+	t.Run("newAndFree", func(t *testing.T) {
+		ci := NewCommandInput(
+			[][]byte{[]byte("foobar")},
+			nil)
+		ci.Free()
+	})
+	t.Run("cmd", func(t *testing.T) {
+		ci := NewCommandInput(
+			[][]byte{[]byte("foobar")},
+			nil)
+		defer ci.Free()
+		assert.Len(t, ci.cmd, 1)
+		assert.EqualValues(t, 1, ci.CmdLen())
+		assert.NotNil(t, ci.Cmd())
+	})
+	t.Run("cmd2", func(t *testing.T) {
+		ci := NewCommandInput(
+			[][]byte{[]byte("foobar"), []byte("snarf")},
+			nil)
+		defer ci.Free()
+		assert.Len(t, ci.cmd, 2)
+		assert.EqualValues(t, 2, ci.CmdLen())
+		assert.NotNil(t, ci.Cmd())
+	})
+	t.Run("noInBuf", func(t *testing.T) {
+		ci := NewCommandInput(
+			[][]byte{[]byte("foobar")},
+			nil)
+		defer ci.Free()
+		assert.EqualValues(t, 0, ci.InBufLen())
+		assert.Equal(t, CharPtr(nil), ci.InBuf())
+	})
+	t.Run("hasInBuf", func(t *testing.T) {
+		ci := NewCommandInput(
+			[][]byte{[]byte("foobar")},
+			[]byte("original oregano"))
+		defer ci.Free()
+		assert.EqualValues(t, 16, ci.InBufLen())
+		assert.NotEqual(t, CharPtr(nil), ci.InBuf())
+	})
+}
diff --git a/internal/cutil/command_output_test.go b/internal/cutil/command_output_test.go
new file mode 100644
index 0000000..74278b1
--- /dev/null
+++ b/internal/cutil/command_output_test.go
@@ -0,0 +1,56 @@
+package cutil
+
+import (
+	"testing"
+	"unsafe"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestCommandOutput(t *testing.T) {
+	t.Run("newAndFree", func(t *testing.T) {
+		co := NewCommandOutput()
+		assert.NotNil(t, co)
+		co.Free()
+	})
+	t.Run("setValues", func(t *testing.T) {
+		co := NewCommandOutput()
+		assert.NotNil(t, co)
+		defer co.Free()
+		testSetString(co.OutBuf(), co.OutBufLen(), "i got style")
+		testSetString(co.Outs(), co.OutsLen(), "i got rhythm")
+		b, s := co.GoValues()
+		assert.EqualValues(t, []byte("i got style"), b)
+		assert.EqualValues(t, "i got rhythm", s)
+	})
+	t.Run("setOnlyOutBuf", func(t *testing.T) {
+		co := NewCommandOutput()
+		assert.NotNil(t, co)
+		defer co.Free()
+		testSetString(co.OutBuf(), co.OutBufLen(), "i got style")
+		b, s := co.GoValues()
+		assert.EqualValues(t, []byte("i got style"), b)
+		assert.EqualValues(t, "", s)
+	})
+	t.Run("setOnlyOuts", func(t *testing.T) {
+		co := NewCommandOutput()
+		assert.NotNil(t, co)
+		defer co.Free()
+		testSetString(co.Outs(), co.OutsLen(), "i got rhythm")
+		b, s := co.GoValues()
+		assert.Nil(t, b)
+		assert.EqualValues(t, "i got rhythm", s)
+	})
+	t.Run("customFreeFunc", func(t *testing.T) {
+		callCount := 0
+		co := NewCommandOutput().SetFreeFunc(func(p unsafe.Pointer) {
+			callCount++
+			free(p)
+		})
+		assert.NotNil(t, co)
+		testSetString(co.OutBuf(), co.OutBufLen(), "i got style")
+		testSetString(co.Outs(), co.OutsLen(), "i got rhythm")
+		co.Free()
+		assert.Equal(t, 2, callCount)
+	})
+}