Re: [Tarantool-patches] [PATCH v2 1/3] 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 13.12.2020 21:30, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patchset!
> 
> On 11.12.2020 15:49, Leonid Vasiliev 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;
> 
> SystemError already reports strerror() in ::log() method.
> I don't think you need to duplicate it. Better add a proper
> description. For example, the path. Grep "diag_set(SystemError"
> in our project and see other usages which try to provide
> more info. The same in other places.

I thought about adding more info to the error, but it seems to me that
only "trace" is useful here. I do not mind your proposal. See full diff
at the end of the letter

> 
>>   		}
>>   		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;
>>   }
>>   
>>


  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..ea7d3cc 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, "failed to open file '%s'", z);
  			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, "failed to open '/dev/null'");
  			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, "failed to write %i bytes to file", nBuf);
  		*piErrno = errno;
+	}
  	return rc;
  }