Bug 22706

Updated now with test cases &c.

http://git.collabora.co.uk/?p=user/davyd/telepathy-glib-davyd.git;a=shortlog;h=refs/heads/gnio-utils

As discussed on IRC, we cannot merge this until it is acceptable to have a hard dependency on a GLib version with GNIO. (Library ABI changing depending on the dependencies against which the library was compiled isn't acceptable.)

Noted. Is there any way we can mark this bug to pop up when we're ready to bump the GLib version so that we don't forget about it?

It occurred to me that since Wocky uses GNIO, these utilities could also be useful in places like Gabble.

Now at http://git.collabora.co.uk/?p=user/danni/telepathy-glib.git;a=shortlog;h=refs/heads/gnio-utils

Someone could review this now, I just pushed a GLib 2.22 dependency to git master for 0.11. Danni, any chance you could sort out any necessary merging/rebasing first?

Rebased!

>  <SECTION>
> +<FILE>gnio-util</FILE>
> +<TITLE>gnio-util</FILE>
> +<INCLUDE>telepathy-glib/gnio-util.h</INCLUDE>
> +tp_g_socket_address_from_variant
> +tp_address_variant_from_g_socket_address
> +</SECTION>

Recommend including this via <telepathy-glib/telepathy-glib.h>, perhaps?

I'd somewhat prefer it if the "public face" of this functionality was via
dbus.h, since it's part of the dbus-glib glue layer (it implicitly depends
on dbus-glib's representations of types). You could even make it an error to
include gnio-util.h directly, as seen in verify.h.

> +GSocketAddress *tp_g_socket_address_from_variant (TpSocketAddressType type,
> +                                                  const GValue *variant);
> +GValue *tp_address_variant_from_g_socket_address (GSocketAddress      *address,
> +                                                  TpSocketAddressType *type);

G_WARN_UNUSED_RESULT on these, please (they'll leak if the result is ignored).

> +#include <telepathy-glib/gnio-util.h>

This should be the first thing included after config.h.

> +GSocketAddress *
> +tp_g_socket_address_from_variant (TpSocketAddressType type,
> +                                  const GValue *variant)

I don't like this function's error behaviour (anything wrong with the GValue
is a critical warning). It should survive being given a bad value over D-Bus -
we don't want the caller to have to pre-check.

Perhaps it could be documented to return NULL without a warning for
unparseable data, instead; add a GError ** if it seems to you to make sense.

The error path should also have some tests I'm afraid.

> +            switch (g_inet_address_get_family (addr))
> +              {
> +                case G_SOCKET_FAMILY_IPV4:
> +                  type_ = TP_SOCKET_ADDRESS_TYPE_IPV4;
> +                  variant = tp_g_value_slice_new (TP_STRUCT_TYPE_SOCKET_ADDRESS_IPV4);
> +                  break;

To be honest I'd prefer an assertion that TP_SOCKET_ADDRESS_TYPE_IPV4 and
TP_SOCKET_ADDRESS_TYPE_IPV6 are numerically the same type (they are), then
using a variable containing one or the other subsequently.

> +            array = g_value_array_new (2);

Please use tp_value_array_build() for this, now that we have it.

> +      default:
> +        g_return_val_if_reached (NULL);

Again, I'd prefer a non-criticalling failure mode - it's possible that GLib
can introduce a new socket type that we don't support, and callers of this
function shouldn't be expected to know ahead of time what we do and don't
support - and it should be tested.

> +  if (type) *type = type_;

if (type != NULL)
  *type = type_;

Also, I'd much prefer type to be called ret_type or something, so type_ can be
renamed to type. Variables differing only in underscores scare me.

> +#define ABST_ADDR "\000123456"
> +#define ABST_ADDR_LEN 7

"\000" "123456" or "\x00pqrstu" or something would be easier for readers to
parse, I think.

I'd like ABST_ATTR to have some specified trailing rubbish -
"\x00pqrstuXYZ\x00" or something - to be able to assert that XYZ is
*not* copied even though there's no terminating NUL.

> +  /* set up an address variant */
> +  g_value_init (&value, G_TYPE_STRING);

tp_value_array_build() again, please

> +  g_assert (strcmp (host, IPV4_ADDR) == 0);
> +  g_assert (port == PORT);

g_assert_cmpstr, g_assert_cmpuint please

> +  /* we seem to need to make a connection in order to initialise the special
> +   * dbus-glib types, I'm sure there must be a better way to do this */
> +  connection = dbus_g_bus_get (DBUS_BUS_SESSION, &error);

The better way is dbus_g_type_specialized_init() from dbus-glib >= 0.82, which
I believe we already depend on?

Or, if you want to connect to the bus for whatever reason, please move the test
to tests/dbus/ so it will run under a temporary session bus - tests in tests/
should not touch the session bus, so that build-bots with no D-Bus session
can still run them without special precautions.

(In reply to comment #7)
> >  <SECTION>
> > +<FILE>gnio-util</FILE>
> > +<TITLE>gnio-util</FILE>
> > +<INCLUDE>telepathy-glib/gnio-util.h</INCLUDE>
> > +tp_g_socket_address_from_variant
> > +tp_address_variant_from_g_socket_address
> > +</SECTION>
> 
> Recommend including this via <telepathy-glib/telepathy-glib.h>, perhaps?

Currently no other class/file does this, should we separately go through and change them all perhaps?

> > +GSocketAddress *tp_g_socket_address_from_variant (TpSocketAddressType type,
> > +                                                  const GValue *variant);
> > +GValue *tp_address_variant_from_g_socket_address (GSocketAddress      *address,
> > +                                                  TpSocketAddressType *type);
> 
> G_WARN_UNUSED_RESULT on these, please (they'll leak if the result is ignored).

Done.

> > +#include <telepathy-glib/gnio-util.h>
> 
> This should be the first thing included after config.h.

Done.

> > +GSocketAddress *
> > +tp_g_socket_address_from_variant (TpSocketAddressType type,
> > +                                  const GValue *variant)
> 
> I don't like this function's error behaviour (anything wrong with the GValue
> is a critical warning). It should survive being given a bad value over D-Bus -
> we don't want the caller to have to pre-check.
> 
> Perhaps it could be documented to return NULL without a warning for
> unparseable data, instead; add a GError ** if it seems to you to make sense.

Added a GError**. Removed most of the g_return_if_fail, except for some that make sense to me.

> The error path should also have some tests I'm afraid.

Ok. I'll get around to writing them.

> > +            switch (g_inet_address_get_family (addr))
> > +              {
> > +                case G_SOCKET_FAMILY_IPV4:
> > +                  type_ = TP_SOCKET_ADDRESS_TYPE_IPV4;
> > +                  variant = tp_g_value_slice_new (TP_STRUCT_TYPE_SOCKET_ADDRESS_IPV4);
> > +                  break;
> 
> To be honest I'd prefer an assertion that TP_SOCKET_ADDRESS_TYPE_IPV4 and
> TP_SOCKET_ADDRESS_TYPE_IPV6 are numerically the same type (they are), then
> using a variable containing one or the other subsequently.

Not sure what you mean. Can you clarify please?

> > +            array = g_value_array_new (2);
> 
> Please use tp_value_array_build() for this, now that we have it.

Handypants! Done.

> > +      default:
> > +        g_return_val_if_reached (NULL);
> 
> Again, I'd prefer a non-criticalling failure mode - it's possible that GLib
> can introduce a new socket type that we don't support, and callers of this
> function shouldn't be expected to know ahead of time what we do and don't
> support - and it should be tested.

Done.

> > +  if (type) *type = type_;
> 
> if (type != NULL)
>   *type = type_;

Done.

> Also, I'd much prefer type to be called ret_type or something, so type_ can be
> renamed to type. Variables differing only in underscores scare me.

Done.

> > +#define ABST_ADDR "\000123456"
> > +#define ABST_ADDR_LEN 7
> 
> "\000" "123456" or "\x00pqrstu" or something would be easier for readers to
> parse, I think.

Done. Also found a mistake in this test, so it was actually testing nothing.

> I'd like ABST_ATTR to have some specified trailing rubbish -
> "\x00pqrstuXYZ\x00" or something - to be able to assert that XYZ is
> *not* copied even though there's no terminating NUL.

Will do when I write some more tests.

> > +  /* set up an address variant */
> > +  g_value_init (&value, G_TYPE_STRING);
> 
> tp_value_array_build() again, please

Done.

> > +  g_assert (strcmp (host, IPV4_ADDR) == 0);
> > +  g_assert (port == PORT);
> 
> g_assert_cmpstr, g_assert_cmpuint please

Done.

> > +  /* we seem to need to make a connection in order to initialise the special
> > +   * dbus-glib types, I'm sure there must be a better way to do this */
> > +  connection = dbus_g_bus_get (DBUS_BUS_SESSION, &error);
> 
> The better way is dbus_g_type_specialized_init() from dbus-glib >= 0.82, which
> I believe we already depend on?

Done.

This looks good for merge, thanks.

Things that do not block merge but would be nice to see in a trivia branch as a follow-up (or you can do them before merging, and consider them pre-reviewed):

> +  AC_DEFINE(HAVE_GIO_UNIX, [], [GIO-Unix is available])

This string turns up in config.h.in, which is (in principle) hand-editable for use on platforms that can't run configure, so it should be "Define if GIO-Unix is available".

> +GValue *tp_address_variant_from_g_socket_address (GSocketAddress      *address,

Please remove the excess spaces.

You still haven't done this:
> > I'd like ABST_ATTR to have some specified trailing rubbish -
> > "\x00pqrstuXYZ\x00" or something - to be able to assert that XYZ is
> > *not* copied even though there's no terminating NUL.
>
> Will do when I write some more tests.

Merged.

Will be in 0.11.0.