From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id A230370202; Wed, 24 Feb 2021 01:47:48 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A230370202 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1614120468; bh=n2AJybn3HbcrKs/plAnB116DUoRMi7/cIJSGsZL6+4M=; h=To:References:Date:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=V1aKuwA9GsGFXswq9fKiJHAn783uiajgz90DiQQ0GdUGIPzVgqwHYNtGAA50ssN32 tbknswZ4AiitSUL83hC6Yc2Noqoa+SZ1Zk63BvbZvH37ch7rnITNdHObpw78LyCJlY C7ITnY3QQ0+kIudnMKhkUfv3DVA7dpM+OR2vE9Ao= Received: from smtp29.i.mail.ru (smtp29.i.mail.ru [94.100.177.89]) (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 835E870202 for ; Wed, 24 Feb 2021 01:47:47 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 835E870202 Received: by smtp29.i.mail.ru with esmtpa (envelope-from ) id 1lEgT3-0004pj-Vx; Wed, 24 Feb 2021 01:47:46 +0300 To: Cyrill Gorcunov References: <20210222182030.76232-1-gorcunov@gmail.com> <41811c65-448c-0de6-68cd-6a64779ca283@tarantool.org> Message-ID: <0482394d-1600-45ca-8b0e-d230d4dfa8fe@tarantool.org> Date: Tue, 23 Feb 2021 23:47:44 +0100 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD975C3EC174F56692243410BA6471F01668A37AB103014B813182A05F538085040240669797206708631ED90A777CD1F2BF89CB204AE6BC24851B280CCFEC27D99 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE75C94F75ECF5F42A4EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006370D24454B2F95E3848638F802B75D45FF5571747095F342E8C7A0BC55FA0FE5FC61D9FADA98E6539A58B4B44D338EA65CFB321789D4D541AD389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C07E7E81EEA8A9722B8941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B64854413538E1713FCC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB8D32BA5DBAC0009BE9E8FC8737B5C224971D55A418CD1ED3F76E601842F6C81A12EF20D2F80756B5F7E9C4E3C761E06A776E601842F6C81A127C277FBC8AE2E8B6BA66BB79834B351D81D268191BDAD3D698AB9A7B718F8C442539A7722CA490C13377AFFFEAFD26923F8577A6DFFEA7CB0EC3B1FCAE4A06993EC92FD9297F6715571747095F342E857739F23D657EF2BD5E8D9A59859A8B603913CCF128142C9089D37D7C0E48F6C5571747095F342E857739F23D657EF2B6825BDBE14D8E7025EC15B47EAE72BACBD9CCCA9EDD067B1EDA766A37F9254B7 X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A24A6D60772A99906F8E1CD14B953EB46D1DBBA17D09CB3AE3355D89D7DBCDD132 X-C1DE0DAB: 0D63561A33F958A550D14CD615F68DD5139607AAE8BACAD35D54E42EED55596AD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7557E988E9157162368E8E86DC7131B365E7726E8460B7C23C X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34A9A0A0BF1A2CAC62C103988B91AD5606BEFACF714E0FC8EEF09F3D0037E8E10ECAAD106494CB350D1D7E09C32AA3244CBABD5F188762B1573A4C49548FC1603FF26BFA4C8A6946B8FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojyK6JYJ15DtJsgVaA4NnIcw== X-Mailru-Sender: 504CC1E875BF3E7D9BC0E5172ADA3110D1DABD9759430778C35EF3B4E5012043F66F58CE4E95587307784C02288277CA03E0582D3806FB6A5317862B1921BA260ED6CFD6382C13A6112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH] say: fix format for fiber()->fid X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Cc: tml Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" >>> diff --git a/src/lib/core/say.c b/src/lib/core/say.c >>> index cbd10e107..6138752a7 100644 >>> --- a/src/lib/core/say.c >>> +++ b/src/lib/core/say.c >>> @@ -792,7 +792,7 @@ say_format_plain_tail(char *buf, int len, int level, const char *filename, >>> if (cord) { >>> SNPRINT(total, snprintf, buf, len, " %s", cord->name); >>> if (fiber() && fiber()->fid != FIBER_ID_SCHED) { >>> - SNPRINT(total, snprintf, buf, len, "/%i/%s", >>> + SNPRINT(total, snprintf, buf, len, "/%u/%s", >>> fiber()->fid, fiber_name(fiber())); >> >> I remember we had some issues about %u not being compatible with uint32_t so >> we did manual casts to 'unsigned'. Not sure though what was the commit/ticket, >> or if it really happened. > > I suspect you rather mean uint64_t, which indeed better convert to > excplicit "long long". No, I mean exactly uint32_t. You don't have a guarantee that it is the same as 'unsigned' AFAIK. > Strictly speaking if we really need to cover > compatibility we would _had_ to use PRIx macros which are so ugly :/ This is why I want us to use casts to 'unsigned' when we use %u. They are less ugly. > So no, uint32_t is known to be safe with %u format. Is it written in the standard somewhere? I honestly don't know. > Side note: I found that we still use %u for some of uint64_t, > in particuar > > if (req->term < raft->volatile_term) { > say_info("RAFT: the message is ignored due to outdated term - " > "current term is %u", raft->volatile_term); > return 0; > } > > probably should use %llu and explicit "long long" for long standing > servers where raft term would overflow 32 bit value. Yes, looks like a bug. But a more appropriate fix would be to investigate why don't we have compile time checks for this. The bug in raft's say_info() is clearly an error and should have been alarmed at compilation. At least clang supports an attribute which you can set for a function and get printf-like checks and warnings in case of format issues. We should adopt this: https://clang.llvm.org/docs/AttributeReference.html#id278 Somewhy I thought we already use it, but now I don't see any usages. It is easy to add, and helps to catch some weird stuff. Lets do it in scope of this patch series to fix the issue entirely. Not just in one exploded place. >> I am fine with not using the cast as long as all platforms and compilers >> build flawlessly. Also it is ok for me if you will add an explicit cast in >> the next version of the patch. Up to you. The same below. > > No, Vlad, I think we don't need explicit casts for 32bit values. Do you have a proof for this? That it is safe. Where in the standard do they say that '%u' is exactly 4 bytes? > I pushed > an update to the branch gorcunov/gh-5846-fid-name and here is a new > changelog. Does it look better? It seems you didn't really read my comment, did you? I said: "Please, add a changelog file" I didn't ask for more info in the commit message. This particular bug in this particular line was obvious. But you didn't add a changelog **file**. It should be added here: https://github.com/tarantool/tarantool/tree/master/changelogs/unreleased After more consideration I also realized we should not use uint32_t for fiber ids. It is monotonically growing. Which means it will overflow in the foreseeable future on a long-running instance. For example, assume one fiber is started and recycled each millisecond (which is quite a small rate). Then uint32 will overflow in less than 50 days. Considering we have instances running for years (at least from what I heard from Mons), this is clearly a bug. On the summary, it seems we need to - Add compile-time format checks using clang __format__ attribute; - Fix all the wrong formats; - Use 64bit ids for fibers. Unless I am wrong somewhere in my thoughts above.