Re: [PATCH] String conversions for memory policy

[Paul Jackson <pj@sgi.com>]

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 <pj@sgi.com> 1.925.600.0401
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/