[Bug 18530] Should implement IPv4 socket for file transfer

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=18530

--- Comment #5 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2012-02-22 11:12:19 PST ---
(In reply to comment #4)
> If I understand the bug report / feature request correctly, the thing being
> asked for here is not local IPv4 as an alternative to local IPv6 to get the
> file to the other guy, it's local IPv4 as an alternative to Unix sockets to get
> the file into Salut

... i.e. accepting, and testing, things other than SOCKET_ADDRESS_TYPE_UNIX as
an argument to ProvideFile. In the tests, that's called in
file_transfer_helper.py, currently with hard-coded arguments:

    def provide_file(self):
        self.address = self.ft_channel.ProvideFile(cs.SOCKET_ADDRESS_TYPE_UNIX,
                cs.SOCKET_ACCESS_CONTROL_LOCALHOST, "", byte_arrays=True)

The requested feature is that at least ProvideFile(cs.SOCKET_ADDRESS_TYPE_IPV4,
cs.SOCKET_ACCESS_CONTROL_LOCALHOST, "") also works. It should also appear in
the AvailableSocketTypes property.

Hopefully you can copy some infrastructure from Gabble to run the tests
repeatedly with various socket and access-control types.

It's not immediately clear to me what's being tested in "Write IPV4 tests"
that's not already tested; in general, if the structure of a test is the same
as another but some parameters are different, it's best to parameterize it and
run it with various combinations of parameters. See Gabble for (many) examples.

I suspect that IPv4 as the transport on the network is actually already tested,
and the IPv6 test is the only one that actually uses IPv6?

> + //FIXME: Is there an enum for these magic numbers?

Yes, TpSocketAddressType. Its values in C are the same as
cs.SOCKET_ADDRESS_TYPE_blah but with a TP_ prefix.

> + GInetAddress *inetAddr;

Please use Telepathy coding style, in which variables look like this:
inet_addr.

> + sock = g_socket_new (family, G_SOCKET_TYPE_STREAM, G_SOCKET_PROTOCOL_TCP, &error);
> + if (error)

It's more conventional to detect failure using the thing returned by the method
that might have failed:

    sock = g_socket_new (..., &error);

    if (sock == NULL)
      {
        /* handle the error; either propagate it or free it */
      }

Your implementation also leaks the GError. You can pass NULL as the GError
address (to ignore it), but it'd be better for the failure case to DEBUG() the
error->message before freeing the error.

> + g_socket_bind (sock, addr, FALSE, &error);
> + if (error)
> + return NULL;

Similar problems. Use the boolean value returned by g_socket_bind() to detect
failure.

You're also leaking the GSocket object - which is the only reason this code
works, because if you didn't leak it, the fd would be closed when the GSocket
was unreffed for the last time. If you're going to use a GSocket rather than
platform sockets (BSD sockets on Unix, Winsock on Windows), you'll need to keep
the GSocket object around for as long as the GIOChannel exists, and make the
GIOChannel not be close-on-unref (because the GSocket is responsible for
closing the fd).

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