Re: [Tarantool-patches] [PATCH 1/2] swim: add SO_BROADCAST option

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi! Thanks for the review!

>> diff --git a/src/lib/swim/swim_io.c b/src/lib/swim/swim_io.c
>> index c8558c43e..45df36ba4 100644
>> --- a/src/lib/swim/swim_io.c
>> +++ b/src/lib/swim/swim_io.c
>> @@ -512,8 +512,23 @@ static inline void
>>  swim_complete_send(struct swim_scheduler *scheduler, struct swim_task *task,
>>  		   ssize_t size)
>>  {
>> -	if (size < 0)
>> -		diag_log();
>> +	if (size < 0) {
>> +		bool is_critical = false;
>> +#if TARGET_OS_DARWIN
>> +		/*
>> +		 * On Mac this error happens regularly if SWIM is bound to
>> +		 * the localhost and tries to broadcast out of the machine. This
>> +		 * is not critical, because will happen in the tests a lot, and
>> +		 * in prod it simply should not bind to localhost if there are
>> +		 * multiple machines in the cluster. Besides, Mac as a platform
>> +		 * is not supposed to be used in prod.
>> +		 */
>> +		struct error *last = diag_last_error(diag_get());
>> +		is_critical = (last->saved_errno == EADDRNOTAVAIL);
>> +#endif
>> +		if (is_critical)
>> +			diag_log();
>> +	}
>>  	if (task->complete != NULL)
>>  		task->complete(task, scheduler, size);
> 
> Vlad, I don't understand. For non-mac users this @is_critical will
> always be false, maybe we better move the whoe branch to TARGET_OS_DARWIN
> then? Ie
> 
> 	if (size < 0) {
> 	#if TARGET_OS_DARWIN
> 		if (is_critical)
> 			diag_log();
> 	#endif
> 	}
> 
> or you made it this way to escape unused parameter compiler warning?

I added the variable on all platforms on purpose. Because if the code would
be done the way you propose above, non-Mac platforms won't log anything.
I, on the contrary, want to log on all platforms, but not certain errors
in certain platforms. Only EADDRNOTAVAIL and only on Mac. Everything else
must be logged. Because the user does not call 'send' directly, and has no
other way to know there was an error except via the logs.

Another option would be to have it like this:

	#if TARGET_OS_DARWIN
		bool is_critical = ...;
		if (is_critical)
			diag_log();
	#else
		diag_log();
	#endif

But I don't like having 2 places handling the critical error. I wanted to
have only one diag_log().

Talking of the warning - I am not sure I understand. It is used on both
platforms. On non-Mac it is the 'if' condition. Even if it is never changed
and inlined by the compiler, it is still used. There can't be a warning.