[PATCH] refactor: simplify open and close wrappers

9 views
Skip to first unread message

Michael Adler

unread,
Aug 1, 2023, 7:23:19 AM8/1/23
to efibootg...@googlegroups.com, Michael Adler
This is (arguably) easier to read and reduces the total lines of code.

Signed-off-by: Michael Adler <michae...@siemens.com>
---
env/env_config_file.c | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/env/env_config_file.c b/env/env_config_file.c
index e270fbb..0b6c5a4 100644
--- a/env/env_config_file.c
+++ b/env/env_config_file.c
@@ -21,12 +21,7 @@
FILE *open_config_file(char *configfilepath, char *mode)
{
VERBOSE(stdout, "Probing config file at %s.\n", configfilepath);
- FILE *config = fopen(configfilepath, mode);
- if (config) {
- return config;
- } else {
- return NULL;
- }
+ return fopen(configfilepath, mode);
}

FILE *open_config_file_from_part(CONFIG_PART *cfgpart, char *mode)
@@ -51,10 +46,7 @@ FILE *open_config_file_from_part(CONFIG_PART *cfgpart, char *mode)

int close_config_file(FILE *config_file_handle)
{
- if (config_file_handle) {
- return fclose(config_file_handle);
- }
- return EINVAL;
+ return config_file_handle ? fclose(config_file_handle) : EINVAL;
}

bool probe_config_file(CONFIG_PART *cfgpart)
--
2.41.0

Jan Kiszka

unread,
Aug 8, 2023, 12:32:00 PM8/8/23
to Michael Adler, efibootg...@googlegroups.com
I wonder if we actually need those wrappers. open_config_file() seems to
exists purely for VERBOSE(), ok. But the NULL check in close_config_file
is definitely pointless when you check its few callers. And
probe_config_file() does not even bother about close_config_file(),
rather goes for fclose directly.

Jan

--
Siemens AG, Technology
Linux Expert Center

Michael Adler

unread,
Aug 16, 2023, 7:18:31 AM8/16/23
to efibootg...@googlegroups.com, Michael Adler
* open_config_file can be simplified to improve readability and
maintenance.
* close_config_file can be inlined because the NULL check is
superfluous.

Signed-off-by: Michael Adler <michae...@siemens.com>
---
env/env_api_fat.c | 4 ++--
env/env_config_file.c | 15 +--------------
include/env_config_file.h | 1 -
tools/bg_envtools.c | 2 +-
4 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/env/env_api_fat.c b/env/env_api_fat.c
index b7540bb..2d698f7 100644
--- a/env/env_api_fat.c
+++ b/env/env_api_fat.c
@@ -106,7 +106,7 @@ bool read_env(CONFIG_PART *part, BG_ENVDATA *env)
}
result = false;
}
- if (close_config_file(config)) {
+ if (fclose(config)) {
VERBOSE(stderr,
"Error closing environment file after reading.\n");
};
@@ -151,7 +151,7 @@ bool write_env(CONFIG_PART *part, BG_ENVDATA *env)
part->devpath);
result = false;
}
- if (close_config_file(config)) {
+ if (fclose(config)) {
VERBOSE(stderr,
"Error closing environment file after writing.\n");
result = false;
diff --git a/env/env_config_file.c b/env/env_config_file.c
index e270fbb..c9cc99f 100644
--- a/env/env_config_file.c
+++ b/env/env_config_file.c
@@ -21,12 +21,7 @@
FILE *open_config_file(char *configfilepath, char *mode)
{
VERBOSE(stdout, "Probing config file at %s.\n", configfilepath);
- FILE *config = fopen(configfilepath, mode);
- if (config) {
- return config;
- } else {
- return NULL;
- }
+ return fopen(configfilepath, mode);
}

FILE *open_config_file_from_part(CONFIG_PART *cfgpart, char *mode)
@@ -49,14 +44,6 @@ FILE *open_config_file_from_part(CONFIG_PART *cfgpart, char *mode)
return config;
}

-int close_config_file(FILE *config_file_handle)
-{
- if (config_file_handle) {
- return fclose(config_file_handle);
- }
- return EINVAL;
-}
-
bool probe_config_file(CONFIG_PART *cfgpart)
{
bool do_unmount = false;
diff --git a/include/env_config_file.h b/include/env_config_file.h
index c142a33..57949a0 100644
--- a/include/env_config_file.h
+++ b/include/env_config_file.h
@@ -18,5 +18,4 @@

FILE *open_config_file_from_part(CONFIG_PART *cfgpart, char *mode);
FILE *open_config_file(char *configfilepath, char *mode);
-int close_config_file(FILE *config_file_handle);
bool probe_config_file(CONFIG_PART *cfgpart);
diff --git a/tools/bg_envtools.c b/tools/bg_envtools.c
index 786d6c1..2d29e46 100644
--- a/tools/bg_envtools.c
+++ b/tools/bg_envtools.c
@@ -150,7 +150,7 @@ bool get_env(char *configfilepath, BG_ENVDATA *data)
result = false;
}

- if (close_config_file(config)) {
+ if (fclose(config)) {
VERBOSE(stderr,
"Error closing environment file after reading.\n");
};
--
2.41.0

Jan Kiszka

unread,
Aug 16, 2023, 9:20:54 AM8/16/23
to Michael Adler, efibootg...@googlegroups.com
Thanks, applied.
Reply all
Reply to author
Forward
0 new messages