From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57707) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XJNMY-0007JO-Fa for qemu-devel@nongnu.org; Mon, 18 Aug 2014 09:52:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XJNMS-0002Iq-Cq for qemu-devel@nongnu.org; Mon, 18 Aug 2014 09:52:42 -0400 Date: Mon, 18 Aug 2014 15:51:55 +0200 From: "Michael S. Tsirkin" Message-ID: <20140818135155.GB28795@redhat.com> References: <1408348473-10104-1-git-send-email-zhang.zhanghailiang@huawei.com> <1408353202.4964.2.camel@localhost.localdomain> <53F1E56C.9000709@msgid.tls.msk.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <53F1E56C.9000709@msgid.tls.msk.ru> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [Qemu-trivial] [PATCH v7] tests/bios-tables-test: check the value returned by fopen() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Tokarev Cc: zhanghailiang , Marcel Apfelbaum , qemu-trivial@nongnu.org, Li Liu , luonengjun@huawei.com, peter.huangpeng@huawei.com, qemu-devel@nongnu.org On Mon, Aug 18, 2014 at 03:37:16PM +0400, Michael Tokarev wrote: > 18.08.2014 13:13, Marcel Apfelbaum wrote: > > On Mon, 2014-08-18 at 15:54 +0800, zhanghailiang wrote: > >> The function fopen() may fail, so check its return value. > >> > >> Signed-off-by: zhanghailiang > >> Signed-off-by: Li Liu > >> Reviewed-by: Alex Benn=E9e > >> --- > >> tests/bios-tables-test.c | 5 +++++ > >> 1 file changed, 5 insertions(+) > >> > >> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c > >> index 045eb27..feb3b58 100644 > >> --- a/tests/bios-tables-test.c > >> +++ b/tests/bios-tables-test.c > >> @@ -790,6 +790,11 @@ int main(int argc, char *argv[]) > >> const char *arch =3D qtest_get_arch(); > >> FILE *f =3D fopen(disk, "w"); > >> int ret; > >> + > >> + if (!f) { > >> + fprintf(stderr, "Couldn't open \"%s\": %s", disk, strerror(= errno)); > >> + return -1; > >> + } > > Hi, > > I think we should use an assert here, we need an indication > > that the test failed and a print to stderr is not enough. >=20 > Guys, please stop misusing (or trying to misuse) assert(). assert() is= for > code path which are impossible by program logic. Here, it is a error c= heck, > not a code logic check -- the fopen() _may_ fail, and this is not somet= hing > the code around makes impossible. So in cases like this (and in other = similar > case like vvfat.log open check), we should either print to stderr and e= xit, > or print elsewhere, but we should NOT use assert(). Think what will be= if > the program will be compiled with -D_NDEBUG and all assert()s are turne= d into > a no-op. We don't support that configuration, a bunch of stuff will be broken if you try. > So, no assert() in cases like this here and elsewhere (not only in qemu= ). It > is not what assert() is provided for. >=20 > Thanks, >=20 > /mjt I partially agree in that asserts produce errors which aren't easy for users to understand, and gives a coredump which takes up disk space. So if we expect some error to trigger, we should print a proper error, and avoid producing a coredump. On the other hand, there are situations where we take a shortcut and decide that error is highly unlikely (even if not 100% impossible), so not worth handling. In these cases assert will make debugging easier in case it does trigger after all. This case here is about a test, so it's used by developers, not users. Thus assert might be appropriate. --=20 MST