From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (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 D48E545C304 for ; Mon, 14 Dec 2020 18:50:22 +0300 (MSK) References: <09684330fa61f01381f05bbbc0e49e567a8a14a7.1607696813.git.lvasiliev@tarantool.org> <94d760ec-8174-c4dd-f65d-2478c24f1a96@tarantool.org> From: Leonid Vasiliev Message-ID: Date: Mon, 14 Dec 2020 18:49:22 +0300 MIME-Version: 1.0 In-Reply-To: <94d760ec-8174-c4dd-f65d-2478c24f1a96@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 1/3] 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: Vladislav Shpilevoy , imeevma@tarantool.org, korablev@tarantool.org, sergos@tarantool.org, m.semkin@corp.mail.ru Cc: Mergen Imeev , tarantool-patches@dev.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 >> >> 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; }