Support sdbus clients

[mardy]

This is built on top of https://github.com/flatpak/xdg-dbus-proxy/pull/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

[sophie-h]

I cannot confirm that the patch fixes the issue for me.

[mardy]

I cannot confirm that the patch fixes the issue for me.

Hi Sophie, can you please tell me how to reproduce the issue?

[bilelmoussaoui]

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

[sophie-h]

You can use this flatpak https://github.com/zecakeh/zbus-flatpak-portal

If you just use cargo run outside a Flatpak it should run properly. Afterwards, you can run it from a flatpak by

flatpak run --filesystem=host --command=./target/debug/zbus-flatpak-portal app.drey.Warp

Where you can use any id of an installed app instead of app.drey.Warp.

This then gives

Creating connection…
thread 'main' panicked at src/main.rs:7:60:
called `Result::unwrap()` on an `Err` value: InputOutput(Custom { kind: BrokenPipe, error: "failed to read from socket" })
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

(Sorry if this is all obvious for you. Just wanted to make sure you can reproduce it.)

[bilelmoussaoui]

You can use this flatpak https://github.com/zecakeh/zbus-flatpak-portal

If you just use cargo run outside a Flatpak it should run properly. Afterwards, you can run it from a flatpak by

flatpak run --filesystem=host --command=./target/debug/zbus-flatpak-portal app.drey.Warp

Where you can use any id of an installed app instead of app.drey.Warp.

This then gives

Creating connection…
thread 'main' panicked at src/main.rs:7:60:
called `Result::unwrap()` on an `Err` value: InputOutput(Custom { kind: BrokenPipe, error: "failed to read from socket" })
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

(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

[mardy]

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 zbus-flatpak-portal program.

I added one more member (sent) to the Buffer structure. It may be possible to do without it, but the code was hard to understand because the pos and size members were being used with a different meaning depending on whether we were reading from or writing to the buffer; and the fact that we pass buffers from one socket to the other increases on this confusion.

I didn't want to make a too invasive change, but if you agree I'd rename the members like this (suggestions welcome!):

• size -> capacity ("size" can also be OK, if you insist :-) )
• pos -> "length", "used", "data_size"?
• sent -> keep as is?

[sophie-h]

I didn't want to make a too invasive change, but if you agree I'd rename the members like this (suggestions welcome!):

• size -> capacity ("size" can also be OK, if you insist :-) )
• pos -> "length", "used", "data_size"?
• sent -> keep as is?

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?

[swick]

This contains commits which are broken by themselves. Can you please squash thing properly so that each commit is working?

[mardy]

This contains commits which are broken by themselves. Can you please squash thing properly so that each commit is working?

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.

[alexlarsson]

I pushed some lightly tested fixes for the issues I found. Please have a look at these. Otherwise this looks good to me.

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.

[swick]

The changes look fine to me.

[alexlarsson]

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.

[swick]

Hard to see what changed because of the indentation changes and rebase.

[alexlarsson]

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.

[swick]

I've taken another look and didn't find anything that stood out.

@mardy can you also please take another look?

[mardy]

@mardy can you also please take another look?

Will do tomorrow :-)

[mardy]

@mardy can you also please take another look?

Will do tomorrow :-)

I confirm that this continues to work for me :-)

[alexlarsson]

merging then