Thanks @ricotz ! That's perfect indeed - keeps the (better) Vala API and fixes the warning.
Hmm, but string
is a C string and thus null-terminated, no? That is most certainly not the case for this API, that should be purely binary. I.e. in some sense Vala is correct and glib is wrong, this really should be uint8[]
. But oh well..
backwards compatibility would break I think
Yes, unfortunately. Not sure what to do about this, is there some versioning in the API somewhere? Although compatibility has broken several times in the past in e.g. vte291 bindings. This is really a "between a rock and a hard place" situation
Socket.receive() declares its buf
argument as uint8[]
. But the underlying g_socket_receive() declares it as gchar*
, which is signed by default. This causes compiler warnings like
./../source/tests/test-umockdev-record.vala:698:36: error: pointer targets in passing argument 2 of ‘g_socket_receive’ differ in signedness [-Werror=pointer-sign]
698 | ssize_t len = conn.receive (buf);
| ^~~~~~
| |
| guint8 * {aka unsigned char *}
/usr/include/glib-2.0/gio/gsocket.h:209:83: note: expected ‘gchar *’ {aka ‘char *’} but argument is of type ‘guint8 *’ {aka ‘unsigned char *’}
The same happens for Socket.send().
Declaring the buffer as char[]
in Vala makes things worse:
../../source/tests/test-umockdev-record.vala:714.44-714.46: error: Argument 3: Cannot convert from `unowned uint8[]' to `unowned char[]'
714 | len = read_buf_delay (20000, conn, buf, 6);
| ^~~
Note: I report this so late as until recently, meson hid all C compiler warnings generated from Vala code. But that changed recently.
Thanks @smcv ! The dbusmock fix is in upstream release 0.30.2, I also just uploaded it to Debian unstable and Fedora, in particular the rawhide update matters for the pipeline here. It ought to work tomorrow-ish when it propagated to the machinery and mirrors.
I didn't look into this at all (EOY holiday season), but it's almost surely related to https://github.com/martinpitt/python-dbusmock/pull/184 , a behaviour change with the private D-Bus instances which was discussed here and in the other threads: Now every test class starts its own D-Bus instance, previously there was only a single global instance. So bisecting should start with trying to revert https://github.com/martinpitt/python-dbusmock/pull/184/commits/78df3c594751599563e359ad02f4532e40277e34 and https://github.com/martinpitt/python-dbusmock/pull/184/commits/6b6d5624331c1116f8a6ddc89bdaefbca4f6aa64 . Possibly https://github.com/martinpitt/python-dbusmock/pull/184/commits/47adffe3fbb02cea303e84bc3782393f6d891259 also broke something in a more subtle way.
Wohoo, thanks @pwithnall ! Much appreciated.
FTR: I cherry-picked the glib branch onto Fedora 37's glib 2.74.1 version (applies cleanly), uploaded it to my test COPR], rpm-ostree override replace
'd it on our current Fedora CoreOS image, and ran the Cockpit tests. They work fine now.
That makes sense -- right, I don't want to rush it, just asking for planning if it's "Fedora 38+ only" or has a chance to reach 37 in a few months or so. So far we only tested the fix with our Cockpit integration tests -- they do cover a fair bit of API in general, but this specific issue only happens against "direct address" services, and they are quite sparse. The only two examples that we found are rpm-ostreed (which is the reason that set off this whole debugging session and this PR) and subscription-manager (but that's Python and thus not affected).
So, let's get this into a devel release and F38/rawhide first. Thanks!
Is there anything that I can do to get this into the 2.74.x stable series? Propose it as a backport MR or so? I'm interested in getting this fix into Fedora 37 and then Fedora CoreOS, as it's blocking our current Cockpit project.
Yay, thanks for fixing CI and the fast review!
Martin Pitt (8f02681f) at 15 Feb 16:29
No worries, thanks for the pointers! So we'll ignore that particular failure for now, as it's truly independent.
debian-stable repeatedly failed the glib:gio / socket
check. It feels like this should be unrelated, as this only touches gdbus. Other MRs were at least landed green. Is this a known flake? Retry ahoi?
The previous style-check failure was obvious enough,but I cannot interpret this failure of style-check-optional. git diff
failed because there is (presumably) no diff? Is that fatal?
Martin Pitt (22da9ce0) at 15 Feb 10:31
gdbus: Never buffer reads during server authentication
Fixed stylelint error.
Martin Pitt (701e4ee1) at 15 Feb 09:27
gdbus: Never buffer reads during server authentication
I created a proper MR for this: !3272 (merged)
This was already reported in several other places:
This was refused (and in our opinion, rightly so) to get worked around in systemd's sd-bus, so let's fix this in gdbus properly.
Otherwise, the content of the buffer is thrown away when switching from reading via a GDataInputStream to unbuffered reads when waiting for the "BEGIN" line.
(The code already tried to protect against over-reading like this by using unbuffered reads for the last few lines of the auth protocol, but it might already be too late at that point. The buffer of the GDataInputStream might already contain the "BEGIN" line for example.)
This matters when connecting a sd-bus client directly to a GDBus client. A sd-bus client optimistically sends the whole auth conversation in one go without waiting for intermediate replies. This is done to improve performance for the many short-lived connections that are typically made.
I am sending this for my colleague @mvollmer. He originally sent this to https://github.com/GNOME/glib/pull/38 , but apparently has some troubles with his GNOME gitlab account.
Fixes #2916