search

storaged-project / udisks

The UDisks project provides a daemon, tools and libraries to access and manipulate disks, storage devices and technologies.
https://storaged.org/doc/udisks2-api/latest/
Other
349 stars 142 forks source link

Dbus object state incorect on method return #117

Open tasleson opened 8 years ago

tasleson commented 8 years ago

I saw this commit: 86492479b170eeb587bb734fedee6d823b360188 when I was looking for something else, which is concerning. If a client API user makes a state change, by the time we return from the method (re-size) the state of the dbus object should reflect the new value(s).

We cannot expect clients to place arbitrary sleeps in their code. Additionally, I don't understand why this lvm test case is calling udev settles.

dwlehman commented 8 years ago

I tend to agree that it is asking a lot of clients to add udev settle or sleep calls like this. @vpodzime, @vojtechtrefny is this related to the issues you've been seeing while running storaged tests?

tsmetana commented 8 years ago

The udev settles might be unnecessary. I lived under the impression that those operations create/modify devices and result in udev events. They should be harmless though.

The sleep should not be necessary and it actually was not there originally: I have added it only after the test failed from time to time on one of the testing VMs. It might also be an issue of the python dbus, not storaged itself.

tasleson commented 8 years ago

Oh I'm sure udev events are occurring quite a bit :-) I've used the same dbus python library a lot and have not experienced this type of issue.

I believe the test cases should closely resemble what we would expect a dbus interface user to use, which would include no settles and sleeps. They both mask issues with the service.

In addition to the dbus properties being correct before returning from a dbus method, I also believe if storaged is creating something which ends up showing up in FS, that storaged should ensure that it exists there as well, before returning back to the client. It's entirely possible that a udev settle might be needed in the implementation of the service.

We should also be testing that when a signal is received that the client can immediately access any remote dbus objects & properties and items on the FS too. To make sure the service has things registered and updated within itself before it broadcasts that it's available.

vpodzime commented 8 years ago

I tend to agree that it is asking a lot of clients to add udev settle or sleep calls like this. @vpodzime, @vojtechtrefny is this related to the issues you've been seeing while running storaged tests?

I'm afraid udev settle is used in many places as a variant of "add some sleep here". Which shouldn't be necessary, of course, but we are not there yet.