[tarantool-patches] Re: [PATCH] Replace sendmsg with sendto shortcut

[Maxim Melentiev]

Pushed with changes to 
https://github.com/tarantool/tarantool/tree/fix_sendmsg

On 21/08/2019 02:34, Alexander Turenko wrote:
> This patch looks good to me.
>
> Minor comments are below. It does not require you to send the patch
> again. You can just answer here to comments and provide a branch with a
> new version of the patch. It is usual when a patch is not changed much.
>
> Kostya, are you have objections here?
>
> WBR, Alexander Turenko.
>
>> Replace sendmsg with sendto shortcut
> Maybe it worth to add 'systemd:' prefix to the commit header (and start
> a header with a small letter so).
>
> On Tue, Aug 20, 2019 at 03:16:35PM +0300, Maxim Melentiev wrote:
>> ping Konstantin Osipov, Alexander Turenko
>>
>> Sorry, forgot to CC in initial email and then pinged in wrong thread.
>>
>> On 20/08/2019 15:01, Max Melentiev (Redacted sender m.melentiev for DMARC)
>> wrote:
>>> There is a problem with calculating .msg_namelen field
>>> of msghdr struct. Instead of
>>>
>>>       .msg_name   = &sa,
>>>       .msg_namelen = sizeof(sa.sun_family) + strlen(sd_unix_path),
>>>
>>> it must set as
>>>
>>>       .msg_namelen = sizeof(sa) // larger value than current invalid one
>>>
>>> It works on linux but when I tried to enable this feature for macOS
>>> it didn't (maybe because of different order of fields in the struct).
>>>
>>> Instead of fixing calculation, I've replaced original sendmsg call
>>> with sendto, because it's a convenient shortcut which
>>> simplifies code and can prevent such mistakes.
> I suggest to add 'Part of #xxxx' or 'Needed for #xxxx'.
>
>>> ---
>>>
>>> Preview: https://github.com/printercu/tarantool/commit/bc30b1093be1d03507bf84ca218b03bec5e0244a
>>>
>>>    src/systemd.c | 13 ++-----------
>>>    1 file changed, 2 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/src/systemd.c b/src/systemd.c
>>> index b6c48afe2..9c0a26dd2 100644
>>> --- a/src/systemd.c
>>> +++ b/src/systemd.c
>>> @@ -100,23 +100,14 @@ int systemd_notify(const char *message) {
>>>    	struct sockaddr_un sa = {
>>>    		.sun_family = AF_UNIX,
>>>    	};
>>> -	struct iovec vec = {
>>> -		.iov_base = (char  *)message,
>>> -		.iov_len  = (size_t )strlen(message)
>>> -	};
>>> -	struct msghdr msg = {
>>> -		.msg_iov    = &vec,
>>> -		.msg_iovlen = 1,
>>> -		.msg_name   = &sa,
>>> -	};
>>>
>>> -	msg.msg_namelen = sizeof(sa.sun_family) + strlen(sd_unix_path);
>>>    	strncpy(sa.sun_path, sd_unix_path, sizeof(sa.sun_path));
>>>    	if (sa.sun_path[0] == '@')
>>>    		sa.sun_path[0] = '\0';
>>>
>>>    	say_debug("systemd: sending message '%s'", message);
>>> -	ssize_t sent = sendmsg(systemd_fd, &msg, MSG_NOSIGNAL);
>>> +	ssize_t sent = sendto(systemd_fd, message, (size_t)strlen(message),
>>> +		MSG_NOSIGNAL, (struct sockaddr*)&sa, sizeof(sa));
> Nit: As I see we usually split an asterics in a pointer type from a
> pointed type with a space and also split a type in parentheses from a
> value when casting. So:
>
> (size_t)strlen(message) -> (size_t) strlen(message)
> (struct sockaddr*)&sa -> (struct sockaddr *) &s
>
>>>    	if (sent == -1) {
>>>    		say_syserror("systemd: failed to send message");
>>>    		return -1;
>>> --
>>> 2.21.0
>>>
>>>
>>> .
>>>
>