[PATCH 0/3] xfs_fsr robustification

[PATCH 0/3] xfs_fsr robustification

[Eric Sandeen]

Coverity whines about xfs_fsr a bit, so this is a small
patch series to clean some things up.  Nothing essential,
AFAICT.

Thanks,
-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/3] xfs_fsr: ensure the line we read from leftofffile is null terminated

[Eric Sandeen]

Ensure that the string we read from leftofffile is NULL
terminated; the buffer gets passed to strchr(), so
it's important that we ensure it ends with NULL.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
index 3818f02..94d235c 100644
--- a/fsr/xfs_fsr.c
+++ b/fsr/xfs_fsr.c
@@ -554,6 +554,8 @@ fsrallfs(char *mtab, int howlong, char *leftofffile)
 			fsrprintf(_("could not read %s, starting with %s\n"),
 				leftofffile, *fs->dev);
 		} else {
+			/* Ensure the buffer we read is null terminated */
+			buf[SMBUFSZ-1] = '\0';
 			for (fs = fsbase; fs < fsend; fs++) {
 				fsname = fs->dev;
 				if ((strncmp(buf,fsname,strlen(fsname)) == 0)

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/3] xfs_fsr: ensure the line we read from leftofffile is null terminated

[Brian Foster]

On Fri, Jun 06, 2014 at 04:03:10PM -0500, Eric Sandeen wrote:
> Ensure that the string we read from leftofffile is NULL
> terminated; the buffer gets passed to strchr(), so
> it's important that we ensure it ends with NULL.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> index 3818f02..94d235c 100644
> --- a/fsr/xfs_fsr.c
> +++ b/fsr/xfs_fsr.c
> @@ -554,6 +554,8 @@ fsrallfs(char *mtab, int howlong, char *leftofffile)
>  			fsrprintf(_("could not read %s, starting with %s\n"),
>  				leftofffile, *fs->dev);
>  		} else {
> +			/* Ensure the buffer we read is null terminated */
> +			buf[SMBUFSZ-1] = '\0';

Maybe just initialize the buffer..?

Brian

>  			for (fs = fsbase; fs < fsend; fs++) {
>  				fsname = fs->dev;
>  				if ((strncmp(buf,fsname,strlen(fsname)) == 0)
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/3] xfs_fsr: ensure the line we read from leftofffile is null terminated

[Eric Sandeen]

On 6/9/14, 8:02 AM, Brian Foster wrote:
> On Fri, Jun 06, 2014 at 04:03:10PM -0500, Eric Sandeen wrote:
>> Ensure that the string we read from leftofffile is NULL
>> terminated; the buffer gets passed to strchr(), so
>> it's important that we ensure it ends with NULL.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
>> index 3818f02..94d235c 100644
>> --- a/fsr/xfs_fsr.c
>> +++ b/fsr/xfs_fsr.c
>> @@ -554,6 +554,8 @@ fsrallfs(char *mtab, int howlong, char *leftofffile)
>>  			fsrprintf(_("could not read %s, starting with %s\n"),
>>  				leftofffile, *fs->dev);
>>  		} else {
>> +			/* Ensure the buffer we read is null terminated */
>> +			buf[SMBUFSZ-1] = '\0';
> 
> Maybe just initialize the buffer..?

and then read no more than SMBUFSZ-1... I dunno, 6 one way, half a dozen
the other?

-Eric

> Brian
> 
>>  			for (fs = fsbase; fs < fsend; fs++) {
>>  				fsname = fs->dev;
>>  				if ((strncmp(buf,fsname,strlen(fsname)) == 0)
>>
>> _______________________________________________
>> xfs mailing list
>> xfs@oss.sgi.com
>> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/3] xfs_fsr: ensure the line we read from leftofffile is null terminated

[Mark Tinguely]

On 06/09/14 09:01, Eric Sandeen wrote:
> On 6/9/14, 8:02 AM, Brian Foster wrote:
>> On Fri, Jun 06, 2014 at 04:03:10PM -0500, Eric Sandeen wrote:
>>> Ensure that the string we read from leftofffile is NULL
>>> terminated; the buffer gets passed to strchr(), so
>>> it's important that we ensure it ends with NULL.
>>>
>>> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
>>> ---
>>>
>>> diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
>>> index 3818f02..94d235c 100644
>>> --- a/fsr/xfs_fsr.c
>>> +++ b/fsr/xfs_fsr.c
>>> @@ -554,6 +554,8 @@ fsrallfs(char *mtab, int howlong, char *leftofffile)
>>>   			fsrprintf(_("could not read %s, starting with %s\n"),
>>>   				leftofffile, *fs->dev);
>>>   		} else {
>>> +			/* Ensure the buffer we read is null terminated */
>>> +			buf[SMBUFSZ-1] = '\0';
>>
>> Maybe just initialize the buffer..?
>
> and then read no more than SMBUFSZ-1... I dunno, 6 one way, half a dozen
> the other?

I like my bike shed pained red with white and blue swirlies. :)

The strings should be NULL terminated for the string ops. It makes more 
sense to NULL one byte than 1024. If the full SMBUFSZ bytes are desired, 
then bump the array for the NULL byte.

--Mark.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/3] xfs_fsr: create a cleanup/return target in packfile()

[Eric Sandeen]

Error handling is a mishmash of closes, frees, etc at every
error point.  Create an "out" target that does this all
in one place.

Minor comment/doc update while we're at it.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
index 94d235c..8b191e6 100644
--- a/fsr/xfs_fsr.c
+++ b/fsr/xfs_fsr.c
@@ -1205,14 +1205,20 @@ out:
  * We already are pretty sure we can and want to
  * defragment the file.  Create the tmp file, copy
  * the data (maintaining holes) and call the kernel
- * extent swap routinte.
+ * extent swap routine.
+ *
+ * Return values:
+ * -1: Some error was encountered
+ *  0: Successfully defragmented the file
+ *  1: No change / No Error
  */
 static int
 packfile(char *fname, char *tname, int fd,
 	 xfs_bstat_t *statp, struct fsxattr *fsxp)
 {
-	int 		tfd;
+	int 		tfd = -1;
 	int		srval;
+	int		retval = -1;	/* Failure is the default */
 	int		nextents, extent, cur_nextents, new_nextents;
 	unsigned	blksz_dio;
 	unsigned	dio_min;
@@ -1220,7 +1226,7 @@ packfile(char *fname, char *tname, int fd,
 	static xfs_swapext_t   sx;
 	struct xfs_flock64  space;
 	off64_t 	cnt, pos;
-	void 		*fbuf;
+	void 		*fbuf = NULL;
 	int 		ct, wc, wc_b4;
 	char		ffname[SMBUFSZ];
 	int		ffd = -1;
@@ -1236,7 +1242,8 @@ packfile(char *fname, char *tname, int fd,
 	if (cur_nextents == 1 || cur_nextents <= nextents) {
 		if (vflag)
 			fsrprintf(_("%s already fully defragmented.\n"), fname);
-		return 1; /* indicates no change/no error */
+		retval = 1; /* indicates no change/no error */
+		goto out;
 	}
 
 	if (dflag)
@@ -1248,15 +1255,14 @@ packfile(char *fname, char *tname, int fd,
 		if (vflag)
 			fsrprintf(_("could not open tmp file: %s: %s\n"),
 				   tname, strerror(errno));
-		return -1;
+		goto out;
 	}
 	unlink(tname);
 
 	/* Setup extended attributes */
 	if (fsr_setup_attr_fork(fd, tfd, statp) != 0) {
 		fsrprintf(_("failed to set ATTR fork on tmp: %s:\n"), tname);
-		close(tfd);
-		return -1;
+		goto out;
 	}
 
 	/* Setup extended inode flags, project identifier, etc */
@@ -1264,15 +1270,13 @@ packfile(char *fname, char *tname, int fd,
 		if (ioctl(tfd, XFS_IOC_FSSETXATTR, fsxp) < 0) {
 			fsrprintf(_("could not set inode attrs on tmp: %s\n"),
 				tname);
-			close(tfd);
-			return -1;
+			goto out;
 		}
 	}
 
 	if ((ioctl(tfd, XFS_IOC_DIOINFO, &dio)) < 0 ) {
 		fsrprintf(_("could not get DirectIO info on tmp: %s\n"), tname);
-		close(tfd);
-		return -1;
+		goto out;
 	}
 
 	dio_min = dio.d_miniosz;
@@ -1294,8 +1298,7 @@ packfile(char *fname, char *tname, int fd,
 
 	if (!(fbuf = (char *)memalign(dio.d_mem, blksz_dio))) {
 		fsrprintf(_("could not allocate buf: %s\n"), tname);
-		close(tfd);
-		return -1;
+		goto out;
 	}
 
 	if (nfrags) {
@@ -1306,9 +1309,7 @@ packfile(char *fname, char *tname, int fd,
 		if ((ffd = open(ffname, openopts, 0666)) < 0) {
 			fsrprintf(_("could not open fragfile: %s : %s\n"),
 				   ffname, strerror(errno));
-			close(tfd);
-			free(fbuf);
-			return -1;
+			goto out;
 		}
 		unlink(ffname);
 	}
@@ -1338,9 +1339,7 @@ packfile(char *fname, char *tname, int fd,
 			if (ioctl(tfd, XFS_IOC_RESVSP64, &space) < 0) {
 				fsrprintf(_("could not pre-allocate tmp space:"
 					" %s\n"), tname);
-				close(tfd);
-				free(fbuf);
-				return -1;
+				goto out;
 			}
 			lseek64(tfd, outmap[extent].bmv_length, SEEK_CUR);
 		}
@@ -1348,11 +1347,7 @@ packfile(char *fname, char *tname, int fd,
 
 	if (lseek64(tfd, 0, SEEK_SET)) {
 		fsrprintf(_("Couldn't rewind on temporary file\n"));
-		close(tfd);
-		if (ffd != -1)
-			close(ffd);
-		free(fbuf);
-		return -1;
+		goto out;
 	}
 
 	/* Check if the temporary file has fewer extents */
@@ -1362,11 +1357,8 @@ packfile(char *fname, char *tname, int fd,
 	if (cur_nextents <= new_nextents) {
 		if (vflag)
 			fsrprintf(_("No improvement will be made (skipping): %s\n"), fname);
-		free(fbuf);
-		close(tfd);
-		if (ffd != -1)
-			close(ffd);
-		return 1; /* no change/no error */
+		retval = 1; /* no change/no error */
+		goto out;
 	}
 
 	/* Loop through block map copying the file. */
@@ -1437,11 +1429,7 @@ packfile(char *fname, char *tname, int fd,
 							tname);
 					}
 				}
-				free(fbuf);
-				close(tfd);
-				if (ffd != -1)
-					close(ffd);
-				return -1;
+				goto out;
 			}
 			if (nfrags) {
 				/* Do a matching write to the tmp file */
@@ -1455,12 +1443,8 @@ packfile(char *fname, char *tname, int fd,
 		}
 	}
 	ftruncate64(tfd, statp->bs_size);
-	if (ffd != -1)
-		close(ffd);
 	fsync(tfd);
 
-	free(fbuf);
-
 	sx.sx_stat     = *statp; /* struct copy */
 	sx.sx_version  = XFS_SX_VERSION;
 	sx.sx_fdtarget = fd;
@@ -1473,8 +1457,7 @@ packfile(char *fname, char *tname, int fd,
                 if (vflag)
                         fsrprintf(_("failed to fchown tmpfile %s: %s\n"),
                                    tname, strerror(errno));
-		close(tfd);
-                return -1;
+		goto out;
         }
 
 	/* Swap the extents */
@@ -1496,8 +1479,7 @@ packfile(char *fname, char *tname, int fd,
 			fsrprintf(_("XFS_IOC_SWAPEXT failed: %s: %s\n"),
 				  fname, strerror(errno));
 		}
-		close(tfd);
-		return -1;
+		goto out;
 	}
 
 	/* Report progress */
@@ -1506,8 +1488,15 @@ packfile(char *fname, char *tname, int fd,
 			  cur_nextents, new_nextents,
 			  (new_nextents <= nextents ? "DONE" : "    " ),
 		          fname);
-	close(tfd);
-	return 0;
+	retval = 0;
+
+out:
+	free(fbuf);
+	if (tfd != -1)
+		close(tfd);
+	if (ffd != -1)
+		close(ffd);
+	return retval;
 }
 
 char *

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/3] xfs_fsr: create a cleanup/return target in packfile()

[Brian Foster]

On Fri, Jun 06, 2014 at 04:04:37PM -0500, Eric Sandeen wrote:
> Error handling is a mishmash of closes, frees, etc at every
> error point.  Create an "out" target that does this all
> in one place.
> 
> Minor comment/doc update while we're at it.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

> 
> diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> index 94d235c..8b191e6 100644
> --- a/fsr/xfs_fsr.c
> +++ b/fsr/xfs_fsr.c
> @@ -1205,14 +1205,20 @@ out:
>   * We already are pretty sure we can and want to
>   * defragment the file.  Create the tmp file, copy
>   * the data (maintaining holes) and call the kernel
> - * extent swap routinte.
> + * extent swap routine.
> + *
> + * Return values:
> + * -1: Some error was encountered
> + *  0: Successfully defragmented the file
> + *  1: No change / No Error
>   */
>  static int
>  packfile(char *fname, char *tname, int fd,
>  	 xfs_bstat_t *statp, struct fsxattr *fsxp)
>  {
> -	int 		tfd;
> +	int 		tfd = -1;
>  	int		srval;
> +	int		retval = -1;	/* Failure is the default */
>  	int		nextents, extent, cur_nextents, new_nextents;
>  	unsigned	blksz_dio;
>  	unsigned	dio_min;
> @@ -1220,7 +1226,7 @@ packfile(char *fname, char *tname, int fd,
>  	static xfs_swapext_t   sx;
>  	struct xfs_flock64  space;
>  	off64_t 	cnt, pos;
> -	void 		*fbuf;
> +	void 		*fbuf = NULL;
>  	int 		ct, wc, wc_b4;
>  	char		ffname[SMBUFSZ];
>  	int		ffd = -1;
> @@ -1236,7 +1242,8 @@ packfile(char *fname, char *tname, int fd,
>  	if (cur_nextents == 1 || cur_nextents <= nextents) {
>  		if (vflag)
>  			fsrprintf(_("%s already fully defragmented.\n"), fname);
> -		return 1; /* indicates no change/no error */
> +		retval = 1; /* indicates no change/no error */
> +		goto out;
>  	}
>  
>  	if (dflag)
> @@ -1248,15 +1255,14 @@ packfile(char *fname, char *tname, int fd,
>  		if (vflag)
>  			fsrprintf(_("could not open tmp file: %s: %s\n"),
>  				   tname, strerror(errno));
> -		return -1;
> +		goto out;
>  	}
>  	unlink(tname);
>  
>  	/* Setup extended attributes */
>  	if (fsr_setup_attr_fork(fd, tfd, statp) != 0) {
>  		fsrprintf(_("failed to set ATTR fork on tmp: %s:\n"), tname);
> -		close(tfd);
> -		return -1;
> +		goto out;
>  	}
>  
>  	/* Setup extended inode flags, project identifier, etc */
> @@ -1264,15 +1270,13 @@ packfile(char *fname, char *tname, int fd,
>  		if (ioctl(tfd, XFS_IOC_FSSETXATTR, fsxp) < 0) {
>  			fsrprintf(_("could not set inode attrs on tmp: %s\n"),
>  				tname);
> -			close(tfd);
> -			return -1;
> +			goto out;
>  		}
>  	}
>  
>  	if ((ioctl(tfd, XFS_IOC_DIOINFO, &dio)) < 0 ) {
>  		fsrprintf(_("could not get DirectIO info on tmp: %s\n"), tname);
> -		close(tfd);
> -		return -1;
> +		goto out;
>  	}
>  
>  	dio_min = dio.d_miniosz;
> @@ -1294,8 +1298,7 @@ packfile(char *fname, char *tname, int fd,
>  
>  	if (!(fbuf = (char *)memalign(dio.d_mem, blksz_dio))) {
>  		fsrprintf(_("could not allocate buf: %s\n"), tname);
> -		close(tfd);
> -		return -1;
> +		goto out;
>  	}
>  
>  	if (nfrags) {
> @@ -1306,9 +1309,7 @@ packfile(char *fname, char *tname, int fd,
>  		if ((ffd = open(ffname, openopts, 0666)) < 0) {
>  			fsrprintf(_("could not open fragfile: %s : %s\n"),
>  				   ffname, strerror(errno));
> -			close(tfd);
> -			free(fbuf);
> -			return -1;
> +			goto out;
>  		}
>  		unlink(ffname);
>  	}
> @@ -1338,9 +1339,7 @@ packfile(char *fname, char *tname, int fd,
>  			if (ioctl(tfd, XFS_IOC_RESVSP64, &space) < 0) {
>  				fsrprintf(_("could not pre-allocate tmp space:"
>  					" %s\n"), tname);
> -				close(tfd);
> -				free(fbuf);
> -				return -1;
> +				goto out;
>  			}
>  			lseek64(tfd, outmap[extent].bmv_length, SEEK_CUR);
>  		}
> @@ -1348,11 +1347,7 @@ packfile(char *fname, char *tname, int fd,
>  
>  	if (lseek64(tfd, 0, SEEK_SET)) {
>  		fsrprintf(_("Couldn't rewind on temporary file\n"));
> -		close(tfd);
> -		if (ffd != -1)
> -			close(ffd);
> -		free(fbuf);
> -		return -1;
> +		goto out;
>  	}
>  
>  	/* Check if the temporary file has fewer extents */
> @@ -1362,11 +1357,8 @@ packfile(char *fname, char *tname, int fd,
>  	if (cur_nextents <= new_nextents) {
>  		if (vflag)
>  			fsrprintf(_("No improvement will be made (skipping): %s\n"), fname);
> -		free(fbuf);
> -		close(tfd);
> -		if (ffd != -1)
> -			close(ffd);
> -		return 1; /* no change/no error */
> +		retval = 1; /* no change/no error */
> +		goto out;
>  	}
>  
>  	/* Loop through block map copying the file. */
> @@ -1437,11 +1429,7 @@ packfile(char *fname, char *tname, int fd,
>  							tname);
>  					}
>  				}
> -				free(fbuf);
> -				close(tfd);
> -				if (ffd != -1)
> -					close(ffd);
> -				return -1;
> +				goto out;
>  			}
>  			if (nfrags) {
>  				/* Do a matching write to the tmp file */
> @@ -1455,12 +1443,8 @@ packfile(char *fname, char *tname, int fd,
>  		}
>  	}
>  	ftruncate64(tfd, statp->bs_size);
> -	if (ffd != -1)
> -		close(ffd);
>  	fsync(tfd);
>  
> -	free(fbuf);
> -
>  	sx.sx_stat     = *statp; /* struct copy */
>  	sx.sx_version  = XFS_SX_VERSION;
>  	sx.sx_fdtarget = fd;
> @@ -1473,8 +1457,7 @@ packfile(char *fname, char *tname, int fd,
>                  if (vflag)
>                          fsrprintf(_("failed to fchown tmpfile %s: %s\n"),
>                                     tname, strerror(errno));
> -		close(tfd);
> -                return -1;
> +		goto out;
>          }
>  
>  	/* Swap the extents */
> @@ -1496,8 +1479,7 @@ packfile(char *fname, char *tname, int fd,
>  			fsrprintf(_("XFS_IOC_SWAPEXT failed: %s: %s\n"),
>  				  fname, strerror(errno));
>  		}
> -		close(tfd);
> -		return -1;
> +		goto out;
>  	}
>  
>  	/* Report progress */
> @@ -1506,8 +1488,15 @@ packfile(char *fname, char *tname, int fd,
>  			  cur_nextents, new_nextents,
>  			  (new_nextents <= nextents ? "DONE" : "    " ),
>  		          fname);
> -	close(tfd);
> -	return 0;
> +	retval = 0;
> +
> +out:
> +	free(fbuf);
> +	if (tfd != -1)
> +		close(tfd);
> +	if (ffd != -1)
> +		close(ffd);
> +	return retval;
>  }
>  
>  char *
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 3/3] xfs_fsr: test for more potential failures in packfile()

[Eric Sandeen]

Test for lseek, ftruncate, and fsync failures in packfile()

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
index 8b191e6..48629fd 100644
--- a/fsr/xfs_fsr.c
+++ b/fsr/xfs_fsr.c
@@ -1325,7 +1325,11 @@ packfile(char *fname, char *tname, int fd,
 				fsrprintf(_("could not trunc tmp %s\n"),
 					   tname);
 			}
-			lseek64(tfd, outmap[extent].bmv_length, SEEK_CUR);
+			if (lseek64(tfd, outmap[extent].bmv_length, SEEK_CUR) < 0) {
+				fsrprintf(_("could not lseek in tmpfile: %s : %s\n"),
+				   tname, strerror(errno));
+				goto out;
+			}
 			continue;
 		} else if (outmap[extent].bmv_length == 0) {
 			/* to catch holes at the beginning of the file */
@@ -1341,7 +1345,11 @@ packfile(char *fname, char *tname, int fd,
 					" %s\n"), tname);
 				goto out;
 			}
-			lseek64(tfd, outmap[extent].bmv_length, SEEK_CUR);
+			if (lseek64(tfd, outmap[extent].bmv_length, SEEK_CUR) < 0) {
+				fsrprintf(_("could not lseek in tmpfile: %s : %s\n"),
+				   tname, strerror(errno));
+				goto out;
+			}
 		}
 	} /* end of space allocation loop */
 
