* [btrfs-progs] [bug][patch] Leaking file handle in scrub_fs_info()
@ 2012-04-24 18:43 Goffredo Baroncelli
2012-04-25 9:39 ` Jan Schmidt
2012-06-05 11:01 ` Hugo Mills
0 siblings, 2 replies; 7+ messages in thread
From: Goffredo Baroncelli @ 2012-04-24 18:43 UTC (permalink / raw)
To: Jan Schmidt; +Cc: linux-btrfs, Hugo Mills
Hi Jan,
I was giving a look to the function scrub_fs_info( ), and to me it seems
that could be a potential file handle leaking problem.
In fact:
static int scrub_fs_info(int fd, char *path,
struct btrfs_ioctl_fs_info_args *fi_args,
struct btrfs_ioctl_dev_info_args **di_ret)
{
[...]
ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args);
if (ret && errno == EINVAL) {
/* path is no mounted btrfs. try if it's a device */
[...]
close(fd); <--- Here the
file handle is
closed
fd = open_file_or_dir(mp); <--- then it is
re-opened
if (fd < 0)
return -errno;
} else if (ret) {
return -errno;
}
[...]
But in the rest of the function:
a) the file handle is not closed
b) the (new) file handle isn't returned
The function "scrub_fs_info()" is called from the functions
1) cmd_scrub_status(), which doesn't use the file handle after the call
to the cmd_scrub_status() [except for a close()]. So no problem at all.
2) scrub_start(), which uses the file handle after the call to the
cmd_scrub_status() functions.
My suggestions is to change scrub_fs_info() to accept only the path.
Then it open (and closes) its own (and private) the file descriptor.
Instead scrub_start(), opens a file descriptor after the call to the
scrub_fs_info() function.
What do you think ?
BR
G.Baroncelli
You can pull the patch below from
http://cassiopea.homelinux.net/git/btrfs-progs-unstable.git
branch
fd-leaking
-----
diff --git a/cmds-scrub.c b/cmds-scrub.c
index c4503f4..486768c 100644
--- a/cmds-scrub.c
+++ b/cmds-scrub.c
@@ -979,19 +979,26 @@ static int scrub_device_info(int fd, u64 devid,
return ret ? -errno : 0;
}
-static int scrub_fs_info(int fd, char *path,
+static int scrub_fs_info( char *path,
struct btrfs_ioctl_fs_info_args *fi_args,
struct btrfs_ioctl_dev_info_args **di_ret)
{
int ret = 0;
int ndevs = 0;
int i = 1;
+ int fd;
struct btrfs_fs_devices *fs_devices_mnt = NULL;
struct btrfs_ioctl_dev_info_args *di_args;
char mp[BTRFS_PATH_NAME_MAX + 1];
memset(fi_args, 0, sizeof(*fi_args));
+ fd = open_file_or_dir(path);
+ if (fd < 0) {
+ fprintf(stderr, "ERROR: can't access to '%s'\n", path);
+ return -1;
+ }
+
ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args);
if (ret && errno == EINVAL) {
/* path is no mounted btrfs. try if it's a device */
@@ -1010,28 +1017,36 @@ static int scrub_fs_info(int fd, char *path,
if (fd < 0)
return -errno;
} else if (ret) {
+ close(fd);
return -errno;
}
- if (!fi_args->num_devices)
+ if (!fi_args->num_devices){
+ close(fd);
return 0;
+ }
di_args = *di_ret = malloc(fi_args->num_devices * sizeof(*di_args));
- if (!di_args)
+ if (!di_args){
+ close(fd);
return -errno;
+ }
for (; i <= fi_args->max_id; ++i) {
BUG_ON(ndevs >= fi_args->num_devices);
ret = scrub_device_info(fd, i, &di_args[ndevs]);
if (ret == -ENODEV)
continue;
- if (ret)
+ if (ret){
+ close(fd);
return ret;
+ }
++ndevs;
}
BUG_ON(ndevs == 0);
+ close(fd);
return 0;
}
@@ -1155,7 +1170,7 @@ static int scrub_start(int argc, char **argv, int
resume)
return 12;
}
- ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args);
+ ret = scrub_fs_info(path, &fi_args, &di_args);
if (ret) {
ERR(!do_quiet, "ERROR: getting dev info for scrub failed: "
"%s\n", strerror(-ret));
@@ -1586,7 +1601,6 @@ static int cmd_scrub_status(int argc, char **argv)
.sun_family = AF_UNIX,
};
int ret;
- int fdmnt;
int i;
int print_raw = 0;
int do_stats_per_dev = 0;
@@ -1615,13 +1629,7 @@ static int cmd_scrub_status(int argc, char **argv)
path = argv[optind];
- fdmnt = open_file_or_dir(path);
- if (fdmnt < 0) {
- fprintf(stderr, "ERROR: can't access to '%s'\n", path);
- return 12;
- }
-
- ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args);
+ ret = scrub_fs_info(path, &fi_args, &di_args);
if (ret) {
fprintf(stderr, "ERROR: getting dev info for scrub failed: "
"%s\n", strerror(-ret));
@@ -1698,7 +1706,6 @@ static int cmd_scrub_status(int argc, char **argv)
out:
free_history(past_scrubs);
free(di_args);
- close(fdmnt);
if (fdres > -1)
close(fdres);
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [btrfs-progs] [bug][patch] Leaking file handle in scrub_fs_info()
2012-04-24 18:43 [btrfs-progs] [bug][patch] Leaking file handle in scrub_fs_info() Goffredo Baroncelli
@ 2012-04-25 9:39 ` Jan Schmidt
2012-04-25 19:07 ` Goffredo Baroncelli
2012-06-05 11:01 ` Hugo Mills
1 sibling, 1 reply; 7+ messages in thread
From: Jan Schmidt @ 2012-04-25 9:39 UTC (permalink / raw)
To: kreijack; +Cc: Goffredo Baroncelli, linux-btrfs, Hugo Mills
Hi Goffredo,
On 24.04.2012 20:43, Goffredo Baroncelli wrote:
> I was giving a look to the function scrub_fs_info( ), and to me it seems
> that could be a potential file handle leaking problem.
It's only a single fd that's closed upon exit, but anyway...
> In fact:
>
> static int scrub_fs_info(int fd, char *path,
> struct btrfs_ioctl_fs_info_args *fi_args,
> struct btrfs_ioctl_dev_info_args **di_ret)
> {
>
> [...]
>
> ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args);
> if (ret && errno == EINVAL) {
> /* path is no mounted btrfs. try if it's a device */
> [...]
> close(fd); <--- Here the
> file handle is
> closed
>
> fd = open_file_or_dir(mp); <--- then it is
> re-opened
> if (fd < 0)
> return -errno;
> } else if (ret) {
> return -errno;
> }
> [...]
>
> But in the rest of the function:
> a) the file handle is not closed
> b) the (new) file handle isn't returned
You're right, that's unintended. I admit that I haven't tested passing a
device instead of a mountpoint that much.
> The function "scrub_fs_info()" is called from the functions
> 1) cmd_scrub_status(), which doesn't use the file handle after the call
> to the cmd_scrub_status() [except for a close()]. So no problem at all.
> 2) scrub_start(), which uses the file handle after the call to the
> cmd_scrub_status() functions.
>
> My suggestions is to change scrub_fs_info() to accept only the path.
> Then it open (and closes) its own (and private) the file descriptor.
>
> Instead scrub_start(), opens a file descriptor after the call to the
> scrub_fs_info() function.
>
> What do you think ?
My naive approach would be to pass an int * to scrub_fs_info. One has to
make sure that scrub_start doesn't rely on "fdmnt" not being updated.
After skipping through it, I think it expects "fdmnt" to be an open fd,
and it looks like it should be exactly the one used in scrub_fs_info.
Would you like to test the int * approach?
> BR
> G.Baroncelli
>
> You can pull the patch below from
>
> http://cassiopea.homelinux.net/git/btrfs-progs-unstable.git
>
> branch
>
> fd-leaking
>
> -----
>
> diff --git a/cmds-scrub.c b/cmds-scrub.c
> index c4503f4..486768c 100644
> --- a/cmds-scrub.c
> +++ b/cmds-scrub.c
> @@ -979,19 +979,26 @@ static int scrub_device_info(int fd, u64 devid,
> return ret ? -errno : 0;
> }
>
> -static int scrub_fs_info(int fd, char *path,
> +static int scrub_fs_info( char *path,
^
Apart from my proposed solution, the spacing in your patch doesn't
follow the style guide. No space here.
> struct btrfs_ioctl_fs_info_args *fi_args,
> struct btrfs_ioctl_dev_info_args **di_ret)
> {
> int ret = 0;
> int ndevs = 0;
> int i = 1;
> + int fd;
> struct btrfs_fs_devices *fs_devices_mnt = NULL;
> struct btrfs_ioctl_dev_info_args *di_args;
> char mp[BTRFS_PATH_NAME_MAX + 1];
>
> memset(fi_args, 0, sizeof(*fi_args));
>
> + fd = open_file_or_dir(path);
> + if (fd < 0) {
> + fprintf(stderr, "ERROR: can't access to '%s'\n", path);
> + return -1;
> + }
> +
> ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args);
> if (ret && errno == EINVAL) {
> /* path is no mounted btrfs. try if it's a device */
> @@ -1010,28 +1017,36 @@ static int scrub_fs_info(int fd, char *path,
> if (fd < 0)
> return -errno;
> } else if (ret) {
> + close(fd);
> return -errno;
> }
>
> - if (!fi_args->num_devices)
> + if (!fi_args->num_devices){
^
space here
> + close(fd);
> return 0;
> + }
>
> di_args = *di_ret = malloc(fi_args->num_devices * sizeof(*di_args));
> - if (!di_args)
> + if (!di_args){
^
space here
> + close(fd);
> return -errno;
> + }
>
> for (; i <= fi_args->max_id; ++i) {
> BUG_ON(ndevs >= fi_args->num_devices);
> ret = scrub_device_info(fd, i, &di_args[ndevs]);
> if (ret == -ENODEV)
> continue;
> - if (ret)
> + if (ret){
^
space here
> + close(fd);
> return ret;
> + }
> ++ndevs;
> }
>
> BUG_ON(ndevs == 0);
>
> + close(fd);
> return 0;
> }
>
> @@ -1155,7 +1170,7 @@ static int scrub_start(int argc, char **argv, int
> resume)
> return 12;
> }
>
> - ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args);
> + ret = scrub_fs_info(path, &fi_args, &di_args);
> if (ret) {
> ERR(!do_quiet, "ERROR: getting dev info for scrub failed: "
> "%s\n", strerror(-ret));
> @@ -1586,7 +1601,6 @@ static int cmd_scrub_status(int argc, char **argv)
> .sun_family = AF_UNIX,
> };
> int ret;
> - int fdmnt;
> int i;
> int print_raw = 0;
> int do_stats_per_dev = 0;
> @@ -1615,13 +1629,7 @@ static int cmd_scrub_status(int argc, char **argv)
>
> path = argv[optind];
>
> - fdmnt = open_file_or_dir(path);
> - if (fdmnt < 0) {
> - fprintf(stderr, "ERROR: can't access to '%s'\n", path);
> - return 12;
> - }
> -
> - ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args);
> + ret = scrub_fs_info(path, &fi_args, &di_args);
> if (ret) {
> fprintf(stderr, "ERROR: getting dev info for scrub failed: "
> "%s\n", strerror(-ret));
> @@ -1698,7 +1706,6 @@ static int cmd_scrub_status(int argc, char **argv)
> out:
> free_history(past_scrubs);
> free(di_args);
> - close(fdmnt);
> if (fdres > -1)
> close(fdres);
>
>
>
>
>
>
Thanks!
-Jan
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [btrfs-progs] [bug][patch] Leaking file handle in scrub_fs_info()
2012-04-25 9:39 ` Jan Schmidt
@ 2012-04-25 19:07 ` Goffredo Baroncelli
0 siblings, 0 replies; 7+ messages in thread
From: Goffredo Baroncelli @ 2012-04-25 19:07 UTC (permalink / raw)
To: Jan Schmidt; +Cc: linux-btrfs, Hugo Mills
On 04/25/2012 11:39 AM, Jan Schmidt wrote:
> Hi Goffredo,
>
> On 24.04.2012 20:43, Goffredo Baroncelli wrote:
>> I was giving a look to the function scrub_fs_info( ), and to me it seems
>> that could be a potential file handle leaking problem.
>
> It's only a single fd that's closed upon exit, but anyway...
Ironically, quite often the problem doesn't exist at all, because if I
1) open a file descriptr
2) close the file descriptr
3) open a new file descriptor
For 1) and 2) the kernel re-use the same file descriptor.
However I think that the scrub_fs_info() could be a more generic
function (not only scrub related) which could be reused in more places.
So a more polite behaviour it is needed.
>
>> In fact:
>>
>> static int scrub_fs_info(int fd, char *path,
>> struct btrfs_ioctl_fs_info_args *fi_args,
>> struct btrfs_ioctl_dev_info_args **di_ret)
>> {
>>
>> [...]
>>
>> ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args);
>> if (ret && errno == EINVAL) {
>> /* path is no mounted btrfs. try if it's a device */
>> [...]
>> close(fd); <--- Here the
>> file handle is
>> closed
>>
>> fd = open_file_or_dir(mp); <--- then it is
>> re-opened
>> if (fd < 0)
>> return -errno;
>> } else if (ret) {
>> return -errno;
>> }
>> [...]
>>
>> But in the rest of the function:
>> a) the file handle is not closed
>> b) the (new) file handle isn't returned
>
> You're right, that's unintended. I admit that I haven't tested passing a
> device instead of a mountpoint that much.
>
>> The function "scrub_fs_info()" is called from the functions
>> 1) cmd_scrub_status(), which doesn't use the file handle after the call
>> to the cmd_scrub_status() [except for a close()]. So no problem at all.
>> 2) scrub_start(), which uses the file handle after the call to the
>> cmd_scrub_status() functions.
>>
>> My suggestions is to change scrub_fs_info() to accept only the path.
>> Then it open (and closes) its own (and private) the file descriptor.
>>
>> Instead scrub_start(), opens a file descriptor after the call to the
>> scrub_fs_info() function.
>>
>> What do you think ?
>
> My naive approach would be to pass an int * to scrub_fs_info. One has to
> make sure that scrub_start doesn't rely on "fdmnt" not being updated.
> After skipping through it, I think it expects "fdmnt" to be an open fd,
> and it looks like it should be exactly the one used in scrub_fs_info.
> Would you like to test the int * approach?
Frankly speaking, I don't like the idea that scrub_fs_info() could
change the file descriptor. I think that it is not good design.
I would like to suggest the following options:
1) scrub_fs_info() opens a file descriptor on the basis of the path
parameters. And then it returns this file descriptor (or via return or
via a int * parameters)
or
2) we move the re-open logic outside the function itself. So
scrub_start() don't need to change the file descriptor.
I prefer the options #2, also because this is a quite common problem: we
can create a generic function which returns a good file descriptor when
a filesystem path and/or the device is passed
What do you think ?
Finally I will take in account your suggestion regarding the spacing.
BR
G.Baroncelli
>
>> BR
>> G.Baroncelli
>>
>> You can pull the patch below from
>>
>> http://cassiopea.homelinux.net/git/btrfs-progs-unstable.git
>>
>> branch
>>
>> fd-leaking
>>
>> -----
>>
>> diff --git a/cmds-scrub.c b/cmds-scrub.c
>> index c4503f4..486768c 100644
>> --- a/cmds-scrub.c
>> +++ b/cmds-scrub.c
>> @@ -979,19 +979,26 @@ static int scrub_device_info(int fd, u64 devid,
>> return ret ? -errno : 0;
>> }
>>
>> -static int scrub_fs_info(int fd, char *path,
>> +static int scrub_fs_info( char *path,
> ^
> Apart from my proposed solution, the spacing in your patch doesn't
> follow the style guide. No space here.
>
>> struct btrfs_ioctl_fs_info_args *fi_args,
>> struct btrfs_ioctl_dev_info_args **di_ret)
>> {
>> int ret = 0;
>> int ndevs = 0;
>> int i = 1;
>> + int fd;
>> struct btrfs_fs_devices *fs_devices_mnt = NULL;
>> struct btrfs_ioctl_dev_info_args *di_args;
>> char mp[BTRFS_PATH_NAME_MAX + 1];
>>
>> memset(fi_args, 0, sizeof(*fi_args));
>>
>> + fd = open_file_or_dir(path);
>> + if (fd < 0) {
>> + fprintf(stderr, "ERROR: can't access to '%s'\n", path);
>> + return -1;
>> + }
>> +
>> ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args);
>> if (ret && errno == EINVAL) {
>> /* path is no mounted btrfs. try if it's a device */
>> @@ -1010,28 +1017,36 @@ static int scrub_fs_info(int fd, char *path,
>> if (fd < 0)
>> return -errno;
>> } else if (ret) {
>> + close(fd);
>> return -errno;
>> }
>>
>> - if (!fi_args->num_devices)
>> + if (!fi_args->num_devices){
> ^
> space here
>
>> + close(fd);
>> return 0;
>> + }
>>
>> di_args = *di_ret = malloc(fi_args->num_devices * sizeof(*di_args));
>> - if (!di_args)
>> + if (!di_args){
> ^
> space here
>
>> + close(fd);
>> return -errno;
>> + }
>>
>> for (; i <= fi_args->max_id; ++i) {
>> BUG_ON(ndevs >= fi_args->num_devices);
>> ret = scrub_device_info(fd, i, &di_args[ndevs]);
>> if (ret == -ENODEV)
>> continue;
>> - if (ret)
>> + if (ret){
> ^
> space here
>
>> + close(fd);
>> return ret;
>> + }
>> ++ndevs;
>> }
>>
>> BUG_ON(ndevs == 0);
>>
>> + close(fd);
>> return 0;
>> }
>>
>> @@ -1155,7 +1170,7 @@ static int scrub_start(int argc, char **argv, int
>> resume)
>> return 12;
>> }
>>
>> - ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args);
>> + ret = scrub_fs_info(path, &fi_args, &di_args);
>> if (ret) {
>> ERR(!do_quiet, "ERROR: getting dev info for scrub failed: "
>> "%s\n", strerror(-ret));
>> @@ -1586,7 +1601,6 @@ static int cmd_scrub_status(int argc, char **argv)
>> .sun_family = AF_UNIX,
>> };
>> int ret;
>> - int fdmnt;
>> int i;
>> int print_raw = 0;
>> int do_stats_per_dev = 0;
>> @@ -1615,13 +1629,7 @@ static int cmd_scrub_status(int argc, char **argv)
>>
>> path = argv[optind];
>>
>> - fdmnt = open_file_or_dir(path);
>> - if (fdmnt < 0) {
>> - fprintf(stderr, "ERROR: can't access to '%s'\n", path);
>> - return 12;
>> - }
>> -
>> - ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args);
>> + ret = scrub_fs_info(path, &fi_args, &di_args);
>> if (ret) {
>> fprintf(stderr, "ERROR: getting dev info for scrub failed: "
>> "%s\n", strerror(-ret));
>> @@ -1698,7 +1706,6 @@ static int cmd_scrub_status(int argc, char **argv)
>> out:
>> free_history(past_scrubs);
>> free(di_args);
>> - close(fdmnt);
>> if (fdres > -1)
>> close(fdres);
>>
>>
>>
>>
>>
>>
>
> Thanks!
> -Jan
> .
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [btrfs-progs] [bug][patch] Leaking file handle in scrub_fs_info()
2012-04-24 18:43 [btrfs-progs] [bug][patch] Leaking file handle in scrub_fs_info() Goffredo Baroncelli
2012-04-25 9:39 ` Jan Schmidt
@ 2012-06-05 11:01 ` Hugo Mills
2012-06-05 17:26 ` [btrfs-progs] [bug][patch V2] " Goffredo Baroncelli
1 sibling, 1 reply; 7+ messages in thread
From: Hugo Mills @ 2012-06-05 11:01 UTC (permalink / raw)
To: kreijack; +Cc: Jan Schmidt, linux-btrfs
[-- Attachment #1: Type: text/plain, Size: 5981 bytes --]
Hi, Goffredo,
I'm trying to do a new integration branch, and the patch inline in
the text is corrupt (plus it has a massively verbose commit message):
Applying: Leaking file handle in scrub_fs_info()
fatal: corrupt patch at line 74
Patch failed at 0001 Leaking file handle in scrub_fs_info()
Normally, I'd use your git repo to get round the fact that every
single patch you send by mail is whitespace-damaged, but:
error: Unable to find 337bfb3250203a66b953ffcf7804418cb7c72052 under http://cassiopea.homelinux.net/git/btrfs-progs-unstable.git
Cannot obtain needed commit 337bfb3250203a66b953ffcf7804418cb7c72052
while processing commit ee3832b47a4b1d1dce8d1cacc802db32901b8c45.
error: Fetch failed.
Plus Jan had some suggested tweaks regarding whitespace -- could
you re-roll the patch and maybe see what you can do to fix your git
repo? (Or at least use git-send-email to send the patch so it doesn't
get mangled).
Thanks,
Hugo.
On Tue, Apr 24, 2012 at 08:43:23PM +0200, Goffredo Baroncelli wrote:
> I was giving a look to the function scrub_fs_info( ), and to me it seems
> that could be a potential file handle leaking problem.
>
>
> In fact:
>
> static int scrub_fs_info(int fd, char *path,
> struct btrfs_ioctl_fs_info_args *fi_args,
> struct btrfs_ioctl_dev_info_args **di_ret)
> {
>
> [...]
>
> ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args);
> if (ret && errno == EINVAL) {
> /* path is no mounted btrfs. try if it's a device */
> [...]
> close(fd); <--- Here the
> file handle is
> closed
>
> fd = open_file_or_dir(mp); <--- then it is
> re-opened
> if (fd < 0)
> return -errno;
> } else if (ret) {
> return -errno;
> }
> [...]
>
> But in the rest of the function:
> a) the file handle is not closed
> b) the (new) file handle isn't returned
>
> The function "scrub_fs_info()" is called from the functions
> 1) cmd_scrub_status(), which doesn't use the file handle after the call
> to the cmd_scrub_status() [except for a close()]. So no problem at all.
> 2) scrub_start(), which uses the file handle after the call to the
> cmd_scrub_status() functions.
>
> My suggestions is to change scrub_fs_info() to accept only the path.
> Then it open (and closes) its own (and private) the file descriptor.
>
> Instead scrub_start(), opens a file descriptor after the call to the
> scrub_fs_info() function.
>
> What do you think ?
>
> BR
> G.Baroncelli
>
> You can pull the patch below from
>
> http://cassiopea.homelinux.net/git/btrfs-progs-unstable.git
>
> branch
>
> fd-leaking
>
> -----
>
> diff --git a/cmds-scrub.c b/cmds-scrub.c
> index c4503f4..486768c 100644
> --- a/cmds-scrub.c
> +++ b/cmds-scrub.c
> @@ -979,19 +979,26 @@ static int scrub_device_info(int fd, u64 devid,
> return ret ? -errno : 0;
> }
>
> -static int scrub_fs_info(int fd, char *path,
> +static int scrub_fs_info( char *path,
> struct btrfs_ioctl_fs_info_args *fi_args,
> struct btrfs_ioctl_dev_info_args **di_ret)
> {
> int ret = 0;
> int ndevs = 0;
> int i = 1;
> + int fd;
> struct btrfs_fs_devices *fs_devices_mnt = NULL;
> struct btrfs_ioctl_dev_info_args *di_args;
> char mp[BTRFS_PATH_NAME_MAX + 1];
>
> memset(fi_args, 0, sizeof(*fi_args));
>
> + fd = open_file_or_dir(path);
> + if (fd < 0) {
> + fprintf(stderr, "ERROR: can't access to '%s'\n", path);
> + return -1;
> + }
> +
> ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args);
> if (ret && errno == EINVAL) {
> /* path is no mounted btrfs. try if it's a device */
> @@ -1010,28 +1017,36 @@ static int scrub_fs_info(int fd, char *path,
> if (fd < 0)
> return -errno;
> } else if (ret) {
> + close(fd);
> return -errno;
> }
>
> - if (!fi_args->num_devices)
> + if (!fi_args->num_devices){
> + close(fd);
> return 0;
> + }
>
> di_args = *di_ret = malloc(fi_args->num_devices * sizeof(*di_args));
> - if (!di_args)
> + if (!di_args){
> + close(fd);
> return -errno;
> + }
>
> for (; i <= fi_args->max_id; ++i) {
> BUG_ON(ndevs >= fi_args->num_devices);
> ret = scrub_device_info(fd, i, &di_args[ndevs]);
> if (ret == -ENODEV)
> continue;
> - if (ret)
> + if (ret){
> + close(fd);
> return ret;
> + }
> ++ndevs;
> }
>
> BUG_ON(ndevs == 0);
>
> + close(fd);
> return 0;
> }
>
> @@ -1155,7 +1170,7 @@ static int scrub_start(int argc, char **argv, int
> resume)
> return 12;
> }
>
> - ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args);
> + ret = scrub_fs_info(path, &fi_args, &di_args);
> if (ret) {
> ERR(!do_quiet, "ERROR: getting dev info for scrub failed: "
> "%s\n", strerror(-ret));
> @@ -1586,7 +1601,6 @@ static int cmd_scrub_status(int argc, char **argv)
> .sun_family = AF_UNIX,
> };
> int ret;
> - int fdmnt;
> int i;
> int print_raw = 0;
> int do_stats_per_dev = 0;
> @@ -1615,13 +1629,7 @@ static int cmd_scrub_status(int argc, char **argv)
>
> path = argv[optind];
>
> - fdmnt = open_file_or_dir(path);
> - if (fdmnt < 0) {
> - fprintf(stderr, "ERROR: can't access to '%s'\n", path);
> - return 12;
> - }
> -
> - ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args);
> + ret = scrub_fs_info(path, &fi_args, &di_args);
> if (ret) {
> fprintf(stderr, "ERROR: getting dev info for scrub failed: "
> "%s\n", strerror(-ret));
> @@ -1698,7 +1706,6 @@ static int cmd_scrub_status(int argc, char **argv)
> out:
> free_history(past_scrubs);
> free(di_args);
> - close(fdmnt);
> if (fdres > -1)
> close(fdres);
>
>
>
>
>
>
--
=== Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ===
PGP key: 515C238D from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
--- Computer Science is not about computers, any more than ---
astronomy is about telescopes.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 190 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [btrfs-progs] [bug][patch V2] Leaking file handle in scrub_fs_info()
2012-06-05 11:01 ` Hugo Mills
@ 2012-06-05 17:26 ` Goffredo Baroncelli
2012-06-05 18:19 ` Hugo Mills
0 siblings, 1 reply; 7+ messages in thread
From: Goffredo Baroncelli @ 2012-06-05 17:26 UTC (permalink / raw)
To: Hugo Mills, kreijack, Jan Schmidt, linux-btrfs
[-- Attachment #1: Type: text/plain, Size: 10246 bytes --]
Hi Hugo,
I was not able to reproduce your error with my repository: I pulled it
without problem.
However, enclosed you can find the patch (as attachment) with the Jan
suggestions about the spaces. You can pull the patch also from my repo (
same branch: fd-leaking). Moreover I inserted the patch as inlined.
Hoping that helps.
--------------------
commit b73abc9465d3a105b5f8f464f2867543a638e5e2
Author: Goffredo Baroncelli <kreijack@inwind.it>
Date: Tue Jun 5 19:11:06 2012 +0200
scrub_fs_info( ) file handle leaking
The function scrub_fs_info( ) closes and reopen a file handle
passed as argument, when a caller uses the file handle even after the
call.
The function scrub_fs_info( ) is updated to remove the file handle
argument, and instead uses a private own file handle.
The callers are updated to not pass the argument.
diff --git a/cmds-scrub.c b/cmds-scrub.c
index c4503f4..6313e02 100644
--- a/cmds-scrub.c
+++ b/cmds-scrub.c
@@ -979,19 +979,26 @@ static int scrub_device_info(int fd, u64 devid,
return ret ? -errno : 0;
}
-static int scrub_fs_info(int fd, char *path,
+static int scrub_fs_info(char *path,
struct btrfs_ioctl_fs_info_args *fi_args,
struct btrfs_ioctl_dev_info_args **di_ret)
{
int ret = 0;
int ndevs = 0;
int i = 1;
+ int fd;
struct btrfs_fs_devices *fs_devices_mnt = NULL;
struct btrfs_ioctl_dev_info_args *di_args;
char mp[BTRFS_PATH_NAME_MAX + 1];
memset(fi_args, 0, sizeof(*fi_args));
+ fd = open_file_or_dir(path);
+ if (fd < 0) {
+ fprintf(stderr, "ERROR: can't access to '%s'\n", path);
+ return -1;
+ }
+
ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args);
if (ret && errno == EINVAL) {
/* path is no mounted btrfs. try if it's a device */
@@ -1010,28 +1017,36 @@ static int scrub_fs_info(int fd, char *path,
if (fd < 0)
return -errno;
} else if (ret) {
+ close(fd);
return -errno;
}
- if (!fi_args->num_devices)
+ if (!fi_args->num_devices) {
+ close(fd);
return 0;
+ }
di_args = *di_ret = malloc(fi_args->num_devices * sizeof(*di_args));
- if (!di_args)
+ if (!di_args) {
+ close(fd);
return -errno;
+ }
for (; i <= fi_args->max_id; ++i) {
BUG_ON(ndevs >= fi_args->num_devices);
ret = scrub_device_info(fd, i, &di_args[ndevs]);
if (ret == -ENODEV)
continue;
- if (ret)
+ if (ret) {
+ close(fd);
return ret;
+ }
++ndevs;
}
BUG_ON(ndevs == 0);
+ close(fd);
return 0;
}
@@ -1155,7 +1170,7 @@ static int scrub_start(int argc, char **argv, int
resume)
return 12;
}
- ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args);
+ ret = scrub_fs_info(path, &fi_args, &di_args);
if (ret) {
ERR(!do_quiet, "ERROR: getting dev info for scrub failed: "
"%s\n", strerror(-ret));
@@ -1586,7 +1601,6 @@ static int cmd_scrub_status(int argc, char **argv)
.sun_family = AF_UNIX,
};
int ret;
- int fdmnt;
int i;
int print_raw = 0;
int do_stats_per_dev = 0;
@@ -1615,13 +1629,7 @@ static int cmd_scrub_status(int argc, char **argv)
path = argv[optind];
- fdmnt = open_file_or_dir(path);
- if (fdmnt < 0) {
- fprintf(stderr, "ERROR: can't access to '%s'\n", path);
- return 12;
- }
-
- ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args);
+ ret = scrub_fs_info(path, &fi_args, &di_args);
if (ret) {
fprintf(stderr, "ERROR: getting dev info for scrub failed: "
"%s\n", strerror(-ret));
@@ -1698,7 +1706,6 @@ static int cmd_scrub_status(int argc, char **argv)
out:
free_history(past_scrubs);
free(di_args);
- close(fdmnt);
if (fdres > -1)
close(fdres);
On 06/05/2012 01:01 PM, Hugo Mills wrote:
> Hi, Goffredo,
>
> I'm trying to do a new integration branch, and the patch inline in
> the text is corrupt (plus it has a massively verbose commit message):
>
> Applying: Leaking file handle in scrub_fs_info()
> fatal: corrupt patch at line 74
> Patch failed at 0001 Leaking file handle in scrub_fs_info()
>
> Normally, I'd use your git repo to get round the fact that every
> single patch you send by mail is whitespace-damaged, but:
>
> error: Unable to find 337bfb3250203a66b953ffcf7804418cb7c72052 under http://cassiopea.homelinux.net/git/btrfs-progs-unstable.git
> Cannot obtain needed commit 337bfb3250203a66b953ffcf7804418cb7c72052
> while processing commit ee3832b47a4b1d1dce8d1cacc802db32901b8c45.
> error: Fetch failed.
>
> Plus Jan had some suggested tweaks regarding whitespace -- could
> you re-roll the patch and maybe see what you can do to fix your git
> repo? (Or at least use git-send-email to send the patch so it doesn't
> get mangled).
>
> Thanks,
> Hugo.
>
> On Tue, Apr 24, 2012 at 08:43:23PM +0200, Goffredo Baroncelli wrote:
>> I was giving a look to the function scrub_fs_info( ), and to me it seems
>> that could be a potential file handle leaking problem.
>>
>>
>> In fact:
>>
>> static int scrub_fs_info(int fd, char *path,
>> struct btrfs_ioctl_fs_info_args *fi_args,
>> struct btrfs_ioctl_dev_info_args **di_ret)
>> {
>>
>> [...]
>>
>> ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args);
>> if (ret && errno == EINVAL) {
>> /* path is no mounted btrfs. try if it's a device */
>> [...]
>> close(fd); <--- Here the
>> file handle is
>> closed
>>
>> fd = open_file_or_dir(mp); <--- then it is
>> re-opened
>> if (fd < 0)
>> return -errno;
>> } else if (ret) {
>> return -errno;
>> }
>> [...]
>>
>> But in the rest of the function:
>> a) the file handle is not closed
>> b) the (new) file handle isn't returned
>>
>> The function "scrub_fs_info()" is called from the functions
>> 1) cmd_scrub_status(), which doesn't use the file handle after the call
>> to the cmd_scrub_status() [except for a close()]. So no problem at all.
>> 2) scrub_start(), which uses the file handle after the call to the
>> cmd_scrub_status() functions.
>>
>> My suggestions is to change scrub_fs_info() to accept only the path.
>> Then it open (and closes) its own (and private) the file descriptor.
>>
>> Instead scrub_start(), opens a file descriptor after the call to the
>> scrub_fs_info() function.
>>
>> What do you think ?
>>
>> BR
>> G.Baroncelli
>>
>> You can pull the patch below from
>>
>> http://cassiopea.homelinux.net/git/btrfs-progs-unstable.git
>>
>> branch
>>
>> fd-leaking
>>
>> -----
>>
>> diff --git a/cmds-scrub.c b/cmds-scrub.c
>> index c4503f4..486768c 100644
>> --- a/cmds-scrub.c
>> +++ b/cmds-scrub.c
>> @@ -979,19 +979,26 @@ static int scrub_device_info(int fd, u64 devid,
>> return ret ? -errno : 0;
>> }
>>
>> -static int scrub_fs_info(int fd, char *path,
>> +static int scrub_fs_info( char *path,
>> struct btrfs_ioctl_fs_info_args *fi_args,
>> struct btrfs_ioctl_dev_info_args **di_ret)
>> {
>> int ret = 0;
>> int ndevs = 0;
>> int i = 1;
>> + int fd;
>> struct btrfs_fs_devices *fs_devices_mnt = NULL;
>> struct btrfs_ioctl_dev_info_args *di_args;
>> char mp[BTRFS_PATH_NAME_MAX + 1];
>>
>> memset(fi_args, 0, sizeof(*fi_args));
>>
>> + fd = open_file_or_dir(path);
>> + if (fd < 0) {
>> + fprintf(stderr, "ERROR: can't access to '%s'\n", path);
>> + return -1;
>> + }
>> +
>> ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args);
>> if (ret && errno == EINVAL) {
>> /* path is no mounted btrfs. try if it's a device */
>> @@ -1010,28 +1017,36 @@ static int scrub_fs_info(int fd, char *path,
>> if (fd < 0)
>> return -errno;
>> } else if (ret) {
>> + close(fd);
>> return -errno;
>> }
>>
>> - if (!fi_args->num_devices)
>> + if (!fi_args->num_devices){
>> + close(fd);
>> return 0;
>> + }
>>
>> di_args = *di_ret = malloc(fi_args->num_devices * sizeof(*di_args));
>> - if (!di_args)
>> + if (!di_args){
>> + close(fd);
>> return -errno;
>> + }
>>
>> for (; i <= fi_args->max_id; ++i) {
>> BUG_ON(ndevs >= fi_args->num_devices);
>> ret = scrub_device_info(fd, i, &di_args[ndevs]);
>> if (ret == -ENODEV)
>> continue;
>> - if (ret)
>> + if (ret){
>> + close(fd);
>> return ret;
>> + }
>> ++ndevs;
>> }
>>
>> BUG_ON(ndevs == 0);
>>
>> + close(fd);
>> return 0;
>> }
>>
>> @@ -1155,7 +1170,7 @@ static int scrub_start(int argc, char **argv, int
>> resume)
>> return 12;
>> }
>>
>> - ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args);
>> + ret = scrub_fs_info(path, &fi_args, &di_args);
>> if (ret) {
>> ERR(!do_quiet, "ERROR: getting dev info for scrub failed: "
>> "%s\n", strerror(-ret));
>> @@ -1586,7 +1601,6 @@ static int cmd_scrub_status(int argc, char **argv)
>> .sun_family = AF_UNIX,
>> };
>> int ret;
>> - int fdmnt;
>> int i;
>> int print_raw = 0;
>> int do_stats_per_dev = 0;
>> @@ -1615,13 +1629,7 @@ static int cmd_scrub_status(int argc, char **argv)
>>
>> path = argv[optind];
>>
>> - fdmnt = open_file_or_dir(path);
>> - if (fdmnt < 0) {
>> - fprintf(stderr, "ERROR: can't access to '%s'\n", path);
>> - return 12;
>> - }
>> -
>> - ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args);
>> + ret = scrub_fs_info(path, &fi_args, &di_args);
>> if (ret) {
>> fprintf(stderr, "ERROR: getting dev info for scrub failed: "
>> "%s\n", strerror(-ret));
>> @@ -1698,7 +1706,6 @@ static int cmd_scrub_status(int argc, char **argv)
>> out:
>> free_history(past_scrubs);
>> free(di_args);
>> - close(fdmnt);
>> if (fdres > -1)
>> close(fdres);
>>
>>
>>
>>
>>
>>
>
[-- Attachment #2: fd-leaking.diff --]
[-- Type: text/x-patch, Size: 2707 bytes --]
diff --git a/cmds-scrub.c b/cmds-scrub.c
index c4503f4..6313e02 100644
--- a/cmds-scrub.c
+++ b/cmds-scrub.c
@@ -979,19 +979,26 @@ static int scrub_device_info(int fd, u64 devid,
return ret ? -errno : 0;
}
-static int scrub_fs_info(int fd, char *path,
+static int scrub_fs_info(char *path,
struct btrfs_ioctl_fs_info_args *fi_args,
struct btrfs_ioctl_dev_info_args **di_ret)
{
int ret = 0;
int ndevs = 0;
int i = 1;
+ int fd;
struct btrfs_fs_devices *fs_devices_mnt = NULL;
struct btrfs_ioctl_dev_info_args *di_args;
char mp[BTRFS_PATH_NAME_MAX + 1];
memset(fi_args, 0, sizeof(*fi_args));
+ fd = open_file_or_dir(path);
+ if (fd < 0) {
+ fprintf(stderr, "ERROR: can't access to '%s'\n", path);
+ return -1;
+ }
+
ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args);
if (ret && errno == EINVAL) {
/* path is no mounted btrfs. try if it's a device */
@@ -1010,28 +1017,36 @@ static int scrub_fs_info(int fd, char *path,
if (fd < 0)
return -errno;
} else if (ret) {
+ close(fd);
return -errno;
}
- if (!fi_args->num_devices)
+ if (!fi_args->num_devices) {
+ close(fd);
return 0;
+ }
di_args = *di_ret = malloc(fi_args->num_devices * sizeof(*di_args));
- if (!di_args)
+ if (!di_args) {
+ close(fd);
return -errno;
+ }
for (; i <= fi_args->max_id; ++i) {
BUG_ON(ndevs >= fi_args->num_devices);
ret = scrub_device_info(fd, i, &di_args[ndevs]);
if (ret == -ENODEV)
continue;
- if (ret)
+ if (ret) {
+ close(fd);
return ret;
+ }
++ndevs;
}
BUG_ON(ndevs == 0);
+ close(fd);
return 0;
}
@@ -1155,7 +1170,7 @@ static int scrub_start(int argc, char **argv, int resume)
return 12;
}
- ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args);
+ ret = scrub_fs_info(path, &fi_args, &di_args);
if (ret) {
ERR(!do_quiet, "ERROR: getting dev info for scrub failed: "
"%s\n", strerror(-ret));
@@ -1586,7 +1601,6 @@ static int cmd_scrub_status(int argc, char **argv)
.sun_family = AF_UNIX,
};
int ret;
- int fdmnt;
int i;
int print_raw = 0;
int do_stats_per_dev = 0;
@@ -1615,13 +1629,7 @@ static int cmd_scrub_status(int argc, char **argv)
path = argv[optind];
- fdmnt = open_file_or_dir(path);
- if (fdmnt < 0) {
- fprintf(stderr, "ERROR: can't access to '%s'\n", path);
- return 12;
- }
-
- ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args);
+ ret = scrub_fs_info(path, &fi_args, &di_args);
if (ret) {
fprintf(stderr, "ERROR: getting dev info for scrub failed: "
"%s\n", strerror(-ret));
@@ -1698,7 +1706,6 @@ static int cmd_scrub_status(int argc, char **argv)
out:
free_history(past_scrubs);
free(di_args);
- close(fdmnt);
if (fdres > -1)
close(fdres);
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [btrfs-progs] [bug][patch V2] Leaking file handle in scrub_fs_info()
2012-06-05 17:26 ` [btrfs-progs] [bug][patch V2] " Goffredo Baroncelli
@ 2012-06-05 18:19 ` Hugo Mills
2012-06-05 20:12 ` Goffredo Baroncelli
0 siblings, 1 reply; 7+ messages in thread
From: Hugo Mills @ 2012-06-05 18:19 UTC (permalink / raw)
To: kreijack; +Cc: linux-btrfs
[-- Attachment #1: Type: text/plain, Size: 14933 bytes --]
On Tue, Jun 05, 2012 at 07:26:34PM +0200, Goffredo Baroncelli wrote:
> Hi Hugo,
>
> I was not able to reproduce your error with my repository: I pulled it
> without problem.
Well, I'd have normally written it off as a corrupt local repo at
this end -- except that David Sterba said he'd also seen a similar
problem pulling from your repo.
> However, enclosed you can find the patch (as attachment) with the Jan
> suggestions about the spaces. You can pull the patch also from my repo (
> same branch: fd-leaking). Moreover I inserted the patch as inlined.
> Hoping that helps.
Not really. Your mailer is breaking things (there's a wrapped line
in there, and it's heavily whitespace-damaged). Could you *please*
learn to use git send-email? Every single patch of yours that I've
seen on the mailing list is badly broken, and takes an age to get it
to the point where "git am" will look at it cleanly. It's incredibly
frustrating. This time around, I think I've managed to get it to work
OK, but it's taken me 10 minutes of buggering around, and I'm getting
irritable, which isn't a good thing when I'm meant to be having a
quiet relaxing week off work...
Hugo.
> --------------------
>
> commit b73abc9465d3a105b5f8f464f2867543a638e5e2
> Author: Goffredo Baroncelli <kreijack@inwind.it>
> Date: Tue Jun 5 19:11:06 2012 +0200
>
> scrub_fs_info( ) file handle leaking
>
> The function scrub_fs_info( ) closes and reopen a file handle
> passed as argument, when a caller uses the file handle even after the
> call.
> The function scrub_fs_info( ) is updated to remove the file handle
> argument, and instead uses a private own file handle.
> The callers are updated to not pass the argument.
>
>
> diff --git a/cmds-scrub.c b/cmds-scrub.c
> index c4503f4..6313e02 100644
> --- a/cmds-scrub.c
> +++ b/cmds-scrub.c
> @@ -979,19 +979,26 @@ static int scrub_device_info(int fd, u64 devid,
> return ret ? -errno : 0;
> }
>
> -static int scrub_fs_info(int fd, char *path,
> +static int scrub_fs_info(char *path,
> struct btrfs_ioctl_fs_info_args *fi_args,
> struct btrfs_ioctl_dev_info_args **di_ret)
> {
> int ret = 0;
> int ndevs = 0;
> int i = 1;
> + int fd;
> struct btrfs_fs_devices *fs_devices_mnt = NULL;
> struct btrfs_ioctl_dev_info_args *di_args;
> char mp[BTRFS_PATH_NAME_MAX + 1];
>
> memset(fi_args, 0, sizeof(*fi_args));
>
> + fd = open_file_or_dir(path);
> + if (fd < 0) {
> + fprintf(stderr, "ERROR: can't access to '%s'\n", path);
> + return -1;
> + }
> +
> ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args);
> if (ret && errno == EINVAL) {
> /* path is no mounted btrfs. try if it's a device */
> @@ -1010,28 +1017,36 @@ static int scrub_fs_info(int fd, char *path,
> if (fd < 0)
> return -errno;
> } else if (ret) {
> + close(fd);
> return -errno;
> }
>
> - if (!fi_args->num_devices)
> + if (!fi_args->num_devices) {
> + close(fd);
> return 0;
> + }
>
> di_args = *di_ret = malloc(fi_args->num_devices * sizeof(*di_args));
> - if (!di_args)
> + if (!di_args) {
> + close(fd);
> return -errno;
> + }
>
> for (; i <= fi_args->max_id; ++i) {
> BUG_ON(ndevs >= fi_args->num_devices);
> ret = scrub_device_info(fd, i, &di_args[ndevs]);
> if (ret == -ENODEV)
> continue;
> - if (ret)
> + if (ret) {
> + close(fd);
> return ret;
> + }
> ++ndevs;
> }
>
> BUG_ON(ndevs == 0);
>
> + close(fd);
> return 0;
> }
>
> @@ -1155,7 +1170,7 @@ static int scrub_start(int argc, char **argv, int
> resume)
> return 12;
> }
>
> - ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args);
> + ret = scrub_fs_info(path, &fi_args, &di_args);
> if (ret) {
> ERR(!do_quiet, "ERROR: getting dev info for scrub failed: "
> "%s\n", strerror(-ret));
> @@ -1586,7 +1601,6 @@ static int cmd_scrub_status(int argc, char **argv)
> .sun_family = AF_UNIX,
> };
> int ret;
> - int fdmnt;
> int i;
> int print_raw = 0;
> int do_stats_per_dev = 0;
> @@ -1615,13 +1629,7 @@ static int cmd_scrub_status(int argc, char **argv)
>
> path = argv[optind];
>
> - fdmnt = open_file_or_dir(path);
> - if (fdmnt < 0) {
> - fprintf(stderr, "ERROR: can't access to '%s'\n", path);
> - return 12;
> - }
> -
> - ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args);
> + ret = scrub_fs_info(path, &fi_args, &di_args);
> if (ret) {
> fprintf(stderr, "ERROR: getting dev info for scrub failed: "
> "%s\n", strerror(-ret));
> @@ -1698,7 +1706,6 @@ static int cmd_scrub_status(int argc, char **argv)
> out:
> free_history(past_scrubs);
> free(di_args);
> - close(fdmnt);
> if (fdres > -1)
> close(fdres);
>
>
>
>
>
>
> On 06/05/2012 01:01 PM, Hugo Mills wrote:
> > Hi, Goffredo,
> >
> > I'm trying to do a new integration branch, and the patch inline in
> > the text is corrupt (plus it has a massively verbose commit message):
> >
> > Applying: Leaking file handle in scrub_fs_info()
> > fatal: corrupt patch at line 74
> > Patch failed at 0001 Leaking file handle in scrub_fs_info()
> >
> > Normally, I'd use your git repo to get round the fact that every
> > single patch you send by mail is whitespace-damaged, but:
> >
> > error: Unable to find 337bfb3250203a66b953ffcf7804418cb7c72052 under http://cassiopea.homelinux.net/git/btrfs-progs-unstable.git
> > Cannot obtain needed commit 337bfb3250203a66b953ffcf7804418cb7c72052
> > while processing commit ee3832b47a4b1d1dce8d1cacc802db32901b8c45.
> > error: Fetch failed.
> >
> > Plus Jan had some suggested tweaks regarding whitespace -- could
> > you re-roll the patch and maybe see what you can do to fix your git
> > repo? (Or at least use git-send-email to send the patch so it doesn't
> > get mangled).
> >
> > Thanks,
> > Hugo.
> >
> > On Tue, Apr 24, 2012 at 08:43:23PM +0200, Goffredo Baroncelli wrote:
> >> I was giving a look to the function scrub_fs_info( ), and to me it seems
> >> that could be a potential file handle leaking problem.
> >>
> >>
> >> In fact:
> >>
> >> static int scrub_fs_info(int fd, char *path,
> >> struct btrfs_ioctl_fs_info_args *fi_args,
> >> struct btrfs_ioctl_dev_info_args **di_ret)
> >> {
> >>
> >> [...]
> >>
> >> ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args);
> >> if (ret && errno == EINVAL) {
> >> /* path is no mounted btrfs. try if it's a device */
> >> [...]
> >> close(fd); <--- Here the
> >> file handle is
> >> closed
> >>
> >> fd = open_file_or_dir(mp); <--- then it is
> >> re-opened
> >> if (fd < 0)
> >> return -errno;
> >> } else if (ret) {
> >> return -errno;
> >> }
> >> [...]
> >>
> >> But in the rest of the function:
> >> a) the file handle is not closed
> >> b) the (new) file handle isn't returned
> >>
> >> The function "scrub_fs_info()" is called from the functions
> >> 1) cmd_scrub_status(), which doesn't use the file handle after the call
> >> to the cmd_scrub_status() [except for a close()]. So no problem at all.
> >> 2) scrub_start(), which uses the file handle after the call to the
> >> cmd_scrub_status() functions.
> >>
> >> My suggestions is to change scrub_fs_info() to accept only the path.
> >> Then it open (and closes) its own (and private) the file descriptor.
> >>
> >> Instead scrub_start(), opens a file descriptor after the call to the
> >> scrub_fs_info() function.
> >>
> >> What do you think ?
> >>
> >> BR
> >> G.Baroncelli
> >>
> >> You can pull the patch below from
> >>
> >> http://cassiopea.homelinux.net/git/btrfs-progs-unstable.git
> >>
> >> branch
> >>
> >> fd-leaking
> >>
> >> -----
> >>
> >> diff --git a/cmds-scrub.c b/cmds-scrub.c
> >> index c4503f4..486768c 100644
> >> --- a/cmds-scrub.c
> >> +++ b/cmds-scrub.c
> >> @@ -979,19 +979,26 @@ static int scrub_device_info(int fd, u64 devid,
> >> return ret ? -errno : 0;
> >> }
> >>
> >> -static int scrub_fs_info(int fd, char *path,
> >> +static int scrub_fs_info( char *path,
> >> struct btrfs_ioctl_fs_info_args *fi_args,
> >> struct btrfs_ioctl_dev_info_args **di_ret)
> >> {
> >> int ret = 0;
> >> int ndevs = 0;
> >> int i = 1;
> >> + int fd;
> >> struct btrfs_fs_devices *fs_devices_mnt = NULL;
> >> struct btrfs_ioctl_dev_info_args *di_args;
> >> char mp[BTRFS_PATH_NAME_MAX + 1];
> >>
> >> memset(fi_args, 0, sizeof(*fi_args));
> >>
> >> + fd = open_file_or_dir(path);
> >> + if (fd < 0) {
> >> + fprintf(stderr, "ERROR: can't access to '%s'\n", path);
> >> + return -1;
> >> + }
> >> +
> >> ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args);
> >> if (ret && errno == EINVAL) {
> >> /* path is no mounted btrfs. try if it's a device */
> >> @@ -1010,28 +1017,36 @@ static int scrub_fs_info(int fd, char *path,
> >> if (fd < 0)
> >> return -errno;
> >> } else if (ret) {
> >> + close(fd);
> >> return -errno;
> >> }
> >>
> >> - if (!fi_args->num_devices)
> >> + if (!fi_args->num_devices){
> >> + close(fd);
> >> return 0;
> >> + }
> >>
> >> di_args = *di_ret = malloc(fi_args->num_devices * sizeof(*di_args));
> >> - if (!di_args)
> >> + if (!di_args){
> >> + close(fd);
> >> return -errno;
> >> + }
> >>
> >> for (; i <= fi_args->max_id; ++i) {
> >> BUG_ON(ndevs >= fi_args->num_devices);
> >> ret = scrub_device_info(fd, i, &di_args[ndevs]);
> >> if (ret == -ENODEV)
> >> continue;
> >> - if (ret)
> >> + if (ret){
> >> + close(fd);
> >> return ret;
> >> + }
> >> ++ndevs;
> >> }
> >>
> >> BUG_ON(ndevs == 0);
> >>
> >> + close(fd);
> >> return 0;
> >> }
> >>
> >> @@ -1155,7 +1170,7 @@ static int scrub_start(int argc, char **argv, int
> >> resume)
> >> return 12;
> >> }
> >>
> >> - ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args);
> >> + ret = scrub_fs_info(path, &fi_args, &di_args);
> >> if (ret) {
> >> ERR(!do_quiet, "ERROR: getting dev info for scrub failed: "
> >> "%s\n", strerror(-ret));
> >> @@ -1586,7 +1601,6 @@ static int cmd_scrub_status(int argc, char **argv)
> >> .sun_family = AF_UNIX,
> >> };
> >> int ret;
> >> - int fdmnt;
> >> int i;
> >> int print_raw = 0;
> >> int do_stats_per_dev = 0;
> >> @@ -1615,13 +1629,7 @@ static int cmd_scrub_status(int argc, char **argv)
> >>
> >> path = argv[optind];
> >>
> >> - fdmnt = open_file_or_dir(path);
> >> - if (fdmnt < 0) {
> >> - fprintf(stderr, "ERROR: can't access to '%s'\n", path);
> >> - return 12;
> >> - }
> >> -
> >> - ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args);
> >> + ret = scrub_fs_info(path, &fi_args, &di_args);
> >> if (ret) {
> >> fprintf(stderr, "ERROR: getting dev info for scrub failed: "
> >> "%s\n", strerror(-ret));
> >> @@ -1698,7 +1706,6 @@ static int cmd_scrub_status(int argc, char **argv)
> >> out:
> >> free_history(past_scrubs);
> >> free(di_args);
> >> - close(fdmnt);
> >> if (fdres > -1)
> >> close(fdres);
> >>
> >>
> >>
> >>
> >>
> >>
> >
>
> diff --git a/cmds-scrub.c b/cmds-scrub.c
> index c4503f4..6313e02 100644
> --- a/cmds-scrub.c
> +++ b/cmds-scrub.c
> @@ -979,19 +979,26 @@ static int scrub_device_info(int fd, u64 devid,
> return ret ? -errno : 0;
> }
>
> -static int scrub_fs_info(int fd, char *path,
> +static int scrub_fs_info(char *path,
> struct btrfs_ioctl_fs_info_args *fi_args,
> struct btrfs_ioctl_dev_info_args **di_ret)
> {
> int ret = 0;
> int ndevs = 0;
> int i = 1;
> + int fd;
> struct btrfs_fs_devices *fs_devices_mnt = NULL;
> struct btrfs_ioctl_dev_info_args *di_args;
> char mp[BTRFS_PATH_NAME_MAX + 1];
>
> memset(fi_args, 0, sizeof(*fi_args));
>
> + fd = open_file_or_dir(path);
> + if (fd < 0) {
> + fprintf(stderr, "ERROR: can't access to '%s'\n", path);
> + return -1;
> + }
> +
> ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args);
> if (ret && errno == EINVAL) {
> /* path is no mounted btrfs. try if it's a device */
> @@ -1010,28 +1017,36 @@ static int scrub_fs_info(int fd, char *path,
> if (fd < 0)
> return -errno;
> } else if (ret) {
> + close(fd);
> return -errno;
> }
>
> - if (!fi_args->num_devices)
> + if (!fi_args->num_devices) {
> + close(fd);
> return 0;
> + }
>
> di_args = *di_ret = malloc(fi_args->num_devices * sizeof(*di_args));
> - if (!di_args)
> + if (!di_args) {
> + close(fd);
> return -errno;
> + }
>
> for (; i <= fi_args->max_id; ++i) {
> BUG_ON(ndevs >= fi_args->num_devices);
> ret = scrub_device_info(fd, i, &di_args[ndevs]);
> if (ret == -ENODEV)
> continue;
> - if (ret)
> + if (ret) {
> + close(fd);
> return ret;
> + }
> ++ndevs;
> }
>
> BUG_ON(ndevs == 0);
>
> + close(fd);
> return 0;
> }
>
> @@ -1155,7 +1170,7 @@ static int scrub_start(int argc, char **argv, int resume)
> return 12;
> }
>
> - ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args);
> + ret = scrub_fs_info(path, &fi_args, &di_args);
> if (ret) {
> ERR(!do_quiet, "ERROR: getting dev info for scrub failed: "
> "%s\n", strerror(-ret));
> @@ -1586,7 +1601,6 @@ static int cmd_scrub_status(int argc, char **argv)
> .sun_family = AF_UNIX,
> };
> int ret;
> - int fdmnt;
> int i;
> int print_raw = 0;
> int do_stats_per_dev = 0;
> @@ -1615,13 +1629,7 @@ static int cmd_scrub_status(int argc, char **argv)
>
> path = argv[optind];
>
> - fdmnt = open_file_or_dir(path);
> - if (fdmnt < 0) {
> - fprintf(stderr, "ERROR: can't access to '%s'\n", path);
> - return 12;
> - }
> -
> - ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args);
> + ret = scrub_fs_info(path, &fi_args, &di_args);
> if (ret) {
> fprintf(stderr, "ERROR: getting dev info for scrub failed: "
> "%s\n", strerror(-ret));
> @@ -1698,7 +1706,6 @@ static int cmd_scrub_status(int argc, char **argv)
> out:
> free_history(past_scrubs);
> free(di_args);
> - close(fdmnt);
> if (fdres > -1)
> close(fdres);
>
--
=== Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ===
PGP key: 515C238D from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
--- Great oxymorons of the world, no. 5: Manifesto Promise ---
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 190 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [btrfs-progs] [bug][patch V2] Leaking file handle in scrub_fs_info()
2012-06-05 18:19 ` Hugo Mills
@ 2012-06-05 20:12 ` Goffredo Baroncelli
0 siblings, 0 replies; 7+ messages in thread
From: Goffredo Baroncelli @ 2012-06-05 20:12 UTC (permalink / raw)
To: Hugo Mills, linux-btrfs
Hi Hugo,
On 06/05/2012 08:19 PM, Hugo Mills wrote:
> On Tue, Jun 05, 2012 at 07:26:34PM +0200, Goffredo Baroncelli
> wrote:
>> Hi Hugo,
>>
>> I was not able to reproduce your error with my repository: I
>> pulled it without problem.
>
> Well, I'd have normally written it off as a corrupt local repo at
> this end -- except that David Sterba said he'd also seen a similar
> problem pulling from your repo.
>
>> However, enclosed you can find the patch (as attachment) with the
>> Jan suggestions about the spaces. You can pull the patch also
>> from my repo ( same branch: fd-leaking). Moreover I inserted the
>> patch as inlined. Hoping that helps.
>
> Not really. Your mailer is breaking things (there's a wrapped line
> in there, and it's heavily whitespace-damaged). Could you *please*
> learn to use git send-email? Every single patch of yours that I've
> seen on the mailing list is badly broken, and takes an age to get
> it to the point where "git am" will look at it cleanly. It's
> incredibly frustrating. This time around, I think I've managed to
> get it to work OK, but it's taken me 10 minutes of buggering
> around, and I'm getting irritable, which isn't a good thing when
> I'm meant to be having a quiet relaxing week off work...
I perfectly understand what means loosing 10 minutes instead of 1
seconds :-)
But was you able to use the patch "attached" (not the one inlined ) ?
>
> Hugo.
>
>> --------------------
>>
>> commit b73abc9465d3a105b5f8f464f2867543a638e5e2 Author: Goffredo
>> Baroncelli <kreijack@inwind.it> Date: Tue Jun 5 19:11:06 2012
>> +0200
>>
>> scrub_fs_info( ) file handle leaking
>>
>> The function scrub_fs_info( ) closes and reopen a file handle
>> passed as argument, when a caller uses the file handle even after
>> the call. The function scrub_fs_info( ) is updated to remove the
>> file handle argument, and instead uses a private own file
>> handle. The callers are updated to not pass the argument.
>>
>>
>> diff --git a/cmds-scrub.c b/cmds-scrub.c index c4503f4..6313e02
>> 100644 --- a/cmds-scrub.c +++ b/cmds-scrub.c @@ -979,19 +979,26
>> @@ static int scrub_device_info(int fd, u64 devid, return ret ?
>> -errno : 0; }
>>
>> -static int scrub_fs_info(int fd, char *path, +static int
>> scrub_fs_info(char *path, struct btrfs_ioctl_fs_info_args
>> *fi_args, struct btrfs_ioctl_dev_info_args **di_ret) { int ret =
>> 0; int ndevs = 0; int i = 1; + int fd; struct
>> btrfs_fs_devices *fs_devices_mnt = NULL; struct
>> btrfs_ioctl_dev_info_args *di_args; char mp[BTRFS_PATH_NAME_MAX +
>> 1];
>>
>> memset(fi_args, 0, sizeof(*fi_args));
>>
>> + fd = open_file_or_dir(path); + if (fd < 0) { +
>> fprintf(stderr, "ERROR: can't access to '%s'\n", path); +
>> return -1; + } + ret = ioctl(fd, BTRFS_IOC_FS_INFO,
>> fi_args); if (ret && errno == EINVAL) { /* path is no mounted
>> btrfs. try if it's a device */ @@ -1010,28 +1017,36 @@ static int
>> scrub_fs_info(int fd, char *path, if (fd < 0) return -errno; }
>> else if (ret) { + close(fd); return -errno; }
>>
>> - if (!fi_args->num_devices) + if
>> (!fi_args->num_devices) { + close(fd); return 0; +
>> }
>>
>> di_args = *di_ret = malloc(fi_args->num_devices *
>> sizeof(*di_args)); - if (!di_args) + if (!di_args) {
>> + close(fd); return -errno; + }
>>
>> for (; i <= fi_args->max_id; ++i) { BUG_ON(ndevs >=
>> fi_args->num_devices); ret = scrub_device_info(fd, i,
>> &di_args[ndevs]); if (ret == -ENODEV) continue; -
>> if (ret) + if (ret) { +
>> close(fd); return ret; + } ++ndevs; }
>>
>> BUG_ON(ndevs == 0);
>>
>> + close(fd); return 0; }
>>
>> @@ -1155,7 +1170,7 @@ static int scrub_start(int argc, char
>> **argv, int resume) return 12; }
>>
>> - ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args); +
>> ret = scrub_fs_info(path, &fi_args, &di_args); if (ret) {
>> ERR(!do_quiet, "ERROR: getting dev info for scrub failed: "
>> "%s\n", strerror(-ret)); @@ -1586,7 +1601,6 @@ static int
>> cmd_scrub_status(int argc, char **argv) .sun_family = AF_UNIX,
>> }; int ret; - int fdmnt; int i; int print_raw = 0; int
>> do_stats_per_dev = 0; @@ -1615,13 +1629,7 @@ static int
>> cmd_scrub_status(int argc, char **argv)
>>
>> path = argv[optind];
>>
>> - fdmnt = open_file_or_dir(path); - if (fdmnt < 0) {
>> - fprintf(stderr, "ERROR: can't access to '%s'\n",
>> path); - return 12; - } - - ret =
>> scrub_fs_info(fdmnt, path, &fi_args, &di_args); + ret =
>> scrub_fs_info(path, &fi_args, &di_args); if (ret) {
>> fprintf(stderr, "ERROR: getting dev info for scrub failed: "
>> "%s\n", strerror(-ret)); @@ -1698,7 +1706,6 @@ static int
>> cmd_scrub_status(int argc, char **argv) out:
>> free_history(past_scrubs); free(di_args); - close(fdmnt);
>> if (fdres > -1) close(fdres);
>>
>>
>>
>>
>>
>>
>> On 06/05/2012 01:01 PM, Hugo Mills wrote:
>>> Hi, Goffredo,
>>>
>>> I'm trying to do a new integration branch, and the patch inline
>>> in the text is corrupt (plus it has a massively verbose commit
>>> message):
>>>
>>> Applying: Leaking file handle in scrub_fs_info() fatal: corrupt
>>> patch at line 74 Patch failed at 0001 Leaking file handle in
>>> scrub_fs_info()
>>>
>>> Normally, I'd use your git repo to get round the fact that
>>> every single patch you send by mail is whitespace-damaged,
>>> but:
>>>
>>> error: Unable to find 337bfb3250203a66b953ffcf7804418cb7c72052
>>> under
>>> http://cassiopea.homelinux.net/git/btrfs-progs-unstable.git
>>> Cannot obtain needed commit
>>> 337bfb3250203a66b953ffcf7804418cb7c72052 while processing
>>> commit ee3832b47a4b1d1dce8d1cacc802db32901b8c45. error: Fetch
>>> failed.
>>>
>>> Plus Jan had some suggested tweaks regarding whitespace --
>>> could you re-roll the patch and maybe see what you can do to
>>> fix your git repo? (Or at least use git-send-email to send the
>>> patch so it doesn't get mangled).
>>>
>>> Thanks, Hugo.
>>>
>>> On Tue, Apr 24, 2012 at 08:43:23PM +0200, Goffredo Baroncelli
>>> wrote:
>>>> I was giving a look to the function scrub_fs_info( ), and to
>>>> me it seems that could be a potential file handle leaking
>>>> problem.
>>>>
>>>>
>>>> In fact:
>>>>
>>>> static int scrub_fs_info(int fd, char *path, struct
>>>> btrfs_ioctl_fs_info_args *fi_args, struct
>>>> btrfs_ioctl_dev_info_args **di_ret) {
>>>>
>>>> [...]
>>>>
>>>> ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args); if (ret && errno
>>>> == EINVAL) { /* path is no mounted btrfs. try if it's a
>>>> device */ [...] close(fd); <--- Here the file handle is
>>>> closed
>>>>
>>>> fd = open_file_or_dir(mp); <--- then it is re-opened if (fd <
>>>> 0) return -errno; } else if (ret) { return -errno; } [...]
>>>>
>>>> But in the rest of the function: a) the file handle is not
>>>> closed b) the (new) file handle isn't returned
>>>>
>>>> The function "scrub_fs_info()" is called from the functions
>>>> 1) cmd_scrub_status(), which doesn't use the file handle
>>>> after the call to the cmd_scrub_status() [except for a
>>>> close()]. So no problem at all. 2) scrub_start(), which uses
>>>> the file handle after the call to the cmd_scrub_status()
>>>> functions.
>>>>
>>>> My suggestions is to change scrub_fs_info() to accept only
>>>> the path. Then it open (and closes) its own (and private) the
>>>> file descriptor.
>>>>
>>>> Instead scrub_start(), opens a file descriptor after the call
>>>> to the scrub_fs_info() function.
>>>>
>>>> What do you think ?
>>>>
>>>> BR G.Baroncelli
>>>>
>>>> You can pull the patch below from
>>>>
>>>> http://cassiopea.homelinux.net/git/btrfs-progs-unstable.git
>>>>
>>>> branch
>>>>
>>>> fd-leaking
>>>>
>>>> -----
>>>>
>>>> diff --git a/cmds-scrub.c b/cmds-scrub.c index
>>>> c4503f4..486768c 100644 --- a/cmds-scrub.c +++
>>>> b/cmds-scrub.c @@ -979,19 +979,26 @@ static int
>>>> scrub_device_info(int fd, u64 devid, return ret ? -errno :
>>>> 0; }
>>>>
>>>> -static int scrub_fs_info(int fd, char *path, +static int
>>>> scrub_fs_info( char *path, struct btrfs_ioctl_fs_info_args
>>>> *fi_args, struct btrfs_ioctl_dev_info_args **di_ret) { int
>>>> ret = 0; int ndevs = 0; int i = 1; + int fd; struct
>>>> btrfs_fs_devices *fs_devices_mnt = NULL; struct
>>>> btrfs_ioctl_dev_info_args *di_args; char
>>>> mp[BTRFS_PATH_NAME_MAX + 1];
>>>>
>>>> memset(fi_args, 0, sizeof(*fi_args));
>>>>
>>>> + fd = open_file_or_dir(path); + if (fd < 0) { +
>>>> fprintf(stderr, "ERROR: can't access to '%s'\n", path); +
>>>> return -1; + } + ret = ioctl(fd, BTRFS_IOC_FS_INFO,
>>>> fi_args); if (ret && errno == EINVAL) { /* path is no mounted
>>>> btrfs. try if it's a device */ @@ -1010,28 +1017,36 @@ static
>>>> int scrub_fs_info(int fd, char *path, if (fd < 0) return
>>>> -errno; } else if (ret) { + close(fd); return -errno; }
>>>>
>>>> - if (!fi_args->num_devices) + if (!fi_args->num_devices){ +
>>>> close(fd); return 0; + }
>>>>
>>>> di_args = *di_ret = malloc(fi_args->num_devices *
>>>> sizeof(*di_args)); - if (!di_args) + if (!di_args){ +
>>>> close(fd); return -errno; + }
>>>>
>>>> for (; i <= fi_args->max_id; ++i) { BUG_ON(ndevs >=
>>>> fi_args->num_devices); ret = scrub_device_info(fd, i,
>>>> &di_args[ndevs]); if (ret == -ENODEV) continue; - if (ret) +
>>>> if (ret){ + close(fd); return ret; + } ++ndevs; }
>>>>
>>>> BUG_ON(ndevs == 0);
>>>>
>>>> + close(fd); return 0; }
>>>>
>>>> @@ -1155,7 +1170,7 @@ static int scrub_start(int argc, char
>>>> **argv, int resume) return 12; }
>>>>
>>>> - ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args); + ret
>>>> = scrub_fs_info(path, &fi_args, &di_args); if (ret) {
>>>> ERR(!do_quiet, "ERROR: getting dev info for scrub failed: "
>>>> "%s\n", strerror(-ret)); @@ -1586,7 +1601,6 @@ static int
>>>> cmd_scrub_status(int argc, char **argv) .sun_family =
>>>> AF_UNIX, }; int ret; - int fdmnt; int i; int print_raw = 0;
>>>> int do_stats_per_dev = 0; @@ -1615,13 +1629,7 @@ static int
>>>> cmd_scrub_status(int argc, char **argv)
>>>>
>>>> path = argv[optind];
>>>>
>>>> - fdmnt = open_file_or_dir(path); - if (fdmnt < 0) { -
>>>> fprintf(stderr, "ERROR: can't access to '%s'\n", path); -
>>>> return 12; - } - - ret = scrub_fs_info(fdmnt, path, &fi_args,
>>>> &di_args); + ret = scrub_fs_info(path, &fi_args, &di_args);
>>>> if (ret) { fprintf(stderr, "ERROR: getting dev info for scrub
>>>> failed: " "%s\n", strerror(-ret)); @@ -1698,7 +1706,6 @@
>>>> static int cmd_scrub_status(int argc, char **argv) out:
>>>> free_history(past_scrubs); free(di_args); - close(fdmnt); if
>>>> (fdres > -1) close(fdres);
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>
>
>> diff --git a/cmds-scrub.c b/cmds-scrub.c index c4503f4..6313e02
>> 100644 --- a/cmds-scrub.c +++ b/cmds-scrub.c @@ -979,19 +979,26
>> @@ static int scrub_device_info(int fd, u64 devid, return ret ?
>> -errno : 0; }
>>
>> -static int scrub_fs_info(int fd, char *path, +static int
>> scrub_fs_info(char *path, struct btrfs_ioctl_fs_info_args
>> *fi_args, struct btrfs_ioctl_dev_info_args **di_ret) { int ret =
>> 0; int ndevs = 0; int i = 1; + int fd; struct btrfs_fs_devices
>> *fs_devices_mnt = NULL; struct btrfs_ioctl_dev_info_args
>> *di_args; char mp[BTRFS_PATH_NAME_MAX + 1];
>>
>> memset(fi_args, 0, sizeof(*fi_args));
>>
>> + fd = open_file_or_dir(path); + if (fd < 0) { +
>> fprintf(stderr, "ERROR: can't access to '%s'\n", path); + return
>> -1; + } + ret = ioctl(fd, BTRFS_IOC_FS_INFO, fi_args); if (ret &&
>> errno == EINVAL) { /* path is no mounted btrfs. try if it's a
>> device */ @@ -1010,28 +1017,36 @@ static int scrub_fs_info(int
>> fd, char *path, if (fd < 0) return -errno; } else if (ret) { +
>> close(fd); return -errno; }
>>
>> - if (!fi_args->num_devices) + if (!fi_args->num_devices) { +
>> close(fd); return 0; + }
>>
>> di_args = *di_ret = malloc(fi_args->num_devices *
>> sizeof(*di_args)); - if (!di_args) + if (!di_args) { +
>> close(fd); return -errno; + }
>>
>> for (; i <= fi_args->max_id; ++i) { BUG_ON(ndevs >=
>> fi_args->num_devices); ret = scrub_device_info(fd, i,
>> &di_args[ndevs]); if (ret == -ENODEV) continue; - if (ret) + if
>> (ret) { + close(fd); return ret; + } ++ndevs; }
>>
>> BUG_ON(ndevs == 0);
>>
>> + close(fd); return 0; }
>>
>> @@ -1155,7 +1170,7 @@ static int scrub_start(int argc, char
>> **argv, int resume) return 12; }
>>
>> - ret = scrub_fs_info(fdmnt, path, &fi_args, &di_args); + ret =
>> scrub_fs_info(path, &fi_args, &di_args); if (ret) {
>> ERR(!do_quiet, "ERROR: getting dev info for scrub failed: "
>> "%s\n", strerror(-ret)); @@ -1586,7 +1601,6 @@ static int
>> cmd_scrub_status(int argc, char **argv) .sun_family = AF_UNIX,
>> }; int ret; - int fdmnt; int i; int print_raw = 0; int
>> do_stats_per_dev = 0; @@ -1615,13 +1629,7 @@ static int
>> cmd_scrub_status(int argc, char **argv)
>>
>> path = argv[optind];
>>
>> - fdmnt = open_file_or_dir(path); - if (fdmnt < 0) { -
>> fprintf(stderr, "ERROR: can't access to '%s'\n", path); - return
>> 12; - } - - ret = scrub_fs_info(fdmnt, path, &fi_args,
>> &di_args); + ret = scrub_fs_info(path, &fi_args, &di_args); if
>> (ret) { fprintf(stderr, "ERROR: getting dev info for scrub
>> failed: " "%s\n", strerror(-ret)); @@ -1698,7 +1706,6 @@ static
>> int cmd_scrub_status(int argc, char **argv) out:
>> free_history(past_scrubs); free(di_args); - close(fdmnt); if
>> (fdres > -1) close(fdres);
>>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-06-05 20:12 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-04-24 18:43 [btrfs-progs] [bug][patch] Leaking file handle in scrub_fs_info() Goffredo Baroncelli
2012-04-25 9:39 ` Jan Schmidt
2012-04-25 19:07 ` Goffredo Baroncelli
2012-06-05 11:01 ` Hugo Mills
2012-06-05 17:26 ` [btrfs-progs] [bug][patch V2] " Goffredo Baroncelli
2012-06-05 18:19 ` Hugo Mills
2012-06-05 20:12 ` Goffredo Baroncelli
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).