Re: [Tarantool-patches] [PATCH 1/2] sql: add missing diag_set on failure when working with files inside SQL module

[Leonid Vasiliev <lvasiliev@tarantool.org>]

Hi! Thank you for the review.

On 10.12.2020 17:15, Sergey Ostanevich wrote:
> Thanks for the patch!
> 
> I have a question if those diags are too low in the call stack?

In the new patchset, I added common `diag_set()` if `sql_execute()`
fails without setting an error to diag.

> Apparently, the unixOpen() is a single space the robust_open()
> is called, so we’d have only one diag set instead of two?

In this case, we will not know exactly where the error occurred.
I think the best way to use the stack diag for such purposes but that is
out of the scope of this problem. I will create a separate task about
these improvements (or find existing ones).

> 
> Following this path - we should just cover UNIXVFS and sql_io_methods
> members with diag set, given the errno is preserved.
> 
> By now the solution is partial, so can be applied only if we’re in a
> rush.
This solution is minimalistic and solves a specific problem (and only
this problem). Because now we have a crash and fixes should be included
in the next release.

> 
> Sergos
> 
>> On 8 Dec 2020, at 22:59, Leonid Vasiliev <lvasiliev@tarantool.org> wrote:
>>
>> From: Mergen Imeev <imeevma@gmail.com>
>>
>> SQL module didn't set an error in the diagnostics area on a file
>> operation failure. This could lead to a crash like in #5537.
>>
>> Part of #5537
>> ---
>> src/box/sql/os_unix.c | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/box/sql/os_unix.c b/src/box/sql/os_unix.c
>> index b351c55..557d709 100644
>> --- a/src/box/sql/os_unix.c
>> +++ b/src/box/sql/os_unix.c
>> @@ -159,14 +159,17 @@ robust_open(const char *z, int f, mode_t m)
>> 		if (fd < 0) {
>> 			if (errno == EINTR)
>> 				continue;
>> +			diag_set(SystemError, strerror(errno));
>> 			break;
>> 		}
>> 		if (fd >= SQL_MINIMUM_FILE_DESCRIPTOR)
>> 			break;
>> 		close(fd);
>> 		fd = -1;
>> -		if (open("/dev/null", f, m) < 0)
>> +		if (open("/dev/null", f, m) < 0) {
>> +			diag_set(SystemError, strerror(errno));
>> 			break;
>> +		}
>> 	}
>> 	if (fd >= 0) {
>> 		if (m != 0) {
>> @@ -831,8 +834,10 @@ seekAndWriteFd(int fd,		/* File descriptor to write to */
>> 		rc = write(fd, pBuf, nBuf);
>> 	} while (rc < 0 && errno == EINTR);
>>
>> -	if (rc < 0)
>> +	if (rc < 0) {
>> +		diag_set(SystemError, strerror(errno));
>> 		*piErrno = errno;
>> +	}
>> 	return rc;
>> }
>>
>> -- 
>> 2.7.4
>>
>