[webkit-reviews] review denied: [Bug 113466]=?UTF-8?Q?=20Don=E2=80=99t=20let=20Plug=2Dins=20use=20System=20V=20shared=20memory=20?=: [Attachment 195467] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Mar 27 21:42:42 PDT 2013
Alexey Proskuryakov <ap at webkit.org> has denied Simon Cooper
<scooper at apple.com>'s request for review:
Bug 113466: Don’t let Plug-ins use System V shared memory
https://bugs.webkit.org/show_bug.cgi?id=113466
Attachment 195467: Patch
https://bugs.webkit.org/attachment.cgi?id=195467&action=review
------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=195467&action=review
Looks pretty cool overall, but needs some style fixing, possibly multiple
rounds.
> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:41
> +#ifdef _SHIM_LOG_WITH_ASL
> +#include <asl.h>
> +#endif
Is there a specific reason to use asl here? That's not what we normally do.
Would fprintf(stderr, ...) or NSLog work?
The macro needs a more descriptive name (it does not matter that it's WITH_ASL
as far as I can tell). How about LOG_SYSV_SHARED_MEMORY_USAGE?
> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:133
> +// This needs to C scoped
> +extern "C" {
This comment is not helpful, as it doesn't explain why. In fact, I'm not even
sure if this is true that we need C linkage.
> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:135
> + // Fake System V shared memory. This is not thread safe.
You could add ASSERT(isMainThread()) to all functions to say this a little more
forcefully.
Would be nice to explain what is fake about it. Does it work as shared memory
at all?
> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:139
> +#ifdef _SHIM_LOG_WITH_ASL
> + static const char *lid = "PluginProcessShim:";
> +#endif
I'd just put this into each log call, it doesn't help readability or
hackability to have this elsewhere.
> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:149
> + struct fake_shm *next;
> + int refcnt;
> + key_t key;
> + size_t size;
> + size_t alloced_size;
> + int shmflag;
> + int shmid;
> + void *address;
WebKit style:
- no formatting, please just use a single space between type and name;
- we strongly prefer full words, and camelCase (so, alloced_size would probably
be allocatedSize or allocationSize).
- except for Objective-C types, stars stick to types.
void* address;
> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:152
> + static struct fake_shm *fake_shm_head = NULL;
No "struct" in variable definitions in C++.
> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:155
> + static key_t shim_ftok(const char *path, int id)
Misplaced star.
> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:163
> + static struct fake_shm *find_by_shmid(int shmid)
No "struct", misplaced star, abbreviations.
These occur many times below, I won't repeat the same comment.
> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:167
> + while (shm_p != NULL) {
WebKit style:
while (shm_p) {
}
or better yet, with a more descriptive name than shm_p.
> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:229
> + if (shm_p->address != NULL) {
> + if (shmaddr == 0 || shmaddr == shm_p->address) {
WebKit style says to never compare to 0, so this should be
if (shm_p->address) {
if (!shmaddr || shmaddr == shm_p->address)
> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:235
> + shm_p->alloced_size = (shm_p->size+4096) & ~0xFFF;
> + rval = shm_p->address = mmap((void *)shmaddr,
shm_p->alloced_size, PROT_READ|PROT_WRITE, MAP_ANON | MAP_PRIVATE, -1, 0);
We always add spaces around binary operations.
> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:248
> +
Extra empty line.
More information about the webkit-reviews
mailing list