From owner-freebsd-fs@FreeBSD.ORG Tue Jan 18 10:54:43 2011 Return-Path: Delivered-To: fs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 5A6A21065694 for ; Tue, 18 Jan 2011 10:54:43 +0000 (UTC) (envelope-from gleb.kurtsou@gmail.com) Received: from mail-gy0-f182.google.com (mail-gy0-f182.google.com [209.85.160.182]) by mx1.freebsd.org (Postfix) with ESMTP id 0EC778FC19 for ; Tue, 18 Jan 2011 10:54:42 +0000 (UTC) Received: by gyf3 with SMTP id 3so2331478gyf.13 for ; Tue, 18 Jan 2011 02:54:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-disposition :content-transfer-encoding:in-reply-to:user-agent; bh=4MglhvI1MPCFFrFhIR/paHx9U7eDwfLC4c5wLM9ZrA8=; b=ETkcxx0AXuLuuj+D6v0KC+tPrZQRhPbEOK79xl725acc7PVE1mNVzoBh9IC9LF28Ir pHa9D5mAsNNR92EHj93qptYEI6g2K5YdUv9/E6NY4dBfc1y4OUJBdBBhmN/92VuvG2jv /lYeHUxxk6VP3V4Sqr4Ytsx4L2fb0HyWF/4wk= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:content-transfer-encoding :in-reply-to:user-agent; b=tN2nxi/VWd5rh+ApESWG9/5GjLfX12qbXD/JSUoOERA4lVRWz+XRWF052h662cATB/ DBBgpwyjglSGofwgUxgTc9WN+O8cZlZWCklgkTKdVPj+5No/MGoD60f5Y9td3oYxRk0w 1GLcS/oa1QD0CDzTJedfSSknizHVOj87AnbZ8= Received: by 10.151.84.8 with SMTP id m8mr795644ybl.276.1295348080943; Tue, 18 Jan 2011 02:54:40 -0800 (PST) Received: from localhost (lan-78-157-92-5.vln.skynet.lt [78.157.92.5]) by mx.google.com with ESMTPS id r24sm2889054yba.6.2011.01.18.02.54.38 (version=SSLv3 cipher=RC4-MD5); Tue, 18 Jan 2011 02:54:39 -0800 (PST) Date: Tue, 18 Jan 2011 12:54:15 +0200 From: Gleb Kurtsou To: Garrett Cooper Message-ID: <20110118105415.GA73120@tops.skynet.lt> References: <20110118041447.GA2087@tops.skynet.lt> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Cc: fs@freebsd.org Subject: Re: Inconsistency and bug with *chflags functions X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 18 Jan 2011 10:54:43 -0000 On (17/01/2011 20:47), Garrett Cooper wrote: > Hi Gleb! > > On Mon, Jan 17, 2011 at 8:14 PM, Gleb Kurtsou wrote: > > On (17/01/2011 16:32), Garrett Cooper wrote: > >> Hi FS folks, > >> > >>     Originally I did this to determine why lchflags had an int > >> argument and chflags/fchflags had unsigned long arguments, and I ran > >> into an interesting mini-project (by accident) after I found a few > >> bugs lurking in the vfs_syscalls.c layer of the kernel. > >> > >>     So here's the deal... > >> > >> Problems: > >> 1. chflags/fchflags claim that the flags argument is an unsigned long, > >> whereas lchflags claims it's an int. This is inconsistent. > >> 2. The kernel interfaces are all implicitly downcasting the flags > >> argument to int. > >> 3. There's a sizing/signedness discrepancy in a few other structures > >> with fixed-width data types (uint32_t). > >> > >> Solution: > >> 1. I opted to convert fflags_t to uint32_t and convert all references > >> which did direct conversions to and from st_flags to fflags_t to avoid > >> 32-bit / 64-bit biarch issues. I downgraded it to uint32_t as we're no > >> where near the limit for the number of usable flags to *chflags(2). > >> 2. *chflags now uses fflags_t instead of int/unsigned long. > >> 3. c_file_flags in dumprestore.h was changed to be in sync with st_flags. > >> 4. di_flags in ufs/ufs/dinode.h was changed to be in sync with st_flags. > >> > >> Compatibility: > >> 1. NetBSD uses unsigned long in their chflags calls (both in kernel > >> and userland) so they're more consistent than we are by not having > >> mixed flags calling convention like us, but uses uint32_t in their > >> data structures (like we do), so they have a 32-bit/64-bit biarch bug > >> (again like we do). > >> 2. OpenBSD is using unsigned int, so I assume that their kernel layer > >> is also using unsigned int (I am basing this purely off the manpage as > >> I haven't looked at their sources, but I could be wrong). > > > > Hi Garrett, > > > > You've changed syscalls definitions but didn't add backward > > compatibility shims for kernel, libc and freebsd32. > > Please enlighten me on how to do that (docs are fine) and I'll go > ahead and add the compatibility shims. I'm not a committer so I don't > yet know these things :). It's unlikely to be documented. You can't just change syscall definition, you should add a new one and mark old COMPAT8 (in this particular case). Implement kernel level wrappers, etc. Symbol versioning within libc is rather different story, I'd suggest you take a look at my patch it should be straightforward. > The only thing is that the old behavior on biarch platforms like amd64 > with COMPAT_FREEBSD32, etc was broken so I don't know if there's any > value in emulating broken behavior. Straight 32-bit and straight > 64-bit archs didn't have this problem. Yes, they are broken, but due to amd64 being little ending and flag argument being bit mask, I think both chflags and fchaflags actually work, although it should also be fixed, imho. Wrapper won't be needed for the new version (with fflags_t). > > I'm working on changing ino_t to 64 bits and nlink_t to 32 bits, we > > could join the effort, as projects look related. My old patch is > > available here: > > https://github.com/downloads/glk/freebsd-ino64/freebsd-ino64-patch.tgz > > Hmmm... I'll take a look at that. FWIW, I was avoiding bumping > fflags_t to uint64_t because it would change alignment of kernel > structures, but I'll take your advice in kind for resolving this > problem. There are other ways to avoid this by downcasting (or just > casting between types in general), which is fine today but I thought > this solution was simple because it was easier to root out affected > areas. Either way, I know that this method is a bit of a damned if you > do, damned if you don't because it restricts us from future additions > if fflags_t goes exceeds 2^32, but that's still the case today if the > field changes in the on-disk structures anyhow. 32 bits should be good enough. Extended attributes are getting more widely used nowadays. > > Please find comments to the patch below. > > Ok :)! > > >>     I'm running make universe just to see if it will barf in the > >> build, but would someone please review this change (I made the change > >> as minimal as possible for ease of review) and provide feedback? > > ... > > >> -             u_int32_t c_file_flags;     /* status flags (chflags) */ > >> +             fflags_t c_file_flags;      /* status flags (chflags) */ > > The struct represents on-disk protocol description, and thus shouldn't be changed. > > fflags_t in this case is uint32_t, so this is a drop-in-place > replacement -- or are you arguing the type more than the width of the > type? It's common practice, this way fflags_t could be changed to different type in the future, but binary compatibility (dump/restore protocol here) will be preserved. As it happened with uid_t, pid_t, off_t, or taking place with ino_t and nlink_t. No typedefs are used in struct ufs2_dinode or protocols/dumprestore.h (ino_t is being fixed). > >>               int32_t c_spare5[2];        /* old blocks, generation number */ > > ... > > >> -     u_int32_t       di_flags;       /*  88: Status flags (chflags). */ > >> +     fflags_t        di_flags;       /*  88: Status flags (chflags). */ > > Please revert for the same reason. > > My response here is the same as above. > > Thanks! > -Garrett