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

No. The freedesktop bug is about the CM side and this one about the client (Empathy) side.

Note that my Gabble 'file-transfer+inet' branch implements Inet4 (and 6) socket support.

My ft_ipv4 [1] branch, (which is done on top of my ft_rework one) implements the Empathy part.

[1] http://git.collabora.co.uk/?p=user/cosimoc/empathy.git;a=shortlog;h=refs/heads/ft_ipv4

Could you clarify where ft_rework ends and ft_ipv4 starts please?

Try now, it didn't let you see the tags for heads of the branches as I cherry-picked a commit from ft_rework to ft_ipv4 and forgot to rebase.

- Your branch doesn't "make check"

  if (priv->socket_address_type == TP_SOCKET_ADDRESS_TYPE_UNIX)
    domain = AF_UNIX;

  if (priv->socket_address_type == TP_SOCKET_ADDRESS_TYPE_IPV4)
should be an "else if"

      res = connect (fd, (struct sockaddr*) &addr, sizeof (addr));
this line is duplicated. You should early return if socket_address_type has a different value. Currently you create an error using errno which wouldn't have a meaning value in that case.

initialize_empty_ac_variant: you don't set any value in the IPV4 case.

do_constructed: having to pull the AvailableSocketTypes seems a bit unfortunate as we already got it from the channel-properties dict (returned by CreateChannel() or in the NewChannels signal). Couldn't we reuse it from there?

Guillaume, thanks for the review.

(In reply to comment #7)
> - Your branch doesn't "make check"

Fixed.

>   if (priv->socket_address_type == TP_SOCKET_ADDRESS_TYPE_UNIX)
>     domain = AF_UNIX;
> 
>   if (priv->socket_address_type == TP_SOCKET_ADDRESS_TYPE_IPV4)
> should be an "else if"
>       res = connect (fd, (struct sockaddr*) &addr, sizeof (addr));
> this line is duplicated. You should early return if socket_address_type has a
> different value. Currently you create an error using errno which wouldn't have
> a meaning value in that case.

Should be all fixed now.

> initialize_empty_ac_variant: you don't set any value in the IPV4 case.

Maybe I misunderstood the purpose of TpSocketAccessControl, but that was intentional; I thought that, as we don't want to impose file transfer being on a specific interface/port, we are fine with the default values the CM chooses for us, so the SocketAccessControl variant we associate with the transfer is always empty. Am I missing something?

> do_constructed: having to pull the AvailableSocketTypes seems a bit unfortunate
> as we already got it from the channel-properties dict (returned by
> CreateChannel() or in the NewChannels signal). Couldn't we reuse it from there?

Right; this would involve adding caching code to EmpathyDispatcher (which makes sense to me if it avoids DBus roundtrips), but IMHO it's outside the scope of this branch. I can work on this in a separate branch once this has landed if we want to have it.

(In reply to comment #8)
> > initialize_empty_ac_variant: you don't set any value in the IPV4 case.
> 
> Maybe I misunderstood the purpose of TpSocketAccessControl, but that was
> intentional; I thought that, as we don't want to impose file transfer being on
> a specific interface/port, we are fine with the default values the CM chooses
> for us, so the SocketAccessControl variant we associate with the transfer is
> always empty. Am I missing something?

If you want to use the Port ACL, you are supposed to set in the variant a Socket_Address_IPv4 structur containing the IP and port FROM which you are going to connect to CM's socket. CM will reject any connection which are not from this ip/port.
Tbh, I think you shouldn't implement Port in Empathy until we have a CM implementing it. Just use LocalHost for now.


> > do_constructed: having to pull the AvailableSocketTypes seems a bit unfortunate
> > as we already got it from the channel-properties dict (returned by
> > CreateChannel() or in the NewChannels signal). Couldn't we reuse it from there?
> 
> Right; this would involve adding caching code to EmpathyDispatcher (which makes
> sense to me if it avoids DBus roundtrips), but IMHO it's outside the scope of
> this branch. I can work on this in a separate branch once this has landed if we
> want to have it.

I agree. Please feel a bug (and a fix ;) about that once your branch is merged.

Merged in master, closing.