Follow-up from "Add windows implementation of dbus-run-session helper tool"
The following discussions from !7 (closed) should be addressed:
-
@smcv started a discussion: (+1 comment) Why does a change to dbus-run-session require changing the libraries that are linked into a manual test?
-
@smcv started a discussion: Same
-
@smcv started a discussion: Same
-
@smcv started a discussion: Don't expose this directly. dbus-run-session should call
_dbus_hash_table_unref()
instead - that's alreadyDBUS_PRIVATE_EXPORT
.In general, a particular data structure should either be refcounted (has
_ref
and_unref
, likeDBusHashTable
) or not refcounted (has_free
, likeDBusString
). A mixture of the two means we got it wrong somewhere. -
@smcv started a discussion: If this is only used in dbus-run-session, I'd prefer to put it in dbus-sysdeps-win-util.c, which means it doesn't need to be
DBUS_PRIVATE_EXPORT
and isn't part of the shared library. -
@smcv started a discussion: Coding style: align pointer stars with the name, not the type, because that's how C precedence works (
char *a, b;
is the same aschar *a; char b;
, but if you write it aschar* a, b
then you might be misled into thinking it's the same aschar *a; char *b;
). -
@smcv started a discussion: (+1 comment) Can we continue to link with
libdbus-1.la
on Unix, and only uselibdbus-internal.la
on Windows? -
@smcv started a discussion: (+1 comment) Coding style: space before open parenthesis
-
@smcv started a discussion: (+3 comments) This is a bit of a weird setup; I'll have to look at the old Bugzilla bug to refresh my memory of how we got here.
-
@smcv started a discussion: Coding style nitpick:
if (!result)
-
@smcv started a discussion: server_handle
andapp_handle
could be uninitialized at this point. If I'm remembering correctly that aHANDLE
is a pointer, use:HANDLE server_handle = NULL; ... out: ... if (server_handle != NULL) CloseHandle (server_handle); ...
-
@smcv started a discussion: dbus_free
should already be aDBusFreeFunction
, so you shouldn't need this cast. -
@smcv started a discussion: If you
goto out
after this point, you'll double-freeenv
. -
@smcv started a discussion: If you
goto out
early,env_table
will be uninitialized. Initialize it toNULL
and only free it if it's non-NULL
. -
@smcv started a discussion: (+1 comment) Is this relevant to the rest of the commit?
-
@smcv started a discussion: (+1 comment) config_file
defaults toNULL
, which means "use the standard session bus config file", so this needs to be something likeif (config_file != NULL) _dbus_string_append_printf (&argv_strings[1], "--config-file=%s", config_file); else _dbus_string_append_printf (&argv_strings[1], "--session");
to match the Unix implementation.