Bug 18530

We're porting Salut to Windows (see e.g. bug 46345). The port must support file transfers.

Salut file transfers being spec incompliant to the point of not supporting a single socket type which'd work on Windows is annoying to that end.

Relevant

13:31 < smcv> oggis_: tbh gabble and salut tubes/FT are copypasta of each other... so you could 
              probably steal from gabble
13:31 < smcv> oggis_: although porting to GSocket would be a massive massive win if you have time

I've implemented this in my clone: http://cgit.collabora.com/git/user/tfischer/telepathy-salut.git/log/?h=tdfischer/gsocket-file-xfer

This would be my first contribution, so I'm not certain if this is the right way to solve it. Feedback is appreciated :)

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:

        IPv4/IPv6/Unix        IPv4/IPv6          IPv4/IPv6/Unix
  sender's UI --> sender's Salut ==> receiver's Salut --> receiver's UI
           localhost             LAN               localhost

Of course, supporting and testing both IPv4 and IPv6 for the middle "hop" would be good too.

(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).

I've updated my branch to address the issues you brought up. The branch also includes some retooling of file_transfer_helper.py to support running the tests on any protocol.

Only issue is I have one failing test, and I'm not sure how to resolve it:

Traceback (most recent call last):
  File "/home/tdfischer/Projects/collabora/telepathy-salut/tests/twisted/saluttest.py", line 135, in exec_test_deferred
    fun(queue, bus, conn)
  File "/home/tdfischer/Projects/collabora/telepathy-salut/tests/twisted/avahi/file-transfer/file_transfer_helper.py", line 151, in test
    if fct():
  File "./avahi/file-transfer/send-file-ipv4.py", line 16, in client_request_file
    e = self.q.expect('dbus-signal', signal='InitialOffsetDefined')
  File "/home/tdfischer/Projects/collabora/telepathy-salut/tests/twisted/servicetest.py", line 170, in expect
    event = self.wait([pattern.subqueue])
  File "/home/tdfischer/Projects/collabora/telepathy-salut/tests/twisted/servicetest.py", line 304, in wait
    raise TimeoutError
TimeoutError
FAIL: avahi/file-transfer/send-file-ipv4.py

Every other test works just fine, including receive-file-ipv4.py. Also, editing file_transfer_helper.py to use IPV4 by default (instead of UNIX) on all the tests produces exactly the same result: all but one test passes.

The ipv4 test is copied verbatim from the disabled ipv6 send test, so I can't be  sure if this is a problem with the copied code or what.

Yeah, as far as I can see, the problem is that same Python bug. Because although you're requesting an IPv4 connection between the test and the CM with ProvideFile, the connection from the CM to the peer (should also be the test) can be anything independently from that. So it's going to hit the same bug in either case.

You can disable that kind of test for now.

Salut upstream, re-review?

(In reply to comment #7)
> Yeah, as far as I can see, the problem is that same Python bug. Because
> although you're requesting an IPv4 connection between the test and the CM with
> ProvideFile, the connection from the CM to the peer (should also be the test)
> can be anything independently from that. So it's going to hit the same bug in
> either case.
> 
> You can disable that kind of test for now.
> 

Oh, and for reference: that bug is about python httplib making incorrect HTTP requests when GETing from IPv6 localhost (::1). It'd seem it's using an IPv6 connection to the peer here.

I reviewed the diff of master...tfischer/tdfischer/gsocket-file-xfer as that was easier given the fixup patches later on. Don't be too afraid to squash together some patches, which logically make sense to do so, before merging this.

Anyway, yeah the branch looks pretty good, great work! I've just a few nitpicks:

-static gboolean setup_local_socket (SalutFileTransferChannel *self);
+static gboolean setup_local_socket (SalutFileTransferChannel *self, TpSocketAddressType address_type, guint access_control);

This line got quite long; one parameter per line please, like the rest of the file.

  GUnixSocketAddress *addr = NULL;
  addr = (GUnixSocketAddress *) g_unix_socket_address_new (path);
  return (GSocketAddress *) addr;

If you made addr a GSocketAddress there wouldn't need to be any casting necessary, no?

get_local_tcp_socket_address (SalutFileTransferChannel *self, GSocketFamily family)

-get_socket_channel (SalutFileTransferChannel *self)
+get_socket_channel (SalutFileTransferChannel *self, TpSocketAddressType address_type, guint access_control)

-setup_local_socket (SalutFileTransferChannel *self)
+setup_local_socket (SalutFileTransferChannel *self, TpSocketAddressType address_type, guint access_control)

One parameter per line.

+        sock = g_socket_new (G_SOCKET_FAMILY_UNIX, G_SOCKET_TYPE_STREAM, G_SOCKET_PROTOCOL_DEFAULT, &error);

Lines should be around 70 characters maximum. This one looks quite long, etc.

+        self.ftProto = ftProtocol

ft_proto and ft_protocol please. Same holds for the rest of your changes to the Python.

(In reply to comment #9)
> Lines should be around 70 characters maximum. This one looks quite long, etc.

When I said "This one looks quite long, etc." I was hoping you would look around to see if there were any other massively long lines.

I understand some of this code is a little mixed when it comes to coding styles, but new code should be using this: http://telepathy.freedesktop.org/wiki/Style

Other than that, the branch looks fine. I assume you're going to have to get someone to merge this for you?

This got merged, right?

Yes, that's the case.