From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp17.mail.ru (smtp17.mail.ru [94.100.176.154]) (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 A111C45C304 for ; Tue, 15 Dec 2020 23:29:59 +0300 (MSK) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\)) From: Sergey Ostanevich In-Reply-To: Date: Tue, 15 Dec 2020 23:29:45 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <09684330fa61f01381f05bbbc0e49e567a8a14a7.1607696813.git.lvasiliev@tarantool.org> <94d760ec-8174-c4dd-f65d-2478c24f1a96@tarantool.org> 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: Leonid Vasiliev Cc: m.semkin@corp.mail.ru, Vladislav Shpilevoy , Mergen Imeev , tarantool-patches@dev.tarantool.org Thanks for the patch, LGTM. Sergos > On 14 Dec 2020, at 18:49, Leonid Vasiliev = wrote: >=20 > Hi! Thank you for the review. >=20 > 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 >>>=20 >>> 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. >>>=20 >>> Part of #5537 >>> --- >>> src/box/sql/os_unix.c | 9 +++++++-- >>> 1 file changed, 7 insertions(+), 2 deletions(-) >>>=20 >>> 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 =3D=3D 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. >=20 > 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 >=20 >>> } >>> if (fd >=3D SQL_MINIMUM_FILE_DESCRIPTOR) >>> break; >>> close(fd); >>> fd =3D -1; >>> - if (open("/dev/null", f, m) < 0) >>> + if (open("/dev/null", f, m) < 0) { >>> + diag_set(SystemError, strerror(errno)); >>> break; >>> + } >>> } >>> if (fd >=3D 0) { >>> if (m !=3D 0) { >>> @@ -831,8 +834,10 @@ seekAndWriteFd(int fd, /* File = descriptor to write to */ >>> rc =3D write(fd, pBuf, nBuf); >>> } while (rc < 0 && errno =3D=3D EINTR); >>> - if (rc < 0) >>> + if (rc < 0) { >>> + diag_set(SystemError, strerror(errno)); >>> *piErrno =3D errno; >>> + } >>> return rc; >>> } >>> =20 >=20 >=20 > src/box/sql/os_unix.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) >=20 > 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 =3D=3D EINTR) > continue; > + diag_set(SystemError, "failed to open file = '%s'", z); > break; > } > if (fd >=3D SQL_MINIMUM_FILE_DESCRIPTOR) > break; > close(fd); > fd =3D -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 >=3D 0) { > if (m !=3D 0) { > @@ -831,8 +834,10 @@ seekAndWriteFd(int fd, /* File = descriptor to write to */ > rc =3D write(fd, pBuf, nBuf); > } while (rc < 0 && errno =3D=3D EINTR); >=20 > - if (rc < 0) > + if (rc < 0) { > + diag_set(SystemError, "failed to write %i bytes to = file", nBuf); > *piErrno =3D errno; > + } > return rc; > }