[Bug 25493] GTalk-compatible file transfers are not implemented

[bugzilla-daemon at freedesktop.org]

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





--- Comment #15 from Simon McVittie <simon.mcvittie at collabora.co.uk>  2010-03-22 13:17:15 PST ---
(In reply to comment #9)
> > This could usefully be on a trivia branch that's merged ahead of the feature
> > work.
> If you want I could cherry pick it, but I don't think it's really necessary
> atm, it's not like it's a bugfix or anything...

The idea of having a trivia branch is that it gets the easy, insta-reviewable
changes out of the way, so that the difficult-to-review branch is smaller. :-)

> > > +  /* This property is here only because jingle-session sets the media-type
> > > +     when constructing the object.. */
> > > +  g_object_class_install_property (object_class, PROP_MEDIA_TYPE,
> > > +      g_param_spec_uint ("media-type", "media type",
> > 
> > I thought the whole concept of media types was part of Jingle RTP, rather than
> > the layer of Jingle at which file transfers appear? If I'm right, then file
> > transfers should have media type JINGLE_MEDIA_TYPE_NONE?
> 
> Well, the way I see it is that you have a 'media type file' for file transfers
> (because, that's what it is, a ICE/UDP stream, in which you send a file).
> But the reason this is there is explained in the comment. The jingle-session
> creates the object with MEDIA_TYPE=MEDIA_TYPE_NONE, then it changes it
> depending on the type of content in the jingle-session. There's just no
> avoiding it with the way the jingle code works, at least, not without
> refactoring it or adding special use cases, which I didn't want to do.

Couldn't file transfers and other non-RTP contents have MEDIA_TYPE_NONE, at
least?

The reason inventing a spurious media type upsets me is that the way
GabbleJingleSession creates objects with a media type at all is an RTP-specific
wart: it's the Jingle-RTP (XEP-0167) media type, which ought to be local to
GabbleJingleMediaRtp and the Telepathy media-channel stuff, leaking through
into generic Jingle (XEP-0166) code like GabbleJingleSession.

We tolerated it so far because *all* our Jingle contents were RTP, and it's
harder to see domain-specific layering breakage when you only have one domain.

In the long term, I think GabbleJingleSession shouldn't know about media types
at all; in the short term, I think FTs (and Tubes) ought to either have no
media type (NONE), or if that's impossible for some reason, they ought to share
a new MEDIA_TYPE_NOT_MEDIA.

If the Jingle session needs to treat RTP and FT contents differently (which it
clearly does - e.g. adjusting @senders in gabble_jingle_content_parse_add),
then that's probably a sign that the GabbleJingleContent base class doesn't
have enough virtual methods (in this case the virtual method could be called
"get_default_senders", perhaps?)

> > > +      if (error != NULL)
> > > +        g_set_error (error, TP_ERRORS, TP_ERROR_OFFLINE,
> > > +            "can't find contact's presence");
> > 
> > No need for the guard, g_set_error copes with NULL as a first argument (that's
> > the whole point of using it instead of "*error = g_error_new ()")
> > 
> 
> Don't know, that was copy/pasted code. Anyways, I won't "fix" this because I
> prefer to have the guard than not have it.

I insist. g_set_error's documentation *starts* with "Does nothing if err is
NULL" and having the unnecessary guard will just make people wonder why this
bit of code is somehow special (answer: actually it isn't).

> > http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=2de68bb15afc8d1
> > > Remove checking for dialect in google transport
> > 
> > What does this have to do with anything? The long commit message should explain
> > why this change is desirable.
> Oh, it's simple, it checks to see if the dialect is 'google' in the 'google
> transport'.. it was not necessary and made no sense because the
> google-transport is already only used when we are in google dialect.
> Sorry, I thought it would be obvious.

I'm glad I caught this, because it's (subtly!) wrong. GabbleJingleTransport is
the gtalk-p2p transport (used as an alternative to ICE-UDP or raw UDP), whereas
the dialect you removed a check for indicates which dialect of Jingle we're
using. (The choice is currently between XEP-0166 draft 0.32, XEP-0166 draft
0.15, the Jingle variant from libjingle 0.4, or the Jingle variant from
libjingle 0.3 - at some point we should add XEP-0166 v1.0, which is not 100%
compatible with 0.32, to that list.)

The official Google Talk clients will only ever use gtalk-p2p combined with
some Google Jingle variant, but Gabble is quite capable of using standard
XEP-0166 for the Jingle session, but the non-standard gtalk-p2p transport.
Indeed, until version 0.7.something we didn't support either of the standard
transports, and we still prefer to use gtalk-p2p if the target supports both
gtalk-p2p and ICE-UDP (because that lets us use Google's relays, if we're
signed in with a GMail account).

> I wanted to make small patches, not necessary all fully working, that's why I
> separated these two changes into two separate commits, to make it easier to
> read/review/understand. I usually don't aim for each patch to be 'valid' but
> rather to be 'isolated set of related changes'.

The general rule of thumb is for commits to be as small as possible, but no
smaller :-)

> > > +  GHashTable *channels; /* Google transport 'channels', not TP channels > > ... and why isn't it called components or component_ids, which would remove the
> > confusion with Telepathy channels?

(Youness and I agreed on IRC that we'd call these "share channels", since
they're a concept specific to the Google file sharing (share-v1) protocol.)

> > > +      while (g_hash_table_iter_next (&iter, NULL, (gpointer) &c))
> > 
> > This violates strict aliasing. Declare c with type gpointer; its real type will
> > be obvious from context.
> Humm, are you sure there will be strict aliasing errors with 'gpointer'? I'm
> not getting any warning at compilation.

Sadly, I think you're still violating strict aliasing rules. The compiler won't
warn about it (because you shut it up with an explicit cast), but it might
silently break your code, I think...

The thing about strict aliasing (apart from the fact that hardly anyone fully
understands it - I don't claim that I do!) is that it describes an assumption
that the compiler is allowed to make in order to optimize the compiled code:
given pointers A and B, if they point to "similar things" (like a struct
_GArray and a const struct _GArray) the compiler pessimistically assumes that
they may point to the same memory ("aliasing"), but if they point to "obviously
different things" (like a struct _GObject and a struct _GArray) the compiler is
allowed to assume that they don't point to the same memory.

Unfortunately, while void * (and hence gpointer) is assumed to be able to alias
any other type, the compiler may assume that a gpointer* (i.e. void**) and a
GObject** do not alias.

I just spotted that you're using the cast (gpointer) rather than (gpointer *),
which means you *might* be safe here, but that's subtle, and complying with
strict aliasing here is easy anyway.

> Also, if I declare it as gpointer, then
> I'd still need to cast it into GabbleJingleContent in order to call
> gabble_jingle_content_parse_info

Probably not, you can use a gpointer where any pointer is expected:

const gchar *stoat_get_name (Stoat *self);

gpointer p = g_object_new (STOAT_TYPE,
    NULL);
Stoat *stoat;

stoat_get_name (p);   /* works */
stoat = p;            /* works */

(Neither of these is valid without a cast in C++, however.)

> > http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=c66e8cb4b2fb
> > > +static gchar *
> > > +generate_temp_url (void)
> > 
> > Couldn't you just use gabble_generate_id()?
> didn't know it exists... but after looking at it, it seems to generate in a
> format or "%lx.%lx/%lx/%lx" or uuid (I don't know the format of uuid), but
> since I'm building a path here, I'm not sure it's a great idea to have '/' in
> there.. i'd prefer just using the simple code I have.

UUIDs look like this: 7c5d3d2d-f4cd-4284-926a-a2e4bbcb178d

I don't really want different bits of Gabble creating miscellaneous
assumed-unique IDs with different code: this is the sort of thing that ought to
be in one place. It's a bit more subtle than it looks - for instance, you're
concatenating integers with a variable width and no separator, which reduces
the random space you're using by making e.g. (123, 45) equivalent to (12, 345)
- and calling a library function seems to me to be the simplest thing possible
here :-)

I'd be happy to change gabble_generate_id to avoid slashes if that's necessary
(underscores?), although slashes really aren't very magical in HTTP (as long as
you don't have two consecutive slashes, /./ or /../, which these IDs don't).

> But as an FYI, casting '42' as a gboolean is bad

I agree, but if you can behave correctly for non-TRUE true values by typing 8
characters fewer (omitting " == TRUE"), it seems a win :-P

(if() and while() would consider 42 to be a true value, so we should too.)

> on some systems (can't remember if it
> was AIX or MPE/x) TRUE is 0xFFFFFFFF, everything else is FALSE...
> (which is also why we do '#define TRUE (1 == 1)', and not '#define TRUE 1')

In GLib (and any sane C-based language), FALSE is 0, 0 is a false value, all
nonzero integers are true values, and TRUE is an unspecified true value
dependent on your compiler. The logic you describe isn't allowed by C99, since
if (42) {...} is meant to execute the "...".

(See §6.8.4.1 of WG14 N1124, available from
<http://www.open-std.org/JTC1/SC22/wg14/www/standards>, if you care :-)


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