[webkit-reviews] review granted: [Bug 210346] [GTK][WPE] Replace evil strtok() calls with fscanf() in MemoryPressureMonitor.cpp : [Attachment 396384] patch

[bugzilla-daemon at webkit.org]

Adrian Perez <aperez at igalia.com> has granted Pablo Saavedra
<psaavedra at igalia.com>'s request for review:
Bug 210346: [GTK][WPE] Replace evil strtok() calls with fscanf() in
MemoryPressureMonitor.cpp
https://bugs.webkit.org/show_bug.cgi?id=210346

Attachment 396384: patch

https://bugs.webkit.org/attachment.cgi?id=396384&action=review




--- Comment #7 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 396384
  --> https://bugs.webkit.org/attachment.cgi?id=396384
patch

Other than the hardcoded buffer sizes, the parsing logic looks
good, and it's nice that using fscanf() in all this code avoids
the need for temporary memory allocations — which is important
when there isn't much left :)

View in context: https://bugs.webkit.org/attachment.cgi?id=396384&action=review

> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:69
> +    char buffer[128 + 1];

Please use a constant for the buffer size:

  #define ZONEINFO_TOKEN_BUFFER_SIZE 128

and then here:

  char buffer[ZONEINFO_TOKEN_BUFFER_SIZE + 1];

> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:77
> +	   r = fscanf(zoneInfoFile, " Node %*u, zone %128[^\n]\n", buffer);

…then here you can use:

  int r = fscanf(zoneInfoFile, "Node %*u, zone %"
STRINGIFY(ZONEINFO_TOKEN_BUFFER_SIZE) "[^\n]\n", buffer);

instead of hardcoding the buffer sizeo in the input format string.

> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:80
> +	   r = fscanf(zoneInfoFile, "%128s", buffer);

Here you could use fscanf(…, "%" STRINGIFY(ZONEINFO_TOKEN_BUFFER_SIZE) "s",
buffer)
instead of hardcoding the token buffer length.

> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:166
> +	   char name[40 + 1];

Same here, you could define CGROUP_NAME_BUFFER_SIZE.

> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:190
> +	   char token[50 + 1];

And here too: MEMINFO_TOKEN_BUFFER_SIZE.