From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS47066 71.19.144.0/20 X-Spam-Status: No, score=-1.9 required=3.0 tests=AWL,BAYES_00, MSGID_FROM_MTA_HEADER shortcircuit=no autolearn=unavailable version=3.3.2 Path: news.gmane.org!not-for-mail From: Eric Wong Newsgroups: gmane.comp.lang.ruby.posix-mq.general Subject: Re: [PATCH] Ability to adopt file descriptors Date: Sun, 4 Jan 2015 01:16:36 +0000 Message-ID: <20150104011636.GA26859@dcvr.yhbt.net> References: <1420305358-21019-1-git-send-email-christopher@lord.ac> <1420305358-21019-1-git-send-email-christopher@lord.ac> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit X-Trace: ger.gmane.org 1420334211 3265 80.91.229.3 (4 Jan 2015 01:16:51 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sun, 4 Jan 2015 01:16:51 +0000 (UTC) To: ruby.posix.mq@librelist.com Original-X-From: ruby.posix.mq@librelist.com Sun Jan 04 02:16:47 2015 Return-path: Envelope-to: gclrpg-ruby.posix.mq@m.gmane.org List-Archive: List-Help: List-Id: List-Post: List-Subscribe: List-Unsubscribe: Precedence: list Original-Sender: ruby.posix.mq@librelist.com Xref: news.gmane.org gmane.comp.lang.ruby.posix-mq.general:121 Archived-At: Received: from zedshaw2.xen.prgmr.com ([71.19.156.177]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1Y7ZoE-0001K1-4F for gclrpg-ruby.posix.mq@m.gmane.org; Sun, 04 Jan 2015 02:16:46 +0100 Received: from zedshaw2.xen.prgmr.com (unknown [IPv6:::1]) by zedshaw2.xen.prgmr.com (Postfix) with ESMTP id 59C9B74DE1 for ; Sun, 4 Jan 2015 01:17:48 +0000 (UTC) Christopher Lord wrote: > This patch adds support for adopting an existing file descriptor, together > with testcases. The need for this comes up when we use systemd with the > ListenMessageQueue directive. For socket activation, systemd opens the POSIX > message queue and expects user code to begin using the file descriptor > without opening it in-process. > > To support the systemd model in the `posix_mq` gem, this patch suggests > imitating the behavior on the Socket class, which uses `#for_fd` to create a > socket class from a descriptor. Hi Christopher, this seems useful, thanks! I have some comments below, but I'll queue it up with my changes so you don't have to resend (unless you have objections to my changes) > One confounding factor exists. POSIX queues have a name but it is difficult > to get access to this name in a safe manner from the file descriptor. One > option would be to `readlink(2)` on `/proc/self/fd/N` to get the name[1], but > note that if the descriptor is unlinked we wouldn't get anything usable. > Rather than risk incorrect behavior and extra complexity, I've decided to > just raise an `ArgumentError` if `#name` is called on adopted descriptors. > Typically one wouldn't need the actual name in a systemd socket-activated > situation, anyway. That's fine. > +static VALUE for_fd(VALUE klass, VALUE socket) > +{ > + VALUE mqv = alloc(klass); > + struct posix_mq *mq = get(mqv, 0); > + > + mq->name = Qnil; > + > + if (rb_respond_to(socket, id_to_i)) { > + VALUE fd_num = rb_funcall(socket, id_to_i, 0); > + mq->des = FD_TO_MQD(NUM2INT(fd_num)); > + } Probably better to skip the to_i conversion to match IO.for_fd semantics. No need to define and use id_to_i anymore. mq->des = FD_TO_MQD(NUM2INT(fd_num)); So the below: > + else { > + rb_raise(rb_eArgError, "provided argument must be (or convertable to) an integer file descriptor"); will raise TypeError instead of ArgumentError to be consistent with IO > + if (mq->des < 0) { > + rb_raise(rb_eArgError, "provided argument must be a valid file descriptor"); > + } This (mq->des < 0) check is unnecessary since we check with mq_getattr anyways. It should be as consistent as possible with the way IO.for_fd works. > + if (mq_getattr(mq->des, &mq->attr) < 0) { > + if (errno) { > + rb_raise(rb_eArgError, "provided file descriptor is not a POSIX message queue"); > + } This should be: rb_sys_fail("provided file descriptor is not a POSIX MQ"); We should not expect errno to be zero if mq_getattr returns < 0 Rest of the C code looks fine; I'll wrap the long lines before pushing. However, the test code has me worried: > --- a/test/test_posix_mq.rb > +++ b/test/test_posix_mq.rb > @@ -17,6 +17,8 @@ class Test_POSIX_MQ < Test::Unit::TestCase > warn "POSIX_MQ#to_io not supported on this platform: #{RUBY_PLATFORM}" > POSIX_MQ.method_defined?(:notify) or > warn "POSIX_MQ#notify not supported on this platform: #{RUBY_PLATFORM}" > + POSIX_MQ.class.method_defined?(:for_fd) or > + warn "POSIX_MQ::for_fd not supported on this platform: #{RUBY_PLATFORM}" > > def setup > @mq = nil > @@ -244,6 +246,17 @@ class Test_POSIX_MQ < Test::Unit::TestCase > assert_nothing_raised { IO.select([@mq], nil, nil, 0) } > end if POSIX_MQ.method_defined?(:to_io) > > + def test_for_fd > + @mq = POSIX_MQ.new @path, IO::CREAT|IO_RDWR, 0666 > + @alt = POSIX_MQ.for_fd(@mq.to_io.to_i) > + assert_equal true, @mq.send("hello", 0) > + assert_equal [ "hello", 0 ], @alt.receive(buf) > + assert_equal "hello", buf > + assert_equal @mq.to_io.to_i, @alt.to_io.to_i > + assert_raises(ArgumentError) { @alt.name } > + assert_raises(ArgumentError) { POSIX_MQ.for_fd(1) } > + end if POSIX_MQ.class.method_defined?(:for_fd) && POSIX_MQ.method_defined(:to_io) I'm not sure how you managed to run your new test at all, I think the following is needed: --- a/test/test_posix_mq.rb +++ b/test/test_posix_mq.rb @@ -17,7 +17,7 @@ class Test_POSIX_MQ < Test::Unit::TestCase warn "POSIX_MQ#to_io not supported on this platform: #{RUBY_PLATFORM}" POSIX_MQ.method_defined?(:notify) or warn "POSIX_MQ#notify not supported on this platform: #{RUBY_PLATFORM}" - POSIX_MQ.class.method_defined?(:for_fd) or + POSIX_MQ.respond_to?(:for_fd) or warn "POSIX_MQ::for_fd not supported on this platform: #{RUBY_PLATFORM}" def setup @@ -247,7 +247,8 @@ class Test_POSIX_MQ < Test::Unit::TestCase end if POSIX_MQ.method_defined?(:to_io) def test_for_fd - @mq = POSIX_MQ.new @path, IO::CREAT|IO_RDWR, 0666 + buf = "" + @mq = POSIX_MQ.new @path, IO::CREAT|IO::RDWR, 0666 @alt = POSIX_MQ.for_fd(@mq.to_io.to_i) assert_equal true, @mq.send("hello", 0) assert_equal [ "hello", 0 ], @alt.receive(buf) @@ -255,7 +256,7 @@ class Test_POSIX_MQ < Test::Unit::TestCase assert_equal @mq.to_io.to_i, @alt.to_io.to_i assert_raises(ArgumentError) { @alt.name } assert_raises(ArgumentError) { POSIX_MQ.for_fd(1) } - end if POSIX_MQ.class.method_defined?(:for_fd) && POSIX_MQ.method_defined(:to_io) + end if POSIX_MQ.respond_to?(:for_fd) && POSIX_MQ.method_defined?(:to_io) def test_notify rd, wr = IO.pipe Also, I think we'll also need to support equivalents of IO#autoclose? and IO#autoclose=, because having two POSIX_MQ objects refer to the same FD will cause problems with the GC performing auto-close by default. Finally, this is probably one of the last threads on this mailing list. Librelist couldn't support rsync-able archives any longer and I'm no longer a fan of subscription-required lists, so I'll migrate this list to mlmmj + public-inbox.org infrastructure next week. The only change will be open-to-all posting without subscriptions, so Cc:-ing everyone is required. public-inbox/ssoma allows cloning mail archives via git.