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

classic Classic list List threaded Threaded
42 messages Options
123
Reply | Threaded
Open this post in threaded view
|

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

Hugh Daschbach-2

Michael Albinus writes:

> Hugh Daschbach <[hidden email]> writes:
>
> Hi Hugh,
>
>>> this succeeds, we could implement a counterpart to the
>>> dbus-monitor
>>> program in Elisp. And you would be able to access this
>>> information
>>
>> Excellent.  I can now parse the output of dbus-monitor.  But
>> capturing
>
> Implementation is more complex than expected. Due to its nature,
> org.freedesktop.DBus.Monitoring.BecomeMonitor requires another
> (parallel) connection to the bus. This is not foreseen yet in
> dbusbind.c;
> will see how it could fly.
>
> What I could provide just now is an implementation which runs in
> *another* Emacs instance. This could be used for monitoring
> only,
> because it is another connection to the bus per definition. Are
> you
> interested to get such a partial implementation?

I'm interested in whatever you want to implement.  I see signature
verification useful for testing rather than an exposed feature.

From what I can see from looking at dbus-monitor output the
correct
property types are being exposed now.  It seems to be working.

So whatever we approach we take, the benefit is early warning of
future
regressions.  You are a better judge of benefit of additional
effort
than I.

A second Emacs instance seems to offer the same asynchronous
output
gathering issues that dbus-monitor poses.  It does eliminates the
ad-hoc
parser.

If you have a longer term goal, I'd suggest pursuing that rather
than
something partial that you'll want to replace later.

But I have no objection to a parallel instance to gather request
signatures.

>>  Do you have
>> an issue with rolling that up in a macro?
>
> No objection. But comments :-)
>
>> (defmacro dbus-test05-test-property (name value expected)
>>  `(let ((byte-array ,name))
>
> I wouldn't call the variable "byte-array"; it could be anything
> during
> test. Call it "property" or alike.

Fixed

>>    (should
>>     (equal
>>      (dbus-register-property
>>       :session dbus--test-service dbus--test-path
>>       dbus--test-interface ,name :read
>
> I would use access type :readwrite. We want also to test
> dbus-set-property.

Yes, I've added a set property test.  I'll move access to a
parameter so
I can do both positive and negative testing; confirm that :read
prevents
writes.

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?  Do we require that
dbus-register-property be used to update a :read access property.

>>       ,value)
>>      `((:property :session ,,dbus--test-interface ,,name)
>>        (,dbus--test-service ,,dbus--test-path))))
>
> What are the double commas good for? Typos?

I had nested quasi-quoted expressions.  I'm working to avoid that.
So
that was a bug.

>>    (should
>>     (equal
>>      (dbus-get-property
>>       :session dbus--test-service dbus--test-path
>>       dbus--test-interface ,name)
>>      ,expected))
>>
>>    ;; a test for `dbus-get-property' shall be added.
>
> That's my typo - dbus-set-property is meant. And yes, it shall
> also be
> here. So you might need macro arguments value1 expected1 value2
> expected2.

I assumed as much.  I just carried the comment around blindly.
I've
changed what I sent you to accept a list of pairs of values and
expected
return sexps.  I use the first pair on the list for
dbus-register-property, verify retrieval, then use
dbus-set-property to
update and verify the property from the remaining pairs.

I need more testing and a cleanup pass.  I'll pass along a better
version when
I think it's ready for review.

I've started a few "should fail" tests.

Cheers,
Hugh




Reply | Threaded
Open this post in threaded view
|

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

Hugh Daschbach-2

Hugh Daschbach writes:

> Michael Albinus writes:
>
>> Hugh Daschbach <[hidden email]> writes:
>>
>> Hi Hugh,
>>
> I need more testing and a cleanup pass.  I'll pass along a
> better
> version when
> I think it's ready for review.
>
> I've started a few "should fail" tests.
I've made a bit of progress.  I have a few tests that fail.  There
doesn't seem to be any type checking on property set.  But have a
look
and see if you concur that these are real errors.  I've attached a
patch.

In addition, I see a failure in dbus-test04-register-method:

> F dbus-test04-register-method
>     Check method registration for an own service.
>     (ert-test-failed
>      ((should
>        (equal
> (should-error
> (dbus-call-method :session dbus--test-service
> dbus--test-path dbus--test-interface method1 :timeout 10 "foo"))
> `(dbus-error ... "The name is not activatable")))
>       :form
>       (equal
>        (dbus-error "org.freedesktop.DBus.Error.ServiceUnknown"
>        "The name org.gnu.Emacs.TestDBus was not provided by any
>        .service files")
>        (dbus-error "org.freedesktop.DBus.Error.ServiceUnknown"
>        "The name is not activatable"))
>       :value nil :explanation
>       (list-elt 2
> (arrays-of-different-length 70 27 "The name
> org.gnu.Emacs.TestDBus was not provided by any .service files"
> "The name is not activatable" first-mismatch-at 9))))
>
Your mileage may vary.

