Repository: ceph/go-ceph
Pull request #302 (2020-06-11 18:55:29)

cephfs: add functions to parse argv and env
While looking at implementing the functions needed for cephfs I looked at the similar functions in rados. Seeing the "placeholder" thing with fresh eyes annoyed me enough that I split the function into a direct wrap of the ceph call and a higher level call for backwards compat with the previous ParseCmdLineArgs function. I also improved a doc comment.

Then using the rados functions as a guide I added ParseConfigArgv and ParseDefaultConfigEnv to cephfs.

Fixes: #245 

## Checklist
- [x] Added tests for features and functional changes
- [x] Public functions and types are documented
- [x] Standard formatting is applied to Go code


phlogistonjohn: Because I don't particularly like what ParseCmdLineArgs is doing, does anyone else want to deprecate that function? Or the reverse: have a similar wrapper in cephfs?
phlogistonjohn: @nixpanic @agarwal-mudit @ansiwen PTAL

Patch:
diff --git a/cephfs/cephfs.go b/cephfs/cephfs.go
index 288ab47..b90f894 100644
--- a/cephfs/cephfs.go
+++ b/cephfs/cephfs.go
@@ -70,6 +70,41 @@ func (mount *MountInfo) ReadDefaultConfigFile() error {
 	return getError(ret)
 }
 
+// ParseConfigArgv configures the mount using a unix style command line
+// argument vector.
+//
+// Implements:
+//  int ceph_conf_parse_argv(struct ceph_mount_info *cmount, int argc, const char **argv);
+func (mount *MountInfo) ParseConfigArgv(argv []string) error {
+	if err := mount.validate(); err != nil {
+		return err
+	}
+	if len(argv) == 0 {
+		return ErrEmptyArgument
+	}
+	cargv := make([]*C.char, len(argv))
+	for i := range argv {
+		cargv[i] = C.CString(argv[i])
+		defer C.free(unsafe.Pointer(cargv[i]))
+	}
+
+	ret := C.ceph_conf_parse_argv(mount.mount, C.int(len(cargv)), &cargv[0])
+	return getError(ret)
+}
+
+// ParseDefaultConfigEnv configures the mount from the default Ceph
+// environment variable CEPH_ARGS.
+//
+// Implements:
+//  int ceph_conf_parse_env(struct ceph_mount_info *cmount, const char *var);
+func (mount *MountInfo) ParseDefaultConfigEnv() error {
+	if err := mount.validate(); err != nil {
+		return err
+	}
+	ret := C.ceph_conf_parse_env(mount.mount, nil)
+	return getError(ret)
+}
+
 // SetConfigOption sets the value of the configuration option identified by
 // the given name.
 //
diff --git a/cephfs/errors.go b/cephfs/errors.go
index b397e9e..52d5aa7 100644
--- a/cephfs/errors.go
+++ b/cephfs/errors.go
@@ -6,6 +6,7 @@ package cephfs
 import "C"
 
 import (
+	"errors"
 	"fmt"
 
 	"github.com/ceph/go-ceph/internal/errutil"
@@ -36,6 +37,14 @@ func getError(e C.int) error {
 
 // Public go errors:
 
+var (
+	// ErrEmptyArgument may be returned if a function argument is passed
+	// a zero-length slice or map.
+	ErrEmptyArgument = errors.New("Argument must contain at least one item")
+)
+
+// Public CephFSErrors:
+
 const (
 	// ErrNotConnected may be returned when client is not connected
 	// to a cluster.
diff --git a/rados/conn.go b/rados/conn.go
index 8bacb1c..a88a81b 100644
--- a/rados/conn.go
+++ b/rados/conn.go
@@ -12,6 +12,8 @@ import (
 	"github.com/ceph/go-ceph/internal/retry"
 )
 
+var argvPlaceholder = "placeholder"
+
 // ClusterStat represents Ceph cluster statistics.
 type ClusterStat struct {
 	Kb          uint64
@@ -190,28 +192,47 @@ func (c *Conn) GetClusterStats() (stat ClusterStat, err error) {
 	}, nil
 }
 
-// ParseCmdLineArgs configures the connection from command line arguments.
-func (c *Conn) ParseCmdLineArgs(args []string) error {
-	// add an empty element 0 -- Ceph treats the array as the actual contents
-	// of argv and skips the first element (the executable name)
-	argc := C.int(len(args) + 1)
-	argv := make([]*C.char, argc)
-
-	// make the first element a string just in case it is ever examined
-	argv[0] = C.CString("placeholder")
-	defer C.free(unsafe.Pointer(argv[0]))
-
-	for i, arg := range args {
-		argv[i+1] = C.CString(arg)
-		defer C.free(unsafe.Pointer(argv[i+1]))
+// ParseConfigArgv configures the connection using a unix style command line
+// argument vector.
+//
+// Implements:
+//  int rados_conf_parse_argv(rados_t cluster, int argc,
+//                            const char **argv);
+func (c *Conn) ParseConfigArgv(argv []string) error {
+	if c.cluster == nil {
+		return ErrNotConnected
+	}
+	if len(argv) == 0 {
+		return ErrEmptyArgument
+	}
+	cargv := make([]*C.char, len(argv))
+	for i := range argv {
+		cargv[i] = C.CString(argv[i])
+		defer C.free(unsafe.Pointer(cargv[i]))
 	}
 
-	ret := C.rados_conf_parse_argv(c.cluster, argc, &argv[0])
+	ret := C.rados_conf_parse_argv(c.cluster, C.int(len(cargv)), &cargv[0])
 	return getError(ret)
 }
 
+// ParseCmdLineArgs configures the connection from command line arguments.
+//
+// This function passes a placeholder value to Ceph as argv[0], see
+// ParseConfigArgv for a version of this function that allows the caller to
+// specify argv[0].
+func (c *Conn) ParseCmdLineArgs(args []string) error {
+	argv := make([]string, len(args)+1)
+	// Ceph expects a proper argv array as the actual contents with the
+	// first element containing the executable name
+	argv[0] = argvPlaceholder
+	for i := range args {
+		argv[i+1] = args[i]
+	}
+	return c.ParseConfigArgv(argv)
+}
+
 // ParseDefaultConfigEnv configures the connection from the default Ceph
-// environment variable(s).
+// environment variable CEPH_ARGS.
 func (c *Conn) ParseDefaultConfigEnv() error {
 	ret := C.rados_conf_parse_env(c.cluster, nil)
 	return getError(ret)
diff --git a/rados/errors.go b/rados/errors.go
index 7be2423..6bf98e6 100644
--- a/rados/errors.go
+++ b/rados/errors.go
@@ -50,6 +50,9 @@ func getErrorIfNegative(ret C.int) error {
 var (
 	// ErrNotConnected is returned when functions are called without a RADOS connection
 	ErrNotConnected = errors.New("RADOS not connected")
+	// ErrEmptyArgument may be returned if a function argument is passed
+	// a zero-length slice or map.
+	ErrEmptyArgument = errors.New("Argument must contain at least one item")
 )
 
 // Public radosErrors:


Test patch:
diff --git a/cephfs/cephfs_test.go b/cephfs/cephfs_test.go
index ddbd112..5162827 100644
--- a/cephfs/cephfs_test.go
+++ b/cephfs/cephfs_test.go
@@ -270,6 +270,55 @@ func TestGetSetConfigOption(t *testing.T) {
 	assert.Equal(t, origVal, currVal)
 }
 
+func TestParseConfigArgv(t *testing.T) {
+	mount, err := CreateMount()
+	require.NoError(t, err)
+	require.NotNil(t, mount)
+	defer func() { assert.NoError(t, mount.Release()) }()
+
+	origVal, err := mount.GetConfigOption("log_file")
+	assert.NoError(t, err)
+
+	err = mount.ParseConfigArgv(
+		[]string{"cephfs.test", "--log_file", "/dev/null"})
+	assert.NoError(t, err)
+
+	currVal, err := mount.GetConfigOption("log_file")
+	assert.NoError(t, err)
+	assert.Equal(t, "/dev/null", currVal)
+	assert.NotEqual(t, "/dev/null", origVal)
+
+	// ensure that an empty slice triggers an error (not a crash)
+	err = mount.ParseConfigArgv([]string{})
+	assert.Error(t, err)
+
+	// ensure we get an error for an invalid mount value
+	badMount := &MountInfo{}
+	err = badMount.ParseConfigArgv(
+		[]string{"cephfs.test", "--log_file", "/dev/null"})
+	assert.Error(t, err)
+}
+
+func TestParseDefaultConfigEnv(t *testing.T) {
+	mount, err := CreateMount()
+	require.NoError(t, err)
+	require.NotNil(t, mount)
+	defer func() { assert.NoError(t, mount.Release()) }()
+
+	origVal, err := mount.GetConfigOption("log_file")
+	assert.NoError(t, err)
+
+	err = os.Setenv("CEPH_ARGS", "--log_file /dev/null")
+	assert.NoError(t, err)
+	err = mount.ParseDefaultConfigEnv()
+	assert.NoError(t, err)
+
+	currVal, err := mount.GetConfigOption("log_file")
+	assert.NoError(t, err)
+	assert.Equal(t, "/dev/null", currVal)
+	assert.NotEqual(t, "/dev/null", origVal)
+}
+
 func TestValidate(t *testing.T) {
 	mount, err := CreateMount()
 	assert.NoError(t, err)
diff --git a/rados/rados_test.go b/rados/rados_test.go
index 3499099..0ce57c8 100644
--- a/rados/rados_test.go
+++ b/rados/rados_test.go
@@ -167,6 +167,31 @@ func (suite *RadosTestSuite) TestParseDefaultConfigEnv() {
 	assert.Equal(suite.T(), curr_val, "/dev/null")
 }
 
+func (suite *RadosTestSuite) TestParseConfigArgv() {
+	prev_val, err := suite.conn.GetConfigOption("log_file")
+	assert.NoError(suite.T(), err, "Invalid option")
+
+	argv := []string{"rados.test", "--log_file", "/dev/null"}
+	err = suite.conn.ParseConfigArgv(argv)
+	assert.NoError(suite.T(), err)
+
+	curr_val, err := suite.conn.GetConfigOption("log_file")
+	assert.NoError(suite.T(), err, "Invalid option")
+
+	assert.NotEqual(suite.T(), prev_val, "/dev/null")
+	assert.Equal(suite.T(), curr_val, "/dev/null")
+
+	// ensure that an empty slice triggers an error (not a crash)
+	err = suite.conn.ParseConfigArgv([]string{})
+	assert.Error(suite.T(), err)
+
+	// ensure we get an error for an invalid conn value
+	badConn := &Conn{}
+	err = badConn.ParseConfigArgv(
+		[]string{"cephfs.test", "--log_file", "/dev/null"})
+	assert.Error(suite.T(), err)
+}
+
 func (suite *RadosTestSuite) TestParseCmdLineArgs() {
 	prev_val, err := suite.conn.GetConfigOption("log_file")
 	assert.NoError(suite.T(), err, "Invalid option")