[webkit-reviews] review granted: [Bug 18343] [GTK] Soup backend must handle upload of multiple files : [Attachment 27182] fixed proposed patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat Jan 31 16:44:40 PST 2009
Mark Rowe (bdash) <mrowe at apple.com> has granted Gustavo Noronha (kov)
<gns at gnome.org>'s request for review:
Bug 18343: [GTK] Soup backend must handle upload of multiple files
https://bugs.webkit.org/show_bug.cgi?id=18343
Attachment 27182: fixed proposed patch
https://bugs.webkit.org/attachment.cgi?id=27182&action=review
------- Additional Comments from Mark Rowe (bdash) <mrowe at apple.com>
I'm going to say r=me, though there are a few issues that should be addressed
before landing.
> #if PLATFORM(GTK)
> #if GLIB_CHECK_VERSION(2,12,0)
> @@ -55,9 +61,24 @@ enum
> {
> ERROR_TRANSPORT,
> ERROR_UNKNOWN_PROTOCOL,
> - ERROR_BAD_NON_HTTP_METHOD
> + ERROR_BAD_NON_HTTP_METHOD,
> + ERROR_UNABLE_TO_MAP_FILE,
> + ERROR_UNABLE_TO_OPEN_FILE,
> +};
Do we need the two different error codes here? The "map file" one seems like
an implementation detail that adds little benefit over the "open file" error
code.
> +
> +struct FileMapping
> +{
> + gpointer ptr;
> + gsize length;
> };
>
> +static void freeFileMapping(gpointer data)
> +{
> + FileMapping* fileMapping = static_cast<FileMapping*>(data);
> + munmap (fileMapping->ptr, fileMapping->length);
> + g_slice_free (FileMapping, fileMapping);
> +}
Shouldn't have a space between the function name and parenthesis in a function
call.
> +
> + struct stat statBuf;
> + fstat(fd, &statBuf);
The struct keyword is unnecessary here.
> +
> + FileMapping* fileMapping = g_slice_new (FileMapping);
There is extra space after the function name.
> + fileMapping->ptr = mmap(NULL, statBuf.st_size,
PROT_READ, MAP_PRIVATE, fd, 0);
> + if (fileMapping->ptr == MAP_FAILED) {
> + ResourceError error("webkit-network-error",
ERROR_UNABLE_TO_MAP_FILE, urlString, strerror(errno));
> + d->client()->didFail(this, error);
> + freeFileMapping(fileMapping);
Calling freeFileMapping here will attempt to munmap a pointer of MAP_FAILED.
Is that ok?
I think we also need to close the file descriptor in this error path to avoid
leaking it.
> + g_object_unref(msg);
> + return false;
> + }
> + fileMapping->length = statBuf.st_size;
> +
> + close(fd);
> +
> + SoupBuffer* soupBuffer;
This declaration should be combined with the initialisation on the next line.
> +
> + soupBuffer =
soup_buffer_new_with_owner(fileMapping->ptr, fileMapping->length, fileMapping,
freeFileMapping);
More information about the webkit-reviews
mailing list