From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: cgroup/dmem: accept a single region when writing to attributes Date: Sun, 22 Mar 2026 04:08:56 +1000 Message-ID: In-Reply-To: <20260319-dmem_max_ebusy-v2-2-b5ce97205269@igalia.com> References: <20260319-dmem_max_ebusy-v2-0-b5ce97205269@igalia.com> <20260319-dmem_max_ebusy-v2-2-b5ce97205269@igalia.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Verdict: Good, minor observations** This replaces the `while` loop that iterated over newline-separated regions= with a flat single-region parse. The motivation (lack of atomicity when mu= ltiple regions could partially succeed) is valid. The control flow is clean. One observation: - After `strstrip(buf)` at line 737, the `strsep(&buf, " \t")` call at line= 741 will always return a non-empty string if `buf[0]` was non-zero (checke= d at line 738), so the `!region_name[0]` check at line 742 is technically r= edundant but harmless as defensive coding. - The `out_put` label correctly drops the region ref via `kref_put`. The ea= rly returns before the region lookup don't need ref cleanup, and the error = path after `get_cg_pool_unlocked` correctly jumps to `out_put`. The refcoun= ting is correct. - **Minor nit**: The patch doesn't strip a trailing newline before parsing.= If userspace writes `"region_name 1024\n"`, `strstrip()` at the top handle= s that. But if input is `"region_name 1024\n\n"` or `"region_name 1024\noth= er_region 2048\n"`, the `memparse` in `dmemcg_parse_limit` would hit the `\= n` and return `-EINVAL` via `*end !=3D '\0'`. This is the desired behavior = (rejecting multi-region input), but it would be slightly more user-friendly= to strip trailing whitespace from `buf` after `skip_spaces`. In practice `= strstrip` already strips trailing whitespace from the entire buffer, so thi= s is fine =E2=80=94 `buf` after `strsep` points past the separator, and `sk= ip_spaces` handles leading whitespace, while `strstrip` already trimmed tra= iling whitespace from the whole original buffer. No issue here. --- Generated by Claude Code Patch Reviewer