tools/counter: close fd when exit

Commit Message

zhangjiao2 Sept. 3, 2024, 8:01 a.m. UTC

From: zhang jiao <[e-mail]>

close fd when exit the program

Signed-off-by: zhang jiao <[e-mail]>
---
 tools/counter/counter_example.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Andy Shevchenko Sept. 3, 2024, 11:19 p.m. UTC | #1

Tue, Sep 03, 2024 at 04:01:21PM +0800, zhangjiao2 kirjoitti:
> From: zhang jiao <[e-mail]>
> 
> close fd when exit the program

Please, mind English grammar and punctuation.
Also this doesn't state what the problem is.

...

>  		if (ret == -1) {
>  			fprintf(stderr, "Error adding watches[%d]: %s\n", i,
>  				strerror(errno));
> +			close(fd);

Since fd is not used in the messaging it's better to close it before printing
anything. Ditto for other cases.

...

>  		if (ret != sizeof(event_data)) {
>  			fprintf(stderr, "Failed to read event data\n");
> +			close(fd);

>  			return -EIO;

Side note: This will return garbage to the userspace. Should be

			return EIO;

>  		}

David Laight Sept. 8, 2024, 8:08 p.m. UTC | #2

From: Andy Shevchenko
> Sent: 04 September 2024 00:20
> 
> Tue, Sep 03, 2024 at 04:01:21PM +0800, zhangjiao2 kirjoitti:
> > From: zhang jiao <[e-mail]>
> >
> > close fd when exit the program
> 
> Please, mind English grammar and punctuation.
> Also this doesn't state what the problem is.

After all, when the program exits the kernel closes all the files.

> >  		if (ret == -1) {
> >  			fprintf(stderr, "Error adding watches[%d]: %s\n", i,
> >  				strerror(errno));
> > +			close(fd);
> 
> Since fd is not used in the messaging it's better to close it before printing
> anything. Ditto for other cases.

Unless close() decides to change errno...

> 
> ...
> 
> >  		if (ret != sizeof(event_data)) {
> >  			fprintf(stderr, "Failed to read event data\n");
> > +			close(fd);
> 
> >  			return -EIO;
> 
> Side note: This will return garbage to the userspace. Should be
> 
> 			return EIO;

If that is the return value from main() it is equally invalid.
Valid return values are 0..255 but 128..255 are likely to be
interpreted as 'killed by a signal'.
So really you only get 0..127.
Mostly used as 0 => success and 1 => failure.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Patch

diff --git a/tools/counter/counter_example.c b/tools/counter/counter_example.c
index be55287b950f..37569208c235 100644
--- a/tools/counter/counter_example.c
+++ b/tools/counter/counter_example.c
@@ -57,12 +57,14 @@  int main(void)
 		if (ret == -1) {
 			fprintf(stderr, "Error adding watches[%d]: %s\n", i,
 				strerror(errno));
+			close(fd);
 			return 1;
 		}
 	}
 	ret = ioctl(fd, COUNTER_ENABLE_EVENTS_IOCTL);
 	if (ret == -1) {
 		perror("Error enabling events");
+		close(fd);
 		return 1;
 	}
 
@@ -70,11 +72,13 @@  int main(void)
 		ret = read(fd, event_data, sizeof(event_data));
 		if (ret == -1) {
 			perror("Failed to read event data");
+			close(fd);
 			return 1;
 		}
 
 		if (ret != sizeof(event_data)) {
 			fprintf(stderr, "Failed to read event data\n");
+			close(fd);
 			return -EIO;
 		}
 
@@ -88,5 +92,6 @@  int main(void)
 		       strerror(event_data[1].status));
 	}
 
+	close(fd);
 	return 0;
 }