[PATCH 00/16] btrfs-progs: Several more static analysis defect fixes

[PATCH 00/16] btrfs-progs: Several more static analysis defect fixes

[Eric Sandeen]

These all apply to the integration branch in Chris's current git tree.
(Which saw the defects rise by from 55 to 65 in the last l6 commits :()

This beats back the defect count again.  Compile tested only, FWIW.

Thanks,
-Eric

[PATCH 01/16] btrfs-progs: fix potential double-frees in cmd_subvol_delete()

[Eric Sandeen]

If we "goto again" in cmd_subvol_delete(), and error out to out:
before re-allocating the dupdname and dupvname pointers, we'll
double-free them.

Set them to NULL after freeing to avoid this.

Resolves-Coverity-CID: 1125944
Resolves-Coverity-CID: 1125945
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-subvolume.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index 63c708e..89b90cf 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -288,6 +288,8 @@ again:
 out:
 	free(dupdname);
 	free(dupvname);
+	dupdname = NULL;
+	dupvname = NULL;
 	cnt++;
 	if (cnt < argc)
 		goto again;
-- 
1.7.1

[PATCH 02/16] btrfs-progs: fix error returns in get_df()

[Eric Sandeen]

get_df returns -ERRNO, or maybe (+)errno, or even 0 in
the case where we inexplicably got 0 total_spaces from
the BTRFS_IOC_SPACE_INFO.

Consistently return a negative error number, and return
-ENOENT rather than 0 for total_spaces == 0, so that the
caller will know that **sargs_ret hasn't been set up.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-filesystem.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 0bfd710..e6642ef 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -106,11 +106,12 @@ static int get_df(int fd, struct btrfs_ioctl_space_args **sargs_ret)
 		fprintf(stderr, "ERROR: couldn't get space info - %s\n",
 			strerror(e));
 		free(sargs);
-		return ret;
+		return -e;
 	}
+	/* This really should never happen */
 	if (!sargs->total_spaces) {
 		free(sargs);
-		return 0;
+		return -ENOENT;
 	}
 	count = sargs->total_spaces;
 	free(sargs);
@@ -128,7 +129,7 @@ static int get_df(int fd, struct btrfs_ioctl_space_args **sargs_ret)
 		fprintf(stderr, "ERROR: get space info count %llu - %s\n",
 				count, strerror(e));
 		free(sargs);
-		return ret;
+		return -e;
 	}
 	*sargs_ret = sargs;
 	return 0;
-- 
1.7.1

Re: [PATCH 02/16] btrfs-progs: fix error returns in get_df()

[Anand Jain]

  I had just retained whats in the original. But this is
  good change. Thanks Eric.

Reviewed-by: Anand Jain <anand.jain@oracle.com>


On 11/07/2013 07:15 AM, Eric Sandeen wrote:
> get_df returns -ERRNO, or maybe (+)errno, or even 0 in
> the case where we inexplicably got 0 total_spaces from
> the BTRFS_IOC_SPACE_INFO.
>
> Consistently return a negative error number, and return
> -ENOENT rather than 0 for total_spaces == 0, so that the
> caller will know that **sargs_ret hasn't been set up.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>   cmds-filesystem.c |    7 ++++---
>   1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index 0bfd710..e6642ef 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
> @@ -106,11 +106,12 @@ static int get_df(int fd, struct btrfs_ioctl_space_args **sargs_ret)
>   		fprintf(stderr, "ERROR: couldn't get space info - %s\n",
>   			strerror(e));
>   		free(sargs);
> -		return ret;
> +		return -e;
>   	}
> +	/* This really should never happen */
>   	if (!sargs->total_spaces) {
>   		free(sargs);
> -		return 0;
> +		return -ENOENT;
>   	}
>   	count = sargs->total_spaces;
>   	free(sargs);
> @@ -128,7 +129,7 @@ static int get_df(int fd, struct btrfs_ioctl_space_args **sargs_ret)
>   		fprintf(stderr, "ERROR: get space info count %llu - %s\n",
>   				count, strerror(e));
>   		free(sargs);
> -		return ret;
> +		return -e;
>   	}
>   	*sargs_ret = sargs;
>   	return 0;
>

[PATCH 03/16] btrfs-progs: use strncpy in btrfs_scan_lblkid()

[Eric Sandeen]

Use strncpy(... ,PATH_MAX) to be sure we don't overflow
the path[PATH_MAX] array.

Resolves-Coverity-CID: 1125941
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 utils.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/utils.c b/utils.c
index a7441c1..c11a7c2 100644
--- a/utils.c
+++ b/utils.c
@@ -1952,7 +1952,7 @@ int btrfs_scan_lblkid(int update_kernel)
 		if (!dev)
 			continue;
 		/* if we are here its definitly a btrfs disk*/
-		strcpy(path, blkid_dev_devname(dev));
+		strncpy(path, blkid_dev_devname(dev), PATH_MAX);
 		if (test_skip_this_disk(path))
 			continue;
 
-- 
1.7.1

[PATCH] btrfs-progs: use strncpy in btrfs_scan_lblkid()

[Anand Jain]

From: Eric Sandeen <sandeen@redhat.com>

Use strncpy(... ,PATH_MAX) to be sure we don't overflow
the path[PATH_MAX] array.

Resolves-Coverity-CID: 1125941
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 utils.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/utils.c b/utils.c
index 753169b..ebd10e5 100644
--- a/utils.c
+++ b/utils.c
@@ -1981,8 +1981,8 @@ int btrfs_scan_lblkid(int update_kernel)
 		dev = blkid_verify(cache, dev);
 		if (!dev)
 			continue;
-		/* if we are here its definitly a btrfs disk*/
-		strcpy(path, blkid_dev_devname(dev));
+		/* if we are here its definitely a btrfs disk*/
+		strncpy(path, blkid_dev_devname(dev), PATH_MAX);
 		if (test_skip_this_disk(path))
 			continue;
 
-- 
1.7.1

[PATCH 04/16] btrfs-progs: fix test for return of realpath in find_mount_root()

[Eric Sandeen]

find_mount_root() tries to test for realpath() failure, but
tests the wrong value.  Fix it.

Resolves-Coverity-CID: 1125940
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-send.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/cmds-send.c b/cmds-send.c
index 39110e7..53e9a53 100644
--- a/cmds-send.c
+++ b/cmds-send.c
@@ -98,7 +98,7 @@ int find_mount_root(const char *path, char **mount_root)
 
 	ret = 0;
 	*mount_root = realpath(longest_match, NULL);
-	if (!mount_root)
+	if (!*mount_root)
 		ret = -errno;
 
 	free(longest_match);
-- 
1.7.1

Re: [PATCH 04/16] btrfs-progs: fix test for return of realpath in find_mount_root()

[Anand Jain]

Reviewed-by: Anand Jain <anand.jain@oracle.com>


On 11/07/2013 07:15 AM, Eric Sandeen wrote:
> find_mount_root() tries to test for realpath() failure, but
> tests the wrong value.  Fix it.
>
> Resolves-Coverity-CID: 1125940
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>   cmds-send.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/cmds-send.c b/cmds-send.c
> index 39110e7..53e9a53 100644
> --- a/cmds-send.c
> +++ b/cmds-send.c
> @@ -98,7 +98,7 @@ int find_mount_root(const char *path, char **mount_root)
>
>   	ret = 0;
>   	*mount_root = realpath(longest_match, NULL);
> -	if (!mount_root)
> +	if (!*mount_root)
>   		ret = -errno;
>
>   	free(longest_match);
>

[PATCH 05/16] btrfs-progs: don't leak fd in test_dev_for_mkfs() error paths

[Eric Sandeen]

Close fd before we return on error paths.

Resolves-Coverity-CID: 1125939
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 utils.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/utils.c b/utils.c
index c11a7c2..c784345 100644
--- a/utils.c
+++ b/utils.c
@@ -1905,10 +1905,12 @@ int test_dev_for_mkfs(char *file, int force_overwrite, char *estr)
 	if (fstat(fd, &st)) {
 		snprintf(estr, sz, "unable to stat %s: %s\n", file,
 			strerror(errno));
+		close(fd);
 		return 1;
 	}
 	if (!S_ISBLK(st.st_mode)) {
 		fprintf(stderr, "'%s' is not a block device\n", file);
+		close(fd);
 		return 1;
 	}
 	close(fd);
-- 
1.7.1

Re: [PATCH 05/16] btrfs-progs: don't leak fd in test_dev_for_mkfs() error paths

[Anand Jain]

Reviewed-by: Anand Jain <anand.jain@oracle.com>


On 11/07/2013 07:15 AM, Eric Sandeen wrote:
> Close fd before we return on error paths.
>
> Resolves-Coverity-CID: 1125939
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>   utils.c |    2 ++
>   1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/utils.c b/utils.c
> index c11a7c2..c784345 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -1905,10 +1905,12 @@ int test_dev_for_mkfs(char *file, int force_overwrite, char *estr)
>   	if (fstat(fd, &st)) {
>   		snprintf(estr, sz, "unable to stat %s: %s\n", file,
>   			strerror(errno));
> +		close(fd);
>   		return 1;
>   	}
>   	if (!S_ISBLK(st.st_mode)) {
>   		fprintf(stderr, "'%s' is not a block device\n", file);
> +		close(fd);
>   		return 1;
>   	}
>   	close(fd);
>

[PATCH 06/16] btrfs-progs: fix leak of "buf" in make_btrfs() error paths

[Eric Sandeen]

If any pwrite failed we leaked the allocated "buf" on
return from the function.  "goto out" takes care of
those paths.

Resolves-Coverity-CID: 1125938
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 utils.c |   62 ++++++++++++++++++++++++++++++++------------------------------
 1 files changed, 32 insertions(+), 30 deletions(-)

diff --git a/utils.c b/utils.c
index c784345..d8ce153 100644
--- a/utils.c
+++ b/utils.c
@@ -210,10 +210,10 @@ int make_btrfs(int fd, const char *device, const char *label,
 
 	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 	ret = pwrite(fd, buf->data, leafsize, blocks[1]);
-	if (ret < 0)
-		return -errno;
-	else if (ret != leafsize)
-		return -EIO;
+	if (ret != leafsize) {
+		ret = (ret < 0 ? -errno : -EIO);
+		goto out;
+	}
 
 	/* create the items for the extent tree */
 	memset(buf->data+sizeof(struct btrfs_header), 0,
@@ -269,10 +269,10 @@ int make_btrfs(int fd, const char *device, const char *label,
 	btrfs_set_header_nritems(buf, nritems);
 	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 	ret = pwrite(fd, buf->data, leafsize, blocks[2]);
-	if (ret < 0)
-		return -errno;
-	else if (ret != leafsize)
-		return -EIO;
+	if (ret != leafsize) {
+		ret = (ret < 0 ? -errno : -EIO);
+		goto out;
+	}
 
 	/* create the chunk tree */
 	memset(buf->data+sizeof(struct btrfs_header), 0,
@@ -356,10 +356,10 @@ int make_btrfs(int fd, const char *device, const char *label,
 	btrfs_set_header_nritems(buf, nritems);
 	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 	ret = pwrite(fd, buf->data, leafsize, blocks[3]);
-	if (ret < 0)
-		return -errno;
-	else if (ret != leafsize)
-		return -EIO;
+	if (ret != leafsize) {
+		ret = (ret < 0 ? -errno : -EIO);
+		goto out;
+	}
 
 	/* create the device tree */
 	memset(buf->data+sizeof(struct btrfs_header), 0,
@@ -395,10 +395,10 @@ int make_btrfs(int fd, const char *device, const char *label,
 	btrfs_set_header_nritems(buf, nritems);
 	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 	ret = pwrite(fd, buf->data, leafsize, blocks[4]);
-	if (ret < 0)
-		return -errno;
-	else if (ret != leafsize)
-		return -EIO;
+	if (ret != leafsize) {
+		ret = (ret < 0 ? -errno : -EIO);
+		goto out;
+	}
 
 	/* create the FS root */
 	memset(buf->data+sizeof(struct btrfs_header), 0,
@@ -408,11 +408,10 @@ int make_btrfs(int fd, const char *device, const char *label,
 	btrfs_set_header_nritems(buf, 0);
 	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 	ret = pwrite(fd, buf->data, leafsize, blocks[5]);
-	if (ret < 0)
-		return -errno;
-	else if (ret != leafsize)
-		return -EIO;
-
+	if (ret != leafsize) {
+		ret = (ret < 0 ? -errno : -EIO);
+		goto out;
+	}
 	/* finally create the csum root */
 	memset(buf->data+sizeof(struct btrfs_header), 0,
 		leafsize-sizeof(struct btrfs_header));
@@ -421,10 +420,10 @@ int make_btrfs(int fd, const char *device, const char *label,
 	btrfs_set_header_nritems(buf, 0);
 	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 	ret = pwrite(fd, buf->data, leafsize, blocks[6]);
-	if (ret < 0)
-		return -errno;
-	else if (ret != leafsize)
-		return -EIO;
+	if (ret != leafsize) {
+		ret = (ret < 0 ? -errno : -EIO);
+		goto out;
+	}
 
 	/* and write out the super block */
 	BUG_ON(sizeof(super) > sectorsize);
@@ -433,13 +432,16 @@ int make_btrfs(int fd, const char *device, const char *label,
 	buf->len = sectorsize;
 	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 	ret = pwrite(fd, buf->data, sectorsize, blocks[0]);
-	if (ret < 0)
-		return -errno;
-	else if (ret != sectorsize)
-		return -EIO;
+	if (ret != sectorsize) {
+		ret = (ret < 0 ? -errno : -EIO);
+		goto out;
+	}
+
+	ret = 0;
 
+out:
 	free(buf);
-	return 0;
+	return ret;
 }
 
 u64 btrfs_device_size(int fd, struct stat *st)
-- 
1.7.1

[PATCH 07/16] btrfs-progs: don't leak buffer on add_file_items() error

[Eric Sandeen]

add_file_items() leaked "buffer" on this error return.
Free it first.

Resolves-Coverity-CID: 1125937
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 mkfs.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/mkfs.c b/mkfs.c
index d576797..f29f5cd 100644
--- a/mkfs.c
+++ b/mkfs.c
@@ -629,6 +629,7 @@ static int add_file_items(struct btrfs_trans_handle *trans,
 		ret_read = pread64(fd, buffer, st->st_size, bytes_read);
 		if (ret_read == -1) {
 			fprintf(stderr, "%s read failed\n", path_name);
+			free(buffer);
 			goto end;
 		}
 
-- 
1.7.1

[PATCH 08/16] btrfs-progs: fix resource leak in scrub_start()

[Eric Sandeen]

In the "nothing to resume" case we return directly and leak
several bits of memory; goto out to free them properly.

Resolves-Coverity-CID: 1125934
Resolves-Coverity-CID: 1125935
Resolves-Coverity-CID: 1125936
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-scrub.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/cmds-scrub.c b/cmds-scrub.c
index 605af45..5f3eade 100644
--- a/cmds-scrub.c
+++ b/cmds-scrub.c
@@ -1261,7 +1261,8 @@ static int scrub_start(int argc, char **argv, int resume)
 		if (!do_quiet)
 			printf("scrub: nothing to resume for %s, fsid %s\n",
 			       path, fsid);
-		return 2;
+		err = 2;
+		goto out;
 	}
 
 	ret = prg_fd = socket(AF_UNIX, SOCK_STREAM, 0);
-- 
1.7.1

Re: [PATCH 08/16] btrfs-progs: fix resource leak in scrub_start()

[Wang Shilong]

Hi Eric,

On 11/07/2013 07:15 AM, Eric Sandeen wrote:
> In the "nothing to resume" case we return directly and leak
> several bits of memory; goto out to free them properly.
>
> Resolves-Coverity-CID: 1125934
> Resolves-Coverity-CID: 1125935
> Resolves-Coverity-CID: 1125936
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>   cmds-scrub.c |    3 ++-
>   1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/cmds-scrub.c b/cmds-scrub.c
> index 605af45..5f3eade 100644
> --- a/cmds-scrub.c
> +++ b/cmds-scrub.c
> @@ -1261,7 +1261,8 @@ static int scrub_start(int argc, char **argv, int resume)
>   		if (!do_quiet)
>   			printf("scrub: nothing to resume for %s, fsid %s\n",
>   			       path, fsid);
> -		return 2;
> +		err = 2;
> +		goto out;
Thanks for tracking this problem, but
i intend to return 2 in such case originally.
return '!err' will revert to return 1 rather than 2.

Thanks,
Wang
>   	}
>   
>   	ret = prg_fd = socket(AF_UNIX, SOCK_STREAM, 0);

Re: [PATCH 08/16] btrfs-progs: fix resource leak in scrub_start()

[Wang Shilong]

On 11/07/2013 09:48 AM, Wang Shilong wrote:
> Hi Eric,
>
> On 11/07/2013 07:15 AM, Eric Sandeen wrote:
>> In the "nothing to resume" case we return directly and leak
>> several bits of memory; goto out to free them properly.
>>
>> Resolves-Coverity-CID: 1125934
>> Resolves-Coverity-CID: 1125935
>> Resolves-Coverity-CID: 1125936
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>   cmds-scrub.c |    3 ++-
>>   1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/cmds-scrub.c b/cmds-scrub.c
>> index 605af45..5f3eade 100644
>> --- a/cmds-scrub.c
>> +++ b/cmds-scrub.c
>> @@ -1261,7 +1261,8 @@ static int scrub_start(int argc, char **argv, 
>> int resume)
>>           if (!do_quiet)
>>               printf("scrub: nothing to resume for %s, fsid %s\n",
>>                      path, fsid);
>> -        return 2;
>> +        err = 2;
>> +        goto out;
> Thanks for tracking this problem, but
> i intend to return 2 in such case originally.
> return '!err' will revert to return 1 rather than 2.
see label out:

if (err)
     return 1

>
> Thanks,
> Wang
>>       }
>>         ret = prg_fd = socket(AF_UNIX, SOCK_STREAM, 0);
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 08/16] btrfs-progs: fix resource leak in scrub_start()

[Eric Sandeen]

On 11/6/13, 7:50 PM, Wang Shilong wrote:
> On 11/07/2013 09:48 AM, Wang Shilong wrote:
>> Hi Eric,
>>
>> On 11/07/2013 07:15 AM, Eric Sandeen wrote:
>>> In the "nothing to resume" case we return directly and leak
>>> several bits of memory; goto out to free them properly.
>>>
>>> Resolves-Coverity-CID: 1125934
>>> Resolves-Coverity-CID: 1125935
>>> Resolves-Coverity-CID: 1125936
>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>>> ---
>>>   cmds-scrub.c |    3 ++-
>>>   1 files changed, 2 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/cmds-scrub.c b/cmds-scrub.c
>>> index 605af45..5f3eade 100644
>>> --- a/cmds-scrub.c
>>> +++ b/cmds-scrub.c
>>> @@ -1261,7 +1261,8 @@ static int scrub_start(int argc, char **argv, int resume)
>>>           if (!do_quiet)
>>>               printf("scrub: nothing to resume for %s, fsid %s\n",
>>>                      path, fsid);
>>> -        return 2;
>>> +        err = 2;
>>> +        goto out;
>> Thanks for tracking this problem, but
>> i intend to return 2 in such case originally.
>> return '!err' will revert to return 1 rather than 2.
> see label out:
> 
> if (err)
>     return 1
> 

Ah, whoops.  Ok, well we still need to fix the leak.  

I just expected that setting err & going to out would return err ;)

So probably:

if (err)
   return err;

will work.

I'll send a V2.

Thanks for catching it on review!

-Eric

Re: [PATCH 08/16] btrfs-progs: fix resource leak in scrub_start()

[Wang Shilong]

On 11/07/2013 11:46 AM, Eric Sandeen wrote:
> On 11/6/13, 7:50 PM, Wang Shilong wrote:
>> On 11/07/2013 09:48 AM, Wang Shilong wrote:
>>> Hi Eric,
>>>
>>> On 11/07/2013 07:15 AM, Eric Sandeen wrote:
>>>> In the "nothing to resume" case we return directly and leak
>>>> several bits of memory; goto out to free them properly.
>>>>
>>>> Resolves-Coverity-CID: 1125934
>>>> Resolves-Coverity-CID: 1125935
>>>> Resolves-Coverity-CID: 1125936
>>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>>>> ---
>>>>    cmds-scrub.c |    3 ++-
>>>>    1 files changed, 2 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/cmds-scrub.c b/cmds-scrub.c
>>>> index 605af45..5f3eade 100644
>>>> --- a/cmds-scrub.c
>>>> +++ b/cmds-scrub.c
>>>> @@ -1261,7 +1261,8 @@ static int scrub_start(int argc, char **argv, int resume)
>>>>            if (!do_quiet)
>>>>                printf("scrub: nothing to resume for %s, fsid %s\n",
>>>>                       path, fsid);
>>>> -        return 2;
>>>> +        err = 2;
>>>> +        goto out;
>>> Thanks for tracking this problem, but
>>> i intend to return 2 in such case originally.
>>> return '!err' will revert to return 1 rather than 2.
>> see label out:
>>
>> if (err)
>>      return 1
>>
> Ah, whoops.  Ok, well we still need to fix the leak.
>
> I just expected that setting err & going to out would return err ;)
>
> So probably:
>
> if (err)
>     return err;
No, we can't.
maybe add a flag, something like:

flag = 0;

/*  we set flag here for a special case */
if (!do_quient)
     printf....
flag = 2;


then in label
out:
...
/* this happen if nothing resume */
     if (flag)
         return flag
/* syntax or other error happens */
     if (err)
         return 1

Previously, 'err' can be an casual positive numbers or error number.
However, we define that we return 1 if syntax error happens.

Thanks,
Wang
>
> will work.
>
> I'll send a V2.
>
> Thanks for catching it on review!
>
> -Eric
>
>

[PATCH 09/16] btrfs-progs: btrfs_scan_kernel(): fd==0 is not an error

[Eric Sandeen]

The error return from open is -1, so test that, not 0,
for success/failure.

Resolves-Coverity-CID: 1125931
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-filesystem.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index e6642ef..df9d1ff 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -362,13 +362,13 @@ static int btrfs_scan_kernel(void *search)
 		}
 
 		fd = open(mnt->mnt_dir, O_RDONLY);
-		if (fd > 0 && !get_df(fd, &space_info_arg)) {
+		if ((fd != -1) && !get_df(fd, &space_info_arg)) {
 			get_label_mounted(mnt->mnt_dir, label);
 			print_one_fs(&fs_info_arg, dev_info_arg,
 					space_info_arg, label, mnt->mnt_dir);
 			free(space_info_arg);
 		}
-		if (fd > 0)
+		if (fd != -1)
 			close(fd);
 		free(dev_info_arg);
 	}
-- 
1.7.1

Re: [PATCH 09/16] btrfs-progs: btrfs_scan_kernel(): fd==0 is not an error

[Anand Jain]

Reviewed-by: Anand Jain <anand.jain@oracle.com>


On 11/07/2013 07:15 AM, Eric Sandeen wrote:
> The error return from open is -1, so test that, not 0,
> for success/failure.
>
> Resolves-Coverity-CID: 1125931
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>   cmds-filesystem.c |    4 ++--
>   1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index e6642ef..df9d1ff 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
> @@ -362,13 +362,13 @@ static int btrfs_scan_kernel(void *search)
>   		}
>
>   		fd = open(mnt->mnt_dir, O_RDONLY);
> -		if (fd > 0 && !get_df(fd, &space_info_arg)) {
> +		if ((fd != -1) && !get_df(fd, &space_info_arg)) {
>   			get_label_mounted(mnt->mnt_dir, label);
>   			print_one_fs(&fs_info_arg, dev_info_arg,
>   					space_info_arg, label, mnt->mnt_dir);
>   			free(space_info_arg);
>   		}
> -		if (fd > 0)
> +		if (fd != -1)
>   			close(fd);
>   		free(dev_info_arg);
>   	}
>

[PATCH 10/16] btrfs-progs: Check for open failure in btrfs_scan_lblkid()

[Eric Sandeen]

open can fail, of course.

Resolves-Coverity-CID: 1125925
Resolves-Coverity-CID: 1125930
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 utils.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/utils.c b/utils.c
index d8ce153..8471148 100644
--- a/utils.c
+++ b/utils.c
@@ -1955,12 +1955,16 @@ int btrfs_scan_lblkid(int update_kernel)
 		dev = blkid_verify(cache, dev);
 		if (!dev)
 			continue;
-		/* if we are here its definitly a btrfs disk*/
+		/* if we are here its definitely a btrfs disk*/
 		strncpy(path, blkid_dev_devname(dev), PATH_MAX);
 		if (test_skip_this_disk(path))
 			continue;
 
 		fd = open(path, O_RDONLY);
+		if (fd < 0) {
+			printf("ERROR: could not open %s\n", path);
+			continue;
+		}
 		btrfs_scan_one_device(fd, path, &tmp_devices,
 				&num_devices, BTRFS_SUPER_INFO_OFFSET);
 		close(fd);
-- 
1.7.1

[PATCH 11/16] btrfs-progs: pass positive errno to strerror in cmd_df()

[Eric Sandeen]

get_df returns a negative error number, but then
we pass it to strerror, which wants a positive value...

Resolves-Coverity-CID: 1125929
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-filesystem.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index df9d1ff..b1291d6 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -173,7 +173,7 @@ static int cmd_df(int argc, char **argv)
 		print_df(sargs);
 		free(sargs);
 	} else {
-		fprintf(stderr, "ERROR: get_df failed %s\n", strerror(ret));
+		fprintf(stderr, "ERROR: get_df failed %s\n", strerror(-ret));
 	}
 
 	close_file_or_dir(fd, dirstream);
-- 
1.7.1

Re: [PATCH 11/16] btrfs-progs: pass positive errno to strerror in cmd_df()

[Anand Jain]

Reviewed-by Anand Jain <anand.jain@oracle.com>

On 11/07/2013 07:15 AM, Eric Sandeen wrote:
> get_df returns a negative error number, but then
> we pass it to strerror, which wants a positive value...
>
> Resolves-Coverity-CID: 1125929
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>   cmds-filesystem.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index df9d1ff..b1291d6 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
> @@ -173,7 +173,7 @@ static int cmd_df(int argc, char **argv)
>   		print_df(sargs);
>   		free(sargs);
>   	} else {
> -		fprintf(stderr, "ERROR: get_df failed %s\n", strerror(ret));
> +		fprintf(stderr, "ERROR: get_df failed %s\n", strerror(-ret));
>   	}
>
>   	close_file_or_dir(fd, dirstream);
>

[PATCH 12/16] btrfs-progs: remove more dead code from check_extent_refs

[Eric Sandeen]

e0a04278 removed a bunch of dead code but left one little
bit; reinit is always 0, so btrfs_read_block_groups is
never called from here.

Resolves-Coverity-CID: 1125926
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-check.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/cmds-check.c b/cmds-check.c
index 668af15..263b4ef 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -5040,7 +5040,6 @@ static int check_extent_refs(struct btrfs_trans_handle *trans,
 	int err = 0;
 	int ret = 0;
 	int fixed = 0;
-	int reinit = 0;
 	int had_dups = 0;
 
 	if (repair) {
@@ -5066,8 +5065,6 @@ static int check_extent_refs(struct btrfs_trans_handle *trans,
 			cache = next_cache_extent(cache);
 		}
 		prune_corrupt_blocks(trans, root->fs_info);
-		if (reinit)
-			btrfs_read_block_groups(root->fs_info->extent_root);
 		reset_cached_block_groups(root->fs_info);
 	}
 
-- 
1.7.1

[PATCH 13/16] btrfs-progs: check btrfs_scan_one_device in btrfs_scan_lblkid()

[Eric Sandeen]

Even if it's "definitely" btrfs at this point,
btrfs_scan_one_device could fail for other reasons.

Check the return value, warn if it fails, and skip
the device register.

Resolves-Coverity-CID: 1125925
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 utils.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/utils.c b/utils.c
index 8471148..ecacc29 100644
--- a/utils.c
+++ b/utils.c
@@ -1937,6 +1937,7 @@ int test_skip_this_disk(char *path)
 int btrfs_scan_lblkid(int update_kernel)
 {
 	int fd = -1;
+	int ret;
 	u64 num_devices;
 	struct btrfs_fs_devices *tmp_devices;
 	blkid_dev_iterate iter = NULL;
@@ -1965,8 +1966,14 @@ int btrfs_scan_lblkid(int update_kernel)
 			printf("ERROR: could not open %s\n", path);
 			continue;
 		}
-		btrfs_scan_one_device(fd, path, &tmp_devices,
+		ret = btrfs_scan_one_device(fd, path, &tmp_devices,
 				&num_devices, BTRFS_SUPER_INFO_OFFSET);
+		if (ret) {
+			printf("ERROR: could not scan %s\n", path);
+			close (fd);
+			continue;
+		}
+
 		close(fd);
 		if (update_kernel)
 			btrfs_register_one_device(path);
-- 
1.7.1

Re: [PATCH 13/16] btrfs-progs: check btrfs_scan_one_device in btrfs_scan_lblkid()

[Anand Jain]

reviewed-by: Anand Jain <anand.jain@oracle.com>


On 11/07/2013 07:15 AM, Eric Sandeen wrote:
> Even if it's "definitely" btrfs at this point,
> btrfs_scan_one_device could fail for other reasons.
>
> Check the return value, warn if it fails, and skip
> the device register.
>
> Resolves-Coverity-CID: 1125925
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>   utils.c |    9 ++++++++-
>   1 files changed, 8 insertions(+), 1 deletions(-)
>
> diff --git a/utils.c b/utils.c
> index 8471148..ecacc29 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -1937,6 +1937,7 @@ int test_skip_this_disk(char *path)
>   int btrfs_scan_lblkid(int update_kernel)
>   {
>   	int fd = -1;
> +	int ret;
>   	u64 num_devices;
>   	struct btrfs_fs_devices *tmp_devices;
>   	blkid_dev_iterate iter = NULL;
> @@ -1965,8 +1966,14 @@ int btrfs_scan_lblkid(int update_kernel)
>   			printf("ERROR: could not open %s\n", path);
>   			continue;
>   		}
> -		btrfs_scan_one_device(fd, path, &tmp_devices,
> +		ret = btrfs_scan_one_device(fd, path, &tmp_devices,
>   				&num_devices, BTRFS_SUPER_INFO_OFFSET);
> +		if (ret) {
> +			printf("ERROR: could not scan %s\n", path);
> +			close (fd);
> +			continue;
> +		}
> +
>   		close(fd);
>   		if (update_kernel)
>   			btrfs_register_one_device(path);
>

[PATCH 14/16] btrfs-progs: check for fstat failure in cmd_defrag

[Eric Sandeen]

Resolves-Coverity-CID: 1125924
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-filesystem.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index b1291d6..2cb067d 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -661,7 +661,13 @@ static int cmd_defrag(int argc, char **argv)
 		if (recursive) {
 			struct stat st;
 
-			fstat(fd, &st);
+			if (fstat(fd, &st)) {
+				fprintf(stderr, "ERROR: failed to stat %s - %s\n",
+						argv[i], strerror(errno));
+				defrag_global_errors++;
+				close_file_or_dir(fd, dirstream);
+				continue;
+			}
 			if (S_ISDIR(st.st_mode)) {
 				ret = nftw(argv[i], defrag_callback, 10,
 						FTW_MOUNT | FTW_PHYS);
-- 
1.7.1

[PATCH 15/16] btrfs-progs: annotate fallthroughs in parse_size

[Eric Sandeen]

We intentionally fall through these case statements;
just annotate it to be clear.

Resolves-Coverity-CID: 1054887
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 utils.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/utils.c b/utils.c
index ecacc29..9aeb5f8 100644
--- a/utils.c
+++ b/utils.c
@@ -1499,16 +1499,22 @@ u64 parse_size(char *s)
 		switch (c) {
 		case 'e':
 			mult *= 1024;
+			/* fallthrough */
 		case 'p':
 			mult *= 1024;
+			/* fallthrough */
 		case 't':
 			mult *= 1024;
+			/* fallthrough */
 		case 'g':
 			mult *= 1024;
+			/* fallthrough */
 		case 'm':
 			mult *= 1024;
+			/* fallthrough */
 		case 'k':
 			mult *= 1024;
+			/* fallthrough */
 		case 'b':
 			break;
 		default:
-- 
1.7.1

[PATCH 16/16] btrfs-progs: annotate fallthroughs in parse_limit

[Eric Sandeen]

We intentionally fall through these case statements;
just annotate it to be clear.

Resolves-Coverity-CID: 1054884
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 cmds-qgroup.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/cmds-qgroup.c b/cmds-qgroup.c
index 4fe776c..5a393bd 100644
--- a/cmds-qgroup.c
+++ b/cmds-qgroup.c
@@ -120,12 +120,15 @@ static int parse_limit(const char *p, unsigned long long *s)
 	case 'T':
 	case 't':
 		size *= 1024;
+		/* fallthrough */
 	case 'G':
 	case 'g':
 		size *= 1024;
+		/* fallthrough */
 	case 'M':
 	case 'm':
 		size *= 1024;
+		/* fallthrough */
 	case 'K':
 	case 'k':
 		size *= 1024;
-- 
1.7.1