* [tarantool-patches] [PATCH] Replace sendmsg with sendto shortcut @ 2019-08-20 12:01 Max Melentiev 2019-08-20 12:16 ` [tarantool-patches] " Maxim Melentiev ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Max Melentiev @ 2019-08-20 12:01 UTC (permalink / raw) To: tarantool-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [tarantool-patches] Re: [PATCH] Replace sendmsg with sendto shortcut 2019-08-20 12:01 [tarantool-patches] [PATCH] Replace sendmsg with sendto shortcut Max Melentiev @ 2019-08-20 12:16 ` Maxim Melentiev 2019-08-20 23:34 ` Alexander Turenko 2019-08-20 13:06 ` Konstantin Osipov 2019-08-22 13:15 ` Kirill Yukhin 2 siblings, 1 reply; 7+ messages in thread From: Maxim Melentiev @ 2019-08-20 12:16 UTC (permalink / raw) To: tarantool-patches; +Cc: kostja, alexander.turenko 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 > > > . > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [tarantool-patches] Re: [PATCH] Replace sendmsg with sendto shortcut 2019-08-20 12:16 ` [tarantool-patches] " Maxim Melentiev @ 2019-08-20 23:34 ` Alexander Turenko 2019-08-21 8:23 ` Maxim Melentiev 0 siblings, 1 reply; 7+ messages in thread From: Alexander Turenko @ 2019-08-20 23:34 UTC (permalink / raw) To: Maxim Melentiev; +Cc: tarantool-patches, kostja 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 > > > > > > . > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [tarantool-patches] Re: [PATCH] Replace sendmsg with sendto shortcut 2019-08-20 23:34 ` Alexander Turenko @ 2019-08-21 8:23 ` Maxim Melentiev 2019-08-21 10:02 ` Alexander Turenko 0 siblings, 1 reply; 7+ messages in thread From: Maxim Melentiev @ 2019-08-21 8:23 UTC (permalink / raw) To: tarantool-patches; +Cc: alexander.turenko 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 >>> >>> >>> . >>> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [tarantool-patches] Re: [PATCH] Replace sendmsg with sendto shortcut 2019-08-21 8:23 ` Maxim Melentiev @ 2019-08-21 10:02 ` Alexander Turenko 0 siblings, 0 replies; 7+ messages in thread From: Alexander Turenko @ 2019-08-21 10:02 UTC (permalink / raw) To: Maxim Melentiev; +Cc: tarantool-patches, Konstantin Osipov, Kirill Yukhin 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 > > > > > > > > > > > > . > > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [tarantool-patches] Re: [PATCH] Replace sendmsg with sendto shortcut 2019-08-20 12:01 [tarantool-patches] [PATCH] Replace sendmsg with sendto shortcut Max Melentiev 2019-08-20 12:16 ` [tarantool-patches] " Maxim Melentiev @ 2019-08-20 13:06 ` Konstantin Osipov 2019-08-22 13:15 ` Kirill Yukhin 2 siblings, 0 replies; 7+ messages in thread From: Konstantin Osipov @ 2019-08-20 13:06 UTC (permalink / raw) To: tarantool-patches; +Cc: Max Melentiev * 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [tarantool-patches] Re: [PATCH] Replace sendmsg with sendto shortcut 2019-08-20 12:01 [tarantool-patches] [PATCH] Replace sendmsg with sendto shortcut Max Melentiev 2019-08-20 12:16 ` [tarantool-patches] " Maxim Melentiev 2019-08-20 13:06 ` Konstantin Osipov @ 2019-08-22 13:15 ` Kirill Yukhin 2 siblings, 0 replies; 7+ messages in thread From: Kirill Yukhin @ 2019-08-22 13:15 UTC (permalink / raw) To: tarantool-patches; +Cc: Max Melentiev 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-08-22 13:15 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-20 12:01 [tarantool-patches] [PATCH] Replace sendmsg with sendto shortcut Max Melentiev 2019-08-20 12:16 ` [tarantool-patches] " Maxim Melentiev 2019-08-20 23:34 ` Alexander Turenko 2019-08-21 8:23 ` Maxim Melentiev 2019-08-21 10:02 ` Alexander Turenko 2019-08-20 13:06 ` Konstantin Osipov 2019-08-22 13:15 ` Kirill Yukhin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox