xfs
[Top] [All Lists]

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

To: Eric Sandeen <sandeen@xxxxxxxxxxx>
Subject: Re: [PATCH 2/3] xfs_fsr: create a cleanup/return target in packfile()
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Mon, 9 Jun 2014 09:02:41 -0400
Cc: Eric Sandeen <sandeen@xxxxxxxxxx>, xfs-oss <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <53922CE5.4000401@xxxxxxxxxxx>
References: <53922B49.1050005@xxxxxxxxxx> <53922CE5.4000401@xxxxxxxxxxx>
User-agent: Mutt/1.5.23 (2014-03-12)
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@xxxxxxxxxx>
> ---

Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>

> 
> 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@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs

<Prev in Thread] Current Thread [Next in Thread>