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 699386EC5D; Tue, 6 Apr 2021 23:42:10 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 699386EC5D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1617741730; bh=RozVI1Qk0azKyNq9RorKvkwE9pSLkcO8UChVwdXjofw=; 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=nQqKxAn4KjBQ4ST7iTtLFm1+YkcXKYeBM1ejQJfcd48rWBV3VnJs3eTUlYi2SnA5V nU0UY38GxzIge0qO4s98cG1s4BgxqWP3yVhsGuurPP2TICUMNTCxK/8C9EvylupKNH 6AEBxRR9Mi9Wms3sPitdViJSV4KVFTPnE8aBh59w= Received: from mail-lj1-f174.google.com (mail-lj1-f174.google.com [209.85.208.174]) (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 E1CB86EC5D for ; Tue, 6 Apr 2021 23:42:08 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E1CB86EC5D Received: by mail-lj1-f174.google.com with SMTP id 184so18032377ljf.9 for ; Tue, 06 Apr 2021 13:42:08 -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=sog7Z0BknVpXxwB89YTGLNlLEMTztlO10etITIGEXko=; b=gfNivaJgipb5QfgjeIui/EGpBeGCsHhCjOFcPvvWf3AVgJmqV2Uofcs819mYo41mjr mnQpMRYxe6vuTXzEG7zN8hu1FLaxK54/rtAOI0kxqXoAdj10cHReVZxxtiH2BH+x4a09 yVAalzWxjBc4wGmSuxqqVgY3dG4P8CMGbVrRBt6GB7V3Z/ZrPSuT+XRidClUhkxOzuqp PwBjxT+jo4HBvkKWRME4X4F8fFNcD1ifog3XJWWSzE8E97rdxKulApOkTaaBUiu7EQxr AKswpeoNVCSXXDZDWbFcB61V+yH1sbfJt1Zxz/xzeazgHF5qbBahgulADyibpC4ZYpLw 3zMw== X-Gm-Message-State: AOAM5322JrG8x7ojhvIBkoPfY/kNAD3zzHFzZfKmMve1vJaeZPIjvxv4 cv7v+BOTVB8yaqcWkgxr4A/oazcfV5G+FA== X-Google-Smtp-Source: ABdhPJyfYx41ikXnzC8/5wwuj5wLiwsBqo8XizlLCiE8p3MwG3kL6lLj3UCPcnBf4InInKGyRYIa/A== X-Received: by 2002:a2e:b16b:: with SMTP id a11mr20217915ljm.132.1617741727727; Tue, 06 Apr 2021 13:42:07 -0700 (PDT) Received: from grain.localdomain ([5.18.199.94]) by smtp.gmail.com with ESMTPSA id e6sm2252307lfj.96.2021.04.06.13.42.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 06 Apr 2021 13:42:06 -0700 (PDT) Received: by grain.localdomain (Postfix, from userid 1000) id A2D395601DD; Tue, 6 Apr 2021 23:42:05 +0300 (MSK) Date: Tue, 6 Apr 2021 23:42:05 +0300 To: Vladislav Shpilevoy Cc: tml Message-ID: References: <20210402123420.885834-1-gorcunov@gmail.com> <20210402123420.885834-4-gorcunov@gmail.com> <5be31f35-b8f4-9800-6804-29957f7634bf@tarantool.org> <1391567c-0cb7-c8f5-d500-21c29cb18746@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1391567c-0cb7-c8f5-d500-21c29cb18746@tarantool.org> User-Agent: Mutt/2.0.5 (2021-01-21) Subject: Re: [Tarantool-patches] [PATCH v20 3/7] box/func: fix modules functions restore 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 Tue, Apr 06, 2021 at 10:02:40PM +0200, Vladislav Shpilevoy wrote: > >> 5. The test also passes if I just replace rlist_foreach_entry_safe with > >> rlist_foreach_entry_safe_reverse in the original code. Which means it > >> won't test anything in case we ever change how do we put the functions > >> to the list, or how we walk the list on reload. > > > > The key moment here is not about the list traverse direction: that's > > why I use for_each macro instead of the broken cycle we had before. > > The order does matter to trigger the issue *without* this patch, > > ie to put functions in reverse order and force the recovery > > (as is done in my test, I even put a comment there). > > > > With the new approach using list_for() it is doesn't matter now > > how exactly we traverse. If that is what you mean. > > Nope, that is not what I mean. But I don't know how to say it > easier, even in Russian. OK, lets go another way. Say we have 3 functions: {1,2,3}. When we call them in order 1, then 2, then 3 the list of functions gonna be {3, 2, 1} because they are prepended. Then we start reloading procedure (we're talking aboout original code). And we have old = {3, 2, 1}, new = { } and we processed 3 and 2 witout an error, so we have old = { 1 }, new = {3, 2}. When we tried to resolve function 1 lets assume it has failed, we jump into restore procedure and restore a sole function 1, which lead to situation where old = { 1 } and function 3 and 2 are simply dissapeared from old module. This is because of _when_ we have failed. If we fail at first element, ie when resolving function {3}, then restore will pass fine and we will have old = {3, 2, 1} without problem. That said current restore procedure can handle only one failing entry andif there were other entries before failing one they will be simply vanished. That's what I've been trying to say. After the patch is doesn't matter in which order we resolve symbols because we resolve every element migrated from old list. Since current code restores only one entry we have a 3 cases: 1) entry failing at head of the list, 2) entry failing at the middle of the list and 3) entry failing at the last element. (1) is restored properly even in current code while (2) and (3) causes a crash and (2) is rather a subset of (3). So I tested (3) only. > >> I propose you to make the test harder to bypass. > > > > The original code restores only one last failed function and > > my test is done the way to trigger the issue. I would like to > > make it more harder to bypass but I don't see an other way. > > Well, did you try? The test would fail regardless of the order, if > you would try to drop the middle function. Or both the first and > the last functions. Or try all the combinations. Nope. See above. Case (1) handled without problem. But no problem I'll extend the test.