Re: [Tarantool-patches] [PATCH v4 2/2] box: introduce _vsession_settings sysview

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the fixes!

On 27/11/2019 10:53, Mergen Imeev wrote:
> Thank you for review! My answer and diff between patches
> below.
> 
> On Sun, Nov 24, 2019 at 04:27:58PM +0100, Vladislav Shpilevoy wrote:
>> Hi! Thanks for the patch!
>>
>>> diff --git a/src/box/session.cc b/src/box/session.cc
>>> index 461d1cf..93d8b42 100644
>>> --- a/src/box/session.cc
>>> +++ b/src/box/session.cc
>>> @@ -338,3 +341,131 @@ generic_session_sync(struct session *session)
>>> +
>>> +struct iterator *
>>> +session_options_create_iterator(struct index *index, enum iterator_type type,
>>> +				const char *key, uint32_t part_count)
>>> +{
>>> +	char *decoded_key = NULL;
>>> +	if (part_count > 0) {
>>> +		assert(part_count == 1);
>>> +		assert(mp_typeof(*key) == MP_STR);
>>> +		uint32_t len;
>>> +		const char *name = mp_decode_str(&key, &len);
>>> +		decoded_key = (char *)malloc(len + 1);
>>> +		if (decoded_key == NULL) {
>>> +			diag_set(OutOfMemory, len + 1, "malloc", "decoded_key");
>>> +			return NULL;
>>> +		}
>>> +		memcpy(decoded_key, name, len);
>>> +		decoded_key[len] = '\0';
>>> +	}
>>> +	struct space *space = space_cache_find(index->def->space_id);
>>> +	struct session_options_iterator *it =
>>> +		(session_options_iterator *)malloc(sizeof(*it));
>>> +	if (it == NULL) {
>>
>> decoded_key leaks here. But heap OOM is such an unlikely event,
>> and is forgotten for be checked so often, that perhaps we should
>> reconsider moving this https://github.com/tarantool/tarantool/issues/3534
>> out of wish list and start using panic in new places. Probably
>> wrapped into a 'heap_alloc()' function (name is discussable).
>>
>> I think, Kirill won't be against this.
> Fixed. I asked about issue #3534 in a scrum, but haven’t
> received a definite answer yet. I will ask about it again,
> but a little later, since now we have to prepare for
> release 2.3.1. Also, it looks like we should replace all
> malloc() checks in one commit.

No, we should not. I mean, we can, but it is not necessary.
We just could stop using malloc() directly in new code. And
would drop it from the old code eventually, alongside with
other changes.

The patchset LGTM. Technically.