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

[bugzilla-daemon at freedesktop.org]

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

--- Comment #29 from Youness Alaoui <youness.alaoui at collabora.co.uk> 2010-04-12 11:15:05 PDT ---
(In reply to comment #28)
> Here's a review of the HTTP part; after you've responded to this and the hashes
> in the tests, hopefully someone else (Sjoerd?) will do the re-review pass, to
> be more likely to catch things I've missed.

Thanks.. I'll do the hashes thing later...

> 
> > static void get_next_manifest_entry (GTalkFileCollection *self,
> >     ShareChannel *share_channel, gboolean error)
> 
> (Whitespace: static void \n get_next... please.)
oups, fixed

> 
> >       filename = g_strdup_printf ("%s%s",
> >           entry->name, entry->folder? ".tar":"");
> 
> I'd prefer more space, and perhaps parentheses, around the ternary operator to
> make it clearer what's going on.
done

> 
> >       guint url_len = source_url != NULL? strlen (source_url) : 0;
> 
> I'd prefer "... = (source_url != NULL ? strlen (source_url) : 0);", and similar
> in the g_strdup_printf() call for the GET.
done

> 
> >           "Host: %s:0\r\n"
> >           "User-Agent: %s\r\n\r\n",
> 
> The Host is rather non-obvious: I take it the strange Host header is part of
> the Google Talk HTTP-over-pseudoTCP protocol? I'd prefer this commented,
> particularly since it's non-obvious whether it includes a resource (in fact it
> does):
> 
>           "Host: %s:0\r\n"            /* e.g. alice at example.com/Empathy:0 */

I added that example comment, but this isn't important.. that Host is what
libjingle 4.0 uses, but anything can be put there and it doesn't affect it...
(iGoogle for example doesn't send a Host header, just a single line GET with no
headers).

> 
> > /* Return the pointer at the end of the line or NULL if not \n found */
> > static gchar *
> > http_read_line (gchar *buffer, guint len)
> 
> This looks like it should be called cut_off_first_line or something? A better
> comment, IMO, would be:
I'm not sure about changing the name of the function, I find it ok as it is. If
you insist, I'll change it.

> 
>   /* If buffer contains a line ending, 0-terminate the first line and
>    * return a pointer to the beginning of the next line. Otherwise
>    * return NULL. */
I changed the comment to match yours since it's better, thanks

> 
> Anything dealing with memory block sizes (like this function) should ideally
> take/return a gsize, which can be bigger than guint on 64-bit, rather than a
> guint (although in practice if you're using IM file transfer for more than 2GB
> it's not going to work :-)
hehe, no, it's a guint because it's used by the received_http_data which has a
'guint len' argument, which itself is from what the nice_data_received which is
also called with a guint... The data received is supposed to be for one udp
packet, so it would never be higher than the max size of a UDP packet (65535),
so it's not gsize, it's a guint and it's more than enough (it could even work
with gshort, but since we could have extra data from a previous unfinished
line, it could exceed gshort, so guint is safe).

> 
> I'd prefer '\0' rather than 0 when 0-terminating strings.
done

> 
> >           DEBUG ("Found server headers line (%d) : %s", strlen (line), line);
> >           DEBUG ("Found client headers line (%d) : %s", strlen (line), line);
> 
> strlen returns a size_t. The printf format for printing a size_t is "%"
> G_GSIZE_FORMAT - this matters on LP64 platforms, like amd64, where sizeof
> (size_t) == sizeof (void *) == 8 and sizeof (int) == 4.
> 
> In http_data_received (twice, for client and server):
indeed, I fixed those, thanks

> 
> >           if (*line == 0)
> 
> I'd prefer '\0', and I think line[0] would be a clearer notation too.
done

> 
> >                       "Content-Length: %llu\r\n"
> 
> "Content-Length: %" G_GUINT64_FORMAT "\r\n", please.
done

> 
> >                   DEBUG ("Unable to find valid filename (%s), result : 404",
> >                       filename);
> 
> I think this should printf status_line, not filename, because otherwise if the
> scanf fails we printf NULL; on (at least) Solaris that crashes in libc, and on
> platforms with a non-crackful libc the debug message "Unable to find valid
> filename ((null))" isn't very helpful.

Ok, I fixed this by checking for filename?filename:"" instead, the purpose of
this message was to print what filename was scanned, but the status line is
already printed in the DEBUG line just above it, so we get both status_line
*and* scanned filename. But a NULL-check was indeed needed.

> 
> >           else if (!g_ascii_strncasecmp (line,
> >                   "Transfer-Encoding: chunked", 26))
> 
> I think you mean 27 - you want the '\0' at the end to match too, to avoid
> matching "Transfer-Encoding: chunked-but-not-as-we-know-it" (but actually, what
> you really want is plain g_ascii_strcasecmp, since both strings are
> '\0'-terminated, and this would avoid hard-coding the length).
Yeah, 27 I guess, I made it into strcasecmp instead as suggested (for
Content-Length too). thanks

> 
> >               if (g_str_has_prefix (share_channel->status_line,
> >                       "HTTP/1.1 200"))
> 
> If the status isn't 200, it can still have a body, so you need to skip
> Content-Length bytes of 404 response, or a stream of chunks. Use the same code
> but set a boolean "skipping" flag, perhaps?

Actually no, there won't be any body.. although yes it could, it will never in
our case.. don't forget that this isn't an HTTP library, it's a subset of HTTP
for the purpose of libjingle compat.. and libjingle will never send a body with
a 404 error. Anyways, I added a g_assert there to make sure it never happens..
I don't really like the idea of adding extra code that isn't necessary.
(Same goes for the request.. it could be a POST with a body, but we never check
the body when receiving a request.. it's a FIXME in there, but it never needs
to be done since we're doing a subset of HTTP, not a real HTTP library).


> 
> >           /* we assume http_data_received never returns consumed > len */
> 
> Looks like a job for g_assert()?
Added g_assert

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