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.4 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: Christopher Lord Newsgroups: gmane.comp.lang.ruby.posix-mq.general Subject: Re: [PATCH] Ability to adopt file descriptors Date: Sat, 3 Jan 2015 20:34:40 -0700 Message-ID: References: <1420305358-21019-1-git-send-email-christopher@lord.ac> <1420305358-21019-1-git-send-email-christopher@lord.ac> <20150104011636.GA26859@dcvr.yhbt.net> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable X-Trace: ger.gmane.org 1420342499 13110 80.91.229.3 (4 Jan 2015 03:34:59 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sun, 4 Jan 2015 03:34:59 +0000 (UTC) To: ruby.posix.mq@librelist.com Original-X-From: ruby.posix.mq@librelist.com Sun Jan 04 04:34:54 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:122 Archived-At: Received: from zedshaw2.xen.prgmr.com ([71.19.156.177]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1Y7bxs-0007kY-Ku for gclrpg-ruby.posix.mq@m.gmane.org; Sun, 04 Jan 2015 04:34:53 +0100 Received: from zedshaw2.xen.prgmr.com (unknown [IPv6:::1]) by zedshaw2.xen.prgmr.com (Postfix) with ESMTP id C91D174DE1 for ; Sun, 4 Jan 2015 03:35:55 +0000 (UTC) Hello, thanks for the review! Basically I wanted to upstream some local c= hanges I had made for an internal project, so 'cleaned up' a patch. A lot= of your code review boils down to =E2=80=9CI should have put more effort= /testing into the 'cleaned up=E2=80=99 version=E2=80=9D :) Always embarr= assing. Comments below: > 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) No major objections, am ok with that. >=20 >> +static VALUE for_fd(VALUE klass, VALUE socket) >> +{ >> + VALUE mqv =3D alloc(klass); >> + struct posix_mq *mq =3D get(mqv, 0); >> + >> + mq->name =3D Qnil; >> + >> + if (rb_respond_to(socket, id_to_i)) { >> + VALUE fd_num =3D rb_funcall(socket, id_to_i, 0); >> + mq->des =3D FD_TO_MQD(NUM2INT(fd_num)); >> + } >=20 > Probably better to skip the to_i conversion to match IO.for_fd > semantics. No need to define and use id_to_i anymore. I had it like that originally, but ended up changing to the to_i version = in the =E2=80=98cleaned up=E2=80=99 version because it seemed cool. I am = perfectly fine with being consistent with IO, thanks for catching that. >=20 >> + else { >> + rb_raise(rb_eArgError, "provided argument must be (or convertable t= o) an integer file descriptor"); >=20 > will raise TypeError instead of ArgumentError to be consistent with IO Yes, that=E2=80=99s message is a TypeError, not an ArgError. I like your = version. >=20 >> + if (mq->des < 0) { >> + rb_raise(rb_eArgError, "provided argument must be a valid file desc= riptor"); >> + } >=20 > 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. Artifact of my cleanup effort. Agreed that it=E2=80=99s (mostly) duplicat= ed. >=20 >> + if (mq_getattr(mq->des, &mq->attr) < 0) { >> + if (errno) { >> + rb_raise(rb_eArgError, "provided file descriptor is not a POSIX me= ssage queue"); >> + } >=20 > This should be: >=20 > rb_sys_fail("provided file descriptor is not a POSIX MQ"); >=20 > We should not expect errno to be zero if mq_getattr returns < 0 I modelled the check above on the dbus/systemd code[1] (sd_is_mq) that ch= ecks that a file descriptor is a posix mq. It checks both the return bein= g less than zero, and that the errno is non-zero. Presumably it=E2=80=99s= because it=E2=80=99s to ensure the sequence point exists between mq_geta= ttr and errno, but honestly it shouldn=E2=80=99t matter. if mq_getattr re= turns less-than-zero, then surely the errno was set as well. Either way i= s fine by me. [1]: http://dbus.freedesktop.org/doc/api/html/sd-daemon_8c_source.html >=20 > Rest of the C code looks fine; I'll wrap the long lines before pushing. >=20 > However, the test code has me worried: >=20 >> --- 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_PLATFOR= M}" >> POSIX_MQ.method_defined?(:notify) or >> warn "POSIX_MQ#notify not supported on this platform: #{RUBY_PLATFO= RM}" >> + POSIX_MQ.class.method_defined?(:for_fd) or >> + warn "POSIX_MQ::for_fd not supported on this platform: #{RUBY_PLA= TFORM}" >>=20 >> def setup >> @mq =3D 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) >>=20 >> + def test_for_fd >> + @mq =3D POSIX_MQ.new @path, IO::CREAT|IO_RDWR, 0666 >> + @alt =3D 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_d= efined(:to_io) >=20 > I'm not sure how you managed to run your new test at all, Yep, you caught me. wrote the tests without running them and missed `buff= =3D =E2=80=9C"`. I had a version mismatch between my ruby=E2=80=99s mini= test and the one used to run the tests in this project. I ran the tests i= n irb and then tried to make double-sure the testcase was perfect because= I didn=E2=80=99t want to change the underlying dependency and screw you = guys up. That said, I was sure I double-checked that=20 X.class.method_defined?(a) =3D=3D X.respond_to?(a) Perhaps there is a difference between versions? or is this just a stylist= ic preference? anyway no problem if you want to change that too. > I think the following is needed: >=20 > --- 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_PLATFOR= M}" > - 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_PLATFO= RM}" >=20 > def setup > @@ -247,7 +247,8 @@ class Test_POSIX_MQ < Test::Unit::TestCase > end if POSIX_MQ.method_defined?(:to_io) >=20 > def test_for_fd > - @mq =3D POSIX_MQ.new @path, IO::CREAT|IO_RDWR, 0666 > + buf =3D "" > + @mq =3D POSIX_MQ.new @path, IO::CREAT|IO::RDWR, 0666 > @alt =3D 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_de= fined(:to_io) > + end if POSIX_MQ.respond_to?(:for_fd) && POSIX_MQ.method_defined?(:to= _io) >=20 > def test_notify > rd, wr =3D IO.pipe > Also, I think we'll also need to support equivalents of IO#autoclose? > and IO#autoclose=3D, because having two POSIX_MQ objects refer to the s= ame > FD will cause problems with the GC performing auto-close by default. I suspect autoclose might be useful for other uses of the change than sys= temd socket activation. I know autoclose can be useful for evented code o= r in situations where the wrapper socket type is ephemerally recreated af= ter going through non-ruby extensions to save memory. I can imagine someo= ne wanting to do something similar with message queues, so autoclose migh= t be useful there.