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 164B78205E; Sun, 24 Jan 2021 19:30:13 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 164B78205E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1611505813; bh=PhIl+Ijkmy88fKoDrD8itVotiX3iTGX+SdFEqNKbKDY=; h=To:References:Date:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=MH/rWLSbtBf6dIUHy1NZddZdZoedaEGpWwAfQhhdhHJie5x9FbW8A3tM/jEe/XMhj SHdc4PoZ2uGEtAkeD7qyFuIjb1TpEDJlSP7WcKqpvUQ49mbhD4XW1RNU2/8z9rOWWC urkoPVzgAyLtUN6Fbqt79WWWyW73G//kHKdlkRGU= Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 4844957E4C for ; Sun, 24 Jan 2021 19:28:31 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4844957E4C Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1l3iFa-0003PX-Mb; Sun, 24 Jan 2021 19:28:31 +0300 To: Cyrill Gorcunov , tml References: <20210118203556.281700-1-gorcunov@gmail.com> <20210118203556.281700-9-gorcunov@gmail.com> Message-ID: <70209f90-7206-eb21-b253-bc4bed776c82@tarantool.org> Date: Sun, 24 Jan 2021 17:28:29 +0100 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.6.1 MIME-Version: 1.0 In-Reply-To: <20210118203556.281700-9-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9F0E84CC1954AA438D626E1E771FA73598B878D9CF0F8340A00894C459B0CD1B961843315A8E9C9F334EADECD3E8E7DA4E677AB1C9F20D3C8D42CF41164E76205 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7CEF52BA550D6CAADEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637DC5FF0CF1FFF13268638F802B75D45FF5571747095F342E8C7A0BC55FA0FE5FC520051DBA2F12A108ED9C9230678EB618E54DF685DA04371389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C07E7E81EEA8A9722B8941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B68424CA1AAF98A6958941B15DA834481F9449624AB7ADAF3735872C767BF85DA29E625A9149C048EE0A3850AC1BE2E7351D6A3D1828C12CDE4AD6D5ED66289B524E70A05D1297E1BB35872C767BF85DA227C277FBC8AE2E8B37C49EB0B884C57175ECD9A6C639B01B4E70A05D1297E1BBC6867C52282FAC85D9B7C4F32B44FF57E8FBB06288C1946000306258E7E6ABB4E4A6367B16DE6309 X-B7AD71C0: 6FEFE4C63DFE2D85469AD6E133326EAB664F5199923B286E81C2AD9CFA0FBF5C9C2BBA594F31363B5803BE1F3B17DC36E8F7B195E1C9783157EE3080892E0192EE3E100BA2433E01 X-C1DE0DAB: 0D63561A33F958A543CB7CD7E4B48B49D6A6405AC1BAD9A4C05BFDC9AE87FEE2D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75448CF9D3A7B2C848410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34BC3EEE75EF3BACCF518185E015DAE119C64CB7F5DC570119B5136EB9E0B10F4FF7113A8AE70672BC1D7E09C32AA3244C75C7CFA224F0B2ACF20B44E15F7863AC5A1673A01BA68E40729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojEcKN7r9rK/3D/6D+LjD6Iw== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822716A2879C9BD7B6F551D3C7448FB3F1D3841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v12 8/8] test: box/cfunc -- add cmod test 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 Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Thanks for the patch! See 5 comments below. > diff --git a/test/box/cfunc2.c b/test/box/cfunc2.c > new file mode 100644 > index 000000000..f6d826183 > --- /dev/null > +++ b/test/box/cfunc2.c > @@ -0,0 +1,132 @@ > +#include > +#include > +#include > + > +#include "module.h" > + > +/* > + * Just make sure we've been called. > + */ > +int > +cfunc_nop(box_function_ctx_t *ctx, const char *args, const char *args_end) > +{ > + (void)ctx; > + (void)args; > + (void)args_end; > + return 0; > +} > + > +/* > + * Fetch first N even numbers (just to make sure the order of > + * arguments is not screwed).> + */ > +int > +cfunc_fetch_evens(box_function_ctx_t *ctx, const char *args, const char *args_end) > +{ > + int arg_count = mp_decode_array(&args); > + if (arg_count != 1) { > + return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s", > + "invalid argument count"); 1. Broken alignment. Here and in other error places. > + } > + int field_count = mp_decode_array(&args); > + if (field_count < 1) { > + return box_error_set(__FILE__, __LINE__, ER_PROC_C, "%s", > + "invalid array size"); > + } > + > + for (int i = 0; i < field_count; i++) { > + int val = mp_decode_uint(&args); > + int needed = 2 * (i+1); 2. Please, use whitespaces before and after binary operators. I really suggest you to revisit the code style guide. Why do you need such a complex function? Why couldn't you simply make a function like 'echo', which validates msgpack and returns it as is? You would be able to check that the args are not corrupted, and are exactly what you have passed. > + if (val != needed) { > + char res[128]; > + snprintf(res, sizeof(res), "%s %d != %d", > + "invalid argument", val, needed); > + return box_error_set(__FILE__, __LINE__, > + ER_PROC_C, "%s", res); > + } > + } > + > + return 0; > +} > diff --git a/test/box/cmod.result b/test/box/cmod.result > new file mode 100644 > index 000000000..f8558c191 > --- /dev/null > +++ b/test/box/cmod.result > @@ -0,0 +1,199 @@ > +-- test-run result file version 2 > +build_path = os.getenv("BUILDDIR") > + | --- > + | ... > +package.cpath = build_path..'/test/box/?.so;'..build_path..'/test/box/?.dylib;'..package.cpath > + | --- > + | ... > + > +cmod = require('cmod') 3. In the beginning of a test we usually write a comment with the issue number and its description. > + | --- > + | ... > +fio = require('fio') > + | --- > + | ... > + > +ext = (jit.os == "OSX" and "dylib" or "so") > + | --- > + | ... > + > +cfunc_path = fio.pathjoin(build_path, "test/box/cfunc.") .. ext > + | --- > + | ... > +cfunc1_path = fio.pathjoin(build_path, "test/box/cfunc1.") .. ext > + | --- > + | ... > +cfunc2_path = fio.pathjoin(build_path, "test/box/cfunc2.") .. ext > + | --- > + | ... > + > +_ = pcall(fio.unlink(cfunc_path)) > + | --- > + | ... > +fio.symlink(cfunc1_path, cfunc_path) > + | --- > + | - true > + | ... > + > +module, err = cmod.load('non-existing-cfunc') > + | --- > + | ... > +assert(err ~= nil) > + | --- > + | - true > + | ... > + > +-- > +-- They all are sitting in cfunc.so > +module, err = cmod.load('cfunc') > + | --- > + | ... > +assert(err == nil) > + | --- > + | - true > + | ... > + > +cfunc_nop, err = module:load('no-such-func') > + | --- > + | ... > +assert(err ~= nil) > + | --- > + | - true > + | ... > +cfunc_nop, err = module:load('cfunc_nop') > + | --- > + | ... > +assert(err == nil) > + | --- > + | - true > + | ... > +cfunc_fetch_evens, err = module:load('cfunc_fetch_evens') > + | --- > + | ... > +assert(err == nil) > + | --- > + | - true > + | ... > +cfunc_multireturn, err = module:load('cfunc_multireturn') > + | --- > + | ... > +assert(err == nil) > + | --- > + | - true > + | ... > +cfunc_args, err = module:load('cfunc_args') > + | --- > + | ... > +assert(err == nil) > + | --- > + | - true > + | ... > +cfunc_sum, err = module:load('cfunc_sum') > + | --- > + | ... > +assert(err == nil) > + | --- > + | - true > + | ... > + > +-- > +-- Make sure they all are callable > +cfunc_nop() > + | --- > + | - true > + | ... > +cfunc_fetch_evens() > + | --- > + | - true > + | ... > +cfunc_multireturn() > + | --- > + | - true > + | ... > +cfunc_args() > + | --- > + | - true > + | ... > + > +-- > +-- Clean old function references and reload a new one. > +_ = pcall(fio.unlink(cfunc_path)) > + | --- > + | ... > +fio.symlink(cfunc2_path, cfunc_path) > + | --- > + | - true > + | ... > + > +module:reload() > + | --- > + | - true > + | ... > + > +cfunc_nop() > + | --- > + | - true > + | ... > +cfunc_multireturn() 4. I am not sure it is correct. The function wasn't reloaded. I thought we decided that to get a new function after reload you need to call load() on the module again. Reason for not changing the existing function objects was that the reload is supposed to happen when the whole application reloads. It means there can be some fibers running the old code for a while after reload until they notice that they must be restarted. If you do reload like you did in this patch, the old fibers will get new functions immediately right under their feet, and this may result into unexpected behaviour for code in these fibers. Another way to get the desired behaviour - use unload() + load() - I assume in this case the old function objects will remain unchanged, right? I suggest to ask Mons what to do with this. And this is why I asked for a proper RFC (or GitHub discussion) before starting to rework the patch. > + | --- > + | - true > + | - [1] > + | - [1] > + | ... > +cfunc_fetch_evens({2,4,6}) > + | --- > + | - true > + | ... > +cfunc_fetch_evens({1,2,3}) -- error > + | --- > + | - null > + | - invalid argument 1 != 2 > + | ... > +cfunc_args(1, "hello") > + | --- > + | - true > + | - [1, 'hello'] > + | ... > +cfunc_sum(1) -- error > + | --- > + | - null > + | - invalid argument count > + | ... > +cfunc_sum(1,2) > + | --- > + | - true > + | - 3 > + | ... > + > +-- > +-- Clean them up > +cfunc_nop:unload() > + | --- > + | - true > + | ... > +cfunc_multireturn:unload() > + | --- > + | - true > + | ... > +cfunc_fetch_evens:unload() > + | --- > + | - true > + | ... > +cfunc_args:unload() > + | --- > + | - true > + | ... > +cfunc_sum:unload() > + | --- > + | - true > + | ... > +module:unload() > + | --- > + | - true > + | ... > + > +-- > +-- Cleanup the generated symlink > +_ = pcall(fio.unlink(cfunc_path)) 5. You didn't test GC, multiple unload (should raise an error), reload when the shared file was deleted, and that the functions are kept loaded even if the module was unloaded. Maybe more, but this is just what I thought of right away.