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

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

[Max Melentiev]

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

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));
 	if (sent == -1) {
 		say_syserror("systemd: failed to send message");
 		return -1;
--
2.21.0

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

[Maxim Melentiev]

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.
> ---
>
> 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));
>   	if (sent == -1) {
>   		say_syserror("systemd: failed to send message");
>   		return -1;
> --
> 2.21.0
>
>
> .
>

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

[Alexander Turenko]

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

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

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

[Alexander Turenko]

Please, keep all participants in To / CC.

CCed Kostja.

CCed Kirill.

WBR, Alexander Turenko.

On Wed, Aug 21, 2019 at 11:23:40AM +0300, Maxim Melentiev wrote:
> 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
> > > > 
> > > > 
> > > > .
> > > > 
> > 

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

[Konstantin Osipov]

* Max Melentiev <dmarc-noreply@freelists.org> [19/08/20 15:05]:
> There is a problem with calculating .msg_namelen field
> of msghdr struct. Instead of

lgtm


-- 
Konstantin Osipov, Moscow, Russia

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

[Kirill Yukhin]

Hello,

On 20 Aug 15:01, Max Melentiev 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've checked your patch into 1.10, 2.1, 2.2 and master.

--
Regards, Kirill Yukhin