MDEV-19725 Incorrect error handling in ALTER TABLE

Some I/O functions and macros that are declared in os0file.h used to
return a Boolean status code (nonzero on success). In MySQL 5.7, they
were changed to return dberr_t instead. Alas, in MariaDB Server 10.2,
some uses of functions were not adjusted to the changed return value.

Until MDEV-19231, the valid values of dberr_t were always nonzero.
This means that some code that was incorrectly checking for a zero
return value from the functions would never detect a failure.

After MDEV-19231, some tests for ALTER ONLINE TABLE would fail with
cmake -DPLUGIN_PERFSCHEMA=NO. It turned out that the wrappers
pfs_os_file_read_no_error_handling_int_fd_func() and
pfs_os_file_write_int_fd_func() were wrongly returning
bool instead of dberr_t. Also the callers of these functions were
wrongly expecting bool (nonzero on success) instead of dberr_t.

This mistake had been made when the addition of these functions was
merged from MySQL 5.6.36 and 5.7.18 into MariaDB Server 10.2.7.

This fix also reverts commit 40becbc3c7a6555d0a4bb186b4336a2899d5995c
which attempted to work around the problem.

6 files changed, 32 insertions, 23 deletions

diff --git a/storage/innobase/include/os0file.h b/storage/innobase/include/os0file.h
index 324ff99f67b..e85bf74201a 100644
--- a/storage/innobase/include/os0file.h
+++ b/storage/innobase/include/os0file.h
@@ -2,7 +2,7 @@
 
 Copyright (c) 1995, 2017, Oracle and/or its affiliates. All Rights Reserved.
 Copyright (c) 2009, Percona Inc.
-Copyright (c) 2013, 2017, MariaDB Corporation.
+Copyright (c) 2013, 2019, MariaDB Corporation.
 
 Portions of this file contain modifications contributed and copyrighted
 by Percona Inc.. Those modifications are
@@ -1197,12 +1197,12 @@ to original un-instrumented file I/O APIs */
 # define os_file_read_no_error_handling(type, file, buf, offset, n, o)	\
 	os_file_read_no_error_handling_func(type, file, buf, offset, n, o)
 # define os_file_read_no_error_handling_int_fd(type, file, buf, offset, n) \
-        (os_file_read_no_error_handling_func(type, OS_FILE_FROM_FD(file), buf, offset, n, NULL) == 0)
+	os_file_read_no_error_handling_func(type, OS_FILE_FROM_FD(file), buf, offset, n, NULL)
 
 # define os_file_write(type, name, file, buf, offset, n)	\
 	os_file_write_func(type, name, file, buf, offset, n)
 # define os_file_write_int_fd(type, name, file, buf, offset, n)	\
-        (os_file_write_func(type, name, OS_FILE_FROM_FD(file), buf, offset, n) == 0)
+	os_file_write_func(type, name, OS_FILE_FROM_FD(file), buf, offset, n)
 
 # define os_file_flush(file)	os_file_flush_func(file)
 
diff --git a/storage/innobase/include/os0file.ic b/storage/innobase/include/os0file.ic
index 7a490bf775a..f363bd5135a 100644
--- a/storage/innobase/include/os0file.ic
+++ b/storage/innobase/include/os0file.ic
@@ -1,7 +1,7 @@
 /*****************************************************************************
 
 Copyright (c) 2010, 2017, Oracle and/or its affiliates. All Rights Reserved.
-Copyright (c) 2013, 2017, MariaDB Corporation.
+Copyright (c) 2013, 2019, MariaDB Corporation.
 
 This program is free software; you can redistribute it and/or modify it under
 the terms of the GNU General Public License as published by the Free Software
@@ -348,9 +348,10 @@ a synchronous read operation.
 @param[in]      n               number of bytes to read
 @param[in]      src_file        caller file name
 @param[in]      src_line        caller line number
-@return	   0 on error, 1 on success */
+@return error code
+@retval	DB_SUCCESS	if the operation succeeded */
 UNIV_INLINE
-bool
+dberr_t
 pfs_os_file_read_no_error_handling_int_fd_func(
 	const IORequest&	type,
 	int			file,
@@ -371,14 +372,14 @@ pfs_os_file_read_no_error_handling_int_fd_func(
 			__FILE__, __LINE__);
 	}
 
-	bool success = os_file_read_no_error_handling_func(
+	dberr_t err = os_file_read_no_error_handling_func(
 		type, OS_FILE_FROM_FD(file), buf, offset, n, NULL);
 
 	if (locker != NULL) {
 		PSI_FILE_CALL(end_file_wait)(locker, n);
 	}
 
-       return(success == DB_SUCCESS);  // Reverse result
+	return err;
 }
 
 /** NOTE! Please use the corresponding macro os_file_write(), not directly
@@ -435,9 +436,10 @@ os_file_write_int_fd() which requests a synchronous write operation.
 @param[in]	n		number of bytes
 @param[in]	src_file	file name where func invoked
 @param[in]	src_line	line where the func invoked
-@return	   0 on error, 1 on success */
+@return error code
+@retval	DB_SUCCESS	if the operation succeeded */
 UNIV_INLINE
-bool
+dberr_t
 pfs_os_file_write_int_fd_func(
 	const IORequest&	type,
 	const char*		name,
@@ -459,14 +461,14 @@ pfs_os_file_write_int_fd_func(
 			__FILE__, __LINE__);
 	}
 