@@ -1365,8 +1373,16 @@ packfile(char *fname, char *tname, int fd,
 	for (extent = 0; extent < nextents; extent++) {
 		pos = outmap[extent].bmv_offset;
 		if (outmap[extent].bmv_block == -1) {
-			lseek64(tfd, outmap[extent].bmv_length, SEEK_CUR);
-			lseek64(fd, outmap[extent].bmv_length, SEEK_CUR);
+			if (lseek64(tfd, outmap[extent].bmv_length, SEEK_CUR) < 0) {
+				fsrprintf(_("could not lseek in tmpfile: %s : %s\n"),
+				   tname, strerror(errno));
+				goto out;
+			}
+			if (lseek64(fd, outmap[extent].bmv_length, SEEK_CUR) < 0) {
+				fsrprintf(_("could not lseek in file: %s : %s\n"),
+				   fname, strerror(errno));
+				goto out;
+			}
 			continue;
 		} else if (outmap[extent].bmv_length == 0) {
 			/* to catch holes at the beginning of the file */
@@ -1442,8 +1458,16 @@ packfile(char *fname, char *tname, int fd,
 			}
 		}
 	}
-	ftruncate64(tfd, statp->bs_size);
-	fsync(tfd);
+	if (ftruncate64(tfd, statp->bs_size) < 0) {
+		fsrprintf(_("could not truncate tmpfile: %s : %s\n"),
+				fname, strerror(errno));
+		goto out;
+	}
+	if (fsync(tfd) < 0) {
+		fsrprintf(_("could not fsync tmpfile: %s : %s\n"),
+				fname, strerror(errno));
+		goto out;
+	}
 
 	sx.sx_stat     = *statp; /* struct copy */
 	sx.sx_version  = XFS_SX_VERSION;

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/3] xfs_fsr: test for more potential failures in packfile()

