From patchwork Sun Dec 18 08:38:50 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Aneesh Kumar K.V" X-Patchwork-Id: 132056 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 5FC6DB6FEB for ; Sun, 18 Dec 2011 19:40:20 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751661Ab1LRIjz (ORCPT ); Sun, 18 Dec 2011 03:39:55 -0500 Received: from e36.co.us.ibm.com ([32.97.110.154]:42093 "EHLO e36.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751335Ab1LRIjy (ORCPT ); Sun, 18 Dec 2011 03:39:54 -0500 Received: from /spool/local by e36.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Sun, 18 Dec 2011 01:39:54 -0700 Received: from d03relay02.boulder.ibm.com (9.17.195.227) by e36.co.us.ibm.com (192.168.1.136) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Sun, 18 Dec 2011 01:39:05 -0700 Received: from d03av04.boulder.ibm.com (d03av04.boulder.ibm.com [9.17.195.170]) by d03relay02.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id pBI8d57Q106492; Sun, 18 Dec 2011 01:39:05 -0700 Received: from d03av04.boulder.ibm.com (loopback [127.0.0.1]) by d03av04.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id pBI8d4YY032578; Sun, 18 Dec 2011 01:39:04 -0700 Received: from skywalker.linux.vnet.ibm.com ([9.124.88.48]) by d03av04.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with ESMTP id pBI8cpcA031780; Sun, 18 Dec 2011 01:38:54 -0700 From: "Aneesh Kumar K.V" To: Al Viro , Sasha Levin Cc: ericvh@gmail.com, rminnich@sandia.gov, lucho@ionkov.net, davem@davemloft.net, jvrao@linux.vnet.ibm.com, v9fs-developer@lists.sourceforge.net, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] 9p: Don't use ATTR_* values from fs.h in userspace facing structs In-Reply-To: <20111217174725.GW2203@ZenIV.linux.org.uk> References: <1324134422-16642-1-git-send-email-levinsasha928@gmail.com> <20111217174725.GW2203@ZenIV.linux.org.uk> User-Agent: Notmuch/0.10.2+102~g61445d1 (http://notmuchmail.org) Emacs/23.3.1 (x86_64-pc-linux-gnu) Date: Sun, 18 Dec 2011 14:08:50 +0530 Message-ID: <87pqfmgtph.fsf@linux.vnet.ibm.com> MIME-Version: 1.0 x-cbid: 11121808-3352-0000-0000-0000016DB721 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Sat, 17 Dec 2011 17:47:25 +0000, Al Viro wrote: > On Sat, Dec 17, 2011 at 05:07:02PM +0200, Sasha Levin wrote: > > struct p9_iattr_dotl is userspace facing, but the 'valid' field is documented > > as follows: > > > > * @valid: bitfield specifying which fields are valid > > * same as in struct iattr > > > > Which means that the user has to know about kernel internal ATTR_* values. > > > > On Fri, 2011-12-16 at 23:30 +0000, Al Viro wrote: > > > They *are* kernel internal values and 9P is asking for trouble exposing > > > them. Translation: tomorrow we might reassign those as we bloody wish > > > and any userland code that happens to rely on their values will break. > > > At which point we'll handle complaints by pointing and laughing. > > > > > > It's a 9P bug; fix it there. Turning random internal constants into a part > > > of ABI is not going to work. > > > > Signed-off-by: Sasha Levin > > --- > > fs/9p/vfs_inode_dotl.c | 31 ++++++++++++++++++++++++++++++- > > include/net/9p/9p.h | 18 ++++++++++++++++++ > > 2 files changed, 48 insertions(+), 1 deletions(-) > > > > diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c > > index 0b5745e..a948214 100644 > > --- a/fs/9p/vfs_inode_dotl.c > > +++ b/fs/9p/vfs_inode_dotl.c > > @@ -523,6 +523,35 @@ v9fs_vfs_getattr_dotl(struct vfsmount *mnt, struct dentry *dentry, > > return 0; > > } > > > > +int v9fs_vfs_iattr_to_9p_valid(u32 ia_valid) > > +{ > > + u32 valid = 0, i; > > + static u32 attr_map[][2] = { > > + {ATTR_MODE, P9_ATTR_MODE}, > > + {ATTR_UID, P9_ATTR_UID}, > > + {ATTR_SIZE, P9_ATTR_SIZE}, > > + {ATTR_ATIME, P9_ATTR_ATIME}, > > + {ATTR_MTIME, P9_ATTR_MTIME}, > > + {ATTR_CTIME, P9_ATTR_CTIME}, > > + {ATTR_ATIME_SET, P9_ATTR_ATIME_SET}, > > + {ATTR_MTIME_SET, P9_ATTR_MTIME_SET}, > > + {ATTR_FORCE, P9_ATTR_FORCE}, > > + {ATTR_ATTR_FLAG, P9_ATTR_ATTR_FLAG}, > > + {ATTR_KILL_SUID, P9_ATTR_KILL_SUID}, > > + {ATTR_KILL_SGID, P9_ATTR_KILL_SGID}, > > + {ATTR_FILE, P9_ATTR_FILE}, > > + {ATTR_KILL_PRIV, P9_ATTR_KILL_PRIV}, > > + {ATTR_OPEN, P9_ATTR_OPEN}, > > + {ATTR_TIMES_SET, P9_ATTR_TIMES_SET}, > > + }; > > a) ATTR_GID is lost > b) passing ATTR_FILE is bloody pointless; look at what it does and > realize that 9p doesn't as much as look at ia_file. > c) ATTR_KILL_PRIV is very dubious; what's the legitimate use of that > puppy in fs code? > > Look, that's the problem with exposing this stuff to protocol; you don't > get clear semantics and are you seriously asking for trouble on kernel > changes. Suppose tomorrow we get rid of e.g. ATTR_KILL_PRIV; what are you > guys going to do? Hope that no 9p server has behaviour dependent on that > flag being set or cleared? > > Don't turn the kernel internals into a part of ABI. And blind bulk remapping > of constants is exactly that... > I am testing this locally. commit 8ebbbcf5185096acccc523fb040e770d7876d4cd Author: Aneesh Kumar K.V Date: Sat Dec 17 21:01:08 2011 +0530 fs/9p: iattr_valid flags are kernel internal flags map them to 9p values. Kernel internal values can change, add protocol values for these constant and use them. Signed-off-by: Aneesh Kumar K.V --- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c index 0b5745e..a81808a 100644 --- a/fs/9p/vfs_inode_dotl.c +++ b/fs/9p/vfs_inode_dotl.c @@ -523,6 +523,46 @@ v9fs_vfs_getattr_dotl(struct vfsmount *mnt, struct dentry *dentry, return 0; } +/* + * Attribute flags. + */ +#define P9_ATTR_MODE (1 << 0) +#define P9_ATTR_UID (1 << 1) +#define P9_ATTR_GID (1 << 2) +#define P9_ATTR_SIZE (1 << 3) +#define P9_ATTR_ATIME (1 << 4) +#define P9_ATTR_MTIME (1 << 5) +#define P9_ATTR_CTIME (1 << 6) +#define P9_ATTR_ATIME_SET (1 << 7) +#define P9_ATTR_MTIME_SET (1 << 8) + +struct dotl_iattr_map { + int iattr_valid; + int p9_iattr_valid; +}; + +static int v9fs_mapped_iattr_valid(int iattr_valid) +{ + int i; + int p9_iattr_valid = 0; + struct dotl_iattr_map dotl_iattr_map[] = { + { ATTR_MODE, P9_ATTR_MODE }, + { ATTR_UID, P9_ATTR_UID }, + { ATTR_GID, P9_ATTR_GID }, + { ATTR_SIZE, P9_ATTR_SIZE }, + { ATTR_ATIME, P9_ATTR_ATIME }, + { ATTR_MTIME, P9_ATTR_MTIME }, + { ATTR_CTIME, P9_ATTR_CTIME }, + { ATTR_ATIME_SET, P9_ATTR_ATIME_SET }, + { ATTR_MTIME_SET, P9_ATTR_MTIME_SET }, + }; + for (i = 0; i < ARRAY_SIZE(dotl_iattr_map); i++) { + if (iattr_valid & dotl_iattr_map[i].iattr_valid) + p9_iattr_valid |= dotl_iattr_map[i].p9_iattr_valid; + } + return p9_iattr_valid; +} + /** * v9fs_vfs_setattr_dotl - set file metadata * @dentry: file whose metadata to set @@ -543,7 +583,7 @@ int v9fs_vfs_setattr_dotl(struct dentry *dentry, struct iattr *iattr) if (retval) return retval; - p9attr.valid = iattr->ia_valid; + p9attr.valid = v9fs_mapped_iattr_valid(iattr->ia_valid); p9attr.mode = iattr->ia_mode; p9attr.uid = iattr->ia_uid; p9attr.gid = iattr->ia_gid;