-        bool success = os_file_write_func(
+	dberr_t err = os_file_write_func(
 		type, name, OS_FILE_FROM_FD(file), buf, offset, n);
 
         if (locker != NULL) {
                 PSI_FILE_CALL(end_file_wait)(locker, n);
         }
 
-        return(success == DB_SUCCESS); // Reverse result
+	return err;
 }
 
 /** NOTE! Please use the corresponding macro os_file_flush(), not directly
diff --git a/storage/innobase/include/row0merge.h b/storage/innobase/include/row0merge.h
index 3b66597c0d7..a1719e0448e 100644
--- a/storage/innobase/include/row0merge.h
+++ b/storage/innobase/include/row0merge.h
@@ -370,7 +370,9 @@ row_merge_buf_sort(
 
 /********************************************************************//**
 Write a merge block to the file system.
-@return whether the request was completed successfully */
+@return whether the request was completed successfully
+@retval	false	on error
+@retval	true	on success */
 UNIV_INTERN
 bool
 row_merge_write(
diff --git a/storage/innobase/os/os0file.cc b/storage/innobase/os/os0file.cc
index 8445f635062..2a89a6fc982 100644
--- a/storage/innobase/os/os0file.cc
+++ b/storage/innobase/os/os0file.cc
@@ -5038,7 +5038,8 @@ Requests a synchronous write operation.
 @param[out]	buf		buffer from which to write
 @param[in]	offset		file offset from the start where to read
 @param[in]	n		number of bytes to read, starting from offset
-@return DB_SUCCESS if request was successful, false if fail */
+@return error code
+@retval	DB_SUCCESS	if the operation succeeded */
 dberr_t
 os_file_write_func(
 	const IORequest&	type,
@@ -5497,7 +5498,8 @@ Requests a synchronous positioned read operation.
 @param[out]	buf		buffer where to read
 @param[in]	offset		file offset from the start where to read
 @param[in]	n		number of bytes to read, starting from offset
-@return DB_SUCCESS or error code */
+@return error code
+@retval	DB_SUCCESS	if the operation succeeded */
 dberr_t
 os_file_read_func(
 	const IORequest&	type,
diff --git a/storage/innobase/row/row0log.cc b/storage/innobase/row/row0log.cc
index 0013fabd98e..012cf8b2963 100644
--- a/storage/innobase/row/row0log.cc
+++ b/storage/innobase/row/row0log.cc
@@ -396,7 +396,7 @@ row_log_online_op(
 		}
 
 		log->tail.blocks++;
-		if (!os_file_write_int_fd(
+		if (DB_SUCCESS != os_file_write_int_fd(
 			    request,
 			    "(modification log)",
 			    log->fd,
@@ -534,7 +534,7 @@ row_log_table_close_func(
 		}
 
 		log->tail.blocks++;
-		if (!os_file_write_int_fd(
+		if (DB_SUCCESS != os_file_write_int_fd(
 			    request,
 			    "(modification log)",
 			    log->fd,
@@ -2658,7 +2658,7 @@ all_done:
 		IORequest		request(IORequest::READ);
 		byte*			buf = index->online_log->head.block;
 
-		if (!os_file_read_no_error_handling_int_fd(
+		if (DB_SUCCESS != os_file_read_no_error_handling_int_fd(
 			    request, index->online_log->fd,
 			    buf, ofs, srv_sort_buf_size)) {
 			ib::error()
@@ -3529,7 +3529,7 @@ all_done:
 
 		byte*	buf = index->online_log->head.block;
 
-		if (!os_file_read_no_error_handling_int_fd(
+		if (DB_SUCCESS != os_file_read_no_error_handling_int_fd(
 			    request, index->online_log->fd,
 			    buf, ofs, srv_sort_buf_size)) {
 			ib::error()
diff --git a/storage/innobase/row/row0merge.cc b/storage/innobase/row/row0merge.cc
index e0c819bea4b..3f8bcb0a430 100644
--- a/storage/innobase/row/row0merge.cc
+++ b/storage/innobase/row/row0merge.cc
@@ -1087,8 +1087,9 @@ row_merge_read(
 	DBUG_EXECUTE_IF("row_merge_read_failure", DBUG_RETURN(FALSE););
 
 	IORequest	request(IORequest::READ);
-	const bool	success = os_file_read_no_error_handling_int_fd(
-		request, fd, buf, ofs, srv_sort_buf_size);
+	const bool	success = DB_SUCCESS
+		== os_file_read_no_error_handling_int_fd(
+			request, fd, buf, ofs, srv_sort_buf_size);
 
 	/* If encryption is enabled decrypt buffer */
 	if (success && log_tmp_is_encrypted()) {
@@ -1115,7 +1116,9 @@ row_merge_read(
 
 /********************************************************************//**
 Write a merge block to the file system.
-@return 0 on error, 1 if write succeded */
+@return whether the request was completed successfully
+@retval	false	on error
+@retval	true	on success */
 UNIV_INTERN
 bool
 row_merge_write(
@@ -1149,7 +1152,7 @@ row_merge_write(
 	}
 
 	IORequest	request(IORequest::WRITE);
-	const bool	success = os_file_write_int_fd(
+	const bool	success = DB_SUCCESS == os_file_write_int_fd(
 		request, "(merge)", fd, out_buf, ofs, buf_len);
 
 #ifdef POSIX_FADV_DONTNEED