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 671156EC40; Sun, 27 Jun 2021 22:42:23 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 671156EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1624822943; bh=/2kYrSMUWc8kS4ko39xcXBMxhtRj+k7I0Lk4t1lcLmA=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=TnfBO9F+Km3xE5GdHYdbB0+AWHV834Cp/6/nfHsPdhFNrV3YNRrX4uIP610XWidJV xBiOpXtPHsujuTMvYDRy1fggkcqYQlsrqY1N+KawWJZi/Nv6u+AeoGrZ4YC6ekT6ib G44Z+dI7GY3IbJaq5Mk0MDS/otNldlpHnTxomCHM= Received: from mail-lf1-f52.google.com (mail-lf1-f52.google.com [209.85.167.52]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 76E996EC40 for ; Sun, 27 Jun 2021 22:42:20 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 76E996EC40 Received: by mail-lf1-f52.google.com with SMTP id d16so27723060lfn.3 for ; Sun, 27 Jun 2021 12:42:20 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=AK4smsCkctRghaLvNpv/78xm1sNe7HQeU1OFC4q8TJw=; b=K6HvDk1X2CVrmWYKcYmOZk/U3EGE4OS3oobkkI26tOsdvIcTgtDTmbOX0c30uKVrcy jcfJvk0QrSeB8617hX5cIKosvnceTW+amUvC/zbodu268UmB4NbC/lWvfEMUMhfM3r+F Gcy7YymhHhW1McW2uKefjJ8P7nVcIEZ1GdyXg+Q1q6yJf+VGJRiuxz/VHfQHzWSbgynF NoPn6ksu5Tq4nwu7p2HDzQm/UwWoIqgdJNWV8Z5JKCquOU0JTNPDBLuKevEWDFiKqBe5 Zu2vERZZ4UagUcMJOF5T1zE3Ec4jD9JbAWKkWhd5FJGDsRdWS0u0q4m8lHuZia5xl2uJ 7gWg== X-Gm-Message-State: AOAM532xGOhPveXxaIyArnkstjEH2ZiBrQkLvHJEaaY72tvZbxvdh58K B7Om277fJKbEPLg6HWuji97g4bcqZj0= X-Google-Smtp-Source: ABdhPJzViF0PFoxTmjqxNacqOnPLD8LaNC7ViGpontW5Tk+H5K7oFhvKHyWZDORswMpEtTciy30AHw== X-Received: by 2002:ac2:4884:: with SMTP id x4mr15690352lfc.108.1624822939352; Sun, 27 Jun 2021 12:42:19 -0700 (PDT) Received: from grain.localdomain ([5.18.199.94]) by smtp.gmail.com with ESMTPSA id d8sm1113820lfi.129.2021.06.27.12.42.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 27 Jun 2021 12:42:18 -0700 (PDT) Received: by grain.localdomain (Postfix, from userid 1000) id 7A32E5A001F; Sun, 27 Jun 2021 22:42:17 +0300 (MSK) Date: Sun, 27 Jun 2021 22:42:17 +0300 To: Vladislav Shpilevoy Cc: tml Message-ID: References: <20210625100707.87807-1-gorcunov@gmail.com> <9652278e-570e-40e5-b2d1-856fe58179fc@tarantool.org> <4b0f4d8f-4e0c-00d0-38ad-0b6abad3aafe@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4b0f4d8f-4e0c-00d0-38ad-0b6abad3aafe@tarantool.org> User-Agent: Mutt/2.0.7 (2021-05-04) Subject: Re: [Tarantool-patches] [PATCH] raft: more precise verification of incoming request state 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: Cyrill Gorcunov via Tarantool-patches Reply-To: Cyrill Gorcunov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" On Sun, Jun 27, 2021 at 04:12:01PM +0200, Vladislav Shpilevoy wrote: ... > > > > thus if compiler use signed int type as internal representation then > > (int)req->state >= (int)raft_state_MAX won't take the branch because > > it is a negative value. > > The very fact you needed to put so much effort into the explanation > of a simple negative number check means it is a huge overkill. Please, > compare with < 0 explicitly. If the compiler will store the enum as uint, > it will drop your condition as unreachable anyway. I put such detailed description because the proper easier fix is to convert the value to unsigned int _explicitly_ (due to implicit type conversion compiler does and it is implementation defined, you simply don't know what exactly compiler does under the hood) and compare it with max, but I've obviously failed to convince you. Never mind. > >> > >> Please, perform a normal < 0 comparison. No need for unsigned cast > >> cast. > > > > We need an explicit cast here because as I said the internal > > implementation is compiler defined. > > It has nothing to do with the compiler-specific implementation. The whole issue is about compiler internals. > It is simply over-engineering. You cast value to unsigned to > check if it is negative. WTF? Lets do a normal comparison. I don't get your "wtf" here, what is wrong with using two complement properties? > > On the other hands I played with a number of gcc and llvm compilers > > and for this code they all does correct comparision (as unsigned > > integers) so maybe we could just leave it as is, simply close > > the bug as won't implement and spend time on more important things? > > I don't mind actually, so up to you just say me what you think. > > I don't mind fixing the issue. But now your patch does not fix it, > unfortunately. If you want to fix it - go on. But it must be truly > fixed then. I rather switch to another bug. The patch fixes exactly what it should, nothing more, nothing less. But this is your code in first place thus if you don't like the way I patched it, I suppose it is quite fine and there is nothing to argue about because the code author usually selects what better fits his architecture. And taking into account that modern compilers doesn't cause problems here we can safely live with it.