Repository: ceph/go-ceph
Pull request #169 (2020-01-31 15:12:50)

rbd: add better error when attempting to remove opened images
Trying to remove an opened read-write image results in an error `Device or resource busy`. This change adds an additional validation case in the `Remove()` method and returns more understandable error.

Signed-off-by: Sven Anderson <sven@redhat.com>

## Checklist
- [x] Added tests for features and functional changes
- [x] Public functions and types are documented
- [x] Standard formatting is applied to Go code


ansiwen: Here the additional check when removing opened image, that I wrote while debugging #122. On second thought I don't like my own PR. The right approach would be to improve the error coming from librbd. Why are we rewriting error messages? Also the remove doesn't fail if the image is opened read-only, as I found out later. I would prefer that we keep at least the low-level API simpler.
phlogistonjohn: > Here the additional check when removing opened image, that I wrote while debugging #122. On second thought I don't like my own PR.

Hehe. OK. We'll try to avoid merging this unless you give the go-ahead.

> The right approach would be to improve the error coming from librbd. Why are we rewriting error messages? Also the remove doesn't fail if the image is opened read-only, as I found out later, so we rejecting cases that librbd would allow. Or we would need to validate also for read-write mode, adding more complexity.

I'm not sure what we're re-writing. Unless you mean that the error check is redundant, which I largely agree with.
I'd say given the complexity you're outlining here there's really nothing wrong with relying on the errors ceph gives (the "Device or resource busy"). We only want additional error handling in the cases where we're doing something "extra". So for all those function calls that rely on an Image we're validating that the image has the fields populated needed to make the librbd function call. In addition to that I'd say we want to avoid crashing due to easy-to-pick-up-footguns, such as closing a connection with an open image leads to a crash ... but that's a discussion for the other issue. :-)
So unless this particular path also leads to crashes we can decline this PR if that's your preference.

> I would prefer that we keep at least the low-level API simpler.

I also would like to keep the low-level APIs simple.

ansiwen: With regard to this (what I consider) [bug](https://tracker.ceph.com/issues/45437) I changed my mind and think it's good to merge this, to avoid segmentation faults after successfully removing opened images.

Patch:
diff --git a/rbd/errors.go b/rbd/errors.go
index 60dbd8a..1d0e579 100644
--- a/rbd/errors.go
+++ b/rbd/errors.go
@@ -61,6 +61,8 @@ var (
 	ErrSnapshotNoName = errors.New("RBD snapshot does not have a name")
 	// ErrImageNotOpen may be returned if an api call requires an open image handle and one is not provided.
 	ErrImageNotOpen = errors.New("RBD image not open")
+	// ErrImageIsOpen may be returned if an api call requires a closed image handle and one is not provided.
+	ErrImageIsOpen = errors.New("RBD image is open")
 	// ErrNotFound may be returned from an api call when the requested item is
 	// missing.
 	ErrNotFound = errors.New("RBD image not found")
diff --git a/rbd/rbd.go b/rbd/rbd.go
index fc0b9df..42decf2 100644
--- a/rbd/rbd.go
+++ b/rbd/rbd.go
@@ -38,6 +38,7 @@ const (
 	imageNeedsName uint32 = 1 << iota
 	imageNeedsIOContext
 	imageIsOpen
+	imageIsNotOpen
 	snapshotNeedsName
 
 	// NoSnapshot indicates that no snapshot name is in use (see OpenImage)
@@ -121,6 +122,8 @@ func (image *Image) validate(req uint32) error {
 		return ErrNoIOContext
 	} else if hasBit(req, imageIsOpen) && image.image == nil {
 		return ErrImageNotOpen
+	} else if hasBit(req, imageIsNotOpen) && image.image != nil {
+		return ErrImageIsOpen
 	}
 
 	return nil
@@ -278,7 +281,7 @@ func (image *Image) Clone(snapname string, c_ioctx *rados.IOContext, c_name stri
 // Implements:
 //  int rbd_remove(rados_ioctx_t io, const char *name);
 func (image *Image) Remove() error {
-	if err := image.validate(imageNeedsIOContext | imageNeedsName); err != nil {
+	if err := image.validate(imageNeedsIOContext | imageNeedsName | imageIsNotOpen); err != nil {
 		return err
 	}
 	return RemoveImage(image.ioctx, image.name)


Test patch:
diff --git a/rbd/rbd_test.go b/rbd/rbd_test.go
index bfd231e..6315495 100644
--- a/rbd/rbd_test.go
+++ b/rbd/rbd_test.go
@@ -263,6 +263,10 @@ func TestDeprecatedImageOpen(t *testing.T) {
 	// writing should fail in read-only mode
 	assert.Error(t, err)
 
+	err = image.Remove()
+	// removing should fail while image is opened
+	assert.Equal(t, err, ErrImageIsOpen)
+
 	err = image.Close()
 	assert.NoError(t, err)