| Message ID | 20230414030830.3829332-1-zenghao@kylinos.cn (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2] cpupower:Fix resource leaks in sysfs_get_enabled() | expand |
On 4/13/23 21:08, Hao Zeng wrote: > When the read return value is equal to 1, a file handle leak will occur > Would like a bit more information on how the error paths are redone, since memory leak isn't the only one that is fixed. > Signed-off-by: Hao Zeng <zenghao@kylinos.cn> > Suggested-by: Shuah Khan <skhan@linuxfoundation.org> > --- > tools/power/cpupower/lib/powercap.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) > > diff --git a/tools/power/cpupower/lib/powercap.c b/tools/power/cpupower/lib/powercap.c > index 0ce29ee4c2e4..02ec5b0bff6b 100644 > --- a/tools/power/cpupower/lib/powercap.c > +++ b/tools/power/cpupower/lib/powercap.c > @@ -40,7 +40,7 @@ static int sysfs_get_enabled(char *path, int *mode) > { > int fd; > char yes_no; > - > + int ret = 0; > *mode = 0; > > fd = open(path, O_RDONLY); > @@ -48,17 +48,18 @@ static int sysfs_get_enabled(char *path, int *mode) > return -1; > > if (read(fd, &yes_no, 1) != 1) { > - close(fd); > - return -1; > + ret = -1; > + goto err; > } > > - if (yes_no == '1') { > - *mode = 1; > - return 0; > - } else if (yes_no == '0') { > - return 0; > + if (yes_no != '1' || yes_no != '0') { > + ret = -1; > + goto err; > } > - return -1; > + *mode = yes_no - '0'; I am not seeing much value in changing the above paths. Leave them unchanged. > +err: > + close(fd); > + return ret; > } > > int powercap_get_enabled(int *mode) thanks, -- Shuah
Hi Shuah , Thanks very much for your review! I tried to understand what you said as best I could and provided V3. I look forward to your review again, it will help me a lot! Best regards -- Hao On Fri, 2023-04-14 at 10:41 -0600, Shuah Khan wrote: > On 4/13/23 21:08, Hao Zeng wrote: > > When the read return value is equal to 1, a file handle leak will > > occur > > > > Would like a bit more information on how the error paths are > redone, since memory leak isn't the only one that is fixed. > > > Signed-off-by: Hao Zeng <zenghao@kylinos.cn> > > Suggested-by: Shuah Khan <skhan@linuxfoundation.org> > > --- > > tools/power/cpupower/lib/powercap.c | 19 ++++++++++--------- > > 1 file changed, 10 insertions(+), 9 deletions(-) > > > > diff --git a/tools/power/cpupower/lib/powercap.c > > b/tools/power/cpupower/lib/powercap.c > > index 0ce29ee4c2e4..02ec5b0bff6b 100644 > > --- a/tools/power/cpupower/lib/powercap.c > > +++ b/tools/power/cpupower/lib/powercap.c > > @@ -40,7 +40,7 @@ static int sysfs_get_enabled(char *path, int > > *mode) > > { > > int fd; > > char yes_no; > > - > > + int ret = 0; > > *mode = 0; > > > > fd = open(path, O_RDONLY); > > @@ -48,17 +48,18 @@ static int sysfs_get_enabled(char *path, int > > *mode) > > return -1; > > > > if (read(fd, &yes_no, 1) != 1) { > > - close(fd); > > - return -1; > > + ret = -1; > > + goto err; > > } > > > > - if (yes_no == '1') { > > - *mode = 1; > > - return 0; > > - } else if (yes_no == '0') { > > - return 0; > > + if (yes_no != '1' || yes_no != '0') { > > + ret = -1; > > + goto err; > > } > > - return -1; > > + *mode = yes_no - '0'; > > I am not seeing much value in changing the above paths. > Leave them unchanged. > > > +err: > > + close(fd); > > + return ret; > > } > > > > int powercap_get_enabled(int *mode) > > thanks, > -- Shuah
diff --git a/tools/power/cpupower/lib/powercap.c b/tools/power/cpupower/lib/powercap.c index 0ce29ee4c2e4..02ec5b0bff6b 100644 --- a/tools/power/cpupower/lib/powercap.c +++ b/tools/power/cpupower/lib/powercap.c @@ -40,7 +40,7 @@ static int sysfs_get_enabled(char *path, int *mode) { int fd; char yes_no; - + int ret = 0; *mode = 0; fd = open(path, O_RDONLY); @@ -48,17 +48,18 @@ static int sysfs_get_enabled(char *path, int *mode) return -1; if (read(fd, &yes_no, 1) != 1) { - close(fd); - return -1; + ret = -1; + goto err; } - if (yes_no == '1') { - *mode = 1; - return 0; - } else if (yes_no == '0') { - return 0; + if (yes_no != '1' || yes_no != '0') { + ret = -1; + goto err; } - return -1; + *mode = yes_no - '0'; +err: + close(fd); + return ret; } int powercap_get_enabled(int *mode)
When the read return value is equal to 1, a file handle leak will occur Signed-off-by: Hao Zeng <zenghao@kylinos.cn> Suggested-by: Shuah Khan <skhan@linuxfoundation.org> --- tools/power/cpupower/lib/powercap.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)