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 0FB036EC5D; Tue, 6 Apr 2021 11:39:00 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0FB036EC5D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1617698340; bh=/thEVkvgPfPIn/29cLt+fBMMMLIwSBmNckaDOOHbMLI=; 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=S/fxs5JetiPKd9MBj6n3VR0WlZFUYS02u8b91rfBxhfinRwaEaulneqXXuuir74o3 kydFArfb9x3YXdphjQgnDBVe+WAaKx7y0Lg23A6k7iGjqB6CRZKHjR4VbZS3the+NA 01B94on8UmuKdGk/2GipHU7GNNjKWRz950HsDEZc= Received: from mail-lf1-f43.google.com (mail-lf1-f43.google.com [209.85.167.43]) (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 990F46EC5D for ; Tue, 6 Apr 2021 11:38:59 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 990F46EC5D Received: by mail-lf1-f43.google.com with SMTP id d13so21438846lfg.7 for ; Tue, 06 Apr 2021 01:38:59 -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=oNYcikSs7I1pxuqssb1dUqC8a0yTGwtrN9m5SYjuvA8=; b=Rzzz1HAcyA8RA5KSlx00j8rEfw4OAnDE1GL8LWh1opjSDoPSf2z7+/zChZzHVN4gQk rI54KV972Yl/gnHVZcB+r9WswNKXKJKMrQh9AODQij12JeDbE79brvYho/H9TIIyNd/z qQm2KBjkBGa9sDxaPjMcp72pbNsSIZD+ufxNq8FjPAeuNo+tOifLGJ/yjwrwaHMngFWA iDi1mJZBAEoRKBAqDonSCrI9N/unjOatxlnyiO19t7RzWqzI0j9uVUMTrnO4EFthXcrU +cEoIJRupaj7LBfqaWTr+aCgD4DD/5HQW0J0Ja8POm8ZWSc9yHiZO1FnUHgJjXwc2PCS YAag== X-Gm-Message-State: AOAM533HROFeOX7zxu7Zs7fLzAhN/+nD9630M/LQKcMWLrohdef41Kv+ Ta+c/6ygH5hUz/0g4vGhLxqHMI+F8sE= X-Google-Smtp-Source: ABdhPJy1envV4CDawCSBLPVHjhyNbh75aNgP+GvfzeusliFYszY+COSWmgzuLLvoktQ8FWYLhOuoaw== X-Received: by 2002:a05:6512:969:: with SMTP id v9mr20386121lft.466.1617698338537; Tue, 06 Apr 2021 01:38:58 -0700 (PDT) Received: from grain.localdomain ([5.18.199.94]) by smtp.gmail.com with ESMTPSA id x21sm2073082lfe.193.2021.04.06.01.38.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 06 Apr 2021 01:38:57 -0700 (PDT) Received: by grain.localdomain (Postfix, from userid 1000) id 201FF5601C4; Tue, 6 Apr 2021 11:38:56 +0300 (MSK) Date: Tue, 6 Apr 2021 11:38:56 +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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5be31f35-b8f4-9800-6804-29957f7634bf@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 Mon, Apr 05, 2021 at 05:47:32PM +0200, Vladislav Shpilevoy wrote: > Thanks for the patch! > > See 6 comments below. > > On 02.04.2021 14:34, Cyrill Gorcunov via Tarantool-patches wrote: > > In commit 96938fafb an ability to hot reload of modules > > 1. Please, add the commit title in parentheses and quotes after the > hash value. OK > > > has been introduced. When module is been reloaded his > > functions are resolved to new symbols but if something > > went wrong it is supposed to restore old symbols from > > the old module. Actually current code restores only > > one function and may crash if there a bunch of functions > > to restore. Lets fix it. > > > > Part-of #4642 > > 2. How is it a part of 4642? It is totally unrelated. It is a > separate bug, existing before 4642, and which could exist after > 4642 without this patch, and which does not block 4642 anyhow. It *is* related. I patch this code later, when move to the new module API interface and I'm not going to continue supporting this bug in the new code. This was the reason why I had to patch the code first. And that's why it is part of 4642. I could make a separate issue for this and fix it if you prefer but definitely in this series. I don't wanna do a double work. > > > diff --git a/changelogs/unreleased/fix-module-reload.md b/changelogs/unreleased/fix-module-reload.md > > new file mode 100644 > > index 000000000..7e189617f > > --- /dev/null > > +++ b/changelogs/unreleased/fix-module-reload.md > > @@ -0,0 +1,4 @@ > > +## bugfix/core > > + > > +* Fix module reloading procedure which may crash in case if > > + new module is corrupted (gh-4642). > > 3. 'module' term is used not only for .so/.dylib files, but also > for Lua modules. You need to be more specific that this is about > compiled files. OK > > + * Some old functions are not found int the new module, > > 4. int -> in. Thanks! > > + * thus restore all migrated functions back to original. > > */ > > diff --git a/test/box/CMakeLists.txt b/test/box/CMakeLists.txt > > index 06bfbbe9d..944831af2 100644 > > --- a/test/box/CMakeLists.txt > > +++ b/test/box/CMakeLists.txt > > @@ -2,4 +2,6 @@ include_directories(${MSGPUCK_INCLUDE_DIRS}) > > build_module(function1 function1.c) > > build_module(reload1 reload1.c) > > build_module(reload2 reload2.c) > > +build_module(func_restore1 func_restore1.c) > > +build_module(func_restore2 func_restore2.c) > > build_module(tuple_bench tuple_bench.c) > > diff --git a/test/box/func_restore.result b/test/box/func_restore.result > > 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. > 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. > > +static int > > +echo_num(box_function_ctx_t *ctx, const char *args, > > + const char *args_end, unsigned int num) > > +{ > > + char res[16]; > > + char *end = mp_encode_uint(res, num); > > + box_return_mp(ctx, res, end); > > + return 0; > > +} > > + > > + > > 6. Between functions we use a single empty line. The same for the other .c > file. Thanks, will do.