Tarantool development patches archive
 help / color / mirror / Atom feed
From: Alexander Turenko <alexander.turenko@tarantool.org>
To: Maxim Melentiev <m.melentiev@corp.mail.ru>
Cc: tarantool-patches@freelists.org,
	Konstantin Osipov <kostja.osipov@gmail.com>,
	Kirill Yukhin <kyukhin@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH] Replace sendmsg with sendto shortcut
Date: Wed, 21 Aug 2019 13:02:22 +0300	[thread overview]
Message-ID: <20190821100221.3pi5fo2vzb36q2cb@tkn_work_nb> (raw)
In-Reply-To: <b4c41e5e-5ca4-1553-3532-0344b60aa740@corp.mail.ru>

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

  reply	other threads:[~2019-08-21 10:02 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-20 12:01 [tarantool-patches] " 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 [this message]
2019-08-20 13:06 ` Konstantin Osipov
2019-08-22 13:15 ` Kirill Yukhin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190821100221.3pi5fo2vzb36q2cb@tkn_work_nb \
    --to=alexander.turenko@tarantool.org \
    --cc=kostja.osipov@gmail.com \
    --cc=kyukhin@tarantool.org \
    --cc=m.melentiev@corp.mail.ru \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH] Replace sendmsg with sendto shortcut' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox