From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Hugh Daschbach Newsgroups: gmane.emacs.bugs Subject: bug#43252: 27.1; DBus properties lack type hints or overrides Date: Fri, 18 Sep 2020 20:32:29 -0700 Message-ID: <875z8aa1bm.fsf@ccss.com> References: <87v9gqquct.fsf@ccss.com> <87imcqdo38.fsf@gmx.de> <87sgbtqylc.fsf@ccss.com> <87sgbtcvqs.fsf@gmx.de> <87pn6xqtsz.fsf@ccss.com> <87363sl4hs.fsf@gmx.de> <877dt3bnfl.fsf@ccss.com> <878sdj84kn.fsf@gmx.de> <871rjbaq05.fsf@ccss.com> <874ko6979w.fsf@gmx.de> <87v9gm9x9i.fsf@ccss.com> <871rj9k78r.fsf@gmx.de> <87imclwow5.fsf@gmx.de> <87pn6t9rbq.fsf@ccss.com> <87y2lggzvd.fsf@gmx.de> <87h7rzadlo.fsf@ccss.com> <87k0wtrir2.fsf@gmx.de> <87een19x9n.fsf@ccss.com> <87a6xor25b.fsf@gmx.de> <87bli49rem.fsf@ccss.com> <87h7rva0ku.fsf@gmx.de> Mime-Version: 1.0 Content-Type: text/plain; format=flowed Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="40848"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: mu4e 1.5.5; emacs 27.1 Cc: 43252@debbugs.gnu.org To: Michael Albinus Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sat Sep 19 05:33:12 2020 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kJTcd-000ATq-59 for geb-bug-gnu-emacs@m.gmane-mx.org; Sat, 19 Sep 2020 05:33:11 +0200 Original-Received: from localhost ([::1]:52342 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1kJTcb-0002ZD-Lc for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 18 Sep 2020 23:33:09 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:54802) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kJTcU-0002Yu-MY for bug-gnu-emacs@gnu.org; Fri, 18 Sep 2020 23:33:02 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:33905) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1kJTcU-0008BA-Dz for bug-gnu-emacs@gnu.org; Fri, 18 Sep 2020 23:33:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1kJTcU-0007wh-A1 for bug-gnu-emacs@gnu.org; Fri, 18 Sep 2020 23:33:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Hugh Daschbach Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sat, 19 Sep 2020 03:33:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 43252 X-GNU-PR-Package: emacs Original-Received: via spool by 43252-submit@debbugs.gnu.org id=B43252.160048636530518 (code B ref 43252); Sat, 19 Sep 2020 03:33:02 +0000 Original-Received: (at 43252) by debbugs.gnu.org; 19 Sep 2020 03:32:45 +0000 Original-Received: from localhost ([127.0.0.1]:45451 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1kJTcD-0007wA-Ai for submit@debbugs.gnu.org; Fri, 18 Sep 2020 23:32:45 -0400 Original-Received: from mail1.ccss.com ([159.203.255.73]:54738) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1kJTcB-0007vv-Jm for 43252@debbugs.gnu.org; Fri, 18 Sep 2020 23:32:44 -0400 Original-Received: by mail1.ccss.com (Postfix, from userid 114) id F24BABF8D8; Fri, 18 Sep 2020 20:32:37 -0700 (PDT) Original-Received: from ccss.com (unknown [192.168.76.11]) by mail1.ccss.com (Postfix) with ESMTP id 01FFFBF5CC; Fri, 18 Sep 2020 20:32:36 -0700 (PDT) Original-Received: from localhost (localhost [127.0.0.1]) by ccss.com (Postfix) with ESMTP id C835F17606CD; Fri, 18 Sep 2020 20:32:36 -0700 (PDT) Original-Received: from ccss.com ([127.0.0.1]) by localhost (ccss.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id FRJJp5hBaioW; Fri, 18 Sep 2020 20:32:30 -0700 (PDT) Original-Received: from klaatu (klaatu.lan [192.168.42.3]) (Authenticated sender: hugh) by ccss.com (Postfix) with ESMTPSA id 989FD1760428; Fri, 18 Sep 2020 20:32:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ccss.com; s=mail; t=1600486350; bh=yYwWBkfsrLxX1OXdluCMA3sDkIgjJejNYNJ18+ehAo4=; h=References:From:To:Cc:Subject:In-reply-to:Date:From; b=myv8dVLpaqXm5J0MRkYRic/o5ZOYJ3CaS8Y20kSr7oa2S/oJDI0WuVz7j1T0a3gUj 7t2/A9aEvgLnvcZFPYajf+PBC+C6xGFjoE5F/xxJvrxdTaIHpm5nztA6JhNc/S7y/C tsGywl92x4qQAeTwYJuqqbyTb9URxqVqjGwzD5eMF/ew7SUjNwky2ey+kilCokuvKy i9wYRNM2HrA7OE2+OGFv3WgJx7sYfmik4OC7xRJ1M6N8Fy6nngd/K+I6cTpePZSKG6 zrOB7GRbwhEaKkcstj4qYmfrxSMVyKEBlMclSKBHNeNQHeY6lASkK/HaGwrd6GVHy0 hl8bn7s3SxhfA== In-reply-to: <87h7rva0ku.fsf@gmx.de> X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:188360 Archived-At: Michael Albinus writes: > Hugh Daschbach writes: > > Hi Hugh, >> There doesn't seem to be any type checking on property set. > > Well, before getting type information in dbus-event, it wasn't > possible > for dbus-property-handler to know the types of values provided > by > org.freedesktop.DBus.Properties.Set method calls. Therefore I've > said, > that the type used in dbus-register-property shall be > inherited. This > decision wasn't dictated by the D-Bus API, it was just an > implementation > restriction. > > Now, that the type information is preserved, I have abandoned > this > restriction. You can register a property with any type, and you > can > overwrite this property via an ofDP.Set call with a value of any > other > type. This is not forbidden by the D-Bus API (but highly > discouraged, I > guess). Makes sense. I'll adjust tests accordingly. >> I'm starting to run out of ideas for additional >> tests. Suggestions >> welcome. > > The major black hole seems to be dbus-introspect* tests. If you > are > interested? I fear writing them will be boring, so I haven't > done them > yet ... > > OTOH, they are not the most important part of Emacs' D-Bus > implementation. I'm willing. Will have a look. >> +(defun dbus-test06-make-property-test (selector name value >> expected) > > I would call it dbus--test-make-property-test. test06 isn't > important, > and the double slash "--" is an indication that this is an > internal > function, which shouldn't leave the dbus-tests.el scope. See the > other > helper functions in the file. Good by me. I'll rename accordingly. >> +;; Since we don't expect this helper function and it's caller >> +;; `dbus-test06-make-property' to be used outside this file, >> we don't >> +;; bother with `eval-and-compile.' It would be appropriate to >> wrap >> +;; this with `eval-and-compile' if that expectation is >> misguided. > > Well, it is uncommon that a function returns a code snippet. I > haven't > checked, but couldn't you achieve your goal by changing this > defun into > a defsubst? Seems like a better approach. I'm new enough at this that I wasn't aware of defsubst. I'll give it a go, thanks. >> +(defmacro dbus-test06-test-property (name value-list) > > Same comment on name here. I would call it dbus--test-property. > >> +The argument VALUES is a list of pairs, where each pair >> +represents a value form and an expected returned value form. >> The >> +first pair in VALUES is used for `dbus-register-property'. >> +Subsequent pairs of the list are tested with >> +`dbus-set-property'." > > The second argument is VALUE-LIST, not VALUES. However, Elisp > encourages > an argument list like > > (defmacro dbus-test-test-property (name &rest value-list) > > This simplifies call conventions, you can call then with several > key-value arguments like > > (dbus--test-property > "ByteArray" > '((:array :byte 1 :byte 2 :byte 3) . (1 2 3)) > '((:array :byte 4 :byte 5 :byte 6) . (4 5 6))) > >> +(defmacro with-dbus-monitor (buffer &rest body) Excellent feedback. Changes incorporated. > Such a macro name would poison your Elisp name space. Keep the > dbus--test prefix, and name the macro like > dbus--test-with-dbus-monitor. > >> + (unwind-protect >> + (progn ,@body) >> + (sit-for 0.5) > > sit-for is problematic, because it would delay the test run by > 0.5 > seconds, unconditionally. People regard this negative, because > the > (whole) Emacs test suite shall run fast. A better check might be > > (with-timeout (1 (dbus--test-timeout-handler)) > (while (accept-process-output process 0 nil t))) Thanks. I knew the sit-for was a hack, worse an unpredictable hack. I should have mentioned that I planned to remove the dbus-monitor wrapper when before final submission. It's useful for debugging the tests. But the tests themselves don't need this. I've folded in your suggestion, but it's scheduled for the chopping block, anyway. I'm still learning. Your feedback is most helpful. Thanks. >> + (should >> + (equal >> + (dbus-register-property >> + :session dbus--test-service dbus--test-path >> + dbus--test-interface "StringArray" :read >> + '(:array "one" "two" :string "three")) >> + `((:property :session ,dbus--test-interface >> "StringArray") >> + (,dbus--test-service "/org/gnu/Emacs/TestDBus")))) > > You might use ,dbus--test-path instead. Here and everywhere > else. Good catch. Thanks. >> + >> + (should ; Should this >> error instead? >> + (equal >> + (dbus-set-property >> ... >> + '(:array "seven" "eight" :string "nine")) > > Good question. dbus-set-property and dbus-get-property do not > propagate > D-Bus errors. Maybe we shall change the functions to do so? I've > asked > this already myself. I don't have a strong opinion either way. I'm just trying to note corner cases. >> + ;; Test mismatched types in array >> + >> + (should ; Oddly enough, >> register works, but get fails >> + (equal > > Hmm, yes. dbus-register-property does not perform a local type > check. And honestly, I don't want to do it; I let the D-Bus > daemon do > the job. Great. >> + dbus--test-interface "MixedArray") >> + '("/node00" "/node01" "/node0/node02"))) > > Yes, dbus-get-property is hit by the mismatched types in the > :array. Isn't > this sufficient? It is. As long as we can predict where errors will be reported. I'll update comments to indicate intended behavior. >> + (should ; This should >> error or the next get should fail >> + (equal >> + (dbus-set-property >> + :session dbus--test-service dbus--test-path >> + dbus--test-interface "ByteValue" 1024) >> + 1024)) > > No error expected. You haven't given 1024 a type (like :byte), > so it is > handled as :uint32. Cool. With the explanation regarding dbus-set-property changing types, this makes perfect sense. > And even if you would have prefixed the value with :byte, there > won't be > an error. In dbusbind.c, byte values are simply computed by > taking the > modulo 255: > > unsigned char val = XFIXNAT (object) & 0xFF; > > ":byte 1024" is equal to ":byte 4". Similar conversions happen > for the > other basic types, based on numbers. Good. I haven't thought deeply enough about DBus to anticipate truncation. I've added a test for this, an extract of which is below. The get returns nil instead of 4. I can change the expected value, but wanted to run this by you first. > Maybe we could add some tests for these conversions? Since they > are not > restricted to property handling, (a) new test(s) dbus-test01-* > would help. I'll have a look. >>> Implementation is more complex than expected. Due to its >>> nature, >> But I have no objection to a parallel instance to gather >> request >> signatures. > > I don't know where we end up. I'm still poking around how to > implement a > second connection to the same bus. If it is not too expensive to > implement I'd prefer this. Fair enough. Your call. >> Which raises the question, should dbus-set-property function >> call fail >> for a local property that isn't :readwrite, or should that only >> be >> prevented by incoming messages? > > dbus-set-property doesn't know, whether a property is registered > locally. I guess an error reply is reasonable, whether the > property is > registered locally, or not. Would be nice. Unless it adds overhead, like an introspection. >> Do we require that dbus-register-property be used to update a >> :read >> access property. > > dbus-set-property shall fail when the property has :read > access. Yes, > such a property can be changed only by > dbus-register-property. But :read > access is intended to tell the clients, that they shouldn't > change the > property; an error in dbus-set-property (returning nil, > respectively) is > appropriate. Cool. I mentioned an additional test above. The get below, extracted from the larger test, returns nil instead of 4: (ert-deftest dbus-test-ad-hoc () (dbus-ignore-errors (dbus-unregister-service :session dbus--test-service)) (dbus-register-service :session dbus--test-service) (should ; Test value truncation (equal (dbus-register-property :session dbus--test-service dbus--test-path dbus--test-interface "ByteValue" :read :byte 1024) `((:property :session ,dbus--test-interface "ByteValue") (,dbus--test-service ,dbus--test-path)))) (should ; Returns 0 instead of 4. (equal (dbus-get-property :session dbus--test-service dbus--test-path dbus--test-interface "ByteValue") 4)) (dbus-unregister-service :session dbus--test-service)) Should I update the expectation to zero? > Best regards, Michael. Cheers, Hugh