Bug 752807

Summary: Do not return -1 from STACK_WIND targets ever
Product: [Fedora] Fedora Reporter: Kaleb KEITHLEY <kkeithle>
Component: hekafsAssignee: Kaleb KEITHLEY <kkeithle>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: high    
Version: 16CC: jdarcy, kkeithle, zaitcev
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: hekafs-0.7-18.fc16 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2011-11-23 00:59:24 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:

Description Kaleb KEITHLEY 2011-11-10 13:24:21 UTC 
Description of problem:

My hekafs server was misconfigured and some functions failed in places
where they normally would not, and that uncovered a problem: whenever
that happened, the client would hang. This happens because we cannot
return errors like normal people do in the kernel, by throwing an
error code. Functions called through STACK_WIND must return zero and
report the error through the callback.

Not sure what errno to set in this case. Generally I just want to
indicate that "something is busted". In the past I would return EDOM.
The ENOSPC seems ridiculous enough here.

Version-Release number of selected component (if applicable):

All

How reproducible:

set uid or gid ranges 0, e.g. uidmin/uidmax: 10000/10000.

Steps to Reproduce:

N/A

Actual results:

N/A

Expected results:

N/A

Additional info:

diff --git a/xlators/features/uidmap/src/uidmap.c b/xlators/features/uidmap/src/uidmap.c
index 91104fd..191fb57 100644
--- a/xlators/features/uidmap/src/uidmap.c
+++ b/xlators/features/uidmap/src/uidmap.c
@@ -34,6 +34,9 @@
 #include "common-utils.h"
 #include "uidmap.h"
 
+/* ugly #includes below */
+#include <errno.h>
+
 #define HFS_UID_LOW_DEFAULT 10000
 #define HFS_UID_HIGH_DEFAULT 19999
 #define HFS_GID_LOW_DEFAULT 10000
