From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 29BD225361 for ; Tue, 6 Aug 2019 03:56:50 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id l2mtmHsHSc31 for ; Tue, 6 Aug 2019 03:56:50 -0400 (EDT) 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 turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 72CB625360 for ; Tue, 6 Aug 2019 03:56:49 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 3/3] box: extend ffi error object API References: <47d9072e60bdc563c7466a4e51db1a61bc71a610.1564657285.git.kshcherbatov@tarantool.org> <55d56797-f937-4088-fdba-7d9dfef51ae6@tarantool.org> From: Kirill Shcherbatov Message-ID: Date: Tue, 6 Aug 2019 10:56:46 +0300 MIME-Version: 1.0 In-Reply-To: <55d56797-f937-4088-fdba-7d9dfef51ae6@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: Vladislav Shpilevoy , Tarantool MailList I'll make review fixes and I'll send a corresponding letter later.Now I'd like to discuss a few unclear moments. >> err1 = >> err1:wrap(err) - add err error object as a reason for err1 object >> this call modifies err1 object and doesn't >> modify err object. > >> err1_parent = >> err1:unwrap() - remove the most recent error in error object, >> return it's parent. the call has no effect when >> there is no parent in given error object. > 3. If it modifies err1, then why do I need to assign a result to > err1? The same below. Let's start discussing this question with second :unwrap method. At first, some Lua variables point to error object (sic: not diag area); Therefore we need a way to return an unwrapped parent object to user. The implemented error:unwrap() method modifies an original error object(removing it's parent) and returns it's parent object. next_err = err:unpack() To make API consistent, I also return an error object in error:wrap(reason) method. This is the only reason for :wrap. We may get rid of it, if it is your strong opinion. >> +/** >> + * Wrap reason error object into given error. >> + * This API replaces box.error.last() value with an updated >> + * error object. > > 4. Why do you need to change the global error? And why is not > it mentioned in the docbot request? In many details my motivation is similar with (3.)th block: to make my API consistent. It is really important to enforce something taking a reference to parent error before unref(ing) it for self (for :unwrap) object. We would like to return reason for user, right? But self object make have the last reference to it. The delete method mustn't be called. Partially :wrap and :unwrap operations are constructors that introduce a new error. So changing box.error.last() seems for me reasonable. > 7. Unwrap does not allow to unwrap a leaf error. > But there is no API to determine if the error is > leaf. So a user can't determine when to stop calling > unwrap.> > I am talking about C public API which you have changed > here. A user can't check error->reason != NULL before > calling box_error_unwrap. I don't mind: I've had a draft with such implementation. Let's do it so. >> +err2 = nil >> + | --- >> + | ... >> +collectgarbage() >> + | --- >> + | - 0 >> + | ... > > 10. Nit: you could nullify all the errors at once, and call > collectgarbage. >What do you mean? I consciously clean up the errors and call the garbage collector in these places. If you put an extra printf in error_unref/destructor you'll see why this is important. (also see your 4th question - this is a coverage tests for this problem) >> + >> +s:drop() >> + | --- >> + | ... > 11. In the RFC you said, that IProto returns a list of error. Where > it is? I haven't implemented this yet. Kostya said that we make do it later.