[Brian Foster]

On Fri, Jun 06, 2014 at 04:06:47PM -0500, Eric Sandeen wrote:
> Test for lseek, ftruncate, and fsync failures in packfile()
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

> 
> diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> index 8b191e6..48629fd 100644
> --- a/fsr/xfs_fsr.c
> +++ b/fsr/xfs_fsr.c
> @@ -1325,7 +1325,11 @@ packfile(char *fname, char *tname, int fd,
>  				fsrprintf(_("could not trunc tmp %s\n"),
>  					   tname);
>  			}
> -			lseek64(tfd, outmap[extent].bmv_length, SEEK_CUR);
> +			if (lseek64(tfd, outmap[extent].bmv_length, SEEK_CUR) < 0) {
> +				fsrprintf(_("could not lseek in tmpfile: %s : %s\n"),
> +				   tname, strerror(errno));
> +				goto out;
> +			}
>  			continue;
>  		} else if (outmap[extent].bmv_length == 0) {
>  			/* to catch holes at the beginning of the file */
> @@ -1341,7 +1345,11 @@ packfile(char *fname, char *tname, int fd,
>  					" %s\n"), tname);
>  				goto out;
>  			}
> -			lseek64(tfd, outmap[extent].bmv_length, SEEK_CUR);
> +			if (lseek64(tfd, outmap[extent].bmv_length, SEEK_CUR) < 0) {
> +				fsrprintf(_("could not lseek in tmpfile: %s : %s\n"),
> +				   tname, strerror(errno));
> +				goto out;
> +			}
>  		}
>  	} /* end of space allocation loop */
>  
> @@ -1365,8 +1373,16 @@ packfile(char *fname, char *tname, int fd,
>  	for (extent = 0; extent < nextents; extent++) {
>  		pos = outmap[extent].bmv_offset;
>  		if (outmap[extent].bmv_block == -1) {
> -			lseek64(tfd, outmap[extent].bmv_length, SEEK_CUR);
> -			lseek64(fd, outmap[extent].bmv_length, SEEK_CUR);
> +			if (lseek64(tfd, outmap[extent].bmv_length, SEEK_CUR) < 0) {
> +				fsrprintf(_("could not lseek in tmpfile: %s : %s\n"),
> +				   tname, strerror(errno));
> +				goto out;
> +			}
> +			if (lseek64(fd, outmap[extent].bmv_length, SEEK_CUR) < 0) {
> +				fsrprintf(_("could not lseek in file: %s : %s\n"),
> +				   fname, strerror(errno));
> +				goto out;
> +			}
>  			continue;
>  		} else if (outmap[extent].bmv_length == 0) {
>  			/* to catch holes at the beginning of the file */
> @@ -1442,8 +1458,16 @@ packfile(char *fname, char *tname, int fd,
>  			}
>  		}
>  	}
> -	ftruncate64(tfd, statp->bs_size);
> -	fsync(tfd);
> +	if (ftruncate64(tfd, statp->bs_size) < 0) {
> +		fsrprintf(_("could not truncate tmpfile: %s : %s\n"),
> +				fname, strerror(errno));
> +		goto out;
> +	}
> +	if (fsync(tfd) < 0) {
> +		fsrprintf(_("could not fsync tmpfile: %s : %s\n"),
> +				fname, strerror(errno));
> +		goto out;
> +	}
>  
>  	sx.sx_stat     = *statp; /* struct copy */
>  	sx.sx_version  = XFS_SX_VERSION;
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs