From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 65A6C431780 for ; Mon, 17 Aug 2020 08:33:22 +0300 (MSK) Date: Mon, 17 Aug 2020 08:33:21 +0300 From: "Alexander V. Tikhonov" Message-ID: <20200817053321.GA13807@hpalx> References: <4eb7b26b9f80393b19536f49ab3985d72ba13d89.1597398597.git.avtikhon@tarantool.org> <20200816212659.t4q55vctby3imgd7@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200816212659.t4q55vctby3imgd7@tkn_work_nb> Subject: Re: [Tarantool-patches] [PATCH v4 1/2] vinyl: check vinyl_dir existence at bootstrap List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org Hi Alexander, thanks a lot for your comments, I've corrected topic message of the commit as you suggested and completely rewrote the commit message in myown words. Also test split from the common one. On Mon, Aug 17, 2020 at 12:26:59AM +0300, Alexander Turenko wrote: > > vinyl: check vinyl_dir existence at bootstrap > > It looks like you added the check, but it was present before the change. > 'Fix check' better describes the change. > > > During implementation of openSUSE got failed box-tap/cfg.test.lua test. > > Found that when memtx_dir wasn't existed, while vinyl_dir existed and > > errno was set to ENOENT, box configuration succeeded, but it shouldn't. > > Reason of this wrong behaviour was that not all of the failure paths in > > xdir_scan() were set errno, but the caller assumed it. > > In fact this sentence describes everything that I asked in [1]. > However, I don't mind including more information. But it should be clear > for a reader: see the suggestions below. > > [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-August/019031.html > > > Usual C convention is to report success or failure using a return > > value and set errno at any error. So a caller usually just checks a > > return value and if it means a failure (usually -1), it checks errno > > to determine an exact reason. > > > > Usual convention in tarantool is a bit different: we use a special > > diagnostics area to report a reason of a failure. > > > > Not all failure paths of xdir_scan() sets errno (including our > > 'invalid instance UUID' case), so we cannot be sure that errno is > > not remains unchanged after a failure of the function. > > > > However the solution with checking errno against ENOENT (No such file > > or directory) is not good. For example: > > > > - What if xdir_scan() would be changed in future and, say, some call > > will rewrite errno after the opendir() call? > > - What if some other call inside xdir_scan() will set ENOENT: say, > > open() in xdir_open_cursor() due to some race? > > > > We lean on implementation details of the callee, not its contract. This > > way is too fragile and it should either check whether the directory > > exists before xdir_scan() call or pass a flag to xdir_scan() whether > > the directory should exist. Decided to use second variant - it does not > > lead to code duplication. > > (Yes, there is a lot of text below. But I'm sure it deserves a time to > read it. I hope it sounds friendly, not edgy.) > > This part of the commit message is like a bag of facts. But what is the > idea or ideas you want to express? > > I'll give a couple of examples from your message. There are facts about > failure reporting conventions. But why it is highlighted here? No > conclusions, no connections to the rest of the message. Or, say, copy of > parts of the discussion about possible solutions: `errno = 0;` vs > `is_dir_required` without mention of the former. A reader has no chance > to understand why it is written here. > > I don't want to continue pointing places, where a reader will be > confused: you can (and should) do it youself. It is better to give > general suggestions, how to explain things. I'm not much experienced > here, to be honest, but there are points I would share. > > First of all, I strongly suggest to avoid copying of another person > wording. When you write an explanation youself, you will ask youself > questions about it. Whether it is correct? Whether it gives ideas you > want to express? How to better structurize it? The only way to improve > this skill is practice. > > I suggest to decide about core ideas, read all materials (code, test > results, discussions), but than move the maretials aside and explain the > ideas in your words. > > This is the main point of this email, so I'll stop here again. Don't > copy-paste. My past review comments had sense in its context, but just > confuses a reader here. Some of them were given as background > information for you, some comments compare different ways to solve the > problem. It is the information for you: to analyze and decide how to > make your patch better. So, again: decide about core ideas and express > them. In your words. > > Of course, you may find youself on the point "I don't know how to > express it". That's the key moment. Here I usually ask myself whether I > really understand what is going on. I got back to the code and > materials, re-read them, perform additional tests, experiment with > different solutions. If things become clear for me after this, I can > continue describing them. But sometimes it appears that I'm unable to > defend a choosen way to solve a problem and I'm going to rewrite my > code. > > It may also appear that my description becomes large and vague: an idea > is missed between details. Maybe cut it off entirely? Or give just core > idea, without details? Or structurize to make it clear where the idea > itself and where details that can be skipped by a reader? > > The last suggestion is to track context of a reader. Imagine that you > know nothing about the problem. After the first paragraph you got ideas > it gives. The next paragraph should be clear in this context. For > example, it may start with: > > - 'There are pitfalls a developer should aware', > - 'There are alternative solutions', > - 'The implementation lean on the following assumptions', > - 'The problem appears only under the following conditions', > - or, say, just 'Usage example', > - maybe even just 'Usual convention is' if it is connected to the > general context later. > > All those clauses marks how paragraphs (ideas, in fact) are connected to > each other and allows a reader to don't lose context while reading. > > > Added subtest to box-tap/cfg.test.lua test file, to check the currently > > fixed issue. > > This is redundant, IMHO. Almost every change should be accompanied by a > test case. > > > diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua > > index 569b5f463..a60aa848e 100755 > > --- a/test/box-tap/cfg.test.lua > > +++ b/test/box-tap/cfg.test.lua > > Our guidelines suggest to extract a bugfix test into a separate file.