[tarantool-patches] Re: [PATCH] travis-ci: set right flags in release testing jobs
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sun Apr 28 11:54:43 MSK 2019
Hi! Thanks for the fixes! See 5 comments below.
>> See 9 comments below, and my fixes on the branch in a separate commit.
>>
>>> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
>>> index 2dfc4fc9d..14177aaab 100644
>>> --- a/src/box/lua/net_box.c
>>> +++ b/src/box/lua/net_box.c
>>> @@ -689,12 +689,12 @@ netbox_decode_sql_info(struct lua_State *L, const char **data)
>>> if (map_size == 2) {
>>> key = mp_decode_uint(data);
>>> assert(key == SQL_INFO_AUTOINCREMENT_IDS);
>>> - (void)key;
>>> + (void) key;
>>
>> 1. Unnecessary diff. Please, drop.
>
> I don't think so. It is okay to fix a code block that you touch in a
> commit to fix it according to a code style.
1. You did not touch this block. This concrete line was not an error nor
warning. In this commit you did not touch 'key' variable at all. So
this change is unrelated. Please, drop. You just took an arbitrary variable
and decided to add a whitespace - this is what is called an unrelated change.
>
> This would be applicable if I would change some unrelated block of code:
This is what you did - 'key' variable is unrelated to other changes and
to the commit message and title.
> here I'm agree with you.
>
>>
>>> uint64_t count = mp_decode_array(data);
>>> assert(count > 0);
>>> lua_createtable(L, 0, count);
>>> for (uint32_t j = 0; j < count; ++j) {
>>> - int64_t id;
>>> + int64_t id = INT64_MIN;
>>
>> 2. Is there any concrete reason why the value is so
>> non-trivial? Why not just simple zero?
>
> We don't verify a data received from a network in net.box for
> correctness. AFAIU from your words it was the design decision. So I
> consider this broken by design, but try to at least set a value that
> less probably appear in a normal output to signal a user that something
> going wrong (when we get something other then int64 here).
>
> Of course, we should give an error in the case, but I left this case
> unverified to keep things consistent.
2. Ok, I do not agree with broken by design, but I do not mind against
INT64_MIN.
>>> diff --git a/src/box/xrow.c b/src/box/xrow.c
>>> index aaed84e38..ccdd9def5 100644
>>> --- a/src/box/xrow.c
>>> +++ b/src/box/xrow.c
>>> @@ -509,9 +509,14 @@ iproto_write_error(int fd, const struct error *e, uint32_t schema_version,
>>> schema_version, sizeof(body) + msg_len);
>>>
>>> body.v_data_len = mp_bswap_u32(msg_len);
>>> - (void) write(fd, header, sizeof(header));
>>> - (void) write(fd, &body, sizeof(body));
>>> - (void) write(fd, e->errmsg, msg_len);
>>> +
>>> + int rc1, rc2, rc3;
>>> + rc1 = write(fd, header, sizeof(header));
>>> + rc2 = write(fd, &body, sizeof(body));
>>> + rc3 = write(fd, e->errmsg, msg_len);
>>> + (void) rc1;
>>> + (void) rc2;
>>> + (void) rc3;
>>
>> 5. Firstly, you could use one 'rc' for all of them. It would be shorter.
>> Secondly, why not just remove these (void) casts? We ignore returned value
>> in many places and it is ok, if it isn't really important.
>>
>> In this concrete place an error has already happened, and it makes
>> no sense to create one another - this is why the values are ignored.
>>
>> =====================================================================
>> body.v_data_len = mp_bswap_u32(msg_len);
>> -
>> - int rc1, rc2, rc3;
>> - rc1 = write(fd, header, sizeof(header));
>> - rc2 = write(fd, &body, sizeof(body));
>> - rc3 = write(fd, e->errmsg, msg_len);
>> - (void) rc1;
>> - (void) rc2;
>> - (void) rc3;
>> + write(fd, header, sizeof(header));
>> + write(fd, &body, sizeof(body));
>> + write(fd, e->errmsg, msg_len);
>> =====================================================================
>
> The write() glibc syscall wrapper is marked with
> __attribute__((__warn_unused_result__)), so we cannot ignore this result
> when -Werror is set. Maybe it is not so for your libc implementation /
> version: in this case you'll not get this warning.
>
> Re using one rc: I had the thought that clang tracks whether a result is
> used or discarded with a next assingment, so I wrote the code in that
> way. But I don't observe such behaviour with clang-6, 7, 8 and 3.9. It
> seems I did remember it wrongly and a result is considered as 'used'
> when we assign it; a cast to void is just to eliminate the 'variable set
> but not used' warning.
>
> GCC complains about (void) write(...);, so we need to assign a result
> into a variable: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425
3. I've tried on my virtual Ubuntu - yep. You are right. The fix below
is ok then.
>
> Rewritten as follows:
>
> ssize_t unused;
> unused = write(fd, header, sizeof(header));
> unused = write(fd, &body, sizeof(body));
> unused = write(fd, e->errmsg, msg_len);
> (void) unused;
>
>>
>>> }
>>>
>>> int
>>> @@ -624,12 +629,15 @@ xrow_decode_dml(struct xrow_header *row, struct request *request,
>>> request->header = row;
>>> request->type = row->type;
>>>
>>> + const char *start = NULL;
>>> + const char *end = NULL;
>>> +
>>> if (row->bodycnt == 0)
>>> goto done;
>>>
>>> assert(row->bodycnt == 1);
>>> - const char *data = (const char *) row->body[0].iov_base;
>>> - const char *end = data + row->body[0].iov_len;
>>> + const char *data = start = (const char *) row->body[0].iov_base;
>>> + end = data + row->body[0].iov_len;
>>> assert((end - data) > 0);
>>>
>>> if (mp_typeof(*data) != MP_MAP || mp_check_map(data, end) > 0) {
>>> @@ -701,8 +709,8 @@ error:
>>> done:
>>> if (key_map) {
>>> enum iproto_key key = (enum iproto_key) bit_ctz_u64(key_map);
>>> - xrow_on_decode_err(row->body[0].iov_base, end,
>>> - ER_MISSING_REQUEST_FIELD, iproto_key_name(key));
>>> + xrow_on_decode_err(start, end, ER_MISSING_REQUEST_FIELD,
>>> + iproto_key_name(key));
>>
>> 6. Why did you add 'start' variable? This code was fully okay, as I
>> understand. Compiler could not raise an error on 'row->body[0].iov_base'
>> usage here. Please, drop this hunk. A patch should not contain unrelated
>> changes making review harder and git history dirtier.
>
> The compiler warning is not false positive here: the code was not okay.
> I'll cite the commit message:
4. There was no compiler warning at all except 'end' variable used after
'done' label, but declared after 'goto' to it. 'start' variable is not
needed here. I've dropped it and nothing happened (I tried on Linux,
gcc-7, with your new compiler option - no errors).
Please, apply the diff below.
============================================================================
@@ -627,14 +627,13 @@ xrow_decode_dml(struct xrow_header *row, struct request *request,
request->header = row;
request->type = row->type;
- const char *start = NULL;
const char *end = NULL;
if (row->bodycnt == 0)
goto done;
assert(row->bodycnt == 1);
- const char *data = start = (const char *) row->body[0].iov_base;
+ const char *data = (const char *) row->body[0].iov_base;
end = data + row->body[0].iov_len;
assert((end - data) > 0);
@@ -707,8 +706,8 @@ error:
done:
if (key_map) {
enum iproto_key key = (enum iproto_key) bit_ctz_u64(key_map);
- xrow_on_decode_err(start, end, ER_MISSING_REQUEST_FIELD,
- iproto_key_name(key));
+ xrow_on_decode_err(row->body[0].iov_base, end,
+ ER_MISSING_REQUEST_FIELD, iproto_key_name(key));
return -1;
}
return 0;
============================================================================
>
>> * xrow_decode_*() functions could read uninitialized data when printing
>> a hex code for a row if the received msgpack is empty, but there are
>> expected keys.
>
> Please, suggest a change for wording if the current one is not clear.
I thought that it was about 'end' variable. There is nothing wrong
about 'row->body[0].iov_base' usage after 'done' label.
>
> My change causes `for (const char *cur = start; cur < end;) { ... }` in
> dump_row_hex() to walk around the loop body and so prevents reading from
> `start` in the loop (NULL < NULL is false).
You assigned 'start' from 'row->body[0].iov_base', and the latter is not
changed in the function. So I do not see what was wrong with reading
this 'iov_base' anywhere in the function.
What is more, in the same function you still use 'row->body[0].iov_base'
here
https://github.com/tarantool/tarantool/blob/Totktonada/gh-4178-set-right-flags-in-release-testing-jobs/src/box/xrow.c#L643
and here
https://github.com/tarantool/tarantool/blob/Totktonada/gh-4178-set-right-flags-in-release-testing-jobs/src/box/xrow.c#L703
So 'start' not only is unrelated, but also inconsistent. I dropped this
diff and everything works without any errors nor warnings.
Please, apply it, for ballot() function:
============================================================================
@@ -1077,14 +1076,13 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot)
ballot->is_ro = false;
vclock_create(&ballot->vclock);
- const char *start = NULL;
const char *end = NULL;
if (row->bodycnt == 0)
goto err;
-
assert(row->bodycnt == 1);
- const char *data = start = (const char *) row->body[0].iov_base;
+
+ const char *data = (const char *) row->body[0].iov_base;
end = data + row->body[0].iov_len;
const char *tmp = data;
if (mp_check(&tmp, end) != 0 || mp_typeof(*data) != MP_MAP)
@@ -1133,7 +1131,8 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot)
}
return 0;
err:
- xrow_on_decode_err(start, end, ER_INVALID_MSGPACK, "packet body");
+ xrow_on_decode_err(row->body[0].iov_base, end, ER_INVALID_MSGPACK,
+ "packet body");
return -1;
}
============================================================================
>
>>
>>> return -1;
>>> }
>>> return 0;
>>> @@ -1071,12 +1079,16 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot)
>>> ballot->is_ro = false;
>>> vclock_create(&ballot->vclock);
>>>
>>> + const char *start = NULL;
>>> + const char *end = NULL;
>>> +
>>> if (row->bodycnt == 0)
>>> goto err;
>>> +
>>
>> 7. Excessive empty line. Please, drop.
>
> Don't know which empty line you considered as excessive.
5. We both know, that it is the line I've commented above - right
below 'goto err'.
> If you can link
> a guide clause about that or describe a formal rule you want to make
> mandatory, then put it here.
There is a rule about not making unrelated changes. This is a sense of
atomic commits. A change is unrelated, when it stands away from other
changes and can be dropped without consequences. Here this empty line
1) stands away from other changes, 2) can be dropped. So it is unrelated.
In addition to formal definition of 'commit' we have something even
better - Vladimir D. responses in freelists.
Grep by 'unrelated':
https://www.freelists.org/post/tarantool-patches/PATCH-22-libcorefiber-Allow-to-extend-default-stack-size,1
https://www.freelists.org/post/tarantool-patches/PATCH-v2-34-sql-use-space-by-name-in-SQL,1
> In the latter case it would be good to
> strengthen it with examples from our code. (However I propose to don't
> spend time on that: it is counter-productive.)
It *is* productive. Because it teaches how to make diffs more atomic
and simple. It makes reviewer's life easier and simplifies search for
errors - the smaller is diff, the easier it is to find all errors.
Talking of 'code examples' - it is not about code. It is about
commits and their diff. First version of this commit looked like a
scattered number of '+' and '-' many of which were like:
---
- (void)key;
+ (void) key;
---
if (row->bodycnt == 0)
goto err;
+
assert(row->bodycnt == 1);
---
And some of the others were just unnecessary.
Obviously, it does not make review process easier. I needed to somehow
determine which of these changes were about the bug, which of them were
just about your personal taste of adding empty lines in the code you've
seen alongside. It is not ok. If you want to fix code style, then send
a separate commit. This commit is declared as
'set right flags in release testing jobs' - how do these new empty lines
and new whitespaces in the random code relate to the commit title and to
the given changes?
Together with the changes proposed by me in this mail, we've dropped
from your original commit already 20 lines of pure diff, both of '+'
and '-'.
More information about the Tarantool-patches
mailing list