From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S261371AbVHBFdP (ORCPT ); Tue, 2 Aug 2005 01:33:15 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S261378AbVHBFdP (ORCPT ); Tue, 2 Aug 2005 01:33:15 -0400 Received: from omx2-ext.sgi.com ([192.48.171.19]:16341 "EHLO omx2.sgi.com") by vger.kernel.org with ESMTP id S261371AbVHBFdN (ORCPT ); Tue, 2 Aug 2005 01:33:13 -0400 Date: Mon, 1 Aug 2005 22:33:04 -0700 From: Paul Jackson To: Christoph Lameter Cc: linux-kernel@vger.kernel.org, akpm@osdl.org, ak@suse.de Subject: Re: [PATCH] String conversions for memory policy Message-Id: <20050801223304.2a8871e8.pj@sgi.com> In-Reply-To: References: <20050729152049.4b172d78.pj@sgi.com> <20050729230026.1aa27e14.pj@sgi.com> <20050730181418.65caed1f.pj@sgi.com> <20050730190126.6bec9186.pj@sgi.com> <20050730191228.15b71533.pj@sgi.com> <20050801160351.71ee630a.pj@sgi.com> <20050801165947.36b5da96.pj@sgi.com> Organization: SGI X-Mailer: Sylpheed version 2.0.0beta5 (GTK+ 2.4.9; i686-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org 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 1.925.600.0401