On Fri, Aug 01, 2014 at 09:59:15AM -0500, Eric Sandeen wrote:
> The allocated fshandlep leaks on most error paths;
> restructure with an out: target that does all necessary
> freeing, and initialize filehandles to -1 so that we
> know whether they need to be closed on the error path.
>
> While we're at it, if gettmpname() fails, we still
> return 0 for an error, because error is initialized
> to 0 and only set otherwise by fsrfile_common.
> So if gettmpname() fails, we return success from the
> function even though we did no work. Fix that
> as well by initializing error to -1.
>
> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> ---
> fsr/xfs_fsr.c | 28 ++++++++++++++--------------
> 1 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> index 48629fd..752d2db 100644
> --- a/fsr/xfs_fsr.c
> +++ b/fsr/xfs_fsr.c
> @@ -809,15 +809,15 @@ fsrfile(char *fname, xfs_ino_t ino)
> {
> xfs_bstat_t statbuf;
> jdm_fshandle_t *fshandlep;
> - int fd, fsfd;
> - int error = 0;
> + int fd = -1, fsfd = -1;
> + int error = -1;
> char *tname;
>
> fshandlep = jdm_getfshandle(getparent (fname) );
> - if (! fshandlep) {
> + if (!fshandlep) {
> fsrprintf(_("unable to construct sys handle for %s: %s\n"),
> fname, strerror(errno));
> - return -1;
> + goto out;
> }
>
> /*
> @@ -828,39 +828,39 @@ fsrfile(char *fname, xfs_ino_t ino)
> if (fsfd < 0) {
> fsrprintf(_("unable to open sys handle for %s: %s\n"),
> fname, strerror(errno));
> - return -1;
> + goto out;
> }
>
> if ((xfs_bulkstat_single(fsfd, &ino, &statbuf)) < 0) {
> fsrprintf(_("unable to get bstat on %s: %s\n"),
> fname, strerror(errno));
> - close(fsfd);
> - return -1;
> + goto out;
> }
>
> fd = jdm_open(fshandlep, &statbuf, O_RDWR|O_DIRECT);
> if (fd < 0) {
> fsrprintf(_("unable to open handle %s: %s\n"),
> fname, strerror(errno));
> - close(fsfd);
> - return -1;
> + goto out;
> }
>
> /* Get the fs geometry */
> if (xfs_getgeom(fsfd, &fsgeom) < 0 ) {
> fsrprintf(_("Unable to get geom on fs for: %s\n"), fname);
> - close(fsfd);
> - return -1;
> + goto out;
> }
>
> - close(fsfd);
> -
> tname = gettmpname(fname);
>
> if (tname)
> error = fsrfile_common(fname, tname, NULL, fd, &statbuf);
>
I was wondering whether this bit to not fail if the path is bad was
intentional (e.g., to avoid breaking through a higher-level loop or
something), but we don't check the return value of this function
anyways. :-|
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> - close(fd);
> +out:
> + if (fsfd >= 0)
> + close(fsfd);
> + if (fd >= 0)
> + close(fd);
> + free(fshandlep);
>
> return error;
> }
> --
> 1.7.1
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
