Repository: ceph/go-ceph
Pull request #386 (2020-10-22 17:25:05)

rbd: metadata "list" function
Light refactoring of the locations of some of the prior image metadata functions.

Add ListMetadata implementing rbd_metadata_list. The names a bit wonky (see commit msg) but I think its fine. Let me know if you think its confusing and we can discuss alternatives.

Fixes: #384 

## 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: It appears that this weeks release of ceph nautilus is now breaking the cephfs admin test suite. Please bear with me as I investigate these failures.
phlogistonjohn: @Mergifyio rebase
mergify[bot]: **Command `rebase`: success**
> **Branch has been successfully rebased**
> 

nixpanic: @mergifyio refresh
mergify[bot]: **Command `refresh`: success**
> **Pull request refreshed**
> 

phlogistonjohn: @Mergifyio rebase
mergify[bot]: **Command `rebase`: success**
> **Branch has been successfully rebased**
> 


Patch:
diff --git a/rbd/metadata.go b/rbd/metadata.go
new file mode 100644
index 0000000..3be2d95
--- /dev/null
+++ b/rbd/metadata.go
@@ -0,0 +1,149 @@
+package rbd
+
+// #cgo LDFLAGS: -lrbd
+// #include <stdlib.h>
+// #include <rbd/librbd.h>
+import "C"
+
+import (
+	"unsafe"
+
+	"github.com/ceph/go-ceph/internal/cutil"
+	"github.com/ceph/go-ceph/internal/retry"
+)
+
+// GetMetadata returns the metadata string associated with the given key.
+//
+// Implements:
+//  int rbd_metadata_get(rbd_image_t image, const char *key, char *value, size_t *vallen)
+func (image *Image) GetMetadata(key string) (string, error) {
+	if err := image.validate(imageIsOpen); err != nil {
+		return "", err
+	}
+
+	c_key := C.CString(key)
+	defer C.free(unsafe.Pointer(c_key))
+
+	var (
+		buf []byte
+		err error
+	)
+	retry.WithSizes(4096, 262144, func(size int) retry.Hint {
+		csize := C.size_t(size)
+		buf = make([]byte, csize)
+		// rbd_metadata_get is a bit quirky and *does not* update the size
+		// value if the size passed in >= the needed size.
+		ret := C.rbd_metadata_get(
+			image.image, c_key, (*C.char)(unsafe.Pointer(&buf[0])), &csize)
+		err = getError(ret)
+		return retry.Size(int(csize)).If(err == errRange)
+	})
+	if err != nil {
+		return "", err
+	}
+	return C.GoString((*C.char)(unsafe.Pointer(&buf[0]))), nil
+}
+
+// SetMetadata updates the metadata string associated with the given key.
+//
+// Implements:
+//  int rbd_metadata_set(rbd_image_t image, const char *key, const char *value)
+func (image *Image) SetMetadata(key string, value string) error {
+	if err := image.validate(imageIsOpen); err != nil {
+		return err
+	}
+
+	c_key := C.CString(key)
+	c_value := C.CString(value)
+	defer C.free(unsafe.Pointer(c_key))
+	defer C.free(unsafe.Pointer(c_value))
+
+	ret := C.rbd_metadata_set(image.image, c_key, c_value)
+	if ret < 0 {
+		return rbdError(ret)
+	}
+
+	return nil
+}
+
+// RemoveMetadata clears the metadata associated with the given key.
+//
+// Implements:
+//  int rbd_metadata_remove(rbd_image_t image, const char *key)
+func (image *Image) RemoveMetadata(key string) error {
+	if err := image.validate(imageIsOpen); err != nil {
+		return err
+	}
+
+	c_key := C.CString(key)
+	defer C.free(unsafe.Pointer(c_key))
+
+	ret := C.rbd_metadata_remove(image.image, c_key)
+	if ret < 0 {
+		return rbdError(ret)
+	}
+
+	return nil
+}
+
+// ListMetadata returns a map containing all metadata assigned to the RBD image.
+//
+// Implements:
+//  int rbd_metadata_list(rbd_image_t image, const char *start, uint64_t max,
+//                        char *keys, size_t *key_len, char *values, size_t *vals_len);
+func (image *Image) ListMetadata() (map[string]string, error) {
+	if err := image.validate(imageIsOpen); err != nil {
+		return nil, err
+	}
+
+	var (
+		err      error
+		keysbuf  []byte
+		keysSize C.size_t
+		valsbuf  []byte
+		valsSize C.size_t
+	)
+	retry.WithSizes(4096, 262144, func(size int) retry.Hint {
+		keysbuf = make([]byte, size)
+		keysSize = C.size_t(size)
+		valsbuf = make([]byte, size)
+		valsSize = C.size_t(size)
+		// the rbd_metadata_list function can use a start point and a limit.
+		// we do not use it and prefer our retry helper and just allocating
+		// buffers large enough to take all the keys and values
+		ret := C.rbd_metadata_list(
+			image.image,
+			(*C.char)(unsafe.Pointer(&empty[0])), // always start at the beginning (no paging)
+			0,                                    // fetch all key-value pairs
+			(*C.char)(unsafe.Pointer(&keysbuf[0])),
+			&keysSize,
+			(*C.char)(unsafe.Pointer(&valsbuf[0])),
+			&valsSize)
+
+		err = getError(ret)
+		nextSize := valsSize
+		if keysSize > nextSize {
+			nextSize = keysSize
+		}
+		return retry.Size(int(nextSize)).If(err == errRange)
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	m := map[string]string{}
+	keys := cutil.SplitBuffer(keysbuf[:keysSize])
+	vals := cutil.SplitBuffer(valsbuf[:valsSize])
+	if len(keys) != len(vals) {
+		// this should not happen (famous last words)
+		return nil, errRange
+	}
+	for i := range keys {
+		m[keys[i]] = vals[i]
+	}
+	return m, nil
+}
+
+// rather than allocate memory every time that ListMetadata is called,
+// define a static byte slice to stand in for the C "empty string"
+var empty = []byte{0}
diff --git a/rbd/rbd.go b/rbd/rbd.go
index b5f0648..7e9ae23 100644
--- a/rbd/rbd.go
+++ b/rbd/rbd.go
@@ -853,80 +853,6 @@ func (image *Image) GetSnapshotNames() (snaps []SnapInfo, err error) {
 	return snaps[:len(snaps)-1], nil
 }
 
-// GetMetadata returns the metadata string associated with the given key.
-//
-// Implements:
-//  int rbd_metadata_get(rbd_image_t image, const char *key, char *value, size_t *vallen)
-func (image *Image) GetMetadata(key string) (string, error) {
-	if err := image.validate(imageIsOpen); err != nil {
-		return "", err
-	}
-
-	c_key := C.CString(key)
-	defer C.free(unsafe.Pointer(c_key))
-
-	var (
-		buf []byte
-		err error
-	)
-	retry.WithSizes(4096, 262144, func(size int) retry.Hint {
-		csize := C.size_t(size)
-		buf = make([]byte, csize)
-		// rbd_metadata_get is a bit quirky and *does not* update the size
-		// value if the size passed in >= the needed size.
-		ret := C.rbd_metadata_get(
-			image.image, c_key, (*C.char)(unsafe.Pointer(&buf[0])), &csize)
-		err = getError(ret)
-		return retry.Size(int(csize)).If(err == errRange)
-	})
-	if err != nil {
-		return "", err
-	}
-	return C.GoString((*C.char)(unsafe.Pointer(&buf[0]))), nil
-}
-
-// SetMetadata updates the metadata string associated with the given key.
-//
-// Implements:
-//  int rbd_metadata_set(rbd_image_t image, const char *key, const char *value)
-func (image *Image) SetMetadata(key string, value string) error {
-	if err := image.validate(imageIsOpen); err != nil {
-		return err
-	}
-
-	c_key := C.CString(key)
-	c_value := C.CString(value)
-	defer C.free(unsafe.Pointer(c_key))
-	defer C.free(unsafe.Pointer(c_value))
-
-	ret := C.rbd_metadata_set(image.image, c_key, c_value)
-	if ret < 0 {
-		return rbdError(ret)
-	}
-
-	return nil
-}
-
-// RemoveMetadata clears the metadata associated with the given key.
-//
-// Implements:
-//  int rbd_metadata_remove(rbd_image_t image, const char *key)
-func (image *Image) RemoveMetadata(key string) error {
-	if err := image.validate(imageIsOpen); err != nil {
-		return err
-	}
-
-	c_key := C.CString(key)
-	defer C.free(unsafe.Pointer(c_key))
-
-	ret := C.rbd_metadata_remove(image.image, c_key)
-	if ret < 0 {
-		return rbdError(ret)
-	}
-
-	return nil
-}
-
 // GetId returns the internal image ID string.
 //
 // Implements:


Test patch:
diff --git a/rbd/metadata_test.go b/rbd/metadata_test.go
new file mode 100644
index 0000000..da5a802
--- /dev/null
+++ b/rbd/metadata_test.go
@@ -0,0 +1,100 @@
+package rbd
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestImageMetadata(t *testing.T) {
+	metadataKey := "mykey"
+	metadataValue := "myvalue"
+
+	conn := radosConnect(t)
+
+	poolname := GetUUID()
+	err := conn.MakePool(poolname)
+	assert.NoError(t, err)
+
+	ioctx, err := conn.OpenIOContext(poolname)
+	require.NoError(t, err)
+
+	name := GetUUID()
+	err = quickCreate(ioctx, name, testImageSize, testImageOrder)
+	assert.NoError(t, err)
+
+	image := GetImage(ioctx, name)
+	// Set a metadata key/value on unopen image
+	err = image.SetMetadata(metadataKey, metadataValue)
+	assert.Equal(t, err, ErrImageNotOpen)
+	// Get the metadata value on unopen image
+	value, err := image.GetMetadata(metadataKey)
+	assert.Equal(t, err, ErrImageNotOpen)
+	assert.Equal(t, "", value)
+	// Remove the metadata key on unopen image
+	err = image.RemoveMetadata(metadataKey)
+	assert.Equal(t, err, ErrImageNotOpen)
+	// check key is removed on unopen image
+	value, err = image.GetMetadata(metadataKey)
+	assert.Equal(t, "", value)
+	assert.Equal(t, err, ErrImageNotOpen)
+	// check ListMetadata for unopened image
+	_, err = image.ListMetadata()
+	assert.Equal(t, err, ErrImageNotOpen)
+
+	image, err = OpenImage(ioctx, name, NoSnapshot)
+	assert.NoError(t, err)
+
+	// Set a metadata key/value
+	err = image.SetMetadata(metadataKey, metadataValue)
+	assert.NoError(t, err)
+	// Get the metadata value
+	value, err = image.GetMetadata(metadataKey)
+	assert.NoError(t, err)
+	assert.Equal(t, metadataValue, value)
+	// List metadata
+	mm, err := image.ListMetadata()
+	assert.NoError(t, err)
+	assert.Len(t, mm, 1)
+	if assert.Contains(t, mm, metadataKey) {
+		assert.Equal(t, mm[metadataKey], metadataValue)
+	}
+	// Remove the metadata key
+	err = image.RemoveMetadata(metadataKey)
+	assert.NoError(t, err)
+	// check key is removed
+	value, err = image.GetMetadata(metadataKey)
+	assert.Equal(t, "", value)
+	assert.Error(t, err)
+
+	// Set some additional metadata values
+	m1 := map[string]string{
+		metadataKey:  metadataValue,
+		"k2":         "himalayas",
+		"matterhorn": "alps",
+		"aconcagua":  "andes",
+	}
+	for k, v := range m1 {
+		err = image.SetMetadata(k, v)
+		assert.NoError(t, err)
+	}
+	// Get the metadata value
+	value, err = image.GetMetadata(metadataKey)
+	assert.NoError(t, err)
+	assert.Equal(t, metadataValue, value)
+	// List metadata
+	mm, err = image.ListMetadata()
+	assert.NoError(t, err)
+	assert.Len(t, mm, 4)
+	assert.Equal(t, m1, mm)
+
+	err = image.Close()
+	assert.NoError(t, err)
+	err = image.Remove()
+	assert.NoError(t, err)
+
+	ioctx.Destroy()
+	conn.DeletePool(poolname)
+	conn.Shutdown()
+}
diff --git a/rbd/rbd_test.go b/rbd/rbd_test.go
index 31f9c24..6ca0906 100644
--- a/rbd/rbd_test.go
+++ b/rbd/rbd_test.go
@@ -1082,67 +1082,6 @@ func TestTrashImage(t *testing.T) {
 	conn.Shutdown()
 }
 
-func TestImageMetadata(t *testing.T) {
-	metadataKey := "mykey"
-	metadataValue := "myvalue"
-
-	conn := radosConnect(t)
-
-	poolname := GetUUID()
-	err := conn.MakePool(poolname)
-	assert.NoError(t, err)
-
-	ioctx, err := conn.OpenIOContext(poolname)
-	require.NoError(t, err)
-
-	name := GetUUID()
-	err = quickCreate(ioctx, name, testImageSize, testImageOrder)
-	assert.NoError(t, err)
-
-	image := GetImage(ioctx, name)
-	// Set a metadata key/value on unopen image
-	err = image.SetMetadata(metadataKey, metadataValue)
-	assert.Equal(t, err, ErrImageNotOpen)
-	// Get the metadata value on unopen image
-	value, err := image.GetMetadata(metadataKey)
-	assert.Equal(t, err, ErrImageNotOpen)
-	assert.Equal(t, "", value)
-	// Remove the metadata key on unopen image
-	err = image.RemoveMetadata(metadataKey)
-	assert.Equal(t, err, ErrImageNotOpen)
-	// check key is removed on unopen image
-	value, err = image.GetMetadata(metadataKey)
-	assert.Equal(t, "", value)
-	assert.Equal(t, err, ErrImageNotOpen)
-
-	image, err = OpenImage(ioctx, name, NoSnapshot)
-	assert.NoError(t, err)
-
-	// Set a metadata key/value
-	err = image.SetMetadata(metadataKey, metadataValue)
-	assert.NoError(t, err)
-	// Get the metadata value
-	value, err = image.GetMetadata(metadataKey)
-	assert.NoError(t, err)
-	assert.Equal(t, metadataValue, value)
-	// Remove the metadata key
-	err = image.RemoveMetadata(metadataKey)
-	assert.NoError(t, err)
-	// check key is removed
-	value, err = image.GetMetadata(metadataKey)
-	assert.Equal(t, "", value)
-	assert.Error(t, err)
-
-	err = image.Close()
-	assert.NoError(t, err)
-	err = image.Remove()
-	assert.NoError(t, err)
-
-	ioctx.Destroy()
-	conn.DeletePool(poolname)
-	conn.Shutdown()
-}
-
 func TestClosedImage(t *testing.T) {
 	t.Skipf("many of the following functions cause a panic or hang, skip this test")