bug#43252: 27.1; DBus properties lack type hints or overrides

[Hugh Daschbach]

Michael Albinus writes:

> Hugh Daschbach <hugh@ccss.com> 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