[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