From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753837AbcAHIk3 (ORCPT ); Fri, 8 Jan 2016 03:40:29 -0500 Received: from mga09.intel.com ([134.134.136.24]:46726 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750794AbcAHIk2 (ORCPT ); Fri, 8 Jan 2016 03:40:28 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,537,1444719600"; d="scan'208";a="877161943" Message-ID: <1452242451.30729.422.camel@linux.intel.com> Subject: Re: [PATCH v1 1/8] lib/string: introduce match_string() helper From: Andy Shevchenko To: Rasmus Villemoes Cc: Tejun Heo , Linus Walleij , Dmitry Eremin-Solenikov , linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, "David S. Miller" , David Airlie , Andrew Morton Date: Fri, 08 Jan 2016 10:40:51 +0200 In-Reply-To: <874mepnizu.fsf@rasmusvillemoes.dk> References: <1452168368-75630-1-git-send-email-andriy.shevchenko@linux.intel.com> <874mepnizu.fsf@rasmusvillemoes.dk> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.2-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2016-01-07 at 23:05 +0100, Rasmus Villemoes wrote: > On Thu, Jan 07 2016, Andy Shevchenko om> wrote: > > > From time to time we have to match a string in an array. Make a > > simple helper > > for that purpose. > >  /** > > + * match_string - matches given string in an array > > + * @array: array of strings > > + * @len: number of strings in the array or 0 for NULL > > terminated arrays > > + * @string: string to match with > > + * > > + * Return: > > + * index of a @string in the @array if matches, or %-ENODATA > > otherwise. > > + */ > > +int match_string(const char * const *array, size_t len, const char > > *string) > > +{ > > + int index = 0; > > + const char *item; > > + > > + do { > > + item = array[index]; > > + if (!item) > > + break; > > + if (!strcmp(item, string)) > > + return index; > > + } while (++index < len || !len); > > + > > + return -ENODATA; > > +} > > +EXPORT_SYMBOL(match_string); > > + > > I'd suggest making it -1 (which, since len is a size_t, is > effectively > infinity) having the meaning "the array is terminated by a NULL > entry". match_string(..., ARRAY_SIZE(my_array), ...) will break if > the > array happens to be empty, which could e.g. happen in a case like > > const char *my_array[] = { > #ifdef CONFIG_THIS >        "this", > #endif > #ifdef CONFIG_THAT >        "that", > #endif > }; It might make sense, though I don't remember current users with such conditions. > I also think the condition/loop above is unreadable. Hmm… For me looks straightforward. > > for (index = 0; index < len; index++) { >     ... > } > > is much clearer. If we switch to -1, it will look indeed simpler. > > Why -ENODATA and not just -1? It is rather unlikely that anyone would > pass on that particular -Exxx value. Not a biggie, just curious. There are few of users already that would like to return error code to upper level. In some cases better to have return match_string(); than ret = match_string(); if (ret < 0)  return -EFOO; return 0; And returning -ENODATA doesn't prevent to have latter, but allows former. > > Would there be more potential users if we had a flag argument > allowing > case-insensitive matching? Would there be more potential users if a > flag > allowed to ask whether the given string is a _prefix_ of one of the > strings in the array, or vice versa? Something like > > #define MATCH_STRING_CASE 0x01 > #define MATCH_STRING_PREFIX_OF_ARRAY_ELEM 0x02 /* yeah, that name > sucks */ > #define MATCH_ARRAY_ELEM_PREFIX_OF_STRING 0x04 /* this too */ > > int match_string(const char * const *array, size_t len, const char > *string, unsigned flags) > { > #define MATCH_PREFIX (MATCH_... | MATCH_...) >     int index; >     const char *item; >     int (*match_func)(const char *, const char *) = >         flags & MATCH_STRING_CASE ? strcasecmp : strcmp; >     int (*prefix_func)(const char *, const char *, size_t) = >         flags & MATCH_STRING_CASE ? strncasecmp : strncmp; >   >     for (index = 0; index < len; ++index) { >         item = array[index]; >         if (!item) >             break; >         if (flags & MATCH_PREFIX) { >             size_t len = strlen(flags & > MATCH_STRING_PREFIX_OF_ARRAY_ELEM ? >                 string : item); >             if (!prefix_func(item, string, len)) >                 return index; >         } else if (!match_func(item, string)) { >             return index; >         } >     } >     return -1; > } > > (Ok, it's not that pretty; maybe it'd be better to use > switch(flags&MATCH_PREFIX) {}. Or maybe just the case-insensitive > part > is worth keeping; in that case the above isn't that bad.) I won't overcomplicate it until we have enough users to consider. Any examples where we need this? And I prefer way to have different prototypes for them instead of net of conditions. Thanks for review. I will send v3 (yeah, this is actually v2) with change you proposed in the first part. For the second one I would like to have real examples before doing anything. -- Andy Shevchenko Intel Finland Oy