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

[bugzilla-daemon at freedesktop.org]

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

--- Comment #28 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-04-12 09:08:31 PDT ---
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.

> static void get_next_manifest_entry (GTalkFileCollection *self,
>     ShareChannel *share_channel, gboolean error)

(Whitespace: static void \n get_next... please.)

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

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

>           "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 */

> /* 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:

  /* 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. */

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 :-)

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

>           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):

>           if (*line == 0)

I'd prefer '\0', and I think line[0] would be a clearer notation too.

>                       "Content-Length: %llu\r\n"

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

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

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

>               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?

>           /* we assume http_data_received never returns consumed > len */

Looks like a job for 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.