From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: util-linux-owner@vger.kernel.org Received: from mail-qc0-f174.google.com ([209.85.216.174]:47269 "EHLO mail-qc0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753049Ab2ETQiu (ORCPT ); Sun, 20 May 2012 12:38:50 -0400 Received: by qcro28 with SMTP id o28so2806806qcr.19 for ; Sun, 20 May 2012 09:38:48 -0700 (PDT) Date: Sun, 20 May 2012 12:38:46 -0400 From: Dave Reisner To: Davidlohr Bueso Cc: Dave Reisner , util-linux@vger.kernel.org Subject: Re: [PATCH 3/3] libmount/utils: Use binary search to compare pseudofs Message-ID: <20120520163846.GD668@rampage> References: <1337474361-22429-1-git-send-email-dreisner@archlinux.org> <1337474361-22429-3-git-send-email-dreisner@archlinux.org> <1337522760.2677.3.camel@offbook> <20120520145106.GC668@rampage> <1337529912.2677.5.camel@offbook> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <1337529912.2677.5.camel@offbook> Sender: util-linux-owner@vger.kernel.org List-ID: On Sun, May 20, 2012 at 06:05:12PM +0200, Davidlohr Bueso wrote: > On Sun, 2012-05-20 at 10:51 -0400, Dave Reisner wrote: > > On Sun, May 20, 2012 at 04:06:00PM +0200, Davidlohr Bueso wrote: > > > On Sat, 2012-05-19 at 20:39 -0400, Dave Reisner wrote: > > > > While we're at it, expand this list of known pseudofs types. > > > > > > > > Signed-off-by: Dave Reisner > > > > --- > > > > libmount/src/utils.c | 68 +++++++++++++++++++++++++++++++------------------- > > > > 1 file changed, 43 insertions(+), 25 deletions(-) > > > > > > > > diff --git a/libmount/src/utils.c b/libmount/src/utils.c > > > > index b824edc..77557f2 100644 > > > > --- a/libmount/src/utils.c > > > > +++ b/libmount/src/utils.c > > > > @@ -72,6 +72,15 @@ int mnt_parse_offset(const char *str, size_t len, uintmax_t *res) > > > > return rc; > > > > } > > > > > > > > +/* used as a callback by bsearch in mnt_fstype_is_pseudofs() */ > > > > +static int fstype_cmp(const void *v1, const void *v2) > > > > +{ > > > > + const char *s1 = *(const char **)v1; > > > > + const char *s2 = *(const char **)v2; > > > > + > > > > + return strcmp(s1, s2); > > > > +} > > > > + > > > > /* returns basename and keeps dirname in the @path, if @path is "/" (root) > > > > * then returns empty string */ > > > > static char *stripoff_last_component(char *path) > > > > @@ -214,31 +223,40 @@ char *mnt_unmangle(const char *str) > > > > */ > > > > int mnt_fstype_is_pseudofs(const char *type) > > > > { > > > > - if (!type) > > > > - return 0; > > > > - if (strcmp(type, "none") == 0 || > > > > - strcmp(type, "proc") == 0 || > > > > - strcmp(type, "tmpfs") == 0 || > > > > - strcmp(type, "sysfs") == 0 || > > > > - strcmp(type, "autofs") == 0 || > > > > - strcmp(type, "devpts") == 0|| > > > > - strcmp(type, "cgroup") == 0 || > > > > - strcmp(type, "devtmpfs") == 0 || > > > > - strcmp(type, "devfs") == 0 || > > > > - strcmp(type, "dlmfs") == 0 || > > > > - strcmp(type, "cpuset") == 0 || > > > > - strcmp(type, "configfs") == 0 || > > > > - strcmp(type, "securityfs") == 0 || > > > > - strcmp(type, "hugetlbfs") == 0 || > > > > - strcmp(type, "rpc_pipefs") == 0 || > > > > - strcmp(type, "fusectl") == 0 || > > > > - strcmp(type, "mqueue") == 0 || > > > > - strcmp(type, "binfmt_misc") == 0 || > > > > - strcmp(type, "fuse.gvfs-fuse-daemon") == 0 || > > > > - strcmp(type, "debugfs") == 0 || > > > > - strcmp(type, "spufs") == 0) > > > > - return 1; > > > > - return 0; > > > > + static const char *pseudofs[] = { > > > > + "anon_inodefs", > > > > + "autofs", > > > > + "bdev", > > > > + "binfmt_misc", > > > > + "cgroup", > > > > + "configfs", > > > > + "cpuset", > > > > + "debugfs", > > > > + "devfs", > > > > + "devpts", > > > > + "devtmpfs", > > > > + "dlmfs", > > > > + "fuse.gvfs-fuse-daemon", > > > > + "fusectl", > > > > + "hugetlbfs", > > > > + "mqueue", > > > > + "nfsd", > > > > + "none", > > > > + "pipefs", > > > > + "proc", > > > > + "pstore", > > > > + "ramfs", > > > > + "rootfs", > > > > + "rpc_pipefs", > > > > + "securityfs", > > > > + "sockfs", > > > > + "spufs", > > > > + "sysfs", > > > > + "tmpfs" > > > > + }; > > > > + > > > > > > What about applying sort (qsort(3)) before searching here? This would > > > correct any manual string sorting that pseudofs[] imposes to the > > > developer. > > > > > > > Hmm, so the overhead from this seems to be negligible, but given that > > this is running as part of a library, wouldn't we need a reentrant > > version of qsort? qsort_r was added to glibc in 2.8, but I'm not > > familiar with how portable this would be to other libc's. > > I normally worry about reentrancy when dealing with interrupt handling, > like signals. Also consider that pseudofs[] isn't global. > qsort_r takes an extra param to the callback to hold an element while its being swapped. The reentrancy of the function isn't guaranteed just because it operates on a non-global var. Regardless, I'm not really in favor of adding this. I think it's reasonable to push a little responsibility on the developer and preemptively ensure that this is sorted. Perhaps add a comment in the function would be sufficient: "make sure this is sorted or Karel will come after you". d > > > > > > + return !(bsearch(&type, pseudofs, ARRAY_SIZE(pseudofs), > > > > + sizeof(char*), fstype_cmp) == NULL); > > > > } > > > > > > > > /** > > > > > > > > > -- > > > To unsubscribe from this list: send the line "unsubscribe util-linux" in > > > the body of a message to majordomo@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > >