| Message ID | 20200413104631.12276-4-email@uajain.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On 13/04/2020 11:46, Umang Jain wrote: > Pointed out by Coverity DefectId=279091 > Looking at testFile(), doesn't the fd also leak after the call to logSetFile(path) if that fails? If there is the same issue in multiple places in a function I think it should be handled by a single patch. It would be nice of course if these releases could be handled automatically - so perhaps converting things to use FileDescriptor classes might help, though an initial glance looks like that will always dup() the FD which might not be particularly desirable ... I'll leave it to you to decide, either rework to convert to FileDescriptor objects on a per-test basis, or just do a v2 of this patch with all paths fixed, perhaps for now just fixing this would make coverity happy :-) -- Kieran > Signed-off-by: Umang Jain <email@uajain.com> > --- > test/log/log_api.cpp | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/test/log/log_api.cpp b/test/log/log_api.cpp > index 33622f8..aeedbf9 100644 > --- a/test/log/log_api.cpp > +++ b/test/log/log_api.cpp > @@ -96,6 +96,7 @@ protected: > lseek(fd, 0, SEEK_SET); > if (read(fd, buf, sizeof(buf)) < 0) { > cerr << "Failed to read tmp log file" << endl; > + close(fd); > return TestFail; > } > close(fd); >
Hi Kieran, On Tue, Apr 14, 2020 at 12:29, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Looking at testFile(), doesn't the fd also leak after the call to > logSetFile(path) if that fails? > > If there is the same issue in multiple places in a function I think it > should be handled by a single patch. > Ah yes, nice catch. I missed to notice that. > > It would be nice of course if these releases could be handled > automatically - so perhaps converting things to use FileDescriptor > classes might help, though an initial glance looks like that will > always > dup() the FD which might not be particularly desirable ... > > I'll leave it to you to decide, either rework to convert to > FileDescriptor objects on a per-test basis, or just do a v2 of this > patch with all paths fixed, perhaps for now just fixing this would > make > coverity happy :-) I prefer to do a v2 of this patch right now. I think that porting tests/ to use FileDescriptor itself is a self-contained task and should deserve a entire patchset. We need to first investigate and analyze a bit, as Laurent mentioned previously and extend FileDescriptor class (if required at the first place).
Hi Kieran, On Tue, Apr 14, 2020 at 12:29:45PM +0100, Kieran Bingham wrote: > On 13/04/2020 11:46, Umang Jain wrote: > > Pointed out by Coverity DefectId=279091 > > Looking at testFile(), doesn't the fd also leak after the call to > logSetFile(path) if that fails? > > If there is the same issue in multiple places in a function I think it > should be handled by a single patch. > > > It would be nice of course if these releases could be handled > automatically - so perhaps converting things to use FileDescriptor > classes might help, though an initial glance looks like that will always > dup() the FD which might not be particularly desirable ... I think this should migrate to File, not FileDescriptor, when that class will have read/write/seek support. > I'll leave it to you to decide, either rework to convert to > FileDescriptor objects on a per-test basis, or just do a v2 of this > patch with all paths fixed, perhaps for now just fixing this would make > coverity happy :-) > > > Signed-off-by: Umang Jain <email@uajain.com> > > --- > > test/log/log_api.cpp | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/test/log/log_api.cpp b/test/log/log_api.cpp > > index 33622f8..aeedbf9 100644 > > --- a/test/log/log_api.cpp > > +++ b/test/log/log_api.cpp > > @@ -96,6 +96,7 @@ protected: > > lseek(fd, 0, SEEK_SET); > > if (read(fd, buf, sizeof(buf)) < 0) { > > cerr << "Failed to read tmp log file" << endl; > > + close(fd); > > return TestFail; > > } > > close(fd);
Hi Umang, On Tue, Apr 14, 2020 at 01:38:26PM +0000, Umang Jain wrote: > On Tue, Apr 14, 2020 at 12:29, Kieran Bingham wrote: > > > > Looking at testFile(), doesn't the fd also leak after the call to > > logSetFile(path) if that fails? > > > > If there is the same issue in multiple places in a function I think it > > should be handled by a single patch. > > Ah yes, nice catch. I missed to notice that. > > > It would be nice of course if these releases could be handled > > automatically - so perhaps converting things to use FileDescriptor > > classes might help, though an initial glance looks like that will > > always > > dup() the FD which might not be particularly desirable ... > > > > I'll leave it to you to decide, either rework to convert to > > FileDescriptor objects on a per-test basis, or just do a v2 of this > > patch with all paths fixed, perhaps for now just fixing this would > > make > > coverity happy :-) > > I prefer to do a v2 of this patch right now. I think that porting > tests/ to use FileDescriptor itself is a self-contained task and > should deserve a entire patchset. We need to first investigate and > analyze a bit, as Laurent mentioned previously and extend > FileDescriptor class (if required at the first place). I agree with you for this patch. For 2/4, however, I'd like to move to FileDescriptor.
diff --git a/test/log/log_api.cpp b/test/log/log_api.cpp index 33622f8..aeedbf9 100644 --- a/test/log/log_api.cpp +++ b/test/log/log_api.cpp @@ -96,6 +96,7 @@ protected: lseek(fd, 0, SEEK_SET); if (read(fd, buf, sizeof(buf)) < 0) { cerr << "Failed to read tmp log file" << endl; + close(fd); return TestFail; } close(fd);
Pointed out by Coverity DefectId=279091 Signed-off-by: Umang Jain <email@uajain.com> --- test/log/log_api.cpp | 1 + 1 file changed, 1 insertion(+)