[PATCH] Unix datagram socket messaging

[Volker Lendecke]

On Wed, Apr 16, 2014 at 12:00:04PM +0200, Stefan (metze) Metzmacher wrote:
> Hi Volker,
> 
> > https://git.samba.org/?p=vl/samba.git/.git;a=shortlog;h=refs/heads/unix-msg
> > 
> > contains the result of Jeremy's preliminary review of the
> > unix datagram based messaging code for source3.
> > 
> > This is the request for formal review and push to master.
> 
> The notify patches seem to be independent, can you move them to be the
> first patches?

They are not entirely independent. They are not necessary
with the current messaging, because we handle messages
always before the client requests. I do not want to put in
additional complexity to the notify code before it is clear
that the rest of the patches can go in at all.

> The same applies to "printing_cups: Call the msg_ctx destructor on exit"
> and "smbd: Call the msg_ctx destructor for background jobs".

Same here. They are not necessary without the rest of the
patches and just add complexity.

> iov_buf* can also be added first and then used in the unix_msg code
> instead of the static copy.

I want to avoid making unix_msg depend on the rest of Samba,
just as I want to make for example pthreadpool independent.

If you think this is overkill, I can rewrite this code to
make use of the tevent_req infrastructure and the rest of
Samba. Do you want me to do this?

> They all have "Reviewed-by: Stefan Metzmacher <metze at samba.org>"
> 
> I haven't fully reviewed the real changes yet, but have some initial
> comments:
> 
> Can we pass tevent_context to poll_funcs_init_tevent() instead of
> using: func.private_data = ev; in the caller?

Yes, I will do that.

> Are we sure we don't get problems when having all messaging sockets in
> just one subdirectory?

My assumption is that by now all sane file systems have
hashed directories and thus direct access should be fast.
Also, we should not have more than a few thousand sockets
per node. Is this a problem for modern file systems? If so,
I can certainly convert this to a hashing scheme with
256/256 subdirectories.

> There're environments with just winbindd running, so we might need a
> periodic
> cleanup run not only in smbd.

Ok, I will add that.

> 
> In
> https://git.samba.org/?p=vl/samba.git/.git;a=commitdiff;h=777954434bb7404659fbb7f519a4069b1100c400
> we have
> 
> +       struct sockaddr_un addr = { 0, };
> 
> ...
> 
> +               ret = bind(ctx->sock, (struct sockaddr *)&addr,
> sizeof(addr));
> 
> I think we should cast with (struct sockaddr *)(void *)&addr, in order
> to avoid
> strict aliasing warnings on some systems.

The correct way to avoid this is to do a memcpy. I will
convert to that.

> After socket() we should set FD_CLOEXEC and O_NONBLOCK similar to
> tsocket_bsd_common_prepare_fd() or set_blocking() and
> smb_set_close_on_exec().
> MSG_DONTWAIT might not be available everywhere.

Ok, I will change that to not use MSG_DONTWAIT.

> + if (errno != EWOULDBLOCK) {
> 
> may need some more magic and also check for EAGAIN,
> see tsocket_bsd_error_from_errno() or commit
> f5a3d74264abb25009e73b1b35d62db34fa62343.
> ... ok, I just found that it's fixed in
> https://git.samba.org/?p=vl/samba.git/.git;a=commitdiff;h=325e62cbb116abdec7cf1466862bd12ecd42224e
> I think we should squash that and also this one
> https://git.samba.org/?p=vl/samba.git/.git;a=commitdiff;h=d44c82a1edddb0d838702e15e00538faae9386bc
> 
> + unlink(ctx->path);
> 
> We may need to check that ctx->path is not an empty string?

Ok, thanks, will do those changes.

Volker

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de