refactor command funcs arguments set up

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

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)
 }

entrypoint.sh

@@ -175,6 +175,7 @@ test_go_ceph() {
     pkgs=(\
         "cephfs" \
         "internal/callbacks" \
+        "internal/cutil" \
         "internal/errutil" \
         "internal/retry" \
         "rados" \

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))
+}

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())
+	})
+}

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)
+}

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)
+	})
+}