Re: [PATCH] String conversions for memory policy

[Christoph Lameter <christoph@lameter.com>]

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 <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/