Could you rework the patch to all of that? Seems that you have a clear
vision as to where to go.
On Mon, 1 Aug 2005, Paul Jackson wrote:
> Christoph wrote:
> > New version without the magic numbers ...
>
> Good.
>
> ===
>
> How about replacing:
>
> static const char *policy_types[] = { "default", "prefer", "bind",
> "interleave" };
>
> with:
>
> static const char *policy_types[] = {
> [MPOL_DEFAULT] = "default",
> [MPOL_PREFERRED] = "prefer",
> [MPOL_BIND] = "bind",
> [MPOL_INTERLEAVE] = "interleave"
> };
>
> so that the mapping of the MPOL_* define constants to strings is
> explicit, not implicit.
>
> ===
>
> Maybe I need more caffeine, but the following tests seem backwards:
>
> + if (buffer + maxlen > p + l + 1)
> + return -ENOSPC;
>
> and
>
> + if (buffer + maxlen > p + 2)
> + return -ENOSPC;
>
> ===
>
> Can the following:
>
> + int l;
> + ...
> +
> + l = strlen(policy_types[mode]);
> + if (buffer + maxlen > p + l + 1)
> + return -ENOSPC;
> + strcpy(p, policy_types[mode]);
> + p += l;
> +
> + if (!nodes_empty(nodes)) {
> +
> + if (buffer + maxlen > p + 2)
> + return -ENOSPC;
> +
> + *p++ = '=';
> + p += nodelist_scnprintf(p, buffer + maxlen - p, nodes);
> + }
>
> be rewritten to:
>
> char *bufend = buffer + maxlen;
> ...
>
> p += scnprintf(p, bufend - p, "%s", policy_types[mode]);
> if (!nodes_empty(nodes)) {
> p += scnprintf(p, bufend - p, "=");
> p += nodelist_scnprintf(p, bufend - p, nodes);
> }
> if (p >= bufend - 1)
> return -ENOSPC;
>
> Less code, more consistent code for each buffer addition, fails with
> ENOSPC in the case that the nodelist only partially fits rather than
> truncating it without notice, and fixes any possible problem with the
> above tests being backwards - by removing the tests ;).
>
> This suggested replacement code does have one weakness, in that it
> cannot distinguish the case that the buffer was exactly the right
> size from the case it was too small, and errors with -ENOSPC in
> either case. I don't think that this case is worth adding extra
> logic to distinguish, in this situation.
>
> ===
>
> + for(mode = 0; mode <= MPOL_MAX; mode++) {
>
> Space after 'for'
>
> ===
>
> There should probably be comments that these routines must
> execute in the context of the task whose mempolicies are
> being displayed or modified.
>
> ==
>
> There should probably be a doc style comment introducing
> the mpol_to_str() and str_to_mpol() routines, as described
> in Documentation/kernel-doc-nano-HOWTO.txt.
>
> --
> I won't rest till it's the best ...
> Programmer, Linux Scalability
> Paul Jackson <[EMAIL PROTECTED]> 1.925.600.0401
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