I'm starting to run out of ideas for additional tests.
Suggestions
welcome.

Cheers,
Hugh


0001-Property-tests-ERT.patch (12K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Michael Albinus
In reply to this post by Hugh Daschbach-2
Hugh Daschbach <[hidden email]> writes:

Hi Hugh,

>> Implementation is more complex than expected. Due to its nature,
>> org.freedesktop.DBus.Monitoring.BecomeMonitor requires another
>> (parallel) connection to the bus. This is not foreseen yet in
>> dbusbind.c;
>> will see how it could fly.
>>
>> What I could provide just now is an implementation which runs in
>> *another* Emacs instance. This could be used for monitoring only,
>> because it is another connection to the bus per definition. Are you
>> interested to get such a partial implementation?
>
> I'm interested in whatever you want to implement.  I see signature
> verification useful for testing rather than an exposed feature.
>
> From what I can see from looking at dbus-monitor output the correct
> property types are being exposed now.  It seems to be working.
>
> So whatever we approach we take, the benefit is early warning of
> future
> regressions.  You are a better judge of benefit of additional effort
> than I.
>
> A second Emacs instance seems to offer the same asynchronous output
> gathering issues that dbus-monitor poses.  It does eliminates the
> ad-hoc
> parser.
>
> If you have a longer term goal, I'd suggest pursuing that rather than
> something partial that you'll want to replace later.
>
> 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.

> 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.

> 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.

> Cheers,
> Hugh

Best regards, Michael.



Reply | Threaded
Open this post in threaded view
|

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

Michael Albinus
In reply to this post by Hugh Daschbach-2
Hugh Daschbach <[hidden email]> writes:

Hi Hugh,

>> I need more testing and a cleanup pass.  I'll pass along a better
>> version when
>> I think it's ready for review.
>>
>> I've started a few "should fail" tests.
>
> I've made a bit of progress.  I have a few tests that fail.  There
> doesn't seem to be any type checking on property set.  But have a look
> and see if you concur that these are real errors.  I've attached a
> patch.

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).

> In addition, I see a failure in dbus-test04-register-method:
>
>> (equal
>>  (dbus-error "org.freedesktop.DBus.Error.ServiceUnknown" "The name org.gnu.Emacs.TestDBus was not provided by any .service files")
>>  (dbus-error "org.freedesktop.DBus.Error.ServiceUnknown" "The name is not activatable"))

Oops, yes. The intention of this check is to see, whether
org.freedesktop.DBus.Error.ServiceUnknown is reported (it is). The
additional explanation doesn't matter, and it seems to be different
depending on the D-Bus daemon implementation (I'm using Fedora 32).

So I've pushed a fix, which checks just for the D-Bus error name, w/o
the additional text.

> 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.

> Cheers,
> Hugh

Best regards, Michael.



Reply | Threaded
Open this post in threaded view
|

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

Michael Albinus
In reply to this post by Hugh Daschbach-2
Hugh Daschbach <[hidden email]> writes:

Hi Hugh,

In general, your tests are very useful. Thanks!

Just some comments on your patch.

> Add tests that should fail, like setting a property with a type
> different from it's type at registration time.

As said the other message, this constraint doesn't exist any longer.
Registered services might want to control, which properties are
set. This could be the type of a property, or a restriction of the value
(for example, just a predefined set of strings could be allowed).

Maybe, we give these services such a mean? That is, they could add an
own handler function in dbus-register-property, which is applied when a
org.freedesktop.DBus.Properties.Set method call is handled. WDYT?

> +(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.

> +;; 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?

> +(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)

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)))

