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

[bugzilla-daemon at freedesktop.org]

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





--- Comment #8 from Danielle Madeley <danielle.madeley at collabora.co.uk>  2010-01-27 02:13:32 PST ---
(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.


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