Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 19 Jan 2017 16:46:05 +0000 (UTC)
From:      "Conrad E. Meyer" <cem@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r312416 - head/sys/ufs/ffs
Message-ID:  <201701191646.v0JGk5p1028456@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: cem
Date: Thu Jan 19 16:46:05 2017
New Revision: 312416
URL: https://svnweb.freebsd.org/changeset/base/312416

Log:
  ffs_vnops: Simplify extattr access
  
  As suggested in r167010, use the structure type and macros to access and
  modify UFS2 extended attributes.  Add assertions that pointers are
  aligned in places where we now access the data through a structure
  pointer, instead of character-by-character.
  
  PR:		216127
  Reported by:	dewayne at heuristicsystems.com.au
  Reviewed by:	kib@
  Sponsored by:	Dell EMC Isilon
  Differential Revision:	https://reviews.freebsd.org/D9225

Modified:
  head/sys/ufs/ffs/ffs_vnops.c

Modified: head/sys/ufs/ffs/ffs_vnops.c
==============================================================================
--- head/sys/ufs/ffs/ffs_vnops.c	Thu Jan 19 16:07:52 2017	(r312415)
+++ head/sys/ufs/ffs/ffs_vnops.c	Thu Jan 19 16:46:05 2017	(r312416)
@@ -100,6 +100,9 @@ __FBSDID("$FreeBSD$");
 #include "opt_directio.h"
 #include "opt_ffs.h"
 
+#define	ALIGNED_TO(ptr, s)	\
+	(((uintptr_t)(ptr) & (_Alignof(s) - 1)) == 0)
+
 #ifdef DIRECTIO
 extern int	ffs_rawread(struct vnode *vp, struct uio *uio, int *workdone);
 #endif
