-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support sdbus clients #54
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
various code style comments
I cannot confirm that the patch fixes the issue for me. |
Hi Sophie, can you please tell me how to reproduce the issue? |
You can use this flatpak https://github.com/zecakeh/zbus-flatpak-portal, you might have to run |
If you just use
Where you can use any id of an installed app instead of app.drey.Warp. This then gives
(Sorry if this is all obvious for you. Just wanted to make sure you can reproduce it.) |
Note that zbus 4.2.1 includes a hack to workaround this issue, so you would have to make sure it is using zbus 4.2.0 |
Thanks for your help! I added one more commit (I guess it can be squashed with the previous ones, but for now I'll keep it separate as the commit message sheds some light on it). Now I can run the I added one more member ( I didn't want to make a too invasive change, but if you agree I'd rename the members like this (suggestions welcome!):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest version does seem to fix the problems for me!
@mardy could you maybe address the small review comments that are open such that we can hopefully move forward with this soon? :)
Maintainers have been very open to making the code clearer. However, maybe you can add these changes as an additional commit to simplify the review? |
This contains commits which are broken by themselves. Can you please squash thing properly so that each commit is working? |
@@ -298,6 +298,7 @@ struct FlatpakProxyClient | |||
AuthState auth_state; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what this commit changes. What is being fixed up exactly?
By "broken" you mean "not building" or "not fixing the problem"? If it's the former, then I guess the problem is the warning/error mentioned by Sophie (which I will fix); if the latter, then I guess I should just squash all commits into one, because only the last commit fixes the issue for good. |
I pushed some lightly tested fixes for the issues I found. Please have a look at these. I'd like to not have any individual commits in the tree where things are broken, so that bisect works, so most commits here should imho be squashed into one commit. However, I made them separate for easier review. |
The changes look fine to me. |
sd-bus sends the BEGIN command in at the same time as the AUTH command, assuming that the authentication will succeed. This ends up confusing xdg-dbus-proxy, as it assumes that BEGIN is not sent at the same time as any other messages. As a result, the server's authentication replies end up interpreted as standard D-Bus messages, failing parsing and resulting in the client being unable to connect. This changes the code to keep track of the number of authentication requests and the number ofresponses from the server to know when the authentication phase of the connection has actually completed. Until the authentication is completed, hold off all client messages received after BEGIN. We only send to the server the authentication commands up (and including) the BEGIN command, but everything past the line terminators is queued into the `extra_input_data` buffer and we stop reading from the client socket. Once the authentication is completed, the (partial) message we saved into `extra_input_data` is queued for the D-Bus server into the first outgoing buffer, and reading from the client socket resumes. Note that in order to cleanly process the partial sendind of data, another offset is introduced into the Buffer structure, which holds the bytes of buffers which have already been sent over the socket. Fixes flatpak#21 (finally!) This is based on initial work by: * Ryan Gonzalez <ryan.gonzalez@collabora.com> * Alberto Mardegan <a.mardegan@omp.ru>
This way we can avoid multiple reads in the pipelined case.
We indent one tab less by moving the check for buffer->pos == 0 to a separate check.
ddb0ed8
to
5073e0c
Compare
I found one more issue in testing, but now it seems to work, so I squashed the functional changes into one commit (with props to ryan and alberto). I think this is fine to go, but please take a second look and test it. |
Hard to see what changed because of the indentation changes and rebase. |
The only real change was to the handling of auth messages. We now delay the setting of AUTH_COMPLETE to after calling got_buffer_from_side() as that would otherwise try to parse the "BEGIN" auth line as a dbus message. See the new_auth_state variable. But, it wouldn't be a bad idea to do a more complete review of the combined change, to ensure fresh eyes on this. |
I've taken another look and didn't find anything that stood out. @mardy can you also please take another look? |
Will do tomorrow :-) |
I confirm that this continues to work for me :-) |
merging then |
This is built on top of #26. I've kept the original commit by @refi64 and added one to address the issue raised during the review.
Whether my commit actually succeeds in addressing that problem is something I'm not sure of, since I couldn't actually reproduce the original issue. But from my first tests it appears that sdbus clients can connect and send methods calls.
Possible fix for #21