[Bug 22706] [0.11] GNIO integration for Telepathy GLib

[bugzilla-daemon at freedesktop.org]

http://bugs.freedesktop.org/show_bug.cgi?id=22706


Simon McVittie <simon.mcvittie at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|telepathy-                  |danielle.madeley at collabora.c
                   |bugs at lists.freedesktop.org  |o.uk
          QAContact|                            |telepathy-
                   |                            |bugs at lists.freedesktop.org




--- Comment #7 from Simon McVittie <simon.mcvittie at collabora.co.uk>  2010-01-26 10:11:23 PST ---
>  <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.


-- 
Configure bugmail: http://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.