Repository: ceph/go-ceph
Pull request #146 (2019-12-31 21:01:23)

rbd: update rbd open, create and remove image functions
First off I am adding two new functions that map to librbd's rbd_open and rbd_open_read_only, currently named OpenImage and OpenImageReadOnly. These return new Image instances.

The Create4 function is renamed to CreateImage and I hope to have that be the canonical way to create an image in the future.

A new standalone RemoveImage function is added that more closely maps to the underlying ceph apis. The current Remove function is re-implemented to use RemoveImage.

The current Open method is marked deprecated. I plan on retaining the Open method for at least one release [1] and then removing it. I think we should do this because I expect Open to be a very commonly used function. 

I would like to see some of the other PRs that clean up the comments and test code merged prior to this one so I can make sure this PR is rebased on those before I make this mergable. Those will enable additional code checks and style are consistent.

[1] - Yeah, it's not strictly needed since we have not promised backwards compat. but I'd prefer to do it for api's that are not a total disaster and then when we start doing proper releases we can remove the bad/deprecated stuff at a predictable cadence.

Resolves: #105

## Checklist
- [x] Added tests for features and functional changes
- [x] Public functions and types are documented
- [x] Standard formatting is applied to Go code


runsisi: if we want to inspect the image in trash, then we may have to add support for opening image by id :)
runsisi: i think the current implementation of `func (image *Image) Open()`, including the one in the pr, is useless, since the `name` field of `Image` is private ?
phlogistonjohn: FWIW I do have patches prepared to add functions  OpenImageById & OpenImageByIdReadOnly but I want to get this api sorted first. :-)
phlogistonjohn: @nixpanic please see my newest patch that converts most test functions away from OpenReadWrite to OpenImage. If you like this workflow I can drop OpenReadWrite and OpenReadOnly (and the tests specific to those funcs).
Please note I had to undo some work in the test function TestImageCopy because I could not understand the flow of that test function. I plan on revisiting it later after we've hashed out the api along with some of the newer functions merged to master that still use img.Open()
phlogistonjohn: @nixpanic et al:
I've extensively reworked the patch series. Please take a look from the start.
nixpanic: Looks good over all, no major concerns with this change :smiley: 
phlogistonjohn: NB: Test suite is going to fail until #166 is resolved.
phlogistonjohn: Updated. Any new changes requested or are we generally OK with this series/approach?
phlogistonjohn: Updated.

Patch:
diff --git a/rbd/rbd.go b/rbd/rbd.go
index e69ff10..ed4db01 100644
--- a/rbd/rbd.go
+++ b/rbd/rbd.go
@@ -61,6 +61,9 @@ const (
 	imageNeedsIOContext
 	imageIsOpen
 	snapshotNeedsName
+
+	// NoSnapshot indicates that no snapshot name is in use (see OpenImage)
+	NoSnapshot = ""
 )
 
 //
@@ -331,32 +334,6 @@ func Create3(ioctx *rados.IOContext, name string, size uint64, features uint64,
 	}, nil
 }
 
-// Create4 creates a new rbd image using provided image options.
-//
-// Implements:
-//  int rbd_create4(rados_ioctx_t io, const char *name, uint64_t size,
-//                 rbd_image_options_t opts);
-func Create4(ioctx *rados.IOContext, name string, size uint64, rio *RbdImageOptions) (image *Image, err error) {
-	if rio == nil {
-		return nil, RBDError(C.EINVAL)
-	}
-
-	c_name := C.CString(name)
-	defer C.free(unsafe.Pointer(c_name))
-
-	ret := C.rbd_create4(C.rados_ioctx_t(ioctx.Pointer()), c_name,
-		C.uint64_t(size), C.rbd_image_options_t(rio.options))
-
-	if ret < 0 {
-		return nil, RBDError(ret)
-	}
-
-	return &Image{
-		ioctx: ioctx,
-		name:  name,
-	}, nil
-}
-
 // Clone a new rbd image from a snapshot.
 //
 // Implements:
@@ -399,10 +376,7 @@ func (image *Image) Remove() error {
 	if err := image.validate(imageNeedsIOContext | imageNeedsName); err != nil {
 		return err
 	}
-
-	c_name := C.CString(image.name)
-	defer C.free(unsafe.Pointer(c_name))
-	return GetError(C.rbd_remove(C.rados_ioctx_t(image.ioctx.Pointer()), c_name))
+	return RemoveImage(image.ioctx, image.name)
 }
 
 // Trash will move an image into the RBD trash, where it will be protected (i.e., salvageable) for
@@ -443,47 +417,46 @@ func (image *Image) Rename(destname string) error {
 	return err
 }
 
-// int rbd_open(rados_ioctx_t io, const char *name, rbd_image_t *image, const char *snap_name);
-// int rbd_open_read_only(rados_ioctx_t io, const char *name, rbd_image_t *image,
-//                const char *snap_name);
+// Open the rbd image.
+//
+// Deprecated: The Open function was provided in earlier versions of the API
+// and now exists to support older code. The use of OpenImage and
+// OpenImageReadOnly is preferred.
 func (image *Image) Open(args ...interface{}) error {
 	if err := image.validate(imageNeedsIOContext | imageNeedsName); err != nil {
 		return err
 	}
 
-	var c_image C.rbd_image_t
-	var c_snap_name *C.char
-	var ret C.int
-	var read_only bool
-
-	c_name := C.CString(image.name)
-
-	defer C.free(unsafe.Pointer(c_name))
+	var (
+		snapName string
+		readOnly bool
+	)
 	for _, arg := range args {
 		switch t := arg.(type) {
 		case string:
-			if t != "" {
-				c_snap_name = C.CString(t)
-				defer C.free(unsafe.Pointer(c_snap_name))
-			}
+			snapName = t
 		case bool:
-			read_only = t
+			readOnly = t
 		default:
 			return errors.New("Unexpected argument")
 		}
 	}
 
-	if read_only {
-		ret = C.rbd_open_read_only(C.rados_ioctx_t(image.ioctx.Pointer()), c_name,
-			&c_image, c_snap_name)
+	var (
+		tmp *Image
+		err error
+	)
+	if readOnly {
+		tmp, err = OpenImageReadOnly(image.ioctx, image.name, snapName)
 	} else {
-		ret = C.rbd_open(C.rados_ioctx_t(image.ioctx.Pointer()), c_name,
-			&c_image, c_snap_name)
+		tmp, err = OpenImage(image.ioctx, image.name, snapName)
+	}
+	if err != nil {
+		return err
 	}
 
-	image.image = c_image
-
-	return GetError(ret)
+	image.image = tmp.image
+	return nil
 }
 
 // Close an open rbd image.
@@ -1286,3 +1259,103 @@ func TrashRestore(ioctx *rados.IOContext, id, name string) error {
 
 	return GetError(C.rbd_trash_restore(C.rados_ioctx_t(ioctx.Pointer()), c_id, c_name))
 }
+
+// OpenImage will open an existing rbd image by name and snapshot name,
+// returning a new opened image. Pass the NoSnapshot sentinel value as the
+// snapName to explicitly indicate that no snapshot name is being provided.
+//
+// Implements:
+//  int rbd_open(rados_ioctx_t io, const char *name,
+//               rbd_image_t *image, const char *snap_name);
+func OpenImage(ioctx *rados.IOContext, name, snapName string) (*Image, error) {
+	cName := C.CString(name)
+	defer C.free(unsafe.Pointer(cName))
+
+	var cSnapName *C.char
+	if snapName != NoSnapshot {
+		cSnapName = C.CString(snapName)
+		defer C.free(unsafe.Pointer(cSnapName))
+	}
+
+	var cImage C.rbd_image_t
+	ret := C.rbd_open(
+		C.rados_ioctx_t(ioctx.Pointer()),
+		cName,
+		&cImage,
+		cSnapName)
+
+	if ret != 0 {
+		return nil, GetError(ret)
+	}
+
+	return &Image{
+		ioctx: ioctx,
+		name:  name,
+		image: cImage,
+	}, nil
+}
+
+// OpenImageReadOnly will open an existing rbd image by name and snapshot name,
+// returning a new opened-for-read image.  Pass the NoSnapshot sentinel value
+// as the snapName to explicitly indicate that no snapshot name is being
+// provided.
+//
+// Implements:
+//  int rbd_open_read_only(rados_ioctx_t io, const char *name,
+//                         rbd_image_t *image, const char *snap_name);
+func OpenImageReadOnly(ioctx *rados.IOContext, name, snapName string) (*Image, error) {
+	cName := C.CString(name)
+	defer C.free(unsafe.Pointer(cName))
+
+	var cSnapName *C.char
+	if snapName != NoSnapshot {
+		cSnapName = C.CString(snapName)
+		defer C.free(unsafe.Pointer(cSnapName))
+	}
+
+	var cImage C.rbd_image_t
+	ret := C.rbd_open_read_only(
+		C.rados_ioctx_t(ioctx.Pointer()),
+		cName,
+		&cImage,
+		cSnapName)
+
+	if ret != 0 {
+		return nil, GetError(ret)
+	}
+
+	return &Image{
+		ioctx: ioctx,
+		name:  name,
+		image: cImage,
+	}, nil
+}
+
+// CreateImage creates a new rbd image using provided image options.
+//
+// Implements:
+//  int rbd_create4(rados_ioctx_t io, const char *name, uint64_t size,
+//                 rbd_image_options_t opts);
+func CreateImage(ioctx *rados.IOContext, name string, size uint64, rio *RbdImageOptions) error {
+
+	if rio == nil {
+		return RBDError(C.EINVAL)
+	}
+
+	c_name := C.CString(name)
+	defer C.free(unsafe.Pointer(c_name))
+
+	ret := C.rbd_create4(C.rados_ioctx_t(ioctx.Pointer()), c_name,
+		C.uint64_t(size), C.rbd_image_options_t(rio.options))
+	return GetError(ret)
+}
+
+// RemoveImage removes the specified rbd image.
+//
+// Implements:
+//  int rbd_remove(rados_ioctx_t io, const char *name);
+func RemoveImage(ioctx *rados.IOContext, name string) error {
+	c_name := C.CString(name)
+	defer C.free(unsafe.Pointer(c_name))
+	return GetError(C.rbd_remove(C.rados_ioctx_t(ioctx.Pointer()), c_name))
+}


Test patch:
diff --git a/rbd/rbd_test.go b/rbd/rbd_test.go
index ed129b5..58bd4b4 100644
--- a/rbd/rbd_test.go
+++ b/rbd/rbd_test.go
@@ -15,6 +15,11 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+var (
+	testImageSize  = uint64(1 << 22)
+	testImageOrder = 22
+)
+
 func GetUUID() string {
 	return uuid.Must(uuid.NewV4()).String()
 }
@@ -70,20 +75,20 @@ func TestImageCreate(t *testing.T) {
 	require.NoError(t, err)
 
 	name := GetUUID()
-	image, err := Create(ioctx, name, 1<<22, 22)
+	image, err := Create(ioctx, name, testImageSize, testImageOrder)
 	assert.NoError(t, err)
 	err = image.Remove()
 	assert.NoError(t, err)
 
 	name = GetUUID()
-	image, err = Create(ioctx, name, 1<<22, 22,
+	image, err = Create(ioctx, name, testImageSize, testImageOrder,
 		RbdFeatureLayering|RbdFeatureStripingV2)
 	assert.NoError(t, err)
 	err = image.Remove()
 	assert.NoError(t, err)
 
 	name = GetUUID()
-	image, err = Create(ioctx, name, 1<<22, 22,
+	image, err = Create(ioctx, name, testImageSize, testImageOrder,
 		RbdFeatureLayering|RbdFeatureStripingV2, 4096, 2)
 	assert.NoError(t, err)
 	err = image.Remove()
@@ -91,11 +96,11 @@ func TestImageCreate(t *testing.T) {
 
 	// invalid order
 	name = GetUUID()
-	_, err = Create(ioctx, name, 1<<22, -1)
+	_, err = Create(ioctx, name, testImageSize, -1)
 	assert.Error(t, err)
 
 	// too many arguments
-	_, err = Create(ioctx, name, 1<<22, 22,
+	_, err = Create(ioctx, name, testImageSize, testImageOrder,
 		RbdFeatureLayering|RbdFeatureStripingV2, 4096, 2, 123)
 	assert.Error(t, err)
 
@@ -115,8 +120,8 @@ func TestImageCreate2(t *testing.T) {
 	assert.NoError(t, err)
 
 	name := GetUUID()
-	image, err := Create2(ioctx, name, 1<<22,
-		RbdFeatureLayering|RbdFeatureStripingV2, 22)
+	image, err := Create2(ioctx, name, testImageSize,
+		RbdFeatureLayering|RbdFeatureStripingV2, testImageOrder)
 	assert.NoError(t, err)
 	err = image.Remove()
 	assert.NoError(t, err)
@@ -137,8 +142,8 @@ func TestImageCreate3(t *testing.T) {
 	assert.NoError(t, err)
 
 	name := GetUUID()
-	image, err := Create3(ioctx, name, 1<<22,
-		RbdFeatureLayering|RbdFeatureStripingV2, 22, 4096, 2)
+	image, err := Create3(ioctx, name, testImageSize,
+		RbdFeatureLayering|RbdFeatureStripingV2, testImageOrder, 4096, 2)
 	assert.NoError(t, err)
 	err = image.Remove()
 	assert.NoError(t, err)
@@ -160,25 +165,25 @@ func TestCreateImageWithOptions(t *testing.T) {
 
 	// nil options, causes a panic if not handled correctly
 	name := GetUUID()
-	image, err := Create4(ioctx, name, 1<<22, nil)
+	err = CreateImage(ioctx, name, testImageSize, nil)
 	assert.Error(t, err)
 
 	options := NewRbdImageOptions()
 
 	// empty/default options
 	name = GetUUID()
-	image, err = Create4(ioctx, name, 1<<22, options)
+	err = CreateImage(ioctx, name, testImageSize, options)
 	assert.NoError(t, err)
-	err = image.Remove()
+	err = RemoveImage(ioctx, name)
 	assert.NoError(t, err)
 
 	// create image with RbdImageOptionOrder
-	err = options.SetUint64(RbdImageOptionOrder, 22)
+	err = options.SetUint64(RbdImageOptionOrder, uint64(testImageOrder))
 	assert.NoError(t, err)
 	name = GetUUID()
-	image, err = Create4(ioctx, name, 1<<22, options)
+	err = CreateImage(ioctx, name, testImageSize, options)
 	assert.NoError(t, err)
-	err = image.Remove()
+	err = RemoveImage(ioctx, name)
 	assert.NoError(t, err)
 	options.Clear()
 
@@ -189,9 +194,9 @@ func TestCreateImageWithOptions(t *testing.T) {
 	err = options.SetString(RbdImageOptionDataPool, datapool)
 	assert.NoError(t, err)
 	name = GetUUID()
-	image, err = Create4(ioctx, name, 1<<22, options)
+	err = CreateImage(ioctx, name, testImageSize, options)
 	assert.NoError(t, err)
-	err = image.Remove()
+	err = RemoveImage(ioctx, name)
 	assert.NoError(t, err)
 	conn.DeletePool(datapool)
 
@@ -215,7 +220,7 @@ func TestGetImageNames(t *testing.T) {
 	createdList := []string{}
 	for i := 0; i < 10; i++ {
 		name := GetUUID()
-		_, err := Create(ioctx, name, 1<<22, 22)
+		err = quickCreate(ioctx, name, testImageSize, testImageOrder)
 		assert.NoError(t, err)
 		createdList = append(createdList, name)
 	}
@@ -238,7 +243,7 @@ func TestGetImageNames(t *testing.T) {
 	conn.Shutdown()
 }
 
-func TestImageOpen(t *testing.T) {
+func TestDeprecatedImageOpen(t *testing.T) {
 	conn := radosConnect(t)
 
 	poolname := GetUUID()
@@ -249,13 +254,19 @@ func TestImageOpen(t *testing.T) {
 	require.NoError(t, err)
 
 	name := GetUUID()
-	image, err := Create(ioctx, name, 1<<22, 22)
+	image, err := Create(ioctx, name, testImageSize, testImageOrder)
 	assert.NoError(t, err)
 
 	// an integer is not a valid argument
 	err = image.Open(123)
 	assert.Error(t, err)
 
+	// open read-write
+	err = image.Open()
+	assert.NoError(t, err)
+	err = image.Close()
+	assert.NoError(t, err)
+
 	// open read-only
 	err = image.Open(true)
 	assert.NoError(t, err)
@@ -288,13 +299,10 @@ func TestImageResize(t *testing.T) {
 
 	name := GetUUID()
 	reqSize := uint64(1024 * 1024 * 4) // 4MB
-	image, err := Create(ioctx, name, reqSize, 22)
+	err = quickCreate(ioctx, name, reqSize, testImageOrder)
 	assert.NoError(t, err)
 
-	err = image.Resize(reqSize * 2)
-	assert.Equal(t, err, ErrImageNotOpen)
-
-	err = image.Open()
+	image, err := OpenImage(ioctx, name, NoSnapshot)
 	assert.NoError(t, err)
 
 	size, err := image.GetSize()
@@ -311,6 +319,9 @@ func TestImageResize(t *testing.T) {
 	err = image.Close()
 	assert.NoError(t, err)
 
+	err = image.Resize(reqSize * 2)
+	assert.Equal(t, err, ErrImageNotOpen)
+
 	err = image.Remove()
 	assert.NoError(t, err)
 
@@ -331,11 +342,11 @@ func TestImageProperties(t *testing.T) {
 
 	name := GetUUID()
 	reqSize := uint64(1024 * 1024 * 4) // 4MB
-	img, err := Create3(ioctx, name, reqSize,
-		RbdFeatureLayering|RbdFeatureStripingV2, 22, 4096, 2)
+	_, err = Create3(ioctx, name, reqSize,
+		RbdFeatureLayering|RbdFeatureStripingV2, testImageOrder, 4096, 2)
 	require.NoError(t, err)
 
-	err = img.Open()
+	img, err := OpenImage(ioctx, name, NoSnapshot)
 	require.NoError(t, err)
 
 	format, err := img.IsOldFormat()
@@ -385,9 +396,10 @@ func TestImageRename(t *testing.T) {
 	require.NoError(t, err)
 
 	name := GetUUID()
-	img, err := Create(ioctx, name, 1<<22, 22)
+	err = quickCreate(ioctx, name, testImageSize, testImageOrder)
 	assert.NoError(t, err)
 
+	img := GetImage(ioctx, name)
 	err = img.Rename(name)
 	assert.Error(t, err)
 
@@ -412,10 +424,10 @@ func TestImageSeek(t *testing.T) {
 	require.NoError(t, err)
 
 	name := GetUUID()
-	img, err := Create(ioctx, name, 1<<22, 22)
+	err = quickCreate(ioctx, name, testImageSize, testImageOrder)
 	assert.NoError(t, err)
 
-	err = img.Open()
+	img, err := OpenImage(ioctx, name, NoSnapshot)
 	assert.NoError(t, err)
 
 	_, err = img.Seek(0, SeekSet)
@@ -450,7 +462,7 @@ func TestImageSeek(t *testing.T) {
 
 	pos, err = img.Seek(0, SeekEnd)
 	assert.NoError(t, err)
-	assert.Equal(t, pos, int64(1<<22))
+	assert.Equal(t, pos, int64(testImageSize))
 
 	_, err = img.Seek(0, -1)
 	assert.Error(t, err)
@@ -480,10 +492,10 @@ func TestImageDiscard(t *testing.T) {
 	require.NoError(t, err)
 
 	name := GetUUID()
-	img, err := Create(ioctx, name, 1<<22, 22)
+	err = quickCreate(ioctx, name, testImageSize, testImageOrder)
 	assert.NoError(t, err)
 
-	err = img.Open()
+	img, err := OpenImage(ioctx, name, NoSnapshot)
 	assert.NoError(t, err)
 
 	n, err := img.Discard(0, 1<<16)
@@ -493,7 +505,7 @@ func TestImageDiscard(t *testing.T) {
 	err = img.Close()
 	assert.NoError(t, err)
 
-	err = img.Open(true)
+	img, err = OpenImageReadOnly(ioctx, name, NoSnapshot)
 	assert.NoError(t, err)
 
 	// when read-only, discard should fail
@@ -525,10 +537,10 @@ func TestIOReaderWriter(t *testing.T) {
 	require.NoError(t, err)
 
 	name := GetUUID()
-	img, err := Create(ioctx, name, 1<<22, 22)
+	err = quickCreate(ioctx, name, testImageSize, testImageOrder)
 	assert.NoError(t, err)
 
-	err = img.Open()
+	img, err := OpenImage(ioctx, name, NoSnapshot)
 	assert.NoError(t, err)
 
 	stats, err := img.Stat()
@@ -620,10 +632,14 @@ func TestReadAt(t *testing.T) {
 	require.NoError(t, err)
 
 	name := GetUUID()
-	img, err := Create(ioctx, name, 1<<22, 22)
+	options := NewRbdImageOptions()
+	defer options.Destroy()
+	err = options.SetUint64(RbdImageOptionOrder, uint64(testImageOrder))
+	assert.NoError(t, err)
+	err = CreateImage(ioctx, name, testImageSize, options)
 	require.NoError(t, err)
 
-	err = img.Open()
+	img, err := OpenImage(ioctx, name, NoSnapshot)
 	assert.NoError(t, err)
 
 	// write 0 bytes should succeed
@@ -666,7 +682,7 @@ func TestReadAt(t *testing.T) {
 	assert.NoError(t, err)
 
 	// writing to a read-only image should fail
-	err = img.Open(true)
+	img, err = OpenImageReadOnly(ioctx, name, NoSnapshot)
 	assert.NoError(t, err)
 
 	_, err = img.WriteAt(data_out, 256)
@@ -693,76 +709,129 @@ func TestImageCopy(t *testing.T) {
 	ioctx, err := conn.OpenIOContext(poolname)
 	require.NoError(t, err)
 
-	name := GetUUID()
-	img, err := Create(ioctx, name, 1<<22, 22)
-	require.NoError(t, err)
+	t.Run("invalidParameters", func(t *testing.T) {
+		name := GetUUID()
+		options := NewRbdImageOptions()
+		defer options.Destroy()
+		err = options.SetUint64(RbdImageOptionOrder, uint64(testImageOrder))
+		assert.NoError(t, err)
+		err = CreateImage(ioctx, name, testImageSize, options)
+		require.NoError(t, err)
 
-	// img not open, should fail
-	err = img.Copy(nil, "")
-	assert.Equal(t, err, ErrImageNotOpen)
+		// img not open, should fail
+		img := GetImage(ioctx, name)
+		err = img.Copy(nil, "")
+		assert.Equal(t, err, ErrImageNotOpen)
 
-	err = img.Open()
-	require.NoError(t, err)
+		img, err := OpenImage(ioctx, name, NoSnapshot)
+		require.NoError(t, err)
 
-	// pass invalid parameters
-	err = img.Copy(nil, "")
-	assert.Error(t, err) // order of errors not enforced
+		// pass invalid parameters
+		err = img.Copy(nil, "")
+		assert.Error(t, err) // order of errors not enforced
 
-	err = img.Copy(ioctx, "")
-	assert.Equal(t, err, ErrNoName)
+		err = img.Copy(ioctx, "")
+		assert.Equal(t, err, ErrNoName)
 
-	err = img.Copy(nil, "duplicate")
-	assert.Equal(t, err, ErrNoIOContext)
+		err = img.Copy(nil, "duplicate")
+		assert.Equal(t, err, ErrNoIOContext)
+
+		err = img.Close()
+		assert.NoError(t, err)
+		err = RemoveImage(ioctx, name)
+		assert.NoError(t, err)
+	})
 
 	// try successful copying
-	name = GetUUID()
-	err = img.Copy(ioctx, name)
-	require.NoError(t, err)
+	t.Run("successfulCopy", func(t *testing.T) {
+		name := GetUUID()
+		options := NewRbdImageOptions()
+		defer options.Destroy()
+		err = options.SetUint64(RbdImageOptionOrder, uint64(testImageOrder))
+		assert.NoError(t, err)
+		err = CreateImage(ioctx, name, testImageSize, options)
+		require.NoError(t, err)
 
-	img2 := GetImage(ioctx, name)
-	err = img2.Open()
-	require.NoError(t, err)
+		img, err := OpenImage(ioctx, name, NoSnapshot)
+		require.NoError(t, err)
 
-	err = img2.Close()
-	assert.NoError(t, err)
+		name2 := GetUUID()
+		err = img.Copy(ioctx, name2)
+		require.NoError(t, err)
 
-	err = img2.Remove()
-	assert.NoError(t, err)
+		img2, err := OpenImage(ioctx, name2, NoSnapshot)
+		require.NoError(t, err)
 
-	err = img.Close()
-	assert.NoError(t, err)
+		err = img2.Close()
+		assert.NoError(t, err)
 
-	// test with Image as parameter
-	name = GetUUID()
-	img2, err = Create(ioctx, name, 1<<22, 22)
-	require.NoError(t, err)
+		err = img2.Remove()
+		assert.NoError(t, err)
 
-	err = img.Copy2(img2)
-	assert.Equal(t, err, ErrImageNotOpen)
+		err = img.Close()
+		assert.NoError(t, err)
+	})
 
-	err = img.Open()
-	assert.NoError(t, err)
+	t.Run("copy2ImageNotOpen", func(t *testing.T) {
+		name := GetUUID()
+		name2 := GetUUID()
+		img := GetImage(ioctx, name)
+		img2 := GetImage(ioctx, name2)
 
-	err = img.Copy2(img2)
-	assert.Equal(t, err, ErrImageNotOpen)
+		err = img.Copy2(img2)
+		assert.Equal(t, err, ErrImageNotOpen)
 
-	err = img2.Open()
-	require.NoError(t, err)
+		options := NewRbdImageOptions()
+		defer options.Destroy()
+		err = options.SetUint64(RbdImageOptionOrder, uint64(testImageOrder))
+		assert.NoError(t, err)
+		err = CreateImage(ioctx, name, testImageSize, options)
+		require.NoError(t, err)
+		img, err = OpenImage(ioctx, name, NoSnapshot)
+		assert.NoError(t, err)
 
-	err = img.Copy2(img2)
-	require.NoError(t, err)
+		err = img.Copy2(img2)
+		assert.Equal(t, err, ErrImageNotOpen)
 
-	err = img2.Close()
-	assert.NoError(t, err)
+		err = img.Close()
+		assert.NoError(t, err)
+		err = RemoveImage(ioctx, name)
+		assert.NoError(t, err)
+	})
 
-	err = img2.Remove()
-	assert.NoError(t, err)
+	t.Run("successfulCopy2", func(t *testing.T) {
+		name := GetUUID()
+		name2 := GetUUID()
 
-	err = img.Close()
-	assert.NoError(t, err)
+		options := NewRbdImageOptions()
+		defer options.Destroy()
+		err = options.SetUint64(RbdImageOptionOrder, uint64(testImageOrder))
+		assert.NoError(t, err)
+		err = CreateImage(ioctx, name, testImageSize, options)
+		require.NoError(t, err)
+		img, err := OpenImage(ioctx, name, NoSnapshot)
+		assert.NoError(t, err)
 
-	err = img.Remove()
-	assert.NoError(t, err)
+		err = CreateImage(ioctx, name2, testImageSize, options)
+		require.NoError(t, err)
+		img2, err := OpenImage(ioctx, name2, NoSnapshot)
+		assert.NoError(t, err)
+
+		err = img.Copy2(img2)
+		require.NoError(t, err)
+
+		err = img2.Close()
+		assert.NoError(t, err)
+
+		err = img2.Remove()
+		assert.NoError(t, err)
+
+		err = img.Close()
+		assert.NoError(t, err)
+
+		err = img.Remove()
+		assert.NoError(t, err)
+	})
 
 	ioctx.Destroy()
 	conn.DeletePool(poolname)
@@ -780,23 +849,25 @@ func TestCreateSnapshot(t *testing.T) {
 	require.NoError(t, err)
 
 	name := GetUUID()
-	img, err := Create(ioctx, name, 1<<22, 22)
+	err = quickCreate(ioctx, name, testImageSize, testImageOrder)
 	assert.NoError(t, err)
 
-	err = img.Open()
+	img, err := OpenImage(ioctx, name, NoSnapshot)
 	assert.NoError(t, err)
 
 	snapshot, err := img.CreateSnapshot("mysnap")
 	assert.NoError(t, err)
 
 	err = img.Close()
-	err = img.Open("mysnap")
 	assert.NoError(t, err)
 
-	snapshot.Remove()
+	snapImage, err := OpenImage(ioctx, name, "mysnap")
 	assert.NoError(t, err)
 
-	err = img.Close()
+	err = snapImage.Close()
+	assert.NoError(t, err)
+
+	snapshot.Remove()
 	assert.NoError(t, err)
 
 	img.Remove()
@@ -818,10 +889,10 @@ func TestParentInfo(t *testing.T) {
 	require.NoError(t, err)
 
 	name := "parent"
-	img, err := Create(ioctx, name, 1<<22, 22, 1)
+	img, err := Create(ioctx, name, testImageSize, testImageOrder, 1)
 	assert.NoError(t, err)
 
-	err = img.Open()
+	img, err = OpenImage(ioctx, name, NoSnapshot)
 	assert.NoError(t, err)
 
 	snapshot, err := img.CreateSnapshot("mysnap")
@@ -831,8 +902,7 @@ func TestParentInfo(t *testing.T) {
 	assert.NoError(t, err)
 
 	// create an image context with the parent+snapshot
-	snapImg := GetImage(ioctx, "parent")
-	err = snapImg.Open("mysnap")
+	snapImg, err := OpenImage(ioctx, name, "mysnap")
 	assert.NoError(t, err)
 
 	// ensure no children prior to clone
@@ -845,10 +915,10 @@ func TestParentInfo(t *testing.T) {
 	_, err = img.Clone("mysnap", ioctx, "child", 1, -1)
 	assert.Error(t, err)
 
-	imgNew, err := img.Clone("mysnap", ioctx, "child", 1, 22)
+	_, err = img.Clone("mysnap", ioctx, "child", 1, testImageOrder)
 	assert.NoError(t, err)
 
-	err = imgNew.Open()
+	imgNew, err := OpenImage(ioctx, "child", NoSnapshot)
 	assert.NoError(t, err)
 	parentPool := make([]byte, 128)
 	parentName := make([]byte, 128)
@@ -1007,12 +1077,9 @@ func TestNotFound(t *testing.T) {
 
 	name := GetUUID()
 
-	img := GetImage(ioctx, name)
-	err = img.Open()
-	assert.Equal(t, err, ErrNotFound)
-
-	err = img.Remove()
+	img, err := OpenImage(ioctx, name, NoSnapshot)
 	assert.Equal(t, err, ErrNotFound)
+	assert.Nil(t, img)
 
 	ioctx.Destroy()
 	conn.DeletePool(poolname)
@@ -1030,10 +1097,10 @@ func TestErrorSnapshotNoName(t *testing.T) {
 	require.NoError(t, err)
 
 	name := GetUUID()
-	img, err := Create(ioctx, name, 1<<22, 22)
+	err = quickCreate(ioctx, name, testImageSize, testImageOrder)
 	assert.NoError(t, err)
 
-	err = img.Open()
+	img, err := OpenImage(ioctx, name, NoSnapshot)
 	assert.NoError(t, err)
 
 	// this actually works for some reason?!
@@ -1081,9 +1148,10 @@ func TestTrashImage(t *testing.T) {
 	require.NoError(t, err)
 
 	name := GetUUID()
-	image, err := Create(ioctx, name, 1<<22, 22)
+	err = quickCreate(ioctx, name, testImageSize, testImageOrder)
 	assert.NoError(t, err)
 
+	image := GetImage(ioctx, name)
 	err = image.Trash(time.Hour)
 	assert.NoError(t, err)
 
@@ -1124,9 +1192,10 @@ func TestImageMetadata(t *testing.T) {
 	require.NoError(t, err)
 
 	name := GetUUID()
-	image, err := Create(ioctx, name, 1<<22, 22)
+	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)
@@ -1142,7 +1211,7 @@ func TestImageMetadata(t *testing.T) {
 	assert.Equal(t, "", value)
 	assert.Equal(t, err, ErrImageNotOpen)
 
-	err = image.Open()
+	image, err = OpenImage(ioctx, name, NoSnapshot)
 	assert.NoError(t, err)
 
 	// Set a metadata key/value
@@ -1183,10 +1252,10 @@ func TestClosedImage(t *testing.T) {
 	require.NoError(t, err)
 
 	name := GetUUID()
-	image, err := Create(ioctx, name, 1<<22, 22)
+	err = quickCreate(ioctx, name, testImageSize, testImageOrder)
 	assert.NoError(t, err)
 
-	err = image.Open()
+	image, err := OpenImage(ioctx, name, NoSnapshot)
 	assert.NoError(t, err)
 
 	// keep the rbdImage around after close
@@ -1259,3 +1328,100 @@ func TestClosedImage(t *testing.T) {
 	conn.DeletePool(poolname)
 	conn.Shutdown()
 }
+
+func TestOpenImage(t *testing.T) {
+	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 = OpenImage(ioctx, name, NoSnapshot)
+	assert.Error(t, err)
+
+	err = quickCreate(ioctx, name, testImageSize, testImageOrder)
+	assert.NoError(t, err)
+
+	oImage, err := OpenImage(ioctx, name, NoSnapshot)
+	assert.NoError(t, err)
+	assert.Equal(t, name, oImage.name)
+	err = oImage.Close()
+	assert.NoError(t, err)
+
+	// open read-only
+	oImage, err = OpenImageReadOnly(ioctx, name, NoSnapshot)
+	assert.NoError(t, err)
+
+	bytes_in := []byte("input data")
+	_, err = oImage.Write(bytes_in)
+	// writing should fail in read-only mode
+	assert.Error(t, err)
+
+	err = oImage.Close()
+	assert.NoError(t, err)
+
+	err = oImage.Remove()
+	assert.NoError(t, err)
+
+	_, err = OpenImageReadOnly(ioctx, name, NoSnapshot)
+	assert.Error(t, err)
+
+	ioctx.Destroy()
+	conn.DeletePool(poolname)
+	conn.Shutdown()
+}
+
+func TestRemoveImage(t *testing.T) {
+	conn := radosConnect(t)
+
+	poolname := GetUUID()
+	err := conn.MakePool(poolname)
+	assert.NoError(t, err)
+
+	ioctx, err := conn.OpenIOContext(poolname)
+	require.NoError(t, err)
+
+	// trying to remove a non-existent image is an error
+	err = RemoveImage(ioctx, "bananarama")
+	require.Error(t, err)
+
+	// create and then remove an image
+	name := GetUUID()
+	options := NewRbdImageOptions()
+	defer options.Destroy()
+	err = CreateImage(ioctx, name, testImageSize, options)
+	assert.NoError(t, err)
+
+	imageNames, err := GetImageNames(ioctx)
+	assert.NoError(t, err)
+	assert.Contains(t, imageNames, name)
+
+	err = RemoveImage(ioctx, name)
+	assert.NoError(t, err)
+
+	imageNames, err = GetImageNames(ioctx)
+	assert.NoError(t, err)
+	assert.NotContains(t, imageNames, name)
+
+	ioctx.Destroy()
+	conn.DeletePool(poolname)
+	conn.Shutdown()
+}
+
+// quickCreate creates an image similar to Create but uses CreateImage.
+// If possible, avoid using this function for new code/tests. It mainly exists
+// to help with refactoring of existing tests.
+func quickCreate(ioctx *rados.IOContext, name string, size uint64, order int) error {
+	options := NewRbdImageOptions()
+	defer options.Destroy()
+	err := options.SetUint64(RbdImageOptionOrder, uint64(order))
+	if err != nil {
+		return err
+	}
+	return CreateImage(ioctx, name, size, options)
+}