@@ -1350,7 +1353,8 @@ uidmap_entrylk(call_frame_t *frame, xlator_t *this,
 		   ((type == ENTRYLK_RDLCK) ? "ENTRYLK_RDLCK" : "ENTRYLK_WRLCK"));
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(entrylk, frame, -1, ENOSPC);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_entrylk_cbk,
@@ -1373,7 +1377,8 @@ uidmap_fentrylk(call_frame_t *frame, xlator_t *this,
 		   ((type == ENTRYLK_RDLCK) ? "ENTRYLK_RDLCK" : "ENTRYLK_WRLCK"));
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(entrylk, frame, -1, ENOSPC);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_fentrylk_cbk,
@@ -1389,7 +1394,8 @@ uidmap_inodelk(call_frame_t *frame, xlator_t *this, const char *volume,
 		  loc_t *loc, int32_t cmd, struct gf_flock *flock)
 {
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(inodelk, frame, -1, ENOSPC);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_inodelk_cbk,
@@ -1409,7 +1415,8 @@ uidmap_finodelk(call_frame_t *frame, xlator_t *this, const char *volume,
 		   frame->root->unique, volume, fd, cmd);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(finodelk, frame, -1, ENOSPC);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_finodelk_cbk,
@@ -1429,7 +1436,8 @@ uidmap_xattrop(call_frame_t *frame, xlator_t *this, loc_t *loc,
 		   frame->root->unique, loc->path, loc->inode->ino, flags);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(xattrop, frame, -1, ENOSPC, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_xattrop_cbk,
@@ -1450,7 +1458,8 @@ uidmap_fxattrop(call_frame_t *frame, xlator_t *this, fd_t *fd,
 		   frame->root->unique, fd, flags);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(fxattrop, frame, -1, ENOSPC, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_fxattrop_cbk,
@@ -1473,7 +1482,9 @@ uidmap_lookup(call_frame_t *frame, xlator_t *this,
 		   loc->inode->ino);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(lookup, frame, -1, ENOSPC,
+					NULL, NULL, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_lookup_cbk,
@@ -1493,7 +1504,8 @@ uidmap_stat(call_frame_t *frame, xlator_t *this, loc_t *loc)
 		   frame->root->unique, loc->path, loc->inode->ino);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(stat, frame, -1, ENOSPC, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_stat_cbk,
@@ -1514,7 +1526,8 @@ uidmap_rchecksum(call_frame_t *frame, xlator_t *this, fd_t *fd,
 		   frame->root->unique, fd->inode->ino);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(rchecksum, frame, -1, ENOSPC, 0, 0);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_rchecksum_cbk,
@@ -1535,7 +1548,8 @@ uidmap_getspec(call_frame_t *frame, xlator_t *this, const char *key,
 		   frame->root->unique, key, flag);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(getspec, frame, -1, ENOSPC, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_getspec_cbk,
@@ -1555,7 +1569,8 @@ uidmap_readlink(call_frame_t *frame, xlator_t *this, loc_t *loc, size_t size)
 		   frame->root->unique, loc->path, loc->inode->ino, size);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(readlink, frame, -1, ENOSPC, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_readlink_cbk,
@@ -1576,7 +1591,9 @@ uidmap_mknod(call_frame_t *frame, xlator_t *this, loc_t *loc,
 		   frame->root->unique, loc->path, loc->inode->ino, mode, dev);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(mknod, frame, -1, ENOSPC, NULL, NULL,
+					NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_mknod_cbk,
@@ -1598,7 +1615,9 @@ uidmap_mkdir(call_frame_t *frame, xlator_t *this, loc_t *loc, mode_t mode,
 		   ((loc->inode)? loc->inode->ino : 0), mode);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(mkdir, frame, -1, ENOSPC,
+					NULL, NULL, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_mkdir_cbk,
@@ -1617,7 +1636,8 @@ uidmap_unlink(call_frame_t *frame, xlator_t *this, loc_t *loc)
 		   frame->root->unique, loc->path, loc->inode->ino);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(unlink, frame, -1, ENOSPC, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_unlink_cbk,
@@ -1636,7 +1656,8 @@ uidmap_rmdir(call_frame_t *frame, xlator_t *this, loc_t *loc, int flags)
 		   frame->root->unique, loc->path, loc->inode->ino, flags);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(rmdir, frame, -1, ENOSPC, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_rmdir_cbk,
@@ -1658,7 +1679,9 @@ uidmap_symlink(call_frame_t *frame, xlator_t *this, const char *linkpath,
 		   ((loc->inode)? loc->inode->ino : 0));
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(symlink, frame, -1, ENOSPC,
+					NULL, NULL, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_symlink_cbk,
@@ -1680,7 +1703,9 @@ uidmap_rename(call_frame_t *frame, xlator_t *this, loc_t *oldloc, loc_t *newloc)
 		   newloc->path, newloc->ino);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(rename, frame, -1, ENOSPC,
+					NULL, NULL, NULL, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_rename_cbk,
@@ -1702,7 +1727,9 @@ uidmap_link(call_frame_t *frame, xlator_t *this, loc_t *oldloc, loc_t *newloc)
 		   newloc->path, newloc->inode->ino);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(link, frame, -1, ENOSPC,
+				NULL, NULL, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_link_cbk,
@@ -1727,7 +1754,9 @@ uidmap_setattr(call_frame_t *frame, xlator_t *this, loc_t *loc,
 		scratch_cs.gid = stbuf->ia_gid;
 		if (((*uidmap_map)(frame->root, this->name) == -1) ||
 		    ((*uidmap_map)(&scratch_cs, this->name) == -1)) {
-			return -1;
+			STACK_UNWIND_STRICT(setattr, frame, -1, ENOSPC,
+						NULL, NULL);
+			return 0;
 		}
 		stbuf->ia_uid = scratch_cs.uid;
 		stbuf->ia_gid = scratch_cs.gid;
@@ -1756,7 +1785,9 @@ uidmap_fsetattr(call_frame_t *frame, xlator_t *this, fd_t *fd,
 		scratch_cs.gid = stbuf->ia_gid;
 		if (((*uidmap_map)(frame->root, this->name) == -1) ||
 		    ((*uidmap_map)(&scratch_cs, this->name) == -1)) {
-			return -1;
+			STACK_UNWIND_STRICT(fsetattr, frame, -1, ENOSPC,
+						NULL, NULL);
+			return 0;
 		}
 		stbuf->ia_uid = scratch_cs.uid;
 		stbuf->ia_gid = scratch_cs.gid;
@@ -1780,7 +1811,8 @@ uidmap_truncate(call_frame_t *frame, xlator_t *this, loc_t *loc,
 		   frame->root->unique, loc->path, loc->inode->ino, offset);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(truncate, frame, -1, ENOSPC, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_truncate_cbk,
@@ -1803,7 +1835,8 @@ uidmap_open(call_frame_t *frame, xlator_t *this, loc_t *loc,
 		   fd, wbflags);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(open, frame, -1, ENOSPC, 0);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_open_cbk,
@@ -1823,7 +1856,9 @@ uidmap_create(call_frame_t *frame, xlator_t *this, loc_t *loc,
 		   frame->root->unique, loc->path, loc->inode->ino, flags, mode);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(create, frame, -1, ENOSPC,
+					0, NULL, NULL, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_create_cbk,
@@ -1843,7 +1878,9 @@ uidmap_readv(call_frame_t *frame, xlator_t *this, fd_t *fd,
 		   frame->root->unique, fd, size, offset);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(readv, frame, -1, ENOSPC,
+					NULL, 0, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_readv_cbk,
@@ -1864,7 +1901,8 @@ uidmap_writev(call_frame_t *frame, xlator_t *this, fd_t *fd,
 		   frame->root->unique, fd, vector, count, offset);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(writev, frame, -1, ENOSPC, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_writev_cbk,
@@ -1884,7 +1922,8 @@ uidmap_statfs(call_frame_t *frame, xlator_t *this, loc_t *loc)
 		   ((loc->inode)? loc->inode->ino : 0));
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(statfs, frame, -1, ENOSPC, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_statfs_cbk,
@@ -1902,7 +1941,8 @@ uidmap_flush(call_frame_t *frame, xlator_t *this, fd_t *fd)
 		   "%"PRId64": (*fd=%p)", frame->root->unique, fd);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(flush, frame, -1, ENOSPC);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_flush_cbk,
@@ -1920,7 +1960,8 @@ uidmap_fsync(call_frame_t *frame, xlator_t *this, fd_t *fd, int32_t flags)
 		   "%"PRId64": (flags=%d, *fd=%p)", frame->root->unique, flags, fd);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(fsync, frame, -1, ENOSPC, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_fsync_cbk,
@@ -1941,7 +1982,8 @@ uidmap_setxattr(call_frame_t *frame, xlator_t *this,
 		   ((loc->inode)? loc->inode->ino : 0), dict, flags);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(setxattr, frame, -1, ENOSPC);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_setxattr_cbk,
@@ -1962,7 +2004,8 @@ uidmap_getxattr(call_frame_t *frame, xlator_t *this,
 		   ((loc->inode)? loc->inode->ino : 0), name);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(getxattr, frame, -1, ENOSPC, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_getxattr_cbk,
@@ -1983,7 +2026,8 @@ uidmap_fsetxattr(call_frame_t *frame, xlator_t *this,
 		   ((fd->inode)? fd->inode->ino : 0), dict, flags);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(fsetxattr, frame, -1, ENOSPC);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_fsetxattr_cbk,
@@ -2004,7 +2048,8 @@ uidmap_fgetxattr(call_frame_t *frame, xlator_t *this,
 		   ((fd->inode)? fd->inode->ino : 0), name);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(fgetxattr, frame, -1, ENOSPC, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_fgetxattr_cbk,
@@ -2025,7 +2070,8 @@ uidmap_removexattr(call_frame_t *frame, xlator_t *this,
 		   ((loc->inode)? loc->inode->ino : 0), name);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(removexattr, frame, -1, ENOSPC);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_removexattr_cbk,
@@ -2045,7 +2091,8 @@ uidmap_opendir(call_frame_t *frame, xlator_t *this, loc_t *loc, fd_t *fd)
 		   frame->root->unique, loc->path, loc->inode->ino, fd);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(opendir, frame, -1, ENOSPC, 0);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_opendir_cbk,
@@ -2064,7 +2111,8 @@ uidmap_readdirp(call_frame_t *frame, xlator_t *this, fd_t *fd, size_t size,
 		   frame->root->unique, fd, size, offset);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(readdirp, frame, -1, ENOSPC, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_readdirp_cbk,
@@ -2085,7 +2133,8 @@ uidmap_readdir(call_frame_t *frame, xlator_t *this, fd_t *fd,
 		   frame->root->unique, fd, size, offset);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(readdir, frame, -1, ENOSPC, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_readdir_cbk,
@@ -2106,7 +2155,8 @@ uidmap_fsyncdir(call_frame_t *frame, xlator_t *this,
 		   frame->root->unique, datasync, fd);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(fsyncdir, frame, -1, ENOSPC);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_fsyncdir_cbk,
@@ -2126,7 +2176,8 @@ uidmap_access(call_frame_t *frame, xlator_t *this, loc_t *loc, int32_t mask)
 		   ((loc->inode)? loc->inode->ino : 0), mask);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(access, frame, -1, ENOSPC);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_access_cbk,
@@ -2146,7 +2197,8 @@ uidmap_ftruncate(call_frame_t *frame, xlator_t *this,
 		   frame->root->unique, offset, fd);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(ftruncate, frame, -1, ENOSPC, NULL, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_ftruncate_cbk,
@@ -2165,7 +2217,8 @@ uidmap_fstat(call_frame_t *frame, xlator_t *this, fd_t *fd)
 		   "%"PRId64": (*fd=%p)", frame->root->unique, fd);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(fstat, frame, -1, ENOSPC, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_fstat_cbk,
@@ -2187,7 +2240,8 @@ uidmap_lk(call_frame_t *frame, xlator_t *this, fd_t *fd,
 		   lock->l_start, lock->l_len, lock->l_pid);
 
 	if ((*uidmap_map)(frame->root, this->name) == -1) {
-		return -1;
+		STACK_UNWIND_STRICT(lk, frame, -1, ENOSPC, NULL);
+		return 0;
 	}
 
 	STACK_WIND(frame, uidmap_lk_cbk,
Comment 1 Kaleb KEITHLEY 2011-11-10 14:51:30 UTC 
Seems to me that EPERM (operation not permitted) or EINVAL (invalid argument) would be better?

EREMOTEIO (remote I/O error), or even just EIO?
Comment 2 Pete Zaitcev 2011-11-10 16:14:55 UTC 
EIO sounds sensible. I didn't want EINVAL because that implies an error
by the client, whereas it's just an internal server issue.
Comment 3 Fedora Update System 2011-11-14 16:38:10 UTC 
hekafs-0.7-18.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/hekafs-0.7-18.fc16
Comment 4 Fedora Update System 2011-11-14 22:24:33 UTC 
Package hekafs-0.7-18.fc16:
* should fix your issue,
* was pushed to the Fedora 16 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing hekafs-0.7-18.fc16'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2011-15882
then log in and leave karma (feedback).
Comment 5 Fedora Update System 2011-11-23 00:59:24 UTC 
hekafs-0.7-18.fc16 has been pushed to the Fedora 16 stable repository.  If problems still persist, please make note of it in this bug report.