@@ -1100,46 +1103,30 @@ ffs_extwrite(struct vnode *vp, struct ui
  * the length of the EA, and possibly the pointer to the entry and to the data.
  */
 static int
-ffs_findextattr(u_char *ptr, u_int length, int nspace, const char *name, u_char **eap, u_char **eac)
+ffs_findextattr(u_char *ptr, u_int length, int nspace, const char *name,
+    struct extattr **eapp, u_char **eac)
 {
-	u_char *p, *pe, *pn, *p0;
-	int eapad1, eapad2, ealength, ealen, nlen;
-	uint32_t ul;
+	struct extattr *eap, *eaend;
+	size_t nlen;
 
-	pe = ptr + length;
 	nlen = strlen(name);
-
-	for (p = ptr; p < pe; p = pn) {
-		p0 = p;
-		bcopy(p, &ul, sizeof(ul));
-		pn = p + ul;
+	KASSERT(ALIGNED_TO(ptr, struct extattr), ("unaligned"));
+	eap = (struct extattr *)ptr;
+	eaend = (struct extattr *)(ptr + length);
+	for (; eap < eaend; eap = EXTATTR_NEXT(eap)) {
 		/* make sure this entry is complete */
-		if (pn > pe)
+		if (EXTATTR_NEXT(eap) > eaend)
 			break;
-		p += sizeof(uint32_t);
-		if (*p != nspace)
-			continue;
-		p++;
-		eapad2 = *p++;
-		if (*p != nlen)
-			continue;
-		p++;
-		if (bcmp(p, name, nlen))
+		if (eap->ea_namespace != nspace || eap->ea_namelength != nlen
+		    || memcmp(eap->ea_name, name, nlen) != 0)
 			continue;
-		ealength = sizeof(uint32_t) + 3 + nlen;
-		eapad1 = 8 - (ealength % 8);
-		if (eapad1 == 8)
-			eapad1 = 0;
-		ealength += eapad1;
-		ealen = ul - ealength - eapad2;
-		p += nlen + eapad1;
-		if (eap != NULL)
-			*eap = p0;
+		if (eapp != NULL)
+			*eapp = eap;
 		if (eac != NULL)
-			*eac = p;
-		return (ealen);
+			*eac = EXTATTR_CONTENT(eap);
+		return (EXTATTR_CONTENT_SIZE(eap));
 	}
-	return(-1);
+	return (-1);
 }
 
 static int
@@ -1380,9 +1367,11 @@ vop_deleteextattr {
 {
 	struct inode *ip;
 	struct fs *fs;
-	uint32_t ealength, ul;
-	int ealen, olen, eapad1, eapad2, error, i, easize;
-	u_char *eae, *p;
+	struct extattr *eap;
+	uint32_t ul;
+	int olen, error, i, easize;
+	u_char *eae;
+	void *tmp;
 
 	ip = VTOI(ap->a_vp);
 	fs = ITOFS(ip);
@@ -1413,39 +1402,30 @@ vop_deleteextattr {
 	if (error)
 		return (error);
 
-	ealength = eapad1 = ealen = eapad2 = 0;
-
+	/* CEM: delete could be done in-place instead */
 	eae = malloc(ip->i_ea_len, M_TEMP, M_WAITOK);
 	bcopy(ip->i_ea_area, eae, ip->i_ea_len);
 	easize = ip->i_ea_len;
 
 	olen = ffs_findextattr(eae, easize, ap->a_attrnamespace, ap->a_name,
-	    &p, NULL);
+	    &eap, NULL);
 	if (olen == -1) {
 		/* delete but nonexistent */
 		free(eae, M_TEMP);
 		ffs_close_ea(ap->a_vp, 0, ap->a_cred, ap->a_td);
-		return(ENOATTR);
+		return (ENOATTR);
 	}
-	bcopy(p, &ul, sizeof ul);
-	i = p - eae + ul;
-	if (ul != ealength) {
-		bcopy(p + ul, p + ealength, easize - i);
-		easize += (ealength - ul);
-	}
-	if (easize > NXADDR * fs->fs_bsize) {
-		free(eae, M_TEMP);
-		ffs_close_ea(ap->a_vp, 0, ap->a_cred, ap->a_td);
-		if (ip->i_ea_area != NULL && ip->i_ea_error == 0)
-			ip->i_ea_error = ENOSPC;
-		return(ENOSPC);
-	}
-	p = ip->i_ea_area;
+	ul = eap->ea_length;
+	i = (u_char *)EXTATTR_NEXT(eap) - eae;
+	bcopy(EXTATTR_NEXT(eap), eap, easize - i);
+	easize -= ul;
+
+	tmp = ip->i_ea_area;
 	ip->i_ea_area = eae;
 	ip->i_ea_len = easize;
-	free(p, M_TEMP);
+	free(tmp, M_TEMP);
 	error = ffs_close_ea(ap->a_vp, 1, ap->a_cred, ap->a_td);
-	return(error);
+	return (error);
 }
 
 /*
@@ -1499,7 +1479,7 @@ vop_getextattr {
 		error = ENOATTR;
 
 	ffs_close_ea(ap->a_vp, 0, ap->a_cred, ap->a_td);
-	return(error);
+	return (error);
 }
 
 /*
@@ -1519,9 +1499,7 @@ vop_listextattr {
 */
 {
 	struct inode *ip;
-	u_char *eae, *p, *pe, *pn;
-	unsigned easize;
-	uint32_t ul;
+	struct extattr *eap, *eaend;
 	int error, ealen;
 
 	ip = VTOI(ap->a_vp);
@@ -1537,31 +1515,31 @@ vop_listextattr {
 	error = ffs_open_ea(ap->a_vp, ap->a_cred, ap->a_td);
 	if (error)
 		return (error);
-	eae = ip->i_ea_area;
-	easize = ip->i_ea_len;
 
 	error = 0;
 	if (ap->a_size != NULL)
 		*ap->a_size = 0;
-	pe = eae + easize;
-	for(p = eae; error == 0 && p < pe; p = pn) {
-		bcopy(p, &ul, sizeof(ul));
-		pn = p + ul;
-		if (pn > pe)
+
+	KASSERT(ALIGNED_TO(ip->i_ea_area, struct extattr), ("unaligned"));
+	eap = (struct extattr *)ip->i_ea_area;
+	eaend = (struct extattr *)(ip->i_ea_area + ip->i_ea_len);
+	for (; error == 0 && eap < eaend; eap = EXTATTR_NEXT(eap)) {
+		/* make sure this entry is complete */
+		if (EXTATTR_NEXT(eap) > eaend)
 			break;
-		p += sizeof(ul);
-		if (*p++ != ap->a_attrnamespace)
+		if (eap->ea_namespace != ap->a_attrnamespace)
 			continue;
-		p++;	/* pad2 */
-		ealen = *p;
-		if (ap->a_size != NULL) {
+
+		ealen = eap->ea_namelength;
+		if (ap->a_size != NULL)
 			*ap->a_size += ealen + 1;
-		} else if (ap->a_uio != NULL) {
-			error = uiomove(p, ealen + 1, ap->a_uio);
-		}
+		else if (ap->a_uio != NULL)
+			error = uiomove(&eap->ea_namelength, ealen + 1,
+			    ap->a_uio);
 	}
+
 	ffs_close_ea(ap->a_vp, 0, ap->a_cred, ap->a_td);
-	return(error);
+	return (error);
 }
 
 /*
@@ -1582,10 +1560,12 @@ vop_setextattr {
 {
 	struct inode *ip;
 	struct fs *fs;
+	struct extattr *eap;
 	uint32_t ealength, ul;
 	ssize_t ealen;
 	int olen, eapad1, eapad2, error, i, easize;
-	u_char *eae, *p;
+	u_char *eae;
+	void *tmp;
 
 	ip = VTOI(ap->a_vp);
 	fs = ITOFS(ip);
@@ -1625,29 +1605,33 @@ vop_setextattr {
 		return (error);
 
 	ealength = sizeof(uint32_t) + 3 + strlen(ap->a_name);
-	eapad1 = 8 - (ealength % 8);
-	if (eapad1 == 8)
-		eapad1 = 0;
-	eapad2 = 8 - (ealen % 8);
-	if (eapad2 == 8)
-		eapad2 = 0;
+	eapad1 = roundup2(ealength, 8) - ealength;
+	eapad2 = roundup2(ealen, 8) - ealen;
 	ealength += eapad1 + ealen + eapad2;
 
+	/*
+	 * CEM: rewrites of the same size or smaller could be done in-place
+	 * instead.  (We don't acquire any fine-grained locks in here either,
+	 * so we could also do bigger writes in-place.)
+	 */
 	eae = malloc(ip->i_ea_len + ealength, M_TEMP, M_WAITOK);
 	bcopy(ip->i_ea_area, eae, ip->i_ea_len);
 	easize = ip->i_ea_len;
 
-	olen = ffs_findextattr(eae, easize,
-	    ap->a_attrnamespace, ap->a_name, &p, NULL);
+	olen = ffs_findextattr(eae, easize, ap->a_attrnamespace, ap->a_name,
+	    &eap, NULL);
         if (olen == -1) {
 		/* new, append at end */
-		p = eae + easize;
+		KASSERT(ALIGNED_TO(eae + easize, struct extattr),
+		    ("unaligned"));
+		eap = (struct extattr *)(eae + easize);
 		easize += ealength;
 	} else {
-		bcopy(p, &ul, sizeof ul);
-		i = p - eae + ul;
+		ul = eap->ea_length;
+		i = (u_char *)EXTATTR_NEXT(eap) - eae;
 		if (ul != ealength) {
-			bcopy(p + ul, p + ealength, easize - i);
+			bcopy(EXTATTR_NEXT(eap), (u_char *)eap + ealength,
+			    easize - i);
 			easize += (ealength - ul);
 		}
 	}
@@ -1656,34 +1640,30 @@ vop_setextattr {
 		ffs_close_ea(ap->a_vp, 0, ap->a_cred, ap->a_td);
 		if (ip->i_ea_area != NULL && ip->i_ea_error == 0)
 			ip->i_ea_error = ENOSPC;
-		return(ENOSPC);
+		return (ENOSPC);
 	}
-	bcopy(&ealength, p, sizeof(ealength));
-	p += sizeof(ealength);
-	*p++ = ap->a_attrnamespace;
-	*p++ = eapad2;
-	*p++ = strlen(ap->a_name);
-	strcpy(p, ap->a_name);
-	p += strlen(ap->a_name);
-	bzero(p, eapad1);
-	p += eapad1;
-	error = uiomove(p, ealen, ap->a_uio);
+	eap->ea_length = ealength;
+	eap->ea_namespace = ap->a_attrnamespace;
+	eap->ea_contentpadlen = eapad2;
+	eap->ea_namelength = strlen(ap->a_name);
+	memcpy(eap->ea_name, ap->a_name, strlen(ap->a_name));
+	bzero(&eap->ea_name[strlen(ap->a_name)], eapad1);
+	error = uiomove(EXTATTR_CONTENT(eap), ealen, ap->a_uio);
 	if (error) {
 		free(eae, M_TEMP);
 		ffs_close_ea(ap->a_vp, 0, ap->a_cred, ap->a_td);
 		if (ip->i_ea_area != NULL && ip->i_ea_error == 0)
 			ip->i_ea_error = error;
-		return(error);
+		return (error);
 	}
-	p += ealen;
-	bzero(p, eapad2);
+	bzero((u_char *)EXTATTR_CONTENT(eap) + ealen, eapad2);
 
-	p = ip->i_ea_area;
+	tmp = ip->i_ea_area;
 	ip->i_ea_area = eae;
 	ip->i_ea_len = easize;
-	free(p, M_TEMP);
+	free(tmp, M_TEMP);
 	error = ffs_close_ea(ap->a_vp, 1, ap->a_cred, ap->a_td);
-	return(error);
+	return (error);
 }
 
 /*



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201701191646.v0JGk5p1028456>