Re: [Tarantool-patches] [PATCH v4 1/2] vinyl: check vinyl_dir existence at bootstrap

[Alexander Turenko <alexander.turenko@tarantool.org>]

> 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.