> +          (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.

> +
> +          (should                             ; Should this error instead?
> +           (equal
> +            (dbus-set-property
> +             :session dbus--test-service dbus--test-path
> +             dbus--test-interface "StringArray"
> +             '(:array "seven" "eight" :string "nine"))
> +            nil))

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.

> +        ;; Test mismatched types in array
> +
> +        (should                         ; Oddly enough, register works, but get fails
> +         (equal
> +          (dbus-register-property
> +           :session dbus--test-service dbus--test-path
> +           dbus--test-interface "MixedArray" :readwrite
> +           '(:array
> +             :object-path "/node00"
> +             :string "/node01"
> +             :object-path "/node0/node02"))
> +          `((:property :session ,dbus--test-interface "MixedArray")
> +    (,dbus--test-service "/org/gnu/Emacs/TestDBus"))))

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.

> +        (should-error
> +         (equal
> +          (dbus-get-property
> +           :session dbus--test-service dbus--test-path
> +           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?

> +        (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.

> +        ;; Test invalid type specification
> +
> +        (should
> +         (equal
> +          (dbus-register-property
> +           :session dbus--test-service dbus--test-path
> +           dbus--test-interface "InvalidType" :readwrite
> +           :keyword 128)
> +          `((:property :session ,dbus--test-interface "InvalidType")
> +    (,dbus--test-service "/org/gnu/Emacs/TestDBus"))))

Oops. This shall be detected in dbus-register-property.

> Cheers,
> Hugh

Best regards, Michael.



Reply | Threaded
Open this post in threaded view
|

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

Michael Albinus
Michael Albinus <[hidden email]> writes:

Hi Hugh,

>> + (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.

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.

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.

>> Cheers,
>> Hugh

Best regards, Michael.



Reply | Threaded
Open this post in threaded view
|

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

Hugh Daschbach-2
In reply to this post by Michael Albinus

Michael Albinus writes:

> Hugh Daschbach <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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

Michael Albinus
Hugh Daschbach <[hidden email]> writes:

Hi Hugh,

>>> +
>>> +          (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.

Well, I have adapted dbus-set-property and dbus-get-property to
propagate the errors.

>>> +        ;; 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.

Should be checked now. When dbus-register-property is called, it applies
internally a dbus-set-property or dbus-get-property now. As side effect,
the value is checked by the D-Bus daemon, and you shall see errors.

>> 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.

Of course I'm wrong, ":byte 1024" shall be the same as "byte 0".

> (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))

Of course 0. As said, I was wrong.

>  (dbus-unregister-service :session dbus--test-service))
>
> Should I update the expectation to zero?

Yes, please.

> Cheers,
> Hugh

Best regards, Michael.



Reply | Threaded
Open this post in threaded view
|

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

Michael Albinus
Hi Hugh,

just FTR, I have added test cases dbus-test01-basic-types and
dbus-test01-compound-types. They use the recently added function
dbus-check-arguments, which generates a new D-Bus message, but without
sending it. As side effect, we get errors from dbusbind.c in case of.

Best regards, Michael.



Reply | Threaded
Open this post in threaded view
|

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

Hugh Daschbach-2

Michael Albinus writes:

> Hi Hugh,
>
> just FTR, I have added test cases dbus-test01-basic-types and
> dbus-test01-compound-types. They use the recently added function
> dbus-check-arguments, which generates a new D-Bus message, but
> without
> sending it. As side effect, we get errors from dbusbind.c in
> case of.
>
> Best regards, Michael.
Thanks for the heads up.  I noticed the error messages.  Happy to
ignore
them.

I have attached two patches for your review.  I think the property
tests
are complete; I've adjusted the tests to expect errors on register
or
set rather than get.  The other patch is my first draft for
testing
introspection.

I could dig deeper into the dbus-introspect-get-interface, but
wanted to
come up for air first.  Let me know if you think it's worth the
effort
given the individual method, signal, and property tests.

And, of course, let me know what you think should be reworked.

Thanks,
Hugh



0001-Property-tests-ERT.patch (10K) Download Attachment
0002-Draft-introspection-tests.patch (14K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Michael Albinus
In reply to this post by Hugh Daschbach-2
Hugh Daschbach <[hidden email]> writes:

Hi Hugh,

> I have attached two patches for your review.  I think the property
> tests are complete; I've adjusted the tests to expect errors on
> register or set rather than get.

Thanks for this. AFAICS, there's nothing left open for bug#43252; I'd
like to close it. Is this OK for you?

> And, of course, let me know what you think should be reworked.

There are only some nits left to comment.

> Add DBus tests to validate property handling.  Includes cycling

We call this beast D-Bus, with a hyphen. Here and everywhere in the
docstrings and comments.

> register, get, set, get, GetAll, and GetManagedObjects over
> several property types.
>
> Add tests that should fail, like setting a property with a type
> different from it's type at registration time.

We add also a ChangeLog style entry to the commit message, see the git
log of dbus-tests.el.

> +The argument SELECTOR indicates whether the test should expand to
> +'dbus-register-property' (if SELECTOR is 'register) or

`dbus-register' (if SELECTOR is `register') or

> +(ert-deftest dbus-test06-test-property-types ()

The "-test" part of the name seems to be superfluous; I'd call it
dbus-test06-property-types. (You see, just nitpicks :-)

> +        (dbus--test-property
> +         "Dictionary"
> +         '((:array
> +            :dict-entry (:string "four" (:variant :string "value of four"))
> +            :dict-entry ("five" (:variant :object-path "/nodex"))
> +            :dict-entry ("six"  (:variant (:array :byte 4 :byte 5 :byte 6))))

This is one possibility to declare a :dict-entry. The other possibility,
with the same result, is

'((:array
   (:dict-entry :string "four" (:variant :string "value of four"))
   (:dict-entry "five" (:variant :object-path "/nodex"))
   (:dict-entry "six"  (:variant (:array :byte 4 :byte 5 :byte 6))))
...

Do you mind to test both?

> +        (should-error                   ; Cannot set property with :read access
> +         (equal
> +          (dbus-set-property
> +           :session dbus--test-service dbus--test-path
> +           dbus--test-interface "StringArray"
> +           '(:array "seven" "eight" :string "nine"))
> +          nil))

The error happens in dbus-set-property. No need to test further for
equal. So you could do

(should-error                   ; Cannot set property with :read access
 (dbus-set-property
  :session dbus--test-service dbus--test-path
  dbus--test-interface "StringArray"
  '(:array "seven" "eight" :string "nine")))

Furthermore, you could test which error is raised (should-error allows
this). Something like

(should-error                   ; Cannot set property with :read access
 (dbus-set-property
  :session dbus--test-service dbus--test-path
  dbus--test-interface "StringArray"
  '(:array "seven" "eight" :string "nine"))
 :type 'dbus-error)

Similar approach for your other should-error forms.

> +        ;; Test integer overflow

I don't see any integer *overflow* in the following tests.

> Thanks,
> Hugh

Best regards, Michael.



Reply | Threaded
Open this post in threaded view
|

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

Michael Albinus
In reply to this post by Hugh Daschbach-2
Hugh Daschbach <[hidden email]> writes:

Hi Hugh,

> I could dig deeper into the dbus-introspect-get-interface, but wanted
> to come up for air first.  Let me know if you think it's worth the
> effort given the individual method, signal, and property tests.

Thanks for this!

It is already very comprehensive (and takes 2 seconds on my laptop, more
than all other tests together). So I guess we could take it as is, until
new tests are triggered by errors in the wild.

OTOH, I don't mind to give this test the :expensive-test tag. Then it
doesn't matter how long it runs.

Given the time it consumes, there might be a need to cache introspection
data. Either the result of dbus-introspect or dbus--parse-xml-buffer, I
guess rather the latter. Do you want to investigate it in dbus.el?

> And, of course, let me know what you think should be reworked.

Here we are. I don't repeat general comments I have given the other review.

> +(defun dbus--test-introspect ()
> +  "Return test introspection string."
> +  "<?xml version=\"1.0\"?>

...

Well, this is one approach. Alternatively, we could regard the
introspection file as test data, which is located in a file called
.../test/lisp/net/dbus-tests/org.gnu.Emacs.TestDBus.xml. This function
(the handler for the Introspect method) would read the file, and return
its contents.

> +(defsubst dbus--test-examine-interface (iface-name
> +                                        expected-properties
> +                                        expected-methods
> +                                        expected-signals
> +                                        expected-annotations)

This is rather C-style of argument indentation. In ELisp, we do
something like

(defsubst dbus--test-examine-interface
  (iface-name expected-properties expected-methods
   expected-signals expected-annotations)
...)

> +  (let ((interface (dbus-introspect-get-interface
> +                    :session
> +                    dbus--test-service
> +                    dbus--test-path
> +                    iface-name)))

A similar comment applies.

> +    (should-not (equal expected nil))

This is (should expected)

> +  (unwind-protect
> +      ;; dbus-introspect-get-node-names
> +      (should
> +       (equal
> +        (dbus-introspect-get-node-names :session dbus--test-service dbus--test-path)
> +        '("node0" "node1")))

A (progn ... is missing after unwind-protect.

> +       '("org.freedesktop.DBus.Deprecated")))

Hmm. Maybe we shall give "org.freedesktop.DBus.Deprecated" a defconst in
dbus.el?

> Thanks,
> Hugh

Best regards, Michael.



Reply | Threaded
Open this post in threaded view
|

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

Michael Albinus
Hugh Daschbach <[hidden email]> writes:

Hi Hugh,

>> Thanks for this. AFAICS, there's nothing left open for bug#43252; I'd
>> like to close it. Is this OK for you?
>
> Yes.  I was about to suggest this.  The issue is resoled.

Closed.

> Cheers,
> Hugh

Best regards, Michael.



Reply | Threaded
Open this post in threaded view
|

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

Michael Albinus
In reply to this post by Michael Albinus
Hugh Daschbach <[hidden email]> writes:

Hi Hugh,

> I've taken a shot at this.  I'm not sure I got the ChangeLog style
> correct.  Let me know if I'm still off the beaten path.

Almost. See comment below.

>> `dbus-register' (if SELECTOR is `register') or
>
> Fixed.

Almost. See comment below.

>> Well, this is one approach. Alternatively, we could regard the
>> introspection file as test data, which is located in a file called
>> .../test/lisp/net/dbus-tests/org.gnu.Emacs.TestDBus.xml. This
>> function (the handler for the Introspect method) would read the file,
>> and return its contents.

Oops, I'm mistaken here. The directory shall be called dbus-resources,
see .../test/file-organization.org. Sorry.

> Is:
>
> (let* ((property
>          (dbus-introspect-get-property
>           :session
>           dbus--test-service
>           dbus--test-path interface
>           property-name))) ...)
>
> preferred over:
>
> (let* ((property
>          (dbus-introspect-get-property :session
>          dbus--test-service
>           dbus--test-path interface property-name)))
>    ...)
>
> If not, I'll take another bite at the apple.

I'd vote for the latter, with the first argument :session preceding
the other arguments in the same line. Like

(let* ((property
        (dbus-introspect-get-property
         :session dbus--test-service
         dbus--test-path interface property-name)))
   ...)

If all arguments fit into 80 columns, do it.

The "canonical" formatting is offered by the `pp' command (pretty
printer). I must confess, that I do not follow all its recommendations.

> Done.  ’dbus-annotation-deprecated’.  Let me know if you think this
> should be
> ‘dbus--annotation-deprecated’ instead.

No, the former is OK. People might want to use it in their packages.

> In other news, this test:
>
>  (should-error
>   (dbus-check-arguments :session dbus--test-service :unix-fd 10)
>   :type 'dbus-error)
>
> works for me in batch mode, but not interactive mode.
>
> On GNU/Linux, (dired (format "/proc/%s/fd" (emacs-pid))) indicates I
> have 14 open file descriptors on a test instance (emacs -q -Q). On my
> development instance, I've got 27 open file descriptors.

I see. Will investigate.

And now my review for the latest patch versions.

> From be45a75b315e56649fa9e39d199fe5e2b50bf69d Mon Sep 17 00:00:00 2001
> From: Hugh Daschbach <[hidden email]>
> Date: Tue, 22 Sep 2020 19:36:20 -0700
> Subject: [PATCH 1/2] Add D-Bus property tests.
>
> * test/lisp/net/dbus-tests.el: Add property tests.
> (dbus--test-run-property-test) (dbus--test-property)
> (dbus-test06-property-types): Test property registration, set, get.

If you have one comment for several functions, use only one parenthesis
per line like

(dbus--test-run-property-test, dbus--test-property)
(dbus-test06-property-types): Test property registration, set, get.

> +(defsubst dbus--test-run-property-test (selector name value expected)
> +  "Generate a property test: register, set, get, getall sequence.
> +This is a helper function for the macro `dbus--test-property'.
> +The argument SELECTOR indicates whether the test should expand to
> +'dbus-register-property' (if SELECTOR is `register') or

`dbus-register-property' (if SELECTOR is `register') or

> +         '((:array
> +            :object-path "/node00"
> +            :object-path "/node01"
> +            :object-path "/node0/node02") .
> +            ("/node00" "/node01" "/node0/node02"))

If a cons cell doesn't fit into one line, you shall move the dot "." to
the beginning of the next line, like

(dbus--test-property
 "ObjectArray"
 '((:array
    :object-path "/node00"
    :object-path "/node01"
    :object-path "/node0/node02")
   . ("/node00" "/node01" "/node0/node02"))

I bet the dot "." has the font-lock-warning-face (red foreground color)
in your buffer. Admittedly, it doesn't look prominent :-(

Move the cursor over the misplaced dot; there shall be a help message about.

> +    ;; Cleanup.
> +    (message "cleanup")
> +    (dbus-unregister-service :session dbus--test-service)))

I don't believe we need this message. We see that we're done :-)

> From 3efb1b38d75572b14ac0526dbd79769d6fa89d10 Mon Sep 17 00:00:00 2001
> From: Hugh Daschbach <[hidden email]>
> Date: Mon, 21 Sep 2020 17:12:49 -0700
> Subject: [PATCH 2/2] Add D-Bus Introspection tests.
>
> * lisp/net/dbus.el (new defconst): D-Bus deprecation name.
>
> * test/lisp/net/dbus-tests.el  (dbus--tests-dir)
> (dbus--test-introspect) (dbus--test-examine-interface)
> (dbus--test-validate-annotations) (dbus--test-validate-property)
> (dbus--test-validate-m-or-s) (dbus--test-validate-signal)
> (dbus--test-validate-method) (dbus-test07-introspection): new tests.

That's not true. All but the last functions are new defuns, not new
tests. And please start the explanation with a capital letter, like "New test."

> +(defconst dbus-annotation-deprecated (concat dbus-interface-dbus ".Deprecated")
> +  "An annotation value indicating a deprecated interface, method, signal, or property.")

The docstring line does not fit 80 chars. What about

"An annotation indicating a deprecated interface, method, signal, or property."

> +(defvar dbus--tests-dir
> +  (file-truename
> +   (expand-file-name "dbus-tests"
> +                     (file-name-directory (or load-file-name
> +                                              buffer-file-name))))
> +  "Directory containing test data files.")

As said, it must be "dbus-resources".

> +(ert-deftest dbus-test07-introspection ()
> +  :tags '(:expensive-test)
> +  "Register an Introspection interface then query it."

:tags must be after the docstring.

> Cheers,
> Hugh

Best regards, Michael.



Reply | Threaded
Open this post in threaded view
|

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

Hugh Daschbach-2

Michael Albinus writes:

> Hugh Daschbach <[hidden email]> writes:
>
> Hi Hugh,
>
> Oops, I'm mistaken here. The directory shall be called
> dbus-resources,
> see .../test/file-organization.org. Sorry.

No worries.  Done.

> I'd vote for the latter, with the first argument :session
> preceding
> the other arguments in the same line. Like
>
> (let* ((property
>         (dbus-introspect-get-property
> :session dbus--test-service
> dbus--test-path interface property-name)))
>    ...)

I think I've addressed this.  I reformatted a few tests that were
similarly indented.

> If you have one comment for several functions, use only one
> parenthesis
> per line like
>
> (dbus--test-run-property-test, dbus--test-property)
> (dbus-test06-property-types): Test property registration, set,
> get.

Done.  I think.


> `dbus-register-property' (if SELECTOR is `register') or

Done.  Apologies for not picking up the quoting issue the first
time you
mentioned it.

> I bet the dot "." has the font-lock-warning-face (red foreground
> color)
> in your buffer. Admittedly, it doesn't look prominent :-(
>
> Move the cursor over the misplaced dot; there shall be a help
> message about.

Done.

> I don't believe we need this message. We see that we're done :-)

Gone.

>> * lisp/net/dbus.el (new defconst): D-Bus deprecation name.
>>
>> * test/lisp/net/dbus-tests.el  (dbus--tests-dir)
>> (dbus--test-introspect) (dbus--test-examine-interface)
>> (dbus--test-validate-annotations)
>> (dbus--test-validate-property)
>> (dbus--test-validate-m-or-s) (dbus--test-validate-signal)
>> (dbus--test-validate-method) (dbus-test07-introspection): new
>> tests.

I think I understand the ChangeLog format.  Additional corrections
welcome.

> The docstring line does not fit 80 chars. What about
>
> "An annotation indicating a deprecated interface, method,
> signal, or property."

Done.

>> +(ert-deftest dbus-test07-introspection ()
>> +  :tags '(:expensive-test)
>> +  "Register an Introspection interface then query it."
>
> :tags must be after the docstring.

Corrected.

>> Cheers,
>> Hugh
>
> Best regards, Michael.

I think I've addressed all the issues you pointed out.  Let me
know if
there's something that still doesn't look right.

Thanks
Hugh


0001-Add-D-Bus-property-tests.patch (15K) Download Attachment
0002-Add-D-Bus-Introspection-tests.patch (15K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Michael Albinus
Hugh Daschbach <[hidden email]> writes:

Hi Hugh,

> I think I've addressed all the issues you pointed out.  Let me know if
> there's something that still doesn't look right.

I've roughly scanned the patches, they look almost OK. Let's wait now
for appearing your name on the copyright file.

Anyway, here's my nit of the day:

> * test/lisp/net/dbus-tests/org.gnu.Emacs.TestDBus.xml: New test data.

This shall be dbus-resources.

> Thanks
> Hugh

Best regards, Michael.



Reply | Threaded
Open this post in threaded view
|

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

Hugh Daschbach-2

Michael Albinus writes:

> Hugh Daschbach <[hidden email]> writes:
>
> Hi Hugh,
>
>> I think I've addressed all the issues you pointed out.  Let me know if
>> there's something that still doesn't look right.
>
> I've roughly scanned the patches, they look almost OK. Let's wait now
> for appearing your name on the copyright file.
>
> Anyway, here's my nit of the day:
>
>> * test/lisp/net/dbus-tests/org.gnu.Emacs.TestDBus.xml: New test data.
>
> This shall be dbus-resources.

Thanks.  Fixed.  Will wait for the paperwork to clear.  Then will rebase
and send you a final copy of the patches.

>> Thanks
>> Hugh
>
> Best regards, Michael.

I've taken a brief look at performance of the introspection tests.  The
long pole seems to be ERT, rather than the tests themselves.

Running the profiler while running ert showed The cpu profiler report
for running dbus-test07-introspection breaks down something like:

- GC      - 30%
- dbus-*  - 30%
- ert-*   - 40%

So I pulled introspection tests out into a separate file, redefined
`should', and reran dbus-test07-introspection.  It completed almost
instantly.  I wrapped the body of dbus-test07-introspection in a dotimes
form with 50 iterations.  That completes in roughly 1.1 seconds.

Comparing runtime and GC cycles for a single run,

running with ERT:  2 gcs in 2.228295 sec
running w/o ERT:   1 gcs in 0.036733 sec

Those numbers come from:

(let ((start (current-time))
      (gcs gcs-done))
  ;; test program call
  (message "%d gcs in %02f sec" (- gcs-done gcs) (float-time (time-since start))))

I'm not well versed in chasing Emacs performance issues, bit this looks
to me more like testing than introspection overhead.  Any
suggestions?


Cheers,
Hugh



Reply | Threaded
Open this post in threaded view
|

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

Michael Albinus
Hugh Daschbach <[hidden email]> writes:

Hi Hugh,

> Fixing that, elp helped me find that the performance hit came from
> mishandling incoming introspection requests in ‘dbus--test-introspect’.

Good to know. Introspection isn't to punish :-)

> I hadn’t registered ‘dbus--test-introspect’ for the two subnodes exposed
> in the xml file.  So introspecting those nodes timed out.  Each timeout
> cost a second.

OK. But maybe you add also one or two tests to check this error functionality.

> I’ve fixed this for the next patch set.  The change was simple enough.
> I think that can wait until the paperwork clears, unless you want an
> earlier look.

Sure, this can wait.

Btw, I have just pushed a larger patch to master. It implements a first
shot on the org.freedesktop.DBus.Monitoring.BecomeMonitor
functionality. If you want to see how it works, just call
(dbus-register-monitor :session) . There will be a new buffer *D-Bus
Monitor*, which shows you D-Bus messages similar to what the UNIX
command "dbus-monitor --session" does. The point is that you can
register your own handler to the monitor; if you don't declare an own
handler, the default handler `dbus-monitor-handler' is taken.

As said, it is just a first shot. Proper documentation is missing, and
not all arguments of `dbus-register-monitor' seem to work as
expected. But you might see which direction it will go; any feedback is
appreciated.

> Cheers,
> Hugh

Best regards, Michael.



Reply | Threaded
Open this post in threaded view
|

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

Michael Albinus
Hugh Daschbach <[hidden email]> writes:

Hi Hugh,

> A new dbus-test07-timeout tests both implicit (dbus-introspect-*) and
> explicit (dbus-call-method) timeouts.  It is tagged :expensive-test.

I like the idea of having test groups (identified by test<NN>). This
makes it more easy to select only tests I'm interested in.

Could you, pls, move the dbus-call-method timeout test into a test of
its own, dbus-test04-call-method-timeout? And the other test might be
called dbus-test07-introspect-timeout.

> Timestamps (perhaps optional) would be useful, I think.

Added. In case of method-return and error messages, I've added also the
time difference to the corresponding method-call.

And while I'm there, I've added also links between serial numbers. That
is, if you click on the serial number of a message-return or error
message, you jump to the corresponding methoid-call. And vice-versa.

> Cheers,
> Hugh

Best regards, Michael.



Reply | Threaded
Open this post in threaded view
|

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

Michael Albinus
Hugh Daschbach <[hidden email]> writes:

Hi Hugh,

>> Could you, pls, move the dbus-call-method timeout test into a test of
>> its own, dbus-test04-call-method-timeout? And the other test might be
>> called dbus-test07-introspect-timeout.
>
> Sure.  I've attached a draft for your review.

Thanks. LGTM.

> Cheers,
> Hugh

Best regards, Michael.



123