From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 8230A45C304 for ; Fri, 11 Dec 2020 18:06:00 +0300 (MSK) References: <09684330fa61f01381f05bbbc0e49e567a8a14a7.1607456485.git.lvasiliev@tarantool.org> <95BFAA11-3CB5-430E-938A-B3AF9701A38E@tarantool.org> From: Leonid Vasiliev Message-ID: <3bf1c67c-6753-2c4d-e362-33b7dfadaec7@tarantool.org> Date: Fri, 11 Dec 2020 18:05:01 +0300 MIME-Version: 1.0 In-Reply-To: <95BFAA11-3CB5-430E-938A-B3AF9701A38E@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH 1/2] sql: add missing diag_set on failure when working with files inside SQL module List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Ostanevich Cc: m.semkin@corp.mail.ru, tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy , Mergen Imeev 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 wrote: >> >> From: Mergen Imeev >> >> 